fix: a confirmation screen drawn again keeps the network it was reviewed for (closes #586)

ConfirmTx set the transaction's chain id to the active network's every
time it drew the screen, so a popup reopened onto a saved confirmation
after a network switch in Settings described the reviewed transfer as
the new network's and sent it there.

show() now sets the chain id only on a transaction Send hands over. One
drawn again from the saved screen keeps its own, so Sign & Send refuses
it while another network is active. The screen labels its amounts in
that network's native currency and asks that network's RPC endpoint for
the fee and the recipient checks.

Model: opus-5-5

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

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

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.