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")