refactor: move the calldata decoder to src/shared/calldataDecoder.js (closes #759)

decodeCalldata() and tokenAmountText() move out of the TxApproval view into
src/shared/calldataDecoder.js. TxApproval, TransactionDetail and the decoder
tests import them from there.

tokenSourcesByChainId() moves from the popup view helpers to
src/shared/state.js, beside currentNetwork(), so the decoder requires nothing
under src/popup/. It reads only the state singleton and the network table.

Model: opus-5-5

README.md

@@ -264,6 +264,8 @@ The shared modules, each in one line:
   messaging, windows and the lock.
 - `buildInfo.js`: the version, license, author, commit and date compiled into
   About.
+- `calldataDecoder.js`: the ERC-20 `approve()` and `transfer()` decoder, which
+  hands other calldata to `uniswap.js`. Popup only.
 - `chainSwitch.js`: the popup's network switch.
 - `chainSwitchFields.js`: the fields a network switch changes.
 - `constants.js`: `DEBUG`, the default endpoints, the ERC-20 ABI and the burn
@@ -1379,7 +1381,7 @@ approval screens show a red warning.
 ### Transaction decoding
 
 TxApproval and TransactionDetail try to decode calldata into a summary
-(`decodeCalldata()` in `src/popup/views/approval.js`). SuccessTx repeats
+(`decodeCalldata()` in `src/shared/calldataDecoder.js`). SuccessTx repeats
 TxApproval's summary. Decoding is a display aid. When it fails, the raw calldata
 is shown in full. No contract gets any treatment beyond a legible confirmation
 screen.

RULES.md

@@ -108,10 +108,7 @@ contradicts either, the originals govern.
 - [ ] All tool invocations via `make` targets, never directly
 - [ ] `make check` = `make test` + `make lint` + `make fmt-check`
 - [ ] `main` always passes `make check`
-- [ ] Branch per issue, cut from `next`. One PR per issue, based on `next`,
-      squash-merged into `next` on a passed independent review. `main` is
-      reached only by the per-milestone PR from `next`; releases are tagged from
-      `main`. `next` and `main` must stay green; feature branches may be red.
+- [ ] Feature branches for all changes, merge to main when done
 - [ ] No force-push to `main`
 - [ ] Pre-commit hook runs `make check`
 

src/popup/views/approval.js

@@ -12,9 +12,13 @@ const {
     formatFee,
     tokenLabel,
     blockscoutOffTokenNoteHtml,
-    tokenSourcesByChainId,
 } = require("./helpers");
-const { state, saveState, currentNetwork } = require("../../shared/state");
+const {
+    state,
+    saveState,
+    currentNetwork,
+    tokenSourcesByChainId,
+} = require("../../shared/state");
 const {
     networkById,
     networkByChainId,
@@ -26,21 +30,24 @@ const {
     getAddress,
     getBigInt,
     getBytes,
-    Interface,
     MaxUint256,
     toUtf8String,
 } = require("ethers");
 const { getPrice, formatUsd } = require("../../shared/prices");
-const { ERC20_ABI } = require("../../shared/constants");
 const { INVISIBLE_CHARACTERS } = require("../../shared/symbolSpoof");
 const {
     resolveTokenDecimals,
     resolveTokenSymbol,
     unknownDecimalsAmount,
 } = require("../../shared/approvalAmount");
+const {
+    decodeCalldata,
+    tokenAmountText,
+} = require("../../shared/calldataDecoder");
 // Four decimals, with the nonzero floor these screens hold: every amount this
 // view renders — the ERC-20 line, the ETH value, the max fee — and every one
-// it carries forward to the wait/success/error screens goes through it.
+// it carries forward to the wait/success/error screens goes through it. The
+// ERC-20 line gets it from tokenAmountText().
 const {
     truncateAmountNeverZero: formatTxValue,
 } = require("../../shared/amountDisplay");
@@ -54,11 +61,8 @@ const {
     typedDataChainRefusal,
 } = require("../../shared/approvalVerify");
 const txStatus = require("./txStatus");
-const uniswap = require("../../shared/uniswap");
 const { runtimeApi, sendMessage } = require("../../shared/browserApi");
 
-const erc20Iface = new Interface(ERC20_ABI);
-
 // The link opens the explorer of the network with `chainId`, or of the active
 // network when none is given. A chain id that names no network has no
 // explorer, so the address is drawn without a link.
@@ -70,141 +74,6 @@ function approvalAddressHtml(address, chainId) {
     return renderAddressHtml(address, { title, chainId });
 }
 
-// The amount line for a decoded ERC-20 call. With a known scale it is the
-// token quantity; with `decimals` null it is the base-unit integer with the
-// unknown scale stated, because formatting it with an assumed scale is what
-// showed a 5,000-token transfer as `0.0000`. `raw` is what the status screens
-// carry, `display` is what the approval screen shows.
-function tokenAmountText(rawAmount, decimals, symbol) {
-    if (decimals === null) {
-        const unknown = unknownDecimalsAmount(rawAmount);
-        return { raw: unknown, display: unknown };
-    }
-    const formatted = formatTxValue(formatUnits(rawAmount, decimals));
-    return {
-        raw: formatted,
-        display: formatted + (symbol ? " " + symbol : ""),
-    };
-}
-
-// Try to decode calldata sent from `fromAddress` to `toAddress` on the network
-// with chain id `chainId`, using known ABIs.
-// Returns { name, description, details } or null.
-function decodeCalldata(data, toAddress, chainId, fromAddress) {
-    if (!data || data === "0x" || data.length < 10) return null;
-
-    // Where a token's name and scale are looked for, for every decoder below:
-    // what the wallet knows of the tokens on the network with `chainId`, which
-    // need not be the active one. The ERC-20 amount line and the swap's Amount
-    // and Min. received lines resolve the scale the same way, and refuse to
-    // format the same way when it is nowhere.
-    const decimalsSources = tokenSourcesByChainId(chainId);
-
-    // Try ERC-20 (approve / transfer)
-    try {
-        const parsed = erc20Iface.parseTransaction({ data });
-        if (parsed) {
-            const tokenSymbol = resolveTokenSymbol(toAddress, decimalsSources);
-            // null when no source knows this token's scale. It is not
-            // defaulted to 18: an amount formatted with a guessed scale is
-            // the wrong number, and for a token with fewer decimals than the
-            // guess it is the wrong number in the direction that reads as
-            // zero. See tokenAmountText().
-            const tokenDecimals = resolveTokenDecimals(
-                toAddress,
-                decimalsSources,
-            );
-            const contractLabel = tokenSymbol
-                ? tokenSymbol + " (" + toAddress + ")"
-                : toAddress;
-
-            if (parsed.name === "approve") {
-                const spender = parsed.args[0];
-                const rawAmount = parsed.args[1];
-                const maxUint = BigInt(
-                    "0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff",
-                );
-                const isUnlimited = rawAmount === maxUint;
-                // An unbounded allowance needs no scale to describe, so it is
-                // still named rather than refused.
-                const amount = isUnlimited
-                    ? { raw: "Unlimited", display: "Unlimited" }
-                    : tokenAmountText(rawAmount, tokenDecimals, tokenSymbol);
-
-                return {
-                    name: "Token Approval",
-                    description: tokenSymbol
-                        ? "Approve spending of your " + tokenSymbol
-                        : "Approve spending of an ERC-20 token",
-                    details: [
-                        {
-                            label: "Token",
-                            value: contractLabel,
-                            address: toAddress,
-                            isToken: true,
-                        },
-                        {
-                            label: "Spender",
-                            value: spender,
-                            address: spender,
-                        },
-                        {
-                            label: "Amount",
-                            value: amount.display,
-                            rawValue: amount.raw,
-                        },
-                    ],
-                };
-            }
-
-            if (parsed.name === "transfer") {
-                const to = parsed.args[0];
-                const rawAmount = parsed.args[1];
-                const amount = tokenAmountText(
-                    rawAmount,
-                    tokenDecimals,
-                    tokenSymbol,
-                );
-
-                return {
-                    name: "Token Transfer",
-                    description: tokenSymbol
-                        ? "Transfer " + tokenSymbol
-                        : "Transfer ERC-20 token",
-                    details: [
-                        {
-                            label: "Token",
-                            value: contractLabel,
-                            address: toAddress,
-                            isToken: true,
-                        },
-                        { label: "Recipient", value: to, address: to },
-                        {
-                            label: "Amount",
-                            value: amount.display,
-                            rawValue: amount.raw,
-                        },
-                    ],
-                };
-            }
-        }
-    } catch {
-        // Not ERC-20 — fall through
-    }
-
-    // Try Uniswap Universal Router
-    const routerResult = uniswap.decode(
-        data,
-        toAddress,
-        chainId,
-        decimalsSources,
-        fromAddress,
-    );
-    if (routerResult) return routerResult;
-
-    return null;
-}
-
 function showPhishingWarning(elementId, isPhishing) {
     const el = $(elementId);
     if (!el) return;
@@ -1298,7 +1167,6 @@ function init(_ctx) {
 module.exports = {
     init,
     show,
-    decodeCalldata,
     formatTypedDataHtml,
     typedDataRefusal,
 };

src/popup/views/helpers.js

@@ -31,7 +31,12 @@ const { escapeHtml } = require("../../shared/html");
 const { isDebug } = require("../../shared/log");
 const { networkByChainId } = require("../../shared/networks");
 const { formatUsd, getPrice } = require("../../shared/prices");
-const { state, saveState, currentNetwork } = require("../../shared/state");
+const {
+    state,
+    saveState,
+    currentNetwork,
+    tokenSourcesByChainId,
+} = require("../../shared/state");
 const { displaySymbol } = require("../../shared/symbolDisplay");
 const { isSpoofedSymbol } = require("../../shared/symbolSpoof");
 const { walletHasRecoveryPhrase } = require("../../shared/wallet");
@@ -305,32 +310,6 @@ function nativeCurrency() {
     return currentNetwork().nativeCurrency;
 }
 
-// What the wallet knows of the tokens on the network with this chain id, or
-// on the active network when no chain id is given, as the `sources` that
-// resolveTokenSymbol() and resolveTokenDecimals() take. A contract address
-// names a token on one network only, and every token balance held is the
-// active network's, so another network gets the tokens tracked on it
-// (state.networkTrackedTokens, see applyChainSwitchFields()) and no balances.
-//
-// The wallets' balances hold what Blockscout reported for each token. So the
-// wallets are returned only while "Use Blockscout" is on.
-function tokenSourcesByChainId(chainId) {
-    const chainIdOrActive = chainId || currentNetwork().chainId;
-    const net = networkByChainId(chainIdOrActive);
-    if (net && net.id === state.networkId) {
-        return {
-            chainId: chainIdOrActive,
-            trackedTokens: state.trackedTokens,
-            wallets: state.useBlockscout ? state.wallets : [],
-        };
-    }
-    return {
-        chainId: chainIdOrActive,
-        trackedTokens: (net && state.networkTrackedTokens[net.id]) || [],
-        wallets: [],
-    };
-}
-
 // The line shown under a token while "Use Blockscout" is off. The token gets
 // the line only when the bundled list and the tracked tokens lack its name or
 // decimals. Otherwise this returns "". The token is a contract address on the
@@ -866,7 +845,6 @@ module.exports = {
     keyMaterialName,
     unknownableAmount,
     nativeCurrency,
-    tokenSourcesByChainId,
     blockscoutOffTokenNoteHtml,
     tokenLabel,
     formatFee,

src/popup/views/send.js

@@ -10,9 +10,13 @@ const {
     renderAddressHtml,
     attachCopyHandlers,
     goBack,
-    tokenSourcesByChainId,
 } = require("./helpers");
-const { state, currentAddress, currentNetwork } = require("../../shared/state");
+const {
+    state,
+    currentAddress,
+    currentNetwork,
+    tokenSourcesByChainId,
+} = require("../../shared/state");
 let ctx;
 const { getProvider } = require("../../shared/balances");
 const { resolveTokenDecimals } = require("../../shared/approvalAmount");

src/popup/views/transactionDetail.js

@@ -27,7 +27,7 @@ const { nativeCurrencyByChainId } = require("../../shared/networks");
 const { formatEther, formatUnits } = require("ethers");
 const { log, debugFetch } = require("../../shared/log");
 const { storedSettingIsOn } = require("../../shared/storedSettings");
-const { decodeCalldata } = require("./approval");
+const { decodeCalldata } = require("../../shared/calldataDecoder");
 
 /**
  * Determine a human-readable transaction type string from tx fields.

src/shared/amountDisplay.js

@@ -2,7 +2,7 @@
 // the one exception to it, in one place. Three call sites had grown their own
 // copy of the truncation — the history and balance lists
 // (`src/shared/transactions.js`), the approval screen's ERC-20 amount line
-// (`src/popup/views/approval.js`) and its Uniswap swap detail lines
+// (`src/shared/calldataDecoder.js`) and its Uniswap swap detail lines
 // (`src/shared/uniswap.js`) — and a fix applied to one of them left the other
 // two showing a different number for the same value.
 //

src/shared/calldataDecoder.js

@@ -0,0 +1,155 @@
+// Decode a transaction's calldata into the summary that TxApproval and
+// TransactionDetail show: ERC-20 `approve` and `transfer` calls here, and
+// Uniswap Universal Router calls through uniswap.js. The summary is data; the
+// views turn its addresses into HTML.
+
+const { formatUnits, Interface } = require("ethers");
+const { ERC20_ABI } = require("./constants");
+const { tokenSourcesByChainId } = require("./state");
+const {
+    resolveTokenDecimals,
+    resolveTokenSymbol,
+    unknownDecimalsAmount,
+} = require("./approvalAmount");
+// Four decimals, with the nonzero floor the approval screens hold.
+const { truncateAmountNeverZero } = require("./amountDisplay");
+const uniswap = require("./uniswap");
+
+const erc20Iface = new Interface(ERC20_ABI);
+
+// The amount line for a decoded ERC-20 call. With a known scale it is the
+// token quantity; with `decimals` null it is the base-unit integer with the
+// unknown scale stated, because formatting it with an assumed scale is what
+// showed a 5,000-token transfer as `0.0000`. `raw` is what the status screens
+// carry, `display` is what the approval screen shows.
+function tokenAmountText(rawAmount, decimals, symbol) {
+    if (decimals === null) {
+        const unknown = unknownDecimalsAmount(rawAmount);
+        return { raw: unknown, display: unknown };
+    }
+    const formatted = truncateAmountNeverZero(formatUnits(rawAmount, decimals));
+    return {
+        raw: formatted,
+        display: formatted + (symbol ? " " + symbol : ""),
+    };
+}
+
+// Try to decode calldata sent from `fromAddress` to `toAddress` on the network
+// with chain id `chainId`, using known ABIs.
+// Returns { name, description, details } or null.
+function decodeCalldata(data, toAddress, chainId, fromAddress) {
+    if (!data || data === "0x" || data.length < 10) return null;
+
+    // Where a token's name and scale are looked for, for every decoder below:
+    // what the wallet knows of the tokens on the network with `chainId`, which
+    // need not be the active one. The ERC-20 amount line and the swap's Amount
+    // and Min. received lines resolve the scale the same way, and refuse to
+    // format the same way when it is nowhere.
+    const decimalsSources = tokenSourcesByChainId(chainId);
+
+    // Try ERC-20 (approve / transfer)
+    try {
+        const parsed = erc20Iface.parseTransaction({ data });
+        if (parsed) {
+            const tokenSymbol = resolveTokenSymbol(toAddress, decimalsSources);
+            // null when no source knows this token's scale. It is not
+            // defaulted to 18: an amount formatted with a guessed scale is
+            // the wrong number, and for a token with fewer decimals than the
+            // guess it is the wrong number in the direction that reads as
+            // zero. See tokenAmountText().
+            const tokenDecimals = resolveTokenDecimals(
+                toAddress,
+                decimalsSources,
+            );
+            const contractLabel = tokenSymbol
+                ? tokenSymbol + " (" + toAddress + ")"
+                : toAddress;
+
+            if (parsed.name === "approve") {
+                const spender = parsed.args[0];
+                const rawAmount = parsed.args[1];
+                const maxUint = BigInt(
+                    "0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff",
+                );
+                const isUnlimited = rawAmount === maxUint;
+                // An unbounded allowance needs no scale to describe, so it is
+                // still named rather than refused.
+                const amount = isUnlimited
+                    ? { raw: "Unlimited", display: "Unlimited" }
+                    : tokenAmountText(rawAmount, tokenDecimals, tokenSymbol);
+
+                return {
+                    name: "Token Approval",
+                    description: tokenSymbol
+                        ? "Approve spending of your " + tokenSymbol
+                        : "Approve spending of an ERC-20 token",
+                    details: [
+                        {
+                            label: "Token",
+                            value: contractLabel,
+                            address: toAddress,
+                            isToken: true,
+                        },
+                        {
+                            label: "Spender",
+                            value: spender,
+                            address: spender,
+                        },
+                        {
+                            label: "Amount",
+                            value: amount.display,
+                            rawValue: amount.raw,
+                        },
+                    ],
+                };
+            }
+
+            if (parsed.name === "transfer") {
+                const to = parsed.args[0];
+                const rawAmount = parsed.args[1];
+                const amount = tokenAmountText(
+                    rawAmount,
+                    tokenDecimals,
+                    tokenSymbol,
+                );
+
+                return {
+                    name: "Token Transfer",
+                    description: tokenSymbol
+                        ? "Transfer " + tokenSymbol
+                        : "Transfer ERC-20 token",
+                    details: [
+                        {
+                            label: "Token",
+                            value: contractLabel,
+                            address: toAddress,
+                            isToken: true,
+                        },
+                        { label: "Recipient", value: to, address: to },
+                        {
+                            label: "Amount",
+                            value: amount.display,
+                            rawValue: amount.raw,
+                        },
+                    ],
+                };
+            }
+        }
+    } catch {
+        // Not ERC-20 — fall through
+    }
+
+    // Try Uniswap Universal Router
+    const routerResult = uniswap.decode(
+        data,
+        toAddress,
+        chainId,
+        decimalsSources,
+        fromAddress,
+    );
+    if (routerResult) return routerResult;
+
+    return null;
+}
+
+module.exports = { decodeCalldata, tokenAmountText };

src/shared/state.js

@@ -22,7 +22,7 @@
 // same thing in the editor is fast feedback in front of the build, not the
 // guarantee.
 
-const { networkById } = require("./networks");
+const { networkById, networkByChainId } = require("./networks");
 const { copyChainSwitchFields } = require("./chainSwitchFields");
 const {
     DEFAULT_STATE,
@@ -125,6 +125,32 @@ function currentNetwork() {
     return networkById(state.networkId);
 }
 
+// What the wallet knows of the tokens on the network with this chain id, or
+// on the active network when no chain id is given, as the `sources` that
+// resolveTokenSymbol() and resolveTokenDecimals() take. A contract address
+// names a token on one network only, and every token balance held is the
+// active network's, so another network gets the tokens tracked on it
+// (state.networkTrackedTokens, see applyChainSwitchFields()) and no balances.
+//
+// The wallets' balances hold what Blockscout reported for each token. So the
+// wallets are returned only while "Use Blockscout" is on.
+function tokenSourcesByChainId(chainId) {
+    const chainIdOrActive = chainId || currentNetwork().chainId;
+    const net = networkByChainId(chainIdOrActive);
+    if (net && net.id === state.networkId) {
+        return {
+            chainId: chainIdOrActive,
+            trackedTokens: state.trackedTokens,
+            wallets: state.useBlockscout ? state.wallets : [],
+        };
+    }
+    return {
+        chainId: chainIdOrActive,
+        trackedTokens: (net && state.networkTrackedTokens[net.id]) || [],
+        wallets: [],
+    };
+}
+
 // `baseline` holds the persisted fields as this page held them when its last
 // loadState() finished, or when its last successful saveState() began.
 // takeStoredNetwork(), takeStoredTrackedTokens() and
@@ -820,5 +846,6 @@ module.exports = {
     loadState,
     currentAddress,
     currentNetwork,
+    tokenSourcesByChainId,
     StateNotLoadedError,
 };

tests/approvalAmount.test.js

@@ -23,7 +23,7 @@ const {
     resolveTokenDecimals,
     unknownDecimalsAmount,
 } = require("../src/shared/approvalAmount");
-const { decodeCalldata } = require("../src/popup/views/approval");
+const { decodeCalldata } = require("../src/shared/calldataDecoder");
 
 const iface = new Interface(ERC20_ABI);
 

tests/approvalDisplayFloor.test.js

@@ -13,7 +13,7 @@
 // asserted on `rawValue` alongside the displayed line.
 //
 // Both amount paths of that screen are covered: the ERC-20 line decoded by
-// `src/popup/views/approval.js`, and the swap's `Amount` and `Min. received`
+// `src/shared/calldataDecoder.js`, and the swap's `Amount` and `Min. received`
 // lines decoded by `src/shared/uniswap.js`.
 
 globalThis.chrome = {
@@ -23,7 +23,7 @@ globalThis.chrome = {
 const { AbiCoder, Interface } = require("ethers");
 const { ERC20_ABI } = require("../src/shared/constants");
 const { state } = require("../src/shared/state");
-const { decodeCalldata } = require("../src/popup/views/approval");
+const { decodeCalldata } = require("../src/shared/calldataDecoder");
 const uniswap = require("../src/shared/uniswap");
 const {
     truncateAmount,

tests/fabricatedDecimals.test.js

@@ -47,7 +47,7 @@ const { fetchTokenBalances } = require("../src/shared/balances");
 const { debugFetch } = require("../src/shared/log");
 const { state } = require("../src/shared/state");
 const { unknownDecimalsAmount } = require("../src/shared/approvalAmount");
-const { decodeCalldata } = require("../src/popup/views/approval");
+const { decodeCalldata } = require("../src/shared/calldataDecoder");
 const { TOKEN_BY_ADDRESS } = require("../src/shared/tokenList");
 
 const HOLDER = "0x" + "a".repeat(40);

tests/resolveTokenSymbol.test.js

@@ -22,7 +22,7 @@ const { Interface } = require("ethers");
 const { ERC20_ABI } = require("../src/shared/constants");
 const { state } = require("../src/shared/state");
 const { resolveTokenSymbol } = require("../src/shared/approvalAmount");
-const { decodeCalldata } = require("../src/popup/views/approval");
+const { decodeCalldata } = require("../src/shared/calldataDecoder");
 
 const iface = new Interface(ERC20_ABI);
 

tests/uniswapRouterAddress.test.js

@@ -47,7 +47,7 @@ globalThis.chrome = {
 const { state } = require("../src/shared/state");
 const { fetchRecentTransactions } = require("../src/shared/transactions");
 const approval = require("../src/popup/views/approval");
-const { decodeCalldata } = approval;
+const { decodeCalldata } = require("../src/shared/calldataDecoder");
 const transactionDetail = require("../src/popup/views/transactionDetail");
 
 const MAINNET = "0x1";

tests/uniswapUnknownDecimals.test.js

@@ -18,7 +18,7 @@ globalThis.chrome = {
 const { AbiCoder, Interface } = require("ethers");
 const { state } = require("../src/shared/state");
 const { unknownDecimalsAmount } = require("../src/shared/approvalAmount");
-const { decodeCalldata } = require("../src/popup/views/approval");
+const { decodeCalldata } = require("../src/shared/calldataDecoder");
 
 const ROUTER = "0x66a9893cc07d91d95644aedd05d03f95e1dba8af";
 const RECIPIENT = "0xC0FfEE0000000000000000000000000000c0fFEe";