From fde8d1cdf040ec0f709b98463b9b877925ecfe57 Mon Sep 17 00:00:00 2001 From: sneak Date: Sun, 18 Oct 2026 22:33:53 +0000 Subject: [PATCH] fix: the confirmation screen's balance read and fee estimate stay on its network The fee estimate starts once the balance is read, and both it and the balance line took the active network at that moment, so a network switched in Settings while the read was in flight put a mainnet balance under a SepoliaETH label and estimated its fee on Sepolia. show() now builds the node of the network it is drawn for once and passes it to the balance read, the fee estimate and the recipient check, and every native symbol on the screen comes from the chain id it is drawn for. Model: opus-5-5 --- README.md | 20 ++++++---- src/popup/views/confirmTx.js | 60 ++++++++++++++++------------ tests/confirmTxNetworkSwitch.test.js | 44 ++++++++++++++++++-- 3 files changed, 87 insertions(+), 37 deletions(-) diff --git a/README.md b/README.md index f6f3f02..47e460c 100644 --- a/README.md +++ b/README.md @@ -1013,8 +1013,10 @@ rendered differently on two screens. The native token's label is a network's `nativeCurrency` in `src/shared/networks.js`: `ETH` on mainnet, `SepoliaETH` on Sepolia. The -wallet's balances and the Send and confirmation screens, which send on the -active network, use the active network's. A transaction's figures use the one of +wallet's balances and the Send screen, which sends on the active network, use +the active network's. The confirmation screen uses the one of the network it is +drawn for, the network active when it was shown, even after the network is +switched in Settings while it is open. A transaction's figures use the one of the network its chain id names, whichever network is active: the value and fee on the approval screen, the amount on the wait, success and error screens, the transaction history and the transaction detail screen, and the refusal of a fee @@ -1728,12 +1730,14 @@ view would leave a wallet one click from deletion. failed. The amount check uses the same balances: the ETH balance, and for a token transfer the token balance as well - Network fee: "Estimating..." then two lines, or "Unable to estimate", - fetched async once the balance is read. The first line is what the - transfer is expected to cost, `gasLimit * gasPrice` (USD in parentheses); - the second is the `gasLimit * maxFeePerGas` reserve the node requires, - which is what the balance check gates on. The second line is omitted on a - network with no type-2 pricing, where the two are the same number, but its - space is reserved either way + fetched async once the balance is read, from the same RPC endpoint as the + balance, even if the network is switched in Settings while the balance is + being read. The first line is what the transfer is expected to cost, + `gasLimit * gasPrice` (USD in parentheses); the second is the + `gasLimit * maxFeePerGas` reserve the node requires, which is what the + balance check gates on. The second line is omitted on a network with no + type-2 pricing, where the two are the same number, but its space is + reserved either way - Warnings: inline warnings from the local checks (scam address, self-send) plus four reserved warning boxes made visible by the async checks — recipient with no transaction history, recipient is a contract, burn diff --git a/src/popup/views/confirmTx.js b/src/popup/views/confirmTx.js index d2f4e06..b76a660 100644 --- a/src/popup/views/confirmTx.js +++ b/src/popup/views/confirmTx.js @@ -17,7 +17,6 @@ const { addressTitle, escapeHtml, displaySymbol, - nativeCurrency, renderAddressHtml, blockieHtml, attachCopyHandlers, @@ -26,6 +25,7 @@ const { formatFee, } = require("./helpers"); const { state, currentNetwork } = require("../../shared/state"); +const { nativeCurrencyByChainId } = require("../../shared/networks"); const { getSignerForAddress } = require("../../shared/wallet"); const { walletDefect } = require("../../shared/walletDefects"); const { decryptWithPassword } = require("../../shared/vault"); @@ -102,7 +102,9 @@ function valueWithUsd(text, usdAmount) { // once its amount is re-derived from the fee estimate. function renderAmount(txInfo) { const isErc20 = txInfo.token !== "ETH"; - const rawSymbol = isErc20 ? txInfo.tokenSymbol || "?" : nativeCurrency(); + const rawSymbol = isErc20 + ? txInfo.tokenSymbol || "?" + : nativeCurrencyByChainId(txInfo.chainId); const price = isErc20 ? getPrice(rawSymbol) : getPrice("ETH"); const amountUsd = price ? parseFloat(txInfo.amount) * price : null; $("confirm-amount").textContent = valueWithUsd( @@ -115,7 +117,9 @@ function renderAmount(txInfo) { // returns, then the balance it read. function renderBalance(txInfo) { const isErc20 = txInfo.token !== "ETH"; - const rawSymbol = isErc20 ? txInfo.tokenSymbol || "?" : nativeCurrency(); + const rawSymbol = isErc20 + ? txInfo.tokenSymbol || "?" + : nativeCurrencyByChainId(txInfo.chainId); const symbol = displaySymbol(rawSymbol); if (balances === null) { $("confirm-balance").textContent = "Loading..."; @@ -144,15 +148,21 @@ function show(txInfo) { maxSendFees = null; balances = null; // The network this screen is drawn for, and the only one it sends on: - // Settings can switch the active network while the screen is open. The - // wait, success and error screens label the amount by it too. + // Settings can switch the active network while the screen is open. This + // screen and the wait, success and error screens label amounts by it. txInfo.chainId = currentNetwork().chainId; + // That network's node, read here once and passed to every read below, so + // a read still in flight when the network is switched, and the fee + // estimate that follows the balance read, stay on it. + const provider = getProvider(state.rpcUrl, state.networkId); const isErc20 = txInfo.token !== "ETH"; // The raw symbol is the price-table key; the capped one is what the // screen says. Truncating before the lookup would silently drop the // price of any token whose symbol is long enough to be capped. - const rawSymbol = isErc20 ? txInfo.tokenSymbol || "?" : nativeCurrency(); + const rawSymbol = isErc20 + ? txInfo.tokenSymbol || "?" + : nativeCurrencyByChainId(txInfo.chainId); const symbol = displaySymbol(rawSymbol); // Transaction type @@ -228,15 +238,15 @@ function show(txInfo) { $("confirm-gas-error").classList.toggle("hidden", !isErc20); $("confirm-gas-error").textContent = "You do not have enough " + - nativeCurrency() + + nativeCurrencyByChainId(txInfo.chainId) + " to pay the network fee for this transfer. Please add " + - nativeCurrency() + + nativeCurrencyByChainId(txInfo.chainId) + " to this address and try again."; // Shown later, once checkRecipientHistory() finds a contract. $("confirm-contract-warning").textContent = "WARNING: The recipient is a smart contract. Sending " + - nativeCurrency() + + nativeCurrencyByChainId(txInfo.chainId) + " or tokens directly to a contract may result in permanent loss of" + " funds."; @@ -280,18 +290,17 @@ function show(txInfo) { // The fee is estimated once the balances are read, because a max ETH // send's amount is the ETH balance minus the estimate's reserve. - readBalances(txInfo).then(() => estimateGas(txInfo)); - checkRecipientHistory(txInfo); + readBalances(txInfo, provider).then(() => estimateGas(txInfo, provider)); + checkRecipientHistory(txInfo, provider); } -// Read the From address's balances from the node of the network this screen -// is drawn for, the node the fee estimate asks: ETH, and for a token send the -// token, at the scale the screen renders the amount with. A token whose scale -// is unknown has no balance to read. Never rejects. -async function readBalances(txInfo) { +// Read the From address's balances from `provider`, the node of the network +// this screen is drawn for: ETH, and for a token send the token, at the scale +// the screen renders the amount with. A token whose scale is unknown has no +// balance to read. Never rejects. +async function readBalances(txInfo, provider) { let read; try { - const provider = getProvider(state.rpcUrl, state.networkId); const decimals = toDecimals(txInfo.tokenDecimals); const readToken = txInfo.token !== "ETH" && decimals !== null; const [ethWei, tokenUnits] = await Promise.all([ @@ -325,7 +334,7 @@ function renderValidation(txInfo) { const isErc20 = txInfo.token !== "ETH"; const symbol = isErc20 ? displaySymbol(txInfo.tokenSymbol || "?") - : nativeCurrency(); + : nativeCurrencyByChainId(txInfo.chainId); // Null while the balances are being read and after a read that failed. // validateTransfer() takes a missing balance as nothing to spend from, so @@ -428,9 +437,10 @@ function setVisible(id, visible) { $(id).style.visibility = visible ? "visible" : "hidden"; } -async function estimateGas(txInfo) { +// Estimate the network fee on `provider`, the node of the network this screen +// is drawn for. +async function estimateGas(txInfo, provider) { try { - const provider = getProvider(state.rpcUrl, state.networkId); const feeData = await provider.getFeeData(); let gasLimit; @@ -478,12 +488,13 @@ async function estimateGas(txInfo) { // does, so the same fee reads the same on both. if (estimateWei !== null && estimateWei < gasCostWei) { $("confirm-fee-amount").textContent = - "~" + formatFee(estimateWei, nativeCurrency()); + "~" + + formatFee(estimateWei, nativeCurrencyByChainId(txInfo.chainId)); $("confirm-fee-reserve").textContent = "up to " + truncateAmountNeverZero(formatEther(gasCostWei)) + " " + - nativeCurrency() + + nativeCurrencyByChainId(txInfo.chainId) + " reserved"; setVisible("confirm-fee-reserve", true); } else { @@ -492,7 +503,7 @@ async function estimateGas(txInfo) { // cost is not below the reserve. Show the reserve alone. $("confirm-fee-amount").textContent = formatFee( gasCostWei, - nativeCurrency(), + nativeCurrencyByChainId(txInfo.chainId), ); setVisible("confirm-fee-reserve", false); } @@ -580,9 +591,8 @@ function showSendError(message) { el.style.visibility = "visible"; } -async function checkRecipientHistory(txInfo) { +async function checkRecipientHistory(txInfo, provider) { try { - const provider = getProvider(state.rpcUrl, state.networkId); const asyncWarnings = await getFullWarnings(txInfo.to, provider, { fromAddress: txInfo.from, }); diff --git a/tests/confirmTxNetworkSwitch.test.js b/tests/confirmTxNetworkSwitch.test.js index 3a670b1..4901df1 100644 --- a/tests/confirmTxNetworkSwitch.test.js +++ b/tests/confirmTxNetworkSwitch.test.js @@ -11,7 +11,8 @@ // (https://git.eeqj.de/sneak/AutistMask/issues/558). The popup can reopen onto // a confirmation screen saved before the network was switched in another // popup, and the screen, drawn again for the new network, showed and checked -// the balance Send had read on the old one. +// the balance Send had read on the old one. The balance read and the fee +// estimate after it stay on that network when it is switched while they run. // // Driven through the real confirmation screen, signer and chain switch, with // only the node and the DOM stubbed. The stub node is on the network its @@ -21,9 +22,15 @@ "use strict"; // The balances the stub node reports on each network, by network name: ETH -// in wei and the token in its base units. And the signed transactions it was -// handed, with its network's chain id. -const mockNode = { balanceWei: {}, tokenUnits: {}, broadcast: [] }; +// in wei and the token in its base units. The networks whose node was asked +// for the fee, in order. And the signed transactions it was handed, with its +// network's chain id. +const mockNode = { + balanceWei: {}, + tokenUnits: {}, + feeAskedOn: [], + broadcast: [], +}; jest.mock("ethers", () => { const actual = jest.requireActual("ethers"); @@ -51,6 +58,7 @@ jest.mock("ethers", () => { ); } async getFeeData() { + mockNode.feeAskedOn.push(this.network.name); return { maxFeePerGas: 20000000000n, maxPriorityFeePerGas: 1000000000n, @@ -188,6 +196,7 @@ beforeEach(() => { sepolia: parseEther("5"), }; mockNode.tokenUnits = { mainnet: 0n, sepolia: 0n }; + mockNode.feeAskedOn = []; mockNode.broadcast = []; txStatus.showWait.mockClear(); txStatus.showError.mockClear(); @@ -417,6 +426,33 @@ describe("the balance read", () => { expect(sendDisabled()).toBe(false); }); + // The fee is estimated once the read returns, so a node chosen then would + // be the new network's. + test("still in flight when Sepolia is chosen in Settings leaves the balance and the fee on mainnet", async () => { + let answer; + const getBalance = jest + .spyOn(JsonRpcProvider.prototype, "getBalance") + .mockImplementationOnce(() => new Promise((r) => (answer = r))); + try { + await review("mainnet"); + } finally { + getBalance.mockRestore(); + } + expect(mockNode.feeAskedOn).toEqual([]); + + await onChainSwitch("sepolia"); + answer(parseEther("5")); + await settle(); + + expect(mockNode.feeAskedOn).toEqual(["mainnet"]); + expect(el("confirm-balance").textContent).toBe("5.0000 ETH"); + // 21000 gas at 10 gwei expected, at 20 gwei reserved. + expect(el("confirm-fee-amount").textContent).toBe("~0.0002 ETH"); + expect(el("confirm-fee-reserve").textContent).toBe( + "up to 0.0004 ETH reserved", + ); + }); + test("that fails states the balance as unknown, not zero, and blocks sending", async () => { const getBalance = jest .spyOn(JsonRpcProvider.prototype, "getBalance")