From 33b97f135fc1a3c6accc1b671b5db79a5dabe732 Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 19 Oct 2026 10:54:29 +0000 Subject: [PATCH] fix: a send the confirmation screen refuses moves nothing on it (closes #583) Sign & Send wrote its refusals, a network changed since review and a gas limit or fee over its bound, into the errors box above the password field. That box reserves one empty line, so a refusal grew it and pushed the password field and Sign & Send down. Each refusal now has its own box with its sentence fixed in index.html, holding its space from the first paint and only made visible, as https://git.eeqj.de/sneak/AutistMask/issues/558 did for the balance messages. One sentence is true of all three bounds and quotes no number; the full reason is logged. A Chrome test raises the stub node's fee only at signing and checks nothing moved; a unit test refuses a gas limit over its bound into the same box. Model: opus-5-5 --- README.md | 17 ++- src/popup/index.html | 19 +++ src/popup/views/confirmTx.js | 59 +++++---- tests/confirmTxFeeBound.test.js | 173 +++++++++++++++++++++++++- tests/confirmTxNetworkSwitch.test.js | 23 +--- tests/e2e/network.js | 11 +- tests/e2e/run.js | 97 ++++++++++++++- tests/trackedTokensPerNetwork.test.js | 6 +- 8 files changed, 342 insertions(+), 63 deletions(-) diff --git a/README.md b/README.md index 22f8d9e..f86cc21 100644 --- a/README.md +++ b/README.md @@ -430,6 +430,11 @@ reserve while sitting on the same side of the estimate, so swapping the two in what [#154](https://git.eeqj.de/sneak/AutistMask/issues/154) was, and it was previously correct by reading only. +It also covers a send that Sign & Send refuses for its network fee: the stub +node quotes an ordinary fee while the confirmation screen is drawn and one over +the bound when Sign & Send asks again, and the refusal must broadcast nothing +and leave the password field and Sign & Send where they were. + It also covers both ways the wait for a sent transaction's receipt ends on the error screen: lookups that still find no receipt 60 seconds after the broadcast, and six lookups in a row that fail. Each must show its own message, and Done @@ -1793,6 +1798,9 @@ view would leave a wallet one click from deletion. estimated, and it says the wallet does not know how many decimal places the token uses and that the transaction cannot be sent; for any other failure it asks the user to go back and try again + - Two more reserved error boxes, made visible when "Sign & Send" refuses a + send because the active network changed or because of its gas limit or fee + (see Transitions), so the refusal moves nothing - Password: an inline field on this screen, not a modal, with its own error line - "Sign & Send" button (disabled if errors, while the balance is being read, @@ -1810,7 +1818,7 @@ view would leave a wallet one click from deletion. - "Sign & Send" after the active network was switched in Settings since this transaction was reviewed → nothing is signed; "The active network changed after this transaction was reviewed, so it was not sent. Please go back - and review it again." in the errors box, no screen change. After a switch, + and review it again." in its own box, no screen change. After a switch, "Back" from Settings draws this screen again from the saved transaction, as a popup reopened onto it does; when Send is beneath it on the stack, as when Send showed it in the same popup, "Back" lands below Send instead @@ -1824,6 +1832,13 @@ view would leave a wallet one click from deletion. fee estimate and the contract and never-sent recipient checks ask that network's RPC endpoint, and the Etherscan label check asks that network's explorer + - "Sign & Send" (correct password) when the gas limit, a fee per gas or the + total fee the RPC endpoint fills in at signing is over its bound in + `assertWithinCeilings()` (`src/shared/approvalVerify.js`) → nothing is + signed; "The gas limit or fee the RPC endpoint quoted for this transaction + is more than this wallet will sign for, so it was not sent. Please go back + and try again." in its own box, the same sentence for all three, no screen + change - "Sign & Send" (wrong password) → "Wrong password." on the password error line, no screen change - "Sign & Send" signs as the From address above, not as whichever address is diff --git a/src/popup/index.html b/src/popup/index.html index b1a42b9..89e3fac 100644 --- a/src/popup/index.html +++ b/src/popup/index.html @@ -697,6 +697,25 @@ id="confirm-fee-unknown-error" class="mb-2 border border-border border-dashed p-2 text-xs invisible" > + + +
${escapeHtml(message)}
`; - el.style.visibility = "visible"; -} - async function checkRecipientHistory(txInfo) { try { const provider = reviewedNetworkProvider(txInfo); @@ -640,6 +635,10 @@ function init(_ctx) { onViewLeave("confirm-tx", clearPassword); $("btn-confirm-send").addEventListener("click", async () => { + // A press shows only its own refusal, if any. + setVisible("confirm-network-changed-error", false); + setVisible("confirm-fee-bound-error", false); + const wallet = state.wallets[state.selectedWallet]; // Every Send button refuses a defective wallet before this screen, // but the popup also reopens onto it from a saved view. @@ -654,11 +653,7 @@ function init(_ctx) { // Settings after a switch draws this screen again from the saved // transaction, as a popup reopened onto it does. if (currentNetwork().chainId !== pendingTx.chainId) { - showSendError( - "The active network changed after this transaction was" + - " reviewed, so it was not sent. Please go back and review" + - " it again.", - ); + setVisible("confirm-network-changed-error", true); return; } @@ -722,12 +717,16 @@ function init(_ctx) { txStatus.showWait(pendingTx, tx.hash); } catch (e) { decryptedSecret = null; - // A fee over the bound is refused before anything is broadcast, so - // there is no transaction that may have reached the network to warn - // about: the message stays on the confirmation screen where the - // user can go back, rather than routing to the sent/failed screen. + // A gas limit or fee over its bound is refused before anything is + // broadcast, so there is no transaction that may have reached the + // network to warn about: the refusal stays on the confirmation + // screen where the user can go back, rather than routing to the + // sent/failed screen. Its sentence is fixed in index.html, so that + // its box can hold its space from the first paint; the reason in + // full, naming the bound, is logged. if (e && e.approvalMismatch) { - showSendError(e.message); + log.errorf("send refused:", e.message); + setVisible("confirm-fee-bound-error", true); return; } const hash = tx ? tx.hash : null; diff --git a/tests/confirmTxFeeBound.test.js b/tests/confirmTxFeeBound.test.js index 7385930..a271e36 100644 --- a/tests/confirmTxFeeBound.test.js +++ b/tests/confirmTxFeeBound.test.js @@ -11,20 +11,115 @@ // // The check is driven here with a fake connected signer rather than a real // one: populateTransaction() returns the fees the node would have produced, -// and sendTransaction() records whether the send actually happened. The real -// DOM path around it — reading the fee error into the reserved errors box — is -// covered by the Chrome e2e suite. +// and sendTransaction() records whether the send actually happened. +// +// The last test drives the real confirmation screen instead, with only the +// node and the DOM stubbed: a gas limit over its own bound at an ordinary fee +// is refused in the same box as a fee over the bound +// (https://git.eeqj.de/sneak/AutistMask/issues/583). That the box moves +// nothing on the screen is checked by the Chrome e2e suite. -globalThis.chrome = { - storage: { local: { get: async () => ({}), set: async () => {} } }, +// What the stub node quotes for that test: a gas limit of 150,000,000 at a +// maximum fee of 2 gwei per gas, 0.3 ETH in all. And the signed transactions +// it was handed. +const mockNode = { + gasLimit: 150000000n, + maxFeePerGas: 2000000000n, + broadcast: [], }; +jest.mock("ethers", () => { + const actual = jest.requireActual("ethers"); + class StubProvider { + constructor(url, network) { + this.network = network; + } + async getNetwork() { + return this.network; + } + async getBalance() { + return actual.parseEther("5"); + } + async getCode() { + return "0x"; + } + async getTransactionCount() { + return 1; + } + async getFeeData() { + return { + maxFeePerGas: mockNode.maxFeePerGas, + maxPriorityFeePerGas: 1000000000n, + gasPrice: 1000000000n, + }; + } + async estimateGas() { + return mockNode.gasLimit; + } + async broadcastTransaction(signed) { + mockNode.broadcast.push(signed); + return { hash: "0x" + "ab".repeat(32) }; + } + } + return { ...actual, JsonRpcProvider: StubProvider }; +}); + +// The screens a send that was not refused goes on to. +jest.mock("../src/popup/views/txStatus", () => ({ + showWait: jest.fn(), + showError: jest.fn(), +})); + +const { makeStorageStub } = require("./support/storageStub"); +global.chrome = { storage: makeStorageStub(), runtime: { sendMessage() {} } }; + global.fetch = jest.fn(() => { throw new Error("tests must not perform network requests"); }); -const { populateVerifyAndSend } = require("../src/popup/views/confirmTx"); +// A stub DOM: every id resolves to a recording element. +const elements = new Map(); + +function makeEl(id) { + const handlers = new Map(); + return { + id, + textContent: "", + innerHTML: "", + value: "", + disabled: false, + style: {}, + dataset: {}, + classList: { + add() {}, + remove() {}, + toggle() {}, + contains: () => false, + }, + handlers, + addEventListener(name, fn) { + handlers.set(name, fn); + }, + querySelectorAll: () => [], + remove() {}, + }; +} + +global.document = { + getElementById(id) { + if (!elements.has(id)) elements.set(id, makeEl(id)); + return elements.get(id); + }, +}; + +const { Wallet } = require("ethers"); +const { encryptWithPassword } = require("../src/shared/vault"); +const { state, loadState } = require("../src/shared/state"); +const txStatus = require("../src/popup/views/txStatus"); +const confirmTx = require("../src/popup/views/confirmTx"); +const { populateVerifyAndSend } = confirmTx; const { + MAX_GAS_LIMIT, MAX_FEE_PER_GAS, MAX_TOTAL_FEE, } = require("../src/shared/approvalVerify"); @@ -98,3 +193,69 @@ describe("populateVerifyAndSend enforces the combined fee bound", () => { expect(signer.sent[0].gasLimit).toBe(under.gasLimit); }); }); + +describe("Sign & Send on the confirmation screen", () => { + const PRIVATE_KEY = "0x" + "11".repeat(32); + const PASSWORD = "correct horse battery staple"; + let encryptedSecret; + + beforeAll(async () => { + await loadState(); + encryptedSecret = await encryptWithPassword(PRIVATE_KEY, PASSWORD); + }); + + function el(id) { + return global.document.getElementById(id); + } + + function shown(id) { + return el(id).style.visibility === "visible"; + } + + // show() reads the balance and estimates the fee without awaiting + // either; this lets both finish. + async function settle() { + for (let i = 0; i < 10; i++) await new Promise((r) => setTimeout(r, 0)); + } + + test("refuses a gas limit over its bound at an ordinary fee in the box for a refused gas limit or fee, and sends nothing", async () => { + expect(mockNode.gasLimit).toBeGreaterThan(MAX_GAS_LIMIT); + expect(mockNode.gasLimit * mockNode.maxFeePerGas).toBeLessThan( + MAX_TOTAL_FEE, + ); + + const from = new Wallet(PRIVATE_KEY).address; + state.wallets = [ + { + type: "key", + name: "Wallet 1", + encryptedSecret, + addresses: [{ address: from }], + }, + ]; + state.selectedWallet = 0; + state.selectedAddress = 0; + confirmTx.init({}); + confirmTx.show({ + from, + to: RECIPIENT, + ensName: null, + amount: "1", + token: "ETH", + tokenSymbol: null, + tokenDecimals: null, + max: false, + }); + await settle(); + expect(el("btn-confirm-send").disabled).toBe(false); + expect(shown("confirm-fee-bound-error")).toBe(false); + + el("confirm-tx-password").value = PASSWORD; + await el("btn-confirm-send").handlers.get("click")(); + + expect(shown("confirm-fee-bound-error")).toBe(true); + expect(mockNode.broadcast).toEqual([]); + expect(txStatus.showWait).not.toHaveBeenCalled(); + expect(txStatus.showError).not.toHaveBeenCalled(); + }); +}); diff --git a/tests/confirmTxNetworkSwitch.test.js b/tests/confirmTxNetworkSwitch.test.js index 9bbcb20..6a998a7 100644 --- a/tests/confirmTxNetworkSwitch.test.js +++ b/tests/confirmTxNetworkSwitch.test.js @@ -440,12 +440,8 @@ describe.each([ expect(mockNode.broadcast).toEqual([]); expect(txStatus.showWait).not.toHaveBeenCalled(); expect(txStatus.showError).not.toHaveBeenCalled(); - expect(el("confirm-errors").style.visibility).toBe("visible"); - expect(el("confirm-errors").innerHTML).toContain( - "The active network changed after this transaction was" + - " reviewed, so it was not sent. Please go back and review" + - " it again.", - ); + expect(shown("confirm-network-changed-error")).toBe(true); + expect(shown("confirm-fee-bound-error")).toBe(false); }); }, ); @@ -497,10 +493,7 @@ describe.each([ expect(mockNode.broadcast).toEqual([]); expect(txStatus.showWait).not.toHaveBeenCalled(); expect(txStatus.showError).not.toHaveBeenCalled(); - expect(el("confirm-errors").innerHTML).toContain( - "The active network changed after this transaction was" + - " reviewed, so it was not sent.", - ); + expect(shown("confirm-network-changed-error")).toBe(true); }); test("is sent on that network, as described, once it is chosen in Settings again and Back pressed", async () => { @@ -579,10 +572,7 @@ describe.each([ expect(signTransaction).not.toHaveBeenCalled(); expect(mockNode.broadcast).toEqual([]); - expect(errors()).toContain( - "The active network changed after this transaction was" + - " reviewed, so it was not sent.", - ); + expect(shown("confirm-network-changed-error")).toBe(true); }); const tokenSend = { @@ -629,10 +619,7 @@ describe.each([ expect(signTransaction).not.toHaveBeenCalled(); expect(mockNode.broadcast).toEqual([]); - expect(errors()).toContain( - "The active network changed after this transaction was" + - " reviewed, so it was not sent.", - ); + expect(shown("confirm-network-changed-error")).toBe(true); }); test("with a fee quoted by a node with no type-2 pricing, shows the fee in that network's native currency and price", async () => { diff --git a/tests/e2e/network.js b/tests/e2e/network.js index e2d2443..9ba2ea4 100644 --- a/tests/e2e/network.js +++ b/tests/e2e/network.js @@ -226,8 +226,9 @@ const RPC_RESULTS = { // The "latest" block, which ethers' getFeeData() reads baseFeePerGas from // to derive maxFeePerGas. Without it every fee is a legacy gasPrice, the // reserve and the estimate collapse to the same number, and the gate tests -// stop being able to distinguish them. -function latestBlock() { +// stop being able to distinguish them. opts.baseFeeWei replaces its base fee, +// read at request time like every other fixture switch. +function latestBlock(opts) { return { hash: "0x" + "11".repeat(32), parentHash: "0x" + "22".repeat(32), @@ -239,7 +240,7 @@ function latestBlock() { gasUsed: "0xf4240", miner: STUB_COUNTERPARTY, extraData: "0x", - baseFeePerGas: hex(BASE_FEE_WEI), + baseFeePerGas: opts.baseFeeWei || hex(BASE_FEE_WEI), transactions: [], }; } @@ -481,7 +482,7 @@ function rpcReply(req, opts, report) { }); } if (req.method === "eth_getBlockByNumber") { - return Object.assign(envelope, { result: latestBlock() }); + return Object.assign(envelope, { result: latestBlock(opts) }); } // The end of the dApp transaction round trip: the raw signed transaction // the background hands to the node. It is recorded verbatim so a test can @@ -625,6 +626,8 @@ function traceEnabled(raw) { * @param {string} [opts.ethBalanceWei] hex wei answered to eth_getBalance; * defaults to zero, which is what every test that predates the funded * fixture expects. + * @param {string} [opts.baseFeeWei] hex wei answered as the latest block's + * baseFeePerGas; defaults to BASE_FEE_WEI. Read at request time. * @param {boolean} [opts.failGasEstimate] answer eth_estimateGas with a * node-side refusal. * @param {boolean} [opts.holdBalance] hold every batch containing an diff --git a/tests/e2e/run.js b/tests/e2e/run.js index 5f23f4f..898ec7b 100644 --- a/tests/e2e/run.js +++ b/tests/e2e/run.js @@ -2232,7 +2232,8 @@ function feeEth(wei) { // What the confirmation screen is showing right now, read out of the DOM in // one pass: whether sending is allowed, which reason it is giving, what the -// fee block says, how tall the whole view is, and where Sign & Send sits in it. +// fee block says, how tall the whole view is, and where the password field and +// Sign & Send sit in it. async function confirmState(page) { return page.evaluate(() => { const el = (id) => document.getElementById(id); @@ -2245,6 +2246,9 @@ async function confirmState(page) { }; return { height: el("view-confirm-tx").getBoundingClientRect().height, + passwordTop: + el("confirm-tx-password").getBoundingClientRect().top - + el("view-confirm-tx").getBoundingClientRect().top, sendTop: el("btn-confirm-send").getBoundingClientRect().top - el("view-confirm-tx").getBoundingClientRect().top, @@ -2262,6 +2266,12 @@ async function confirmState(page) { amountFeeError: shown("confirm-amount-fee-error"), gasError: shown("confirm-gas-error"), feeUnknownError: shown("confirm-fee-unknown-error"), + // Its sentence is wrapped across lines in index.html. + feeBoundError: shown("confirm-fee-bound-error") + ? el("confirm-fee-bound-error") + .textContent.replace(/\s+/g, " ") + .trim() + : "", sendDisabled: el("btn-confirm-send").disabled, }; }); @@ -3057,6 +3067,91 @@ test("a token that lies about decimals() at signing time broadcasts nothing (#30 } }); +// ------------------------- a send Sign & Send refuses moves nothing (#583) +// +// README, No Layout Shift. Sign & Send refuses a send on the confirmation +// screen when the active network changed since review and when its gas limit +// or fee is over its bound, and each refusal's sentence sits in a box that held +// its space from the first paint. This drives the total fee refusal, the longer +// of the two sentences. + +// The base fee the stub node quotes once Sign & Send is pressed. ethers asks +// twice it plus the 1 gwei priority fee per gas, 50,001 gwei, so the 21,000-gas +// send would allow a fee of about 1.05 ETH, over the 1 ETH this wallet signs +// for (MAX_TOTAL_FEE in src/shared/approvalVerify.js). +const OVER_BOUND_BASE_FEE_WEI = 25000n * 10n ** 9n; + +test("a send refused for its fee leaves the password field and Sign & Send where they were (#583)", async (env) => { + env.errors.expect("confirmTx logging the refused send", /send refused/); + try { + await goToConfirm(env.page, { + token: "ETH", + balance: FUNDED_ETH_TEXT + " ETH", + amount: COMFORTABLE_AMOUNT, + }); + await waitForEstimate(env.page); + const before = await confirmState(env.page); + assert( + !before.sendDisabled && before.feeBoundError === "", + "the screen is not ready to send: " + JSON.stringify(before), + ); + + // Only now, with the screen drawn and its fee estimated, does the + // node quote a fee over the bound. + env.routeOpts.baseFeeWei = toHexWei(OVER_BOUND_BASE_FEE_WEI); + const sent = env.routeOpts.broadcastTransactions.length; + await fillPasswordAndSend(env.page); + await env.page.waitForFunction( + () => + getComputedStyle( + document.getElementById("confirm-fee-bound-error"), + ).visibility === "visible", + null, + { timeout: 60000 }, + ); + const after = await confirmState(env.page); + + // Printed pass or fail, as the dust threshold test does. + console.log( + "# refused send: password field top " + + before.passwordTop + + " -> " + + after.passwordTop + + "px, Sign & Send top " + + before.sendTop + + " -> " + + after.sendTop + + "px", + ); + assert( + env.routeOpts.broadcastTransactions.length === sent, + "a send over the fee bound reached the RPC", + ); + assert( + after.feeBoundError === + "The gas limit or fee the RPC endpoint quoted for this" + + " transaction is more than this wallet will sign for, so" + + " it was not sent. Please go back and try again.", + "wrong fee refusal: " + JSON.stringify(after.feeBoundError), + ); + assert( + after.passwordTop === before.passwordTop, + "the password field moved " + + (after.passwordTop - before.passwordTop) + + "px when the send was refused", + ); + assert( + after.sendTop === before.sendTop, + "Sign & Send moved " + + (after.sendTop - before.sendTop) + + "px when the send was refused", + ); + } finally { + env.routeOpts.baseFeeWei = undefined; + await backToAddress(env.page); + } +}); + // ------------------------------ the wait for a receipt ending in error (#315) // // README.md (WaitTx) documents two ways the wait ends on the error screen: a diff --git a/tests/trackedTokensPerNetwork.test.js b/tests/trackedTokensPerNetwork.test.js index 3a56674..97b03b8 100644 --- a/tests/trackedTokensPerNetwork.test.js +++ b/tests/trackedTokensPerNetwork.test.js @@ -313,9 +313,9 @@ describe("Back after a switch from the Settings gear", () => { boot.node("confirm-tx-password").value = "any password"; await boot.click("btn-confirm-send"); expect(boot.visibleViews()).toEqual(["confirm-tx"]); - expect(boot.node("confirm-errors").innerHTML).toContain( - "The active network changed after this transaction was reviewed", - ); + expect( + boot.node("confirm-network-changed-error").style.visibility, + ).toBe("visible"); // Refused before the password is even tried. expect(boot.text("confirm-tx-password-error")).toBe(""); }); -- 2.54.0