fix: the swap decoder reads each command's type with its router's mask (closes #535)

Each Universal Router address now carries the mask its own source reads a
command's type with: 0x1f for UniversalRouterV1, 0x3f for V1_2 and V2, 0x7f
for V2_1_1 and V2_1_2. The fixed 0x1f dropped the 0x20 bit, so
EXECUTE_SUB_PLAN (0x21) was listed as a V3 exact-out swap. A sub-plan step is
now listed as "Execute Sub-Plan (its steps are not shown)". Its commands are
not read and may spend from or pay the user wherever the step stands, so with
one the summary names no swap direction, Token In and Token Out read
"Unknown (could not be decoded)", and there is no Amount or Min. received.

Model: opus-5-5

README.md

@@ -1196,9 +1196,9 @@ The swap's `Token In` and `Token Out` lines name a currency, not an amount; each
 reads `Unknown (not named in the calldata)` when the decoder found no token for
 that side, or `Unknown (could not be decoded)` instead when a V4 swap action in
 the transaction could not be read or the transaction has an `EXECUTE_SUB_PLAN`
-step (see Transaction Decoding below). A V4 step with a swap action always sets
-the output side, and sets the input side only when no earlier step set it. It
-finds each side by three rules:
+step, which leaves both sides unknown (see Transaction Decoding below). A V4
+step with a swap action always sets the output side, and sets the input side
+only when no earlier step set it. It finds each side by three rules:
 
 1. Its input side is the currency its `SETTLE` or `SETTLE_ALL` names, or else
    the one currency that some swap action in the step spends and none buys. Its
@@ -2037,7 +2037,9 @@ view would leave a wallet one click from deletion.
 - **Elements**:
     - "Back" button, "Delete Wallet" heading
     - Warning naming the wallet and stating that deletion is permanent and any
-      funds are unrecoverable without the recovery phrase
+      funds are unrecoverable without the recovery phrase, the private key or
+      the extended private key, according to the wallet's kind
+      (`keyMaterialName()` in `src/popup/views/helpers.js`)
     - Password input
     - Error line
     - "Confirm Delete" button
@@ -2078,9 +2080,10 @@ view would leave a wallet one click from deletion.
       cannot be unlocked again, and that no password is needed to delete it
     - What deletion does and does not do: it erases the copy of the key stored
       on this device; nothing on chain changes and no money is moved
-    - The route back — adding the wallet again with the recovery phrase and a
-      new password — and, in bold, that without that phrase written down the
-      deletion loses everything the wallet holds, forever
+    - The route back — adding the wallet again, with a new password, from a kept
+      copy of its recovery phrase, private key or extended private key (named as
+      on DeleteWallet) — and, in bold, that without that copy the deletion loses
+      everything the wallet holds, forever
     - That the other wallets are not touched
     - The wallet's name, and a text input asking for it to be typed back. A name
       that shows nothing at all (only spaces, or only characters that paint
@@ -2138,8 +2141,9 @@ view would leave a wallet one click from deletion.
       the address **only if it has on-chain activity**. An address that was
       never used is not found by that scan. The text is written by
       `recoveryPathText()` rather than sitting in `index.html`, so it can name
-      the wallet's own kind of key material: an xprv wallet has no recovery
-      phrase to re-import.
+      the wallet's own kind of key material through `keyMaterialName()`, as
+      DeleteWallet and DeleteWalletLostPassword do: an xprv wallet has no
+      recovery phrase to re-import.
     - A warning when the address holds anything, ETH or any tracked ERC-20,
       followed by the holdings themselves via `balanceLinesForAddress()` and the
       USD total via `formatAddressTotal()` (see
@@ -2895,9 +2899,12 @@ Currently supported:
   `UniversalRouterV1_2_V2Support` and `UniversalRouterV2`, and `0x7f` for
   `UniversalRouterV2_1_1` and `UniversalRouterV2_1_2`. The allow-revert flag
   (`0x80`) therefore does not change a step's name. The decoder does not read
-  the commands inside an `EXECUTE_SUB_PLAN` (`0x21`) step: that step is listed
-  as `Execute Sub-Plan (its steps are not shown)`, and what those commands spend
-  or receive is missing from the other lines.
+  the commands inside an `EXECUTE_SUB_PLAN` (`0x21`) step, and wherever that
+  step stands they may spend the user's tokens or pay the user. That step is
+  listed as `Execute Sub-Plan (its steps are not shown)`, and whatever the other
+  steps state, the summary names no swap direction, `Token In` and `Token Out`
+  read `Unknown (could not be decoded)`, and there is no `Amount` or
+  `Min. received` line.
 
 ### Non-Goals Forever
 

src/popup/index.html

@@ -1123,7 +1123,8 @@
                 <p class="text-xs mb-3">
                     Deleting
                     <strong id="delete-wallet-name"></strong> is permanent. Any
-                    funds will be unrecoverable without your recovery phrase.
+                    funds will be unrecoverable without your
+                    <span id="delete-wallet-key-material"></span>.
                 </p>
                 <div class="mb-2">
                     <label class="block mb-1">Password</label>
@@ -1174,13 +1175,13 @@
                     money at its addresses is not moved or destroyed.
                 </p>
                 <p class="text-xs mb-2">
-                    If you have the recovery phrase for this wallet written
-                    down, add the wallet again afterwards with a new password
-                    and you will have it back.
+                    If you have kept a copy of this wallet's
+                    <span id="delete-wallet-lost-key-material"></span>, add the
+                    wallet again afterwards with a new password and you will
+                    have it back.
                     <strong
-                        >If you do not have it written down, deleting this
-                        wallet means losing everything it holds,
-                        forever.</strong
+                        >If you have not, deleting this wallet means losing
+                        everything it holds, forever.</strong
                     >
                 </p>
                 <p class="text-xs mb-3">Your other wallets are not touched.</p>

src/popup/views/deleteAddress.js

@@ -17,9 +17,9 @@ const {
     attachCopyHandlers,
     addressHoldsFunds,
     balanceLinesForAddress,
+    keyMaterialName,
 } = require("./helpers");
 const { formatAddressTotal, getAddressValue } = require("../../shared/prices");
-const { walletHasRecoveryPhrase } = require("../../shared/wallet");
 const { state, saveState } = require("../../shared/state");
 const {
     canRemoveAddress,
@@ -54,9 +54,7 @@ function setFlash(msg) {
 // The noun follows the wallet: an xprv wallet holds no recovery phrase, and
 // this screen is offered on xprv wallets too.
 function recoveryPathText(wallet) {
-    const secret = walletHasRecoveryPhrase(wallet)
-        ? "recovery phrase"
-        : "extended private key";
+    const secret = keyMaterialName(wallet);
     return (
         "Getting the address back into this list is not easy, so be sure. " +
         "Adding an address derives the next unused one, not this one, and " +

src/popup/views/deleteWallet.js

@@ -7,6 +7,7 @@ const {
     goBack,
     clearViewStack,
     onViewLeave,
+    keyMaterialName,
 } = require("./helpers");
 const { state, saveState } = require("../../shared/state");
 const { decryptWithPassword } = require("../../shared/vault");
@@ -75,6 +76,9 @@ function show(walletIdx) {
     clear();
     deleteWalletIndex = walletIdx;
     $("delete-wallet-name").textContent = displayName(walletIdx);
+    $("delete-wallet-key-material").textContent = keyMaterialName(
+        state.wallets[walletIdx],
+    );
     showView("delete-wallet-confirm");
 }
 
@@ -94,6 +98,9 @@ function showLostPassword() {
     clearLostPassword();
     $("delete-wallet-lost-name").textContent = name;
     $("delete-wallet-lost-name-echo").textContent = name;
+    $("delete-wallet-lost-key-material").textContent = keyMaterialName(
+        state.wallets[walletIdx],
+    );
     // showView() runs the leave hook of delete-wallet-confirm, which nulls
     // deleteWalletIndex, so this screen's own selection is recorded after
     // it and not before.

src/popup/views/helpers.js

@@ -25,6 +25,7 @@ const { isDebug } = require("../../shared/log");
 const { formatUsd, getPrice } = require("../../shared/prices");
 const { state, saveState, currentNetwork } = require("../../shared/state");
 const { displaySymbol } = require("../../shared/symbolDisplay");
+const { walletHasRecoveryPhrase } = require("../../shared/wallet");
 const { markViewRendered } = require("../viewRouter");
 
 // When views are added, removed, or transitions between them change,
@@ -418,6 +419,16 @@ function addressHoldsFunds(addr) {
     return false;
 }
 
+// The name of a wallet's key material, which is also its only backup: the
+// recovery phrase of an HD wallet, the private key of a key wallet, the
+// extended private key of an xprv wallet. DeleteWallet,
+// DeleteWalletLostPassword and DeleteAddress name the backup with it.
+function keyMaterialName(wallet) {
+    if (walletHasRecoveryPhrase(wallet)) return "recovery phrase";
+    if (wallet.type === "key") return "private key";
+    return "extended private key";
+}
+
 // The fewest characters of an address any caller may ask to display. The
 // 10-character cap inside truncateMiddle() is the other half of the same
 // guarantee; this is the half that used to be spelled out at each call
@@ -725,6 +736,7 @@ module.exports = {
     balanceLine,
     balanceLinesForAddress,
     addressHoldsFunds,
+    keyMaterialName,
     unknownableAmount,
     nativeCurrency,
     tokenLabel,

src/shared/uniswap.js

@@ -688,9 +688,8 @@ function decode(data, toAddress, chainId, sources) {
             try {
                 if (cmdId === 0x21) {
                     // EXECUTE_SUB_PLAN runs a further list of commands held in
-                    // its input, which this decoder does not read. What those
-                    // commands spend or receive is missing from every other
-                    // line, so the step says so.
+                    // its input, which this decoder does not read. The step
+                    // says so, and after this loop neither side is named.
                     hasSubPlan = true;
                     commandNames[i] += " (its steps are not shown)";
                 }
@@ -795,14 +794,28 @@ function decode(data, toAddress, chainId, sources) {
             }
         }
 
+        // The commands of an EXECUTE_SUB_PLAN step may spend the user's tokens
+        // or pay the user, before or after any other step, and a permit among
+        // them would replace the input side. What the other steps state is
+        // then not the whole swap, so both sides are set unknown, with no
+        // figure, wherever the step stands.
+        if (hasSubPlan) {
+            inputToken = null;
+            inputAmount = null;
+            outputToken = null;
+            minOutput = null;
+            outputEstablished = true;
+        }
+
         // Resolve token info. A null token on either side means no step named
-        // a currency for it, or a V4 step with a swap action left it
-        // unknown; tokenInfo() refuses rather than calling it ETH.
-        // UNWRAP_WETH turns WETH into ETH, so it makes the output ETH when the
-        // output side is WETH, or when no step set the output side. A V4 step
-        // with a swap action did set it, even when it set it unknown. Any other
-        // output keeps its own token and figure: a swap that buys USDC and
-        // then unwraps the WETH it did not spend receives USDC.
+        // a currency for it, a V4 step with a swap action left it unknown, or
+        // an EXECUTE_SUB_PLAN step did; tokenInfo() refuses rather than calling
+        // it ETH. UNWRAP_WETH turns WETH into ETH, so it makes the output ETH
+        // when the output side is WETH, or when no step set the output side. A
+        // V4 step with a swap action, or an EXECUTE_SUB_PLAN step, did set it,
+        // even when it set it unknown. Any other output keeps its own token
+        // and figure: a swap that buys USDC and then unwraps the WETH it did
+        // not spend receives USDC.
         const outputIsWeth =
             present(outputToken) &&
             WETH_ADDRESSES.includes(outputToken.toLowerCase());

tests/deleteWalletLostPassword.test.js

@@ -259,6 +259,36 @@ describe("reaching the screen", () => {
     });
 });
 
+// Both screens tell the user what they need to get the wallet back, and a
+// key or xprv wallet has no recovery phrase: telling its owner to rely on
+// one sends them looking for words that do not exist
+// (https://git.eeqj.de/sneak/AutistMask/issues/529).
+describe("what the screens say the wallet's backup is", () => {
+    test.each([
+        ["hd", "recovery phrase"],
+        ["key", "private key"],
+        ["xprv", "extended private key"],
+    ])(
+        "a wallet of type %s is told about its %s",
+        async (type, keyMaterial) => {
+            const { deleteWallet, state } = load();
+            state.wallets[1].type = type;
+
+            deleteWallet.show(1);
+            expect(state.currentView).toBe("delete-wallet-confirm");
+            expect(node("delete-wallet-key-material").textContent).toBe(
+                keyMaterial,
+            );
+
+            await click("btn-delete-wallet-lost-password");
+            expect(state.currentView).toBe(VIEW);
+            expect(node("delete-wallet-lost-key-material").textContent).toBe(
+                keyMaterial,
+            );
+        },
+    );
+});
+
 describe("the typed confirmation", () => {
     test("a name that is not the wallet's deletes nothing", async () => {
         const { deleteWallet, state, storage } = load();

tests/uniswap.test.js

@@ -109,6 +109,12 @@ function encodeV4Swap(actions, params) {
     return coder.encode(["bytes", "bytes[]"], [actions, params]);
 }
 
+// Helper: encode an EXECUTE_SUB_PLAN input (command 0x21), which holds the
+// sub-plan's own commands and inputs
+function encodeSubPlan(commands, inputs) {
+    return coder.encode(["bytes", "bytes[]"], [commands, inputs]);
+}
+
 // V4 inner action IDs, as src/shared/uniswap.js names them.
 const V4_SWAP_EXACT_IN = 0x07;
 const V4_SWAP_EXACT_IN_SINGLE_ID = 0x06;
@@ -868,7 +874,7 @@ describe("uniswap decoder", () => {
         );
     });
 
-    // https://git.eeqj.de/sneak/AutistMask/issues/535 \u2014 every command's type
+    // https://git.eeqj.de/sneak/AutistMask/issues/535 — every command's type
     // was read from its low five bits, as only UniversalRouterV1 reads it, so
     // the later routers listed 0x21 as a V3 exact-out swap, and
     // UniversalRouterV2_1_2 listed 0x40 as a V3 exact-in swap.
@@ -926,18 +932,12 @@ describe("uniswap decoder", () => {
     ])(
         "an EXECUTE_SUB_PLAN step $flag the allow-revert flag says its steps are not shown",
         ({ command }) => {
-            const subPlan = coder.encode(
-                ["bytes", "bytes[]"],
-                [
-                    "0x00",
-                    [
-                        encodeV3SwapExactIn(USER_ADDR, 2000000n, ONE_WETH, [
-                            USDT_ADDR,
-                            WETH_ADDR,
-                        ]),
-                    ],
-                ],
-            );
+            const subPlan = encodeSubPlan("0x00", [
+                encodeV3SwapExactIn(USER_ADDR, 2000000n, ONE_WETH, [
+                    USDT_ADDR,
+                    WETH_ADDR,
+                ]),
+            ]);
             const data = buildExecute(
                 solidityPacked(["uint8"], [command]),
                 [subPlan],
@@ -961,23 +961,72 @@ describe("uniswap decoder", () => {
         },
     );
 
-    // The permit names the input side; the output side is in the sub-plan.
-    test("a permit, then an EXECUTE_SUB_PLAN step, keeps the permit's token and says the rest is not shown", () => {
-        const data = buildExecute(
-            solidityPacked(["uint8", "uint8"], [0x0a, 0x21]),
-            [encodePermit2(USDT_ADDR, 1000000n, ROUTER_ADDR), "0x"],
-            9999999999n,
-        );
-        const result = uniswap.decode(data, ROUTER_ADDR, "0x1");
-        expect(detail(result, "Token In").address).toBe(USDT_ADDR);
-        expect(detail(result, "Amount").value).toBe("1.0000 USDT");
-        expect(detail(result, "Token Out").value).toBe(
-            "Unknown (could not be decoded)",
-        );
-        expect(detail(result, "Steps").value).toBe(
-            "Permit2 Permit \u2192 Execute Sub-Plan (its steps are not shown)",
-        );
-    });
+    // The steps inside a sub-plan may spend the user's tokens or pay the user,
+    // so the steps read beside it are not the whole swap, whether they come
+    // before it or after it. No side is named and no figure is given.
+    test.each([
+        {
+            // 2,000 USDT for at least 1 WETH, paid to the router; the sub-plan
+            // swaps that WETH for USDC.
+            beside: "a swap before",
+            commands: "0x0021",
+            inputs: [
+                encodeV3SwapExactIn(ROUTER_ADDR, 2000000000n, ONE_WETH, [
+                    USDT_ADDR,
+                    WETH_ADDR,
+                ]),
+                encodeSubPlan("0x00", [
+                    encodeV3SwapExactIn(USER_ADDR, ONE_WETH, 2000000000n, [
+                        WETH_ADDR,
+                        USDC_ADDR,
+                    ]),
+                ]),
+            ],
+            steps: "V3 Swap (Exact In) \u2192 Execute Sub-Plan (its steps are not shown)",
+        },
+        {
+            // The sub-plan buys WETH with the user's USDC; 1 WETH is then
+            // swapped for at least 2,000 DAI.
+            beside: "a swap after",
+            commands: "0x2100",
+            inputs: [
+                encodeSubPlan("0x00", [
+                    encodeV3SwapExactIn(ROUTER_ADDR, 2000000000n, ONE_WETH, [
+                        USDC_ADDR,
+                        WETH_ADDR,
+                    ]),
+                ]),
+                encodeV3SwapExactIn(USER_ADDR, ONE_WETH, 2000n * ONE_WETH, [
+                    WETH_ADDR,
+                    DAI_ADDR,
+                ]),
+            ],
+            steps: "Execute Sub-Plan (its steps are not shown) \u2192 V3 Swap (Exact In)",
+        },
+        {
+            // A permit among the sub-plan's steps would replace this one.
+            beside: "a permit before",
+            commands: "0x0a21",
+            inputs: [encodePermit2(USDT_ADDR, 1000000n, ROUTER_ADDR), "0x"],
+            steps: "Permit2 Permit \u2192 Execute Sub-Plan (its steps are not shown)",
+        },
+    ])(
+        "$beside an EXECUTE_SUB_PLAN step does not name either side",
+        ({ commands, inputs, steps }) => {
+            const data = buildExecute(commands, inputs, 9999999999n);
+            const result = uniswap.decode(data, ROUTER_ADDR, "0x1");
+            expect(detail(result, "Steps").value).toBe(steps);
+            expect(result.name).toBe("Uniswap Swap");
+            expect(detail(result, "Token In").value).toBe(
+                "Unknown (could not be decoded)",
+            );
+            expect(detail(result, "Amount")).toBeUndefined();
+            expect(detail(result, "Token Out").value).toBe(
+                "Unknown (could not be decoded)",
+            );
+            expect(detail(result, "Min. received")).toBeUndefined();
+        },
+    );
 
     test("shows the deadline as a UTC date and time", () => {
         const result = uniswap.decode(FIRST_SWAP_CALLDATA, ROUTER_ADDR, "0x1");