diff --git a/README.md b/README.md index 5cd23cd..e8eeee4 100644 --- a/README.md +++ b/README.md @@ -1013,10 +1013,12 @@ 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 -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 +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 its +transaction was reviewed on, the network active when Send showed it, even after +the network is switched in Settings. 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 above 1 ETH. A chain id that names no network reads `ETH`. Wherever this document shows ETH as the label of a native balance, value or fee, in a "Native @@ -1781,12 +1783,16 @@ view would leave a wallet one click from deletion. the contract's own answer is read at signing time only to be compared with it, and a disagreement is a refusal rather than a preference for either value (`src/shared/transferAmount.js`) - - "Sign & Send" after the active network was switched in Settings while this - screen was open → nothing is signed; "The active network changed after + - "Sign & Send" after the active network was switched in Settings since this + transaction was reviewed, with this screen open or before the popup + reopened onto it → 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. The transaction - carries the chain id of the network the screen was shown for, so it cannot - be signed for another + carries the chain id of the network active when Send showed this screen, + and keeps it when the screen is drawn again from the saved transaction, so + it cannot be signed for another. The screen describes it on that network: + its amounts are in that network's native currency, and the fee estimate + and the recipient checks ask that network's RPC endpoint - "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/views/confirmTx.js b/src/popup/views/confirmTx.js index e26811a..96d9ebe 100644 --- a/src/popup/views/confirmTx.js +++ b/src/popup/views/confirmTx.js @@ -11,7 +11,6 @@ const { addressTitle, escapeHtml, displaySymbol, - nativeCurrency, renderAddressHtml, blockieHtml, attachCopyHandlers, @@ -20,6 +19,10 @@ const { formatFee, } = require("./helpers"); const { state, currentNetwork } = require("../../shared/state"); +const { + networkByChainId, + nativeCurrencyByChainId, +} = require("../../shared/networks"); const { getSignerForAddress } = require("../../shared/wallet"); const { walletDefect } = require("../../shared/walletDefects"); const { decryptWithPassword } = require("../../shared/vault"); @@ -87,7 +90,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( @@ -101,16 +106,21 @@ function show(txInfo) { feeStatus = FEE_PENDING; feeWei = null; maxSendFees = 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. - txInfo.chainId = currentNetwork().chainId; + // The network this screen is drawn for, and the only one it sends on: the + // one active when Send handed the transaction over. Drawn again from the + // saved transaction, as when the popup reopens onto this screen, it keeps + // that network, so Sign & Send refuses once Settings has switched to + // another. This screen and the wait, success and error screens label + // amounts by it. + if (!txInfo.chainId) txInfo.chainId = currentNetwork().chainId; 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 @@ -212,9 +222,9 @@ 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."; // Both shown later, by checkRecipientHistory(). The first sentence says @@ -222,12 +232,12 @@ function show(txInfo) { $("confirm-recipient-warning").textContent = "WARNING: The recipient address has never sent a transaction and" + " holds no " + - nativeCurrency() + + nativeCurrencyByChainId(txInfo.chainId) + ". This may indicate a fresh or unused address. Double-check the" + " address before sending."; $("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,7 +290,7 @@ function renderValidation(txInfo) { const isErc20 = txInfo.token !== "ETH"; const symbol = isErc20 ? displaySymbol(txInfo.tokenSymbol || "?") - : nativeCurrency(); + : nativeCurrencyByChainId(txInfo.chainId); const { canSend, codes } = validateTransfer({ isErc20, @@ -369,9 +379,23 @@ function setVisible(id, visible) { $(id).style.visibility = visible ? "visible" : "hidden"; } +// The node of the network this transaction was reviewed for, which need not be +// the active one: the popup can reopen onto this screen after Settings switched +// the network. Its RPC URL is the one Settings has for that network: the +// active network's is state.rpcUrl, any other's the one remembered for it (see +// applyChainSwitchFields()). +function reviewedNetworkProvider(txInfo) { + const net = networkByChainId(txInfo.chainId); + if (net.id === state.networkId) { + return getProvider(state.rpcUrl, net.id); + } + const remembered = state.networkEndpoints[net.id] || {}; + return getProvider(remembered.rpcUrl || net.defaultRpcUrl, net.id); +} + async function estimateGas(txInfo) { try { - const provider = getProvider(state.rpcUrl, state.networkId); + const provider = reviewedNetworkProvider(txInfo); const feeData = await provider.getFeeData(); let gasLimit; @@ -419,12 +443,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 { @@ -433,7 +458,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); } @@ -523,7 +548,7 @@ function showSendError(message) { async function checkRecipientHistory(txInfo) { try { - const provider = getProvider(state.rpcUrl, state.networkId); + const provider = reviewedNetworkProvider(txInfo); const asyncWarnings = await getFullWarnings(txInfo.to, provider, { fromAddress: txInfo.from, }); @@ -564,8 +589,10 @@ function init(_ctx) { return; } - // Back from Settings only unhides this screen, still drawn for the - // network that was active when it was shown. + // The screen describes the network the transaction was reviewed for, + // which Settings may have switched away from since: with this screen + // open, where Back only unhides it, or before the popup reopened onto + // it. if (currentNetwork().chainId !== pendingTx.chainId) { showSendError( "The active network changed after this transaction was" + diff --git a/tests/confirmTxNetworkSwitch.test.js b/tests/confirmTxNetworkSwitch.test.js index a612744..29d06e8 100644 --- a/tests/confirmTxNetworkSwitch.test.js +++ b/tests/confirmTxNetworkSwitch.test.js @@ -7,25 +7,36 @@ // reviewed on Sepolia went out as 1 ETH on mainnet once mainnet was chosen in // Settings in between. // +// The same holds when the popup was closed after the switch and reopened, and +// Back drew the screen again from the transaction it saved +// (https://git.eeqj.de/sneak/AutistMask/issues/586): the screen was drawn for +// the new network, and Sign & Send sent the reviewed transfer there. +// // 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 // provider was built for, so a broadcast records the network it went to. "use strict"; -// The signed transactions the stub node was handed, with its network's chain id. -const mockNode = { broadcast: [] }; +// The nodes asked for the fee, by network name and RPC URL, in order. And the +// signed transactions the stub node was handed, with its network's chain id. +const mockNode = { feeAsked: [], broadcast: [] }; jest.mock("ethers", () => { const actual = jest.requireActual("ethers"); class StubProvider { constructor(url, network) { + this.url = url; this.network = network; } async getNetwork() { return this.network; } async getFeeData() { + mockNode.feeAsked.push({ + network: this.network.name, + url: this.url, + }); return { maxFeePerGas: 20000000000n, maxPriorityFeePerGas: 1000000000n, @@ -128,6 +139,16 @@ const { encryptWithPassword } = require("../src/shared/vault"); const { state, loadState } = require("../src/shared/state"); const { onChainSwitch } = require("../src/shared/chainSwitch"); const { NETWORKS } = require("../src/shared/networks"); +const { + goBack, + pushCurrentView, + setBackRenderer, + showView, +} = require("../src/popup/views/helpers"); +const { + makeBackRenderer, + resetRenderedViews, +} = require("../src/popup/viewRouter"); const txStatus = require("../src/popup/views/txStatus"); const confirmTx = require("../src/popup/views/confirmTx"); @@ -150,6 +171,7 @@ beforeAll(async () => { beforeEach(() => { elements.clear(); + mockNode.feeAsked = []; mockNode.broadcast = []; txStatus.showWait.mockClear(); txStatus.showError.mockClear(); @@ -187,6 +209,12 @@ async function review(networkId) { tokenDecimals: null, max: false, }); + await settle(); +} + +// show() estimates the fee and checks the recipient without awaiting either; +// this lets both finish. +async function settle() { for (let i = 0; i < 10; i++) await new Promise((r) => setTimeout(r, 0)); } @@ -238,6 +266,60 @@ describe.each([ }, ); +describe.each([ + ["sepolia", "mainnet", "SepoliaETH"], + ["mainnet", "sepolia", "ETH"], +])( + "a transfer reviewed on %s, with %s chosen in Settings, the popup reopened and Back pressed", + (reviewedOn, switchedTo, symbol) => { + test("is described on the network it was reviewed for, and neither signed nor sent", async () => { + await review(reviewedOn); + // The user's own node for that network, set in Settings. + const ownNode = "https://" + reviewedOn + ".node.example.invalid"; + state.rpcUrl = ownNode; + // The Settings gear, then its network select. + pushCurrentView(); + showView("settings"); + await onChainSwitch(switchedTo); + + // The reopened popup loads what was saved onto a blank page, and + // Back draws the confirmation screen again from it. + await loadState(); + elements.clear(); + resetRenderedViews(); + setBackRenderer(makeBackRenderer(state, { confirmTx })); + confirmTx.init({}); + mockNode.feeAsked = []; + goBack(); + await settle(); + + expect(state.currentView).toBe("confirm-tx"); + expect(el("confirm-type").textContent).toBe( + "Native " + symbol + " transfer", + ); + expect(el("confirm-balance").textContent).toBe("5.0000 " + symbol); + expect(el("confirm-fee-amount").textContent).toBe( + "~0.0002 " + symbol, + ); + expect(mockNode.feeAsked).toEqual([ + { network: reviewedOn, url: ownNode }, + ]); + expect(el("btn-confirm-send").disabled).toBe(false); + + await signAndSend(); + + expect(signTransaction).not.toHaveBeenCalled(); + 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.", + ); + }); + }, +); + // The check in the click handler tells the user; the send itself carries the // chain id the screen was drawn for, so a signer on another network's node // cannot sign it even if the network changes after that check.