fix: TransactionDetail drops a Blockscout answer for another transaction (closes #675)

TransactionDetail asks Blockscout for its transaction and does not wait for the answer. If the user pressed Back and opened another transaction before Blockscout answered, the first answer filled the second screen with its block, network details, decoded details and raw data. Now the answer is dropped once TransactionDetail shows another transaction. This check sits beside the "Use Blockscout" check.

Model: opus-5-5

README.md

@@ -2279,6 +2279,9 @@ view would leave a wallet one click from deletion.
           no user data and is made whether or not "Use Blockscout" is ticked.
     - If the network changes before either "Save" stores its URL, the URL is not
       saved and a flash message says so
+    - Either "Save" stores its URL only if the input still shows that URL when
+      the endpoint answers. Otherwise a flash message says that the URL was not
+      saved
     - Etherscan:
         - "Check recipients on Etherscan" checkbox, unticked until the user
           ticks it

src/popup/views/settings.js

@@ -339,17 +339,31 @@ function showNewSettings() {
 // Another page may have stored a different URL since then. So `url` is set in
 // the stored record too. "Saved." then means that the stored record holds
 // `url`.
-async function saveEndpoint(field, url, networkId) {
+//
+// `input` is the id of the input that the user typed `url` in. When another
+// page changes the network, "Use Blockscout" or "Check recipients on
+// Etherscan", this page draws Settings again. A redraw during the endpoint
+// check puts the URL from `state` back in the input. So if the input no longer
+// shows `url`, nothing is saved. The flash message then says that the URL was
+// changed during the check. The save compares the input and sets `url` in
+// `state` in one step. So a redraw after the comparison shows `url`.
+async function saveEndpoint(field, input, url, networkId) {
     let refused = false;
+    let inputChanged = false;
     await saveState((stored) => {
         refused =
             state.networkId !== networkId || stored.networkId !== networkId;
-        if (refused) return false;
+        inputChanged = $(input).value.trim() !== url;
+        if (refused || inputChanged) return false;
         state[field] = url;
         stored[field] = url;
     });
     if (refused) {
         showFlash("The URL was not saved because the network changed.");
+    } else if (inputChanged) {
+        showFlash(
+            "The URL was not saved because it was changed during the check.",
+        );
     } else {
         showFlash("Saved.");
     }
@@ -401,7 +415,7 @@ function init(_ctx) {
             showFlash("Could not reach endpoint.");
             return;
         }
-        await saveEndpoint("rpcUrl", url, net.id);
+        await saveEndpoint("rpcUrl", "settings-rpc", url, net.id);
     });
 
     $("btn-save-blockscout").addEventListener("click", async () => {
@@ -425,7 +439,7 @@ function init(_ctx) {
             showFlash("Could not reach endpoint.");
             return;
         }
-        await saveEndpoint("blockscoutUrl", url, net.id);
+        await saveEndpoint("blockscoutUrl", "settings-blockscout", url, net.id);
     });
 
     // Each change is made inside the save, holding the lock. So

src/popup/views/transactionDetail.js

@@ -311,15 +311,12 @@ function populateOnChainDetails(txData, chainId) {
     }
 }
 
-// Whether TransactionDetail is on screen for the transaction with `txHash` on
-// `chainId`.
-function isOnScreen(txHash, chainId) {
+// Whether TransactionDetail still holds the transaction with `txHash` on
+// `chainId`. Opening another transaction replaces the one TransactionDetail
+// holds. A visit to Settings does not.
+function holdsTransaction(txHash, chainId) {
     const tx = state.viewData.tx;
-    return (
-        state.currentView === "transaction" &&
-        tx?.hash === txHash &&
-        tx?.chainId === chainId
-    );
+    return tx?.hash === txHash && tx?.chainId === chainId;
 }
 
 // Reads the block, network details and calldata from Blockscout, and decodes
@@ -327,9 +324,8 @@ function isOnScreen(txHash, chainId) {
 // asked. An answer that lands after the setting went off is dropped
 // (src/shared/storedSettings.js). Either way none of them is shown.
 //
-// Blockscout can answer after the user has left this screen or opened another
-// transaction. That answer is dropped. If that request fails, its error is not
-// logged.
+// Blockscout can answer after the user has opened another transaction. That
+// answer is dropped.
 async function loadFullTxDetails(txHash, chainId) {
     if (!(await storedSettingIsOn("useBlockscout"))) return;
     const section = $("tx-detail-calldata-section");
@@ -348,7 +344,7 @@ async function loadFullTxDetails(txHash, chainId) {
         if (!resp.ok) return;
         const txData = await resp.json();
         if (!(await storedSettingIsOn("useBlockscout"))) return;
-        if (!isOnScreen(txHash, chainId)) return;
+        if (!holdsTransaction(txHash, chainId)) return;
 
         // Populate on-chain detail fields (block, nonce, gas, fee)
         populateOnChainDetails(txData, chainId);
@@ -415,9 +411,7 @@ async function loadFullTxDetails(txHash, chainId) {
             attachCopyHandlers(container);
         }
     } catch (e) {
-        if (isOnScreen(txHash, chainId)) {
-            log.errorf("loadCalldata failed:", e.message);
-        }
+        log.errorf("loadCalldata failed:", e.message);
     }
 }
 

tests/settingsEndpointSave.test.js

@@ -47,3 +47,123 @@ test.each([
         expect(page.text("flash-msg")).toBe("Saved.");
     },
 );
+
+// The user types a URL in Settings and taps "Save". While the check waits for
+// the endpoint's answer, another page turns "Use Blockscout" on. This page
+// follows that change and draws Settings again, with the stored URL in the
+// input. The check then passes. The typed URL is not stored. The flash message
+// says that the URL was changed during the check
+// (https://git.eeqj.de/sneak/AutistMask/issues/695).
+test.each([
+    { button: "btn-save-rpc", input: "settings-rpc", field: "rpcUrl" },
+    {
+        button: "btn-save-blockscout",
+        input: "settings-blockscout",
+        field: "blockscoutUrl",
+    },
+])(
+    "$button stores nothing when Settings is drawn again during the check",
+    async ({ button, input, field }) => {
+        const page = await bootPopup(unversionedValidProfile());
+        await page.click("btn-settings");
+        const storedUrl = page.value(input);
+        let answer;
+        globalThis.fetch.mockReturnValue(
+            new Promise((resolve) => {
+                answer = resolve;
+            }),
+        );
+
+        page.node(input).value = "https://typed.example.invalid";
+        const saving = page.node(button).listeners.click.map((fn) => fn());
+        await page.settle();
+        // The browser announces this write to this page.
+        await page.storage.local.set({
+            autistmask: {
+                ...page.storage.read("autistmask"),
+                useBlockscout: true,
+            },
+        });
+        await page.settle();
+        expect(page.node("settings-use-blockscout").checked).toBe(true);
+        // This answer passes both endpoint checks on Ethereum Mainnet.
+        answer({
+            ok: true,
+            json: async () => ({ result: NETWORKS.mainnet.chainId }),
+        });
+        await Promise.all(saving);
+        await page.settle();
+
+        expect(page.storage.read("autistmask")[field]).toBe(storedUrl);
+        expect(page.value(input)).toBe(storedUrl);
+        expect(page.text("flash-msg")).toBe(
+            "The URL was not saved because it was changed during the check.",
+        );
+    },
+);
+
+// As above, but another write holds the lock. So this page's follow of the
+// change to "Use Blockscout" waits for the lock. The endpoint answers while the
+// input still shows the typed URL. The save then asks for the lock, after the
+// follow did. So the follow draws Settings again before the save compares the
+// input.
+test.each([
+    { button: "btn-save-rpc", input: "settings-rpc", field: "rpcUrl" },
+    {
+        button: "btn-save-blockscout",
+        input: "settings-blockscout",
+        field: "blockscoutUrl",
+    },
+])(
+    "$button stores nothing when Settings is drawn again while the save waits for the lock",
+    async ({ button, input, field }) => {
+        const page = await bootPopup(unversionedValidProfile());
+        await page.click("btn-settings");
+        const storedUrl = page.value(input);
+        let answer;
+        globalThis.fetch.mockReturnValue(
+            new Promise((resolve) => {
+                answer = resolve;
+            }),
+        );
+        const typedUrl = "https://typed.example.invalid";
+
+        page.node(input).value = typedUrl;
+        const saving = page.node(button).listeners.click.map((fn) => fn());
+        await page.settle();
+        let releaseLock;
+        const lockHeld = navigator.locks.request(
+            "autistmask",
+            () =>
+                new Promise((resolve) => {
+                    releaseLock = resolve;
+                }),
+        );
+        // The browser announces this write to this page.
+        await page.storage.local.set({
+            autistmask: {
+                ...page.storage.read("autistmask"),
+                useBlockscout: true,
+            },
+        });
+        await page.settle();
+        // This answer passes both endpoint checks on Ethereum Mainnet.
+        answer({
+            ok: true,
+            json: async () => ({ result: NETWORKS.mainnet.chainId }),
+        });
+        await page.settle();
+        expect(page.value(input)).toBe(typedUrl);
+        releaseLock();
+        await lockHeld;
+        await Promise.all(saving);
+        await page.settle();
+
+        expect(page.node("settings-use-blockscout").checked).toBe(true);
+        expect(page.storage.read("autistmask")[field]).toBe(storedUrl);
+        expect(page.value(input)).toBe(storedUrl);
+        expect(page.text("flash-msg")).toBe(
+            "The URL was not saved because it was changed during the check.",
+        );
+    },
+);

tests/transactionDetailLateAnswer.test.js

@@ -2,10 +2,10 @@
 // transaction on screen (https://git.eeqj.de/sneak/AutistMask/issues/675).
 //
 // The screen asks Blockscout for its transaction and does not wait for the
-// answer. So the user can press Back and open a second transaction before
+// answer. So the user can press Back and open another transaction before
 // Blockscout answers for the first. The first answer must then fill nothing on
-// the second transaction's screen. If the first request fails instead, its
-// error must not be logged.
+// the second transaction's screen. An answer that lands while Settings is up
+// must still fill TransactionDetail.
 //
 // Driven through the real popup over a stored profile
 // (tests/support/popupBoot.js). Each transaction is opened as a history screen
@@ -21,50 +21,68 @@ const {
     ADDRESS,
 } = require("./support/popupBoot");
 
-const BLOCKSCOUT = "https://eth.blockscout.com/api/v2";
+const MAINNET_BLOCKSCOUT = "https://eth.blockscout.com/api/v2";
+const SEPOLIA_BLOCKSCOUT = "https://eth-sepolia.blockscout.com/api/v2";
 const RECIPIENT = "0x2222222222222222222222222222222222222222";
 
-// A history entry for a send of 1 ETH from ADDRESS to RECIPIENT on mainnet.
-function historyEntry(hash) {
+// A history entry for a send of 1 `symbol` from ADDRESS to RECIPIENT, on the
+// network with `chainId`.
+function historyEntry(hash, chainId, symbol) {
     return {
         hash,
         from: ADDRESS,
         to: RECIPIENT,
         value: "1.0000",
         exactValue: "1.0",
-        symbol: "ETH",
+        symbol,
         timestamp: 1790000000,
         isError: false,
         direction: "sent",
         directionLabel: "Sent",
         contractAddress: null,
-        chainId: "0x1",
+        chainId,
     };
 }
 
-const FIRST = historyEntry("0x" + "aa".repeat(32));
-const SECOND = historyEntry("0x" + "bb".repeat(32));
+const FIRST = historyEntry("0x" + "aa".repeat(32), "0x1", "ETH");
+const SECOND = historyEntry("0x" + "bb".repeat(32), "0x1", "ETH");
+// A transaction on Sepolia with the same hash as FIRST.
+const FIRST_ON_SEPOLIA = historyEntry(FIRST.hash, "0xaa36a7", "SepoliaETH");
 
-// What Blockscout reports for each transaction.
+// The URL that asks Blockscout for `entry`, on the network it was sent on.
+function requestUrl(entry) {
+    const blockscout =
+        entry.chainId === "0x1" ? MAINNET_BLOCKSCOUT : SEPOLIA_BLOCKSCOUT;
+    return blockscout + "/transactions/" + entry.hash;
+}
+
+// What Blockscout reports for each transaction, by the URL that asks for it.
 const ANSWERS = {
-    [FIRST.hash]: {
+    [requestUrl(FIRST)]: {
         block_number: 21000001,
         nonce: 4441,
         fee: { value: "21000000000000" },
         raw_input: "0x11111111",
         method: "firstMethod",
     },
-    [SECOND.hash]: {
+    [requestUrl(SECOND)]: {
         block_number: 21000002,
         nonce: 4442,
         fee: { value: "42000000000000" },
         raw_input: "0x22222222",
         method: "secondMethod",
     },
+    [requestUrl(FIRST_ON_SEPOLIA)]: {
+        block_number: 21000003,
+        nonce: 4443,
+        fee: { value: "63000000000000" },
+        raw_input: "0x33333333",
+        method: "sepoliaMethod",
+    },
 };
 
 // The text each answer puts on screen: its block, nonce, fee, method name and
-// raw data. No text from one answer appears in the other.
+// raw data. No text from one answer appears in another.
 const FIRST_SHOWN = [
     "21000001",
     "4441",
@@ -79,9 +97,13 @@ const SECOND_SHOWN = [
     "secondMethod",
     "0x22222222",
 ];
-
-// The error the first request fails with in the second test.
-const FIRST_FAILURE = "Blockscout did not answer the first request";
+const FIRST_ON_SEPOLIA_SHOWN = [
+    "21000003",
+    "4443",
+    "0.000063 SepoliaETH",
+    "sepoliaMethod",
+    "0x33333333",
+];
 
 // Everything drawn in the fields that Blockscout's answer fills.
 function drawnDetails(page) {
@@ -102,71 +124,79 @@ function open(entry) {
     require("../src/popup/views/transactionDetail").show(entry);
 }
 
-// Boots the popup on Home with "Use Blockscout" on. The user opens FIRST,
-// whose request to Blockscout is held. Then the user presses Back and opens
-// SECOND, whose request is answered at once. Returns the page and the held
-// request's two outcomes: answer() lets FIRST's answer land, and fail() makes
-// the request fail.
-async function secondOpenedWhileFirstIsOut() {
+// The URLs of every request to Blockscout so far, in order.
+function requestedUrls() {
+    return globalThis.fetch.mock.calls.map(([url]) => url);
+}
+
+// Boots the popup on Home with "Use Blockscout" on. Blockscout answers every
+// request at once from ANSWERS, except the request for `held`. Returns the
+// page and answer(), which lets the answer for `held` land.
+async function bootWithHeldRequest(held) {
     const page = await bootPopup(
         unversionedValidProfile({ useBlockscout: true }),
     );
     let answer;
-    let fail;
-    const firstHeld = new Promise((resolve, reject) => {
+    const heldAnswer = new Promise((resolve) => {
         answer = resolve;
-        fail = reject;
     });
     globalThis.fetch = jest.fn(async (url) => {
-        const hash = String(url).slice((BLOCKSCOUT + "/transactions/").length);
-        if (hash === FIRST.hash) await firstHeld;
-        return { ok: true, status: 200, json: async () => ANSWERS[hash] };
+        if (url === requestUrl(held)) await heldAnswer;
+        return { ok: true, status: 200, json: async () => ANSWERS[url] };
     });
+    return { page, answer: () => answer() };
+}
 
-    open(FIRST);
+// The user opens `first`, whose request to Blockscout is held. Then the user
+// presses Back and opens `second`. Then the answer for `first` lands. Returns
+// the page.
+async function firstAnswerLandsOnSecond(first, second) {
+    const { page, answer } = await bootWithHeldRequest(first);
+    open(first);
     await page.settle();
     await page.click("btn-tx-back");
     expect(page.visibleViews()).toEqual(["main"]);
-    open(SECOND);
+    open(second);
     await page.settle();
-    expect(globalThis.fetch.mock.calls.map(([url]) => url)).toEqual([
-        BLOCKSCOUT + "/transactions/" + FIRST.hash,
-        BLOCKSCOUT + "/transactions/" + SECOND.hash,
-    ]);
-    return {
-        page,
-        answer: () => answer(),
-        fail: () => fail(new Error(FIRST_FAILURE)),
-    };
+    expect(requestedUrls()).toEqual([requestUrl(first), requestUrl(second)]);
+    answer();
+    await page.settle();
+    expect(page.visibleViews()).toEqual(["transaction"]);
+    return page;
 }
 
-beforeEach(() => {
-    jest.spyOn(console, "error").mockImplementation(() => {});
-});
-
 afterEach(() => {
     cleanupPopup();
-    jest.restoreAllMocks();
 });
 
 test("an answer for the first transaction that lands on the second one's screen fills nothing", async () => {
-    const { page, answer } = await secondOpenedWhileFirstIsOut();
-    answer();
-    await page.settle();
+    const page = await firstAnswerLandsOnSecond(FIRST, SECOND);
 
-    expect(page.visibleViews()).toEqual(["transaction"]);
     const drawn = drawnDetails(page);
     for (const text of SECOND_SHOWN) expect(drawn).toContain(text);
     for (const text of FIRST_SHOWN) expect(drawn).not.toContain(text);
 });
 
-test("a request for the first transaction that fails on the second one's screen logs nothing", async () => {
-    const { page, fail } = await secondOpenedWhileFirstIsOut();
-    fail();
+test("an answer for a hash on mainnet that lands on the same hash's screen on Sepolia fills nothing", async () => {
+    const page = await firstAnswerLandsOnSecond(FIRST, FIRST_ON_SEPOLIA);
+
+    const drawn = drawnDetails(page);
+    for (const text of FIRST_ON_SEPOLIA_SHOWN) expect(drawn).toContain(text);
+    for (const text of FIRST_SHOWN) expect(drawn).not.toContain(text);
+});
+
+test("an answer that lands while Settings is up still fills TransactionDetail", async () => {
+    const { page, answer } = await bootWithHeldRequest(FIRST);
+    open(FIRST);
     await page.settle();
+    await page.click("btn-settings");
+    expect(page.visibleViews()).toEqual(["settings"]);
+    answer();
+    await page.settle();
+    await page.click("btn-settings");
 
     expect(page.visibleViews()).toEqual(["transaction"]);
+    expect(requestedUrls()).toEqual([requestUrl(FIRST)]);
     const drawn = drawnDetails(page);
-    for (const text of SECOND_SHOWN) expect(drawn).toContain(text);
-    expect(console.error.mock.calls.flat()).not.toContain(FIRST_FAILURE);
+    for (const text of FIRST_SHOWN) expect(drawn).toContain(text);
 });