diff --git a/README.md b/README.md index 4c8360e..b9523f4 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 network 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,12 @@ 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 and fee the RPC + endpoint fills in at signing go over the bounds `assertWithinCeilings()` + in `src/shared/approvalVerify.js` sets → nothing is signed; "This + transaction would allow a network fee of more than this wallet will sign + for, so it was not sent. Please go back and try again." in its own box, 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..84d2b00 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 +634,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 +652,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; } @@ -724,10 +718,13 @@ function init(_ctx) { 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 + // 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 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..9785c82 100644 --- a/tests/confirmTxFeeBound.test.js +++ b/tests/confirmTxFeeBound.test.js @@ -12,7 +12,7 @@ // 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 +// DOM path around it — the refusal shown in the box reserved for it — is // covered by the Chrome e2e suite. globalThis.chrome = { 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..9e6abdf 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,90 @@ 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 fee is over +// the bound, and each refusal's sentence sits in a box that held its space from +// the first paint. This drives the fee refusal, the longer of the two. + +// 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 === + "This transaction would allow a network fee of 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(""); });