From 5b8ec36c5918372b4f52ca16be03a437cdd8df0b Mon Sep 17 00:00:00 2001 From: sneak Date: Tue, 3 Nov 2026 08:23:08 +0000 Subject: [PATCH] fix: a saved wait in another shape reaches SuccessTx when its receipt arrives (closes #734) A saved wait whose decoded record is in another shape threw when its receipt arrived. WaitTx then stayed on screen with the counter stopped. decodedDetailsHtml() now shows only the lines whose label and value are text. SuccessTx shows the amount and recipient when no line can be read. WaitTx and ErrorTx now show each allowance's Allowance token line. Long amounts use break-words, so words stay whole. ErrorTx sets up copying when it has no hash, so the token's address can be copied. Model: opus-5-5 --- README.md | 4 +- src/popup/index.html | 7 +- src/popup/views/txStatus.js | 84 +++++++++++------ tests/swapStatusAmount.test.js | 3 + tests/txStatus.test.js | 168 +++++++++++++++++++++++++++++++-- 5 files changed, 222 insertions(+), 44 deletions(-) diff --git a/README.md b/README.md index 25c7f61..b437b89 100644 --- a/README.md +++ b/README.md @@ -787,7 +787,7 @@ Shown after a broadcast. The screen holds: - the heading "Transaction Broadcast", with no "Back" button; - the amount and symbol; - each `Moved from the sender` amount of a swap; -- each `Permit2 allowance` amount of a swap; +- each `Permit2 allowance` amount of a swap, with its `Allowance token` line; - the recipient; - the transaction hash, with an explorer link; - the counter "Waiting for confirmation... Ns". @@ -827,7 +827,7 @@ The screen holds: - the heading "Transaction Failed"; - the amount; - each `Moved from the sender` amount of a swap; -- each `Permit2 allowance` amount of a swap; +- each `Permit2 allowance` amount of a swap, with its `Allowance token` line; - the recipient; - the error message, in a dashed box; - the transaction hash, when one exists; diff --git a/src/popup/index.html b/src/popup/index.html index 4b4b418..56cdef6 100644 --- a/src/popup/index.html +++ b/src/popup/index.html @@ -810,7 +810,10 @@

Transaction Broadcast

Amount
-
+
@@ -859,7 +862,7 @@
Amount
diff --git a/src/popup/views/txStatus.js b/src/popup/views/txStatus.js index 6955c9d..43af5a4 100644 --- a/src/popup/views/txStatus.js +++ b/src/popup/views/txStatus.js @@ -73,25 +73,53 @@ function toAddressHtml(address, chainId) { return renderAddressHtml(address, { title, chainId }); } -// The amount of each `Moved from the sender` and `Permit2 allowance` line of a -// decoded swap, for the wait and error screens to show under the Amount figure. -// That figure can leave these amounts out, so alone it can understate what can -// leave the wallet. `decoded` can come back from storage in any shape, so this -// shows only the lines whose value is text. -function movedOrAllowedHtml(decoded) { +// A decoded record can come back from storage in any shape, so these screens +// show only the lines whose label and value are text. An address that is not +// text would throw in toAddressHtml(). +function isReadableLine(d) { + return ( + d && + typeof d.label === "string" && + typeof d.value === "string" && + (!d.address || typeof d.address === "string") + ); +} + +// What a decoded line with an address shows: the address, under the token's +// symbol when the line names a token. +function decodedAddressHtml(d, chainId) { + if (!d.isToken) return toAddressHtml(d.address, chainId); + const sym = tokenLabel(d.address, chainId) || "Unknown token"; + return ( + `
${escapeHtml(sym)}
` + + blockscoutOffTokenNoteHtml(d.address, chainId) + + toAddressHtml(d.address, chainId) + ); +} + +// Each `Moved from the sender`, `Permit2 allowance` and `Allowance token` line +// of a decoded swap, for the wait and error screens to show under the Amount +// figure. That figure can leave these amounts out, so alone it can understate +// what can leave the wallet. The token line says which token an allowance +// covers, which `Unlimited` alone does not. +function movedOrAllowedHtml(decoded, chainId) { if (!decoded || !Array.isArray(decoded.details)) return ""; let html = ""; for (const d of decoded.details) { if ( - !d || + !isReadableLine(d) || (d.label !== "Moved from the sender" && - d.label !== "Permit2 allowance") || - typeof d.value !== "string" + d.label !== "Permit2 allowance" && + d.label !== "Allowance token") ) continue; html += `
`; html += `
${escapeHtml(d.label)}
`; - html += `
${escapeHtml(d.value)}
`; + if (d.address) { + html += `
${decodedAddressHtml(d, chainId)}
`; + } else { + html += `
${escapeHtml(d.value)}
`; + } html += `
`; } return html; @@ -135,7 +163,7 @@ function startWait(txInfo, txHash, broadcastTime, pollNow) { txInfo.amount + (symbol ? " " + symbol : ""); $("wait-tx-token-note").innerHTML = blockscoutOffTokenNoteHtml(txInfo.token, txInfo.chainId) + - movedOrAllowedHtml(txInfo.decoded); + movedOrAllowedHtml(txInfo.decoded, txInfo.chainId); $("wait-tx-to").innerHTML = toAddressHtml(txInfo.to, txInfo.chainId); $("wait-tx-hash").innerHTML = txHashHtml(txHash, txInfo.chainId); attachCopyHandlers("view-wait-tx"); @@ -246,7 +274,7 @@ function showWait(txInfo, txHash) { // to name a network. txInfo.token and txInfo.tokenSymbol are not checked here. // blockscoutOffTokenNoteHtml() shows no line for a token that is not text. // Everything else only compares and coalesces them, and tokenSymbol is null for -// ETH. txInfo.decoded is not checked either: see movedOrAllowedHtml(). +// ETH. txInfo.decoded is not checked either: see isReadableLine(). function restoreWait() { const d = state.viewData; if (!d || !d.pendingWait) return false; @@ -293,29 +321,25 @@ function showSuccess(txInfo, txHash, blockNumber) { ctx.doRefreshAndRender(); } +// The decoded action, or "" when no line of it can be read. function decodedDetailsHtml(decoded, chainId) { - if (!decoded || !decoded.details) return ""; + if (!decoded || !Array.isArray(decoded.details)) return ""; + const lines = decoded.details.filter(isReadableLine); + if (lines.length === 0) return ""; let html = `
`; - if (decoded.name) { + if (decoded.name && typeof decoded.name === "string") { html += `
Action
`; html += `
${escapeHtml(decoded.name)}
`; } - if (decoded.description) { + if (decoded.description && typeof decoded.description === "string") { html += `
Description
`; html += `
${escapeHtml(decoded.description)}
`; } - for (const d of decoded.details) { + for (const d of lines) { html += `
`; html += `
${escapeHtml(d.label)}
`; if (d.address) { - if (d.isToken) { - const sym = tokenLabel(d.address, chainId) || "Unknown token"; - html += `
${escapeHtml(sym)}
`; - html += blockscoutOffTokenNoteHtml(d.address, chainId); - html += toAddressHtml(d.address, chainId); - } else { - html += toAddressHtml(d.address, chainId); - } + html += decodedAddressHtml(d, chainId); } else { html += `
${escapeHtml(d.value)}
`; } @@ -329,13 +353,13 @@ function renderSuccess() { const d = state.viewData; if (!d || !d.hash) return; - const hasDecoded = d.decoded && d.decoded.details; + const decodedHtml = decodedDetailsHtml(d.decoded, d.chainId); // When decoded details are present, the Amount and To are already // shown inside the decoded well — hide the top-level duplicates. const summarySection = $("success-tx-summary").parentElement; const toSection = $("success-tx-to").parentElement; - if (hasDecoded) { + if (decodedHtml) { summarySection.classList.add("hidden"); toSection.classList.add("hidden"); } else { @@ -351,8 +375,8 @@ function renderSuccess() { // Show decoded calldata details if present const decodedEl = $("success-tx-decoded"); - if (decodedEl && hasDecoded) { - decodedEl.innerHTML = decodedDetailsHtml(d.decoded, d.chainId); + if (decodedEl && decodedHtml) { + decodedEl.innerHTML = decodedHtml; decodedEl.classList.remove("hidden"); } else if (decodedEl) { decodedEl.classList.add("hidden"); @@ -392,18 +416,18 @@ function renderError() { d.amount + (d.symbol ? " " + d.symbol : ""); $("error-tx-token-note").innerHTML = blockscoutOffTokenNoteHtml(d.token, d.chainId) + - movedOrAllowedHtml(d.decoded); + movedOrAllowedHtml(d.decoded, d.chainId); $("error-tx-to").innerHTML = toAddressHtml(d.to, d.chainId); $("error-tx-message").textContent = d.message; if (d.hash) { $("error-tx-hash").innerHTML = txHashHtml(d.hash, d.chainId); $("error-tx-hash-section").classList.remove("hidden"); - attachCopyHandlers("view-error-tx"); } else { $("error-tx-hash-section").classList.add("hidden"); } + attachCopyHandlers("view-error-tx"); showView("error-tx"); } diff --git a/tests/swapStatusAmount.test.js b/tests/swapStatusAmount.test.js index a54ff5c..10a3159 100644 --- a/tests/swapStatusAmount.test.js +++ b/tests/swapStatusAmount.test.js @@ -458,6 +458,9 @@ describe("a swap that gives another spender an allowance", () => { const html = node(id).innerHTML; expect(html).toContain("Permit2 allowance"); expect(html).toContain("Unlimited"); + expect(html).toContain("Allowance token"); + expect(html).toContain('
USDC
'); + expect(html).toContain(`data-copy="${USDC}"`); } test("the wait screen shows the allowance under the Amount figure", async () => { diff --git a/tests/txStatus.test.js b/tests/txStatus.test.js index 7c97782..e983c7a 100644 --- a/tests/txStatus.test.js +++ b/tests/txStatus.test.js @@ -617,8 +617,8 @@ describe("SuccessTx and ErrorTx for a Sepolia transaction with mainnet active", const decoded = { name: "Transfer", details: [ - { label: "Token", address: TOKEN, isToken: true }, - { label: "Recipient", address: RECIPIENT }, + { label: "Token", value: TOKEN, address: TOKEN, isToken: true }, + { label: "Recipient", value: RECIPIENT, address: RECIPIENT }, ], }; mockReceiptLookup.mockResolvedValue({ blockNumber: 21000000 }); @@ -644,7 +644,9 @@ describe("SuccessTx and ErrorTx for a Sepolia transaction with mainnet active", }; const decoded = { name: "Transfer", - details: [{ label: "Token", address: TOKEN, isToken: true }], + details: [ + { label: "Token", value: TOKEN, address: TOKEN, isToken: true }, + ], }; mockReceiptLookup.mockResolvedValue({ blockNumber: 21000000 }); txStatus.showWait({ ...sepoliaTx, decoded }, TX_HASH); @@ -747,6 +749,21 @@ describe("the Moved from the sender lines of a swap on WaitTx and ErrorTx", () = { details: [moved(7)] }, { details: [moved({ toString: 1 })] }, { details: [{ label: "Permit2 allowance", value: 7 }] }, + { + details: [ + { label: "Permit2 allowance", value: "text", address: 7 }, + ], + }, + { + details: [ + { + label: "Allowance token", + value: "text", + address: 7, + isToken: true, + }, + ], + }, ]) { expect(noteOnReopen(saved)).toBe(""); } @@ -757,11 +774,22 @@ describe("the Moved from the sender lines of a swap on WaitTx and ErrorTx", () = }); describe("the Permit2 allowance lines of a swap on WaitTx and ErrorTx", () => { + const USDC = "0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48"; const allowance = (value) => ({ label: "Permit2 allowance", value }); const decoded = { details: [ allowance("Unlimited"), - { label: "Allowance token", value: "USDC" }, + { + label: "Allowance token", + value: "USDC (" + USDC + ")", + address: USDC, + isToken: true, + }, + { + label: "Allowance spender", + value: RECIPIENT, + address: RECIPIENT, + }, allowance("400.0000 DAI"), ], }; @@ -771,9 +799,53 @@ describe("the Permit2 allowance lines of a swap on WaitTx and ErrorTx", () => { expect(html).toContain("Permit2 allowance"); expect(html).toContain("Unlimited"); expect(html).toContain("<b>400.0000</b> DAI"); - expect(html).not.toContain("Allowance token"); + expect(html).not.toContain("Allowance spender"); + expect(html).not.toContain(RECIPIENT); } + // `Unlimited` alone does not say which token it covers. The address gets + // the row of its own that every address gets, in the transaction's network. + function expectTokenLine(id, explorer) { + const html = getElement(id).innerHTML; + expect(html.indexOf("Unlimited")).toBeLessThan( + html.indexOf("Allowance token"), + ); + expect(html).toContain('
USDC
'); + expect(html).toContain(`data-copy="${USDC}"`); + expect(html).toContain(`href="${explorer}/address/${USDC}"`); + expect(html).toMatch(/
.*class="am-address"/); + } + + test("WaitTx says which token an allowance covers", () => { + mockReceiptLookup.mockResolvedValue(null); + txStatus.showWait({ ...TX_INFO, decoded }, TX_HASH); + expectTokenLine("wait-tx-token-note", "https://etherscan.io"); + }); + + test("ErrorTx says which token an allowance covers", () => { + txStatus.showError({ ...TX_INFO, decoded }, TX_HASH, "It failed."); + expectTokenLine("error-tx-token-note", "https://etherscan.io"); + }); + + test("the token's address links to the transaction's network", () => { + const sepoliaTx = { + ...TX_INFO, + chainId: NETWORKS.sepolia.chainId, + decoded, + }; + txStatus.showError(sepoliaTx, TX_HASH, "It failed."); + expect(getElement("error-tx-token-note").innerHTML).toContain( + `href="https://sepolia.etherscan.io/address/${USDC}"`, + ); + }); + + test("the token's address can be copied on an ErrorTx with no hash", () => { + const view = getElement("view-error-tx"); + view.querySelectorAll = jest.fn(() => []); + txStatus.showError({ ...TX_INFO, decoded }, null, "It failed."); + expect(view.querySelectorAll).toHaveBeenCalledWith("[data-copy]"); + }); + test("WaitTx shows each line's amount, escaped", () => { mockReceiptLookup.mockResolvedValue(null); txStatus.showWait({ ...TX_INFO, decoded }, TX_HASH); @@ -795,7 +867,8 @@ describe("the Permit2 allowance lines of a swap on WaitTx and ErrorTx", () => { }); // An amount of about 47 digits or more is wider than the screen. Without the -// break-all class its last digits are cut off at the edge. +// break-words class its last digits are cut off at the edge. That class keeps +// the words of a line such as `(decimals unknown)` whole. describe("a long amount on WaitTx and ErrorTx", () => { test("each amount under the Amount figure wraps", () => { const decoded = { @@ -806,8 +879,10 @@ describe("a long amount on WaitTx and ErrorTx", () => { }; txStatus.showError({ ...TX_INFO, decoded }, TX_HASH, "It failed."); const html = getElement("error-tx-token-note").innerHTML; - expect(html).toContain('
1.0000 ETH<'); - expect(html).toContain('
Unlimited<'); + expect(html).toContain( + '
1.0000 ETH<', + ); + expect(html).toContain('
Unlimited<'); }); test("the Amount figure wraps", () => { @@ -816,14 +891,87 @@ describe("a long amount on WaitTx and ErrorTx", () => { "utf8", ); expect(html).toMatch( - /id="wait-tx-summary"\s+class="font-bold break-all"/, + /id="wait-tx-summary"\s+class="font-bold break-words"/, ); expect(html).toMatch( - /id="error-tx-summary"\s+class="font-bold break-all"/, + /id="error-tx-summary"\s+class="font-bold break-words"/, ); }); }); +// A saved wait comes back from storage, in whatever shape it is in. A throw when +// its receipt arrives would leave the popup on WaitTx with the counter stopped. +describe("a saved wait in another shape when its receipt arrives", () => { + const LINE = { label: "Steps", value: "V3_SWAP_EXACT_IN" }; + + async function receiptForSavedWait(decoded) { + mockReceiptLookup.mockResolvedValue({ blockNumber: 21000000 }); + state.viewData = { + pendingWait: { + hash: TX_HASH, + txInfo: { ...TX_INFO, decoded }, + broadcastTime: Date.now(), + }, + }; + expect(txStatus.restoreWait()).toBe(true); + await jest.advanceTimersByTimeAsync(0); + } + + test.each([ + { details: 7 }, + { details: "text" }, + { details: { label: "Steps", value: "text" } }, + { details: [null] }, + { details: [7] }, + { details: ["text"] }, + { details: [{ value: "text" }] }, + { details: [{ label: 7, value: "text" }] }, + { details: [{ label: { toString: 1 }, value: "text" }] }, + { details: [{ label: "Steps" }] }, + { details: [{ label: "Steps", value: 7 }] }, + { details: [{ label: "Steps", value: { toString: 1 } }] }, + { details: [{ label: "Steps", address: RECIPIENT }] }, + { details: [{ label: "Steps", value: "text", address: 7 }] }, + { + details: [ + { label: "Steps", value: "text", address: 7, isToken: true }, + ], + }, + { name: "Swap", description: "A swap", details: [null] }, + ])("%j shows SuccessTx with the amount and recipient", async (decoded) => { + await receiptForSavedWait(decoded); + expect(state.currentView).toBe("success-tx"); + expect( + getElement("success-tx-decoded").classList.contains("hidden"), + ).toBe(true); + expect( + getElement("success-tx-summary-parent").classList.contains( + "hidden", + ), + ).toBe(false); + expect(getElement("success-tx-summary").textContent).toBe("0.0050 ETH"); + expect( + getElement("success-tx-to-parent").classList.contains("hidden"), + ).toBe(false); + expect(getElement("success-tx-to").innerHTML).toContain(RECIPIENT); + }); + + test.each([ + { details: [null, LINE, 7] }, + { name: 7, details: [LINE] }, + { name: { toString: 1 }, details: [LINE] }, + { description: 7, details: [LINE] }, + { description: { toString: 1 }, details: [LINE] }, + ])("%j shows SuccessTx with the line it can read", async (decoded) => { + await receiptForSavedWait(decoded); + expect(state.currentView).toBe("success-tx"); + const html = getElement("success-tx-decoded").innerHTML; + expect(html).toContain("V3_SWAP_EXACT_IN"); + expect(html).not.toContain("Action"); + expect(html).not.toContain("Description"); + }); +}); + describe("wait-tx is a view the popup may reopen onto", () => { // The resume feature is wired through RESTORABLE_VIEWS: restoreView() // refuses any view not in the set, so dropping "wait-tx" from it kills