feat: remove an address from an HD wallet, behind a confirmation (closes #162)

Address rows on Home gain an [x] control, on wallets that derive addresses from
an extended key and hold more than one, opening a DeleteAddress confirmation
screen.

Removal cannot destroy anything: the key material stays. Derivation indices are
not renumbered, so the next "+" derives the next unused index rather than
resurrecting the removed one. The confirmation states the real route back --
delete the whole wallet in Settings, which asks for the password and destroys
the stored recovery phrase, then import it again -- and notes that the scan
which follows only finds addresses with on-chain activity. The copy varies by
wallet type, since an xprv wallet has no recovery phrase.

Removing an address that holds a balance is allowed, with a warning naming no
figure; the funds are at the address on-chain and stay there either way.
Selection and active address move only when the removed address was the one
selected, and site permissions are dropped for it alone.

The state transition shares its address comparison, permission cleanup and
active-changed broadcast with the wallet-level removal.

tests/deleteAddress.test.js

@@ -0,0 +1,158 @@
+// Tests for the copy on the address-removal confirmation (issue #162).
+//
+// The screen's whole job is to warn before a destructive-looking action, so
+// the copy is the substance and is tested as such. Two things it must not
+// get wrong: what it takes to get the address back — the app refuses both
+// obvious routes — and what counts as holding something, which is any
+// ERC-20 as well as ETH, at any size, including a balance that rounds to
+// zero at the four decimals the balance lines render. The DOM behaviour
+// around them is driven against the real popup by tests/e2e/run.js.
+
+// helpers.js pulls in state.js, which reads chrome.storage.local at load.
+globalThis.chrome = {
+    storage: { local: { get: async () => ({}), set: async () => {} } },
+};
+
+const { addressHoldsFunds } = require("../src/popup/views/helpers");
+const {
+    recoveryPathText,
+    balanceWarningHtml,
+} = require("../src/popup/views/deleteAddress");
+const { prices, clearPrices } = require("../src/shared/prices");
+
+const USDC = "0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48";
+
+const EMPTY = { address: "0x1", balance: "0.0000", tokenBalances: [] };
+const ETH_ONLY = { address: "0x1", balance: "1.5", tokenBalances: [] };
+const DUST = { address: "0x1", balance: "0.00001", tokenBalances: [] };
+const TOKEN_ONLY = {
+    address: "0x1",
+    balance: "0.0000",
+    tokenBalances: [{ address: USDC, symbol: "USDC", balance: "2500.0" }],
+};
+const ZERO_TOKEN = {
+    address: "0x1",
+    balance: "0",
+    tokenBalances: [{ address: USDC, symbol: "USDC", balance: "0" }],
+};
+
+afterEach(() => {
+    clearPrices();
+});
+
+describe("what the screen says it takes to get the address back", () => {
+    // The screen used to promise the address "can be brought back at any
+    // time by importing this wallet's recovery phrase again". That import is
+    // refused as a duplicate for as long as the wallet is present, which it
+    // always is here — a wallet never gives up its last address.
+    test("it does not promise a re-import while the wallet is here", () => {
+        const text = recoveryPathText({ type: "hd" });
+        expect(text).not.toMatch(/at any time/);
+        expect(text).toContain("is refused while this wallet is still here");
+    });
+
+    test("it names deleting the whole wallet as the route back", () => {
+        expect(recoveryPathText({ type: "hd" })).toContain(
+            "delete the whole wallet in Settings",
+        );
+    });
+
+    // The scan after a re-import finds used addresses only, so an address
+    // that never saw a transaction does not come back at all. Saying so is
+    // the difference between a warning and a false reassurance.
+    test("it states the limit: only on-chain activity is found", () => {
+        const text = recoveryPathText({ type: "hd" });
+        expect(text).toContain("only finds addresses that have on-chain");
+        expect(text).toContain("never been used is not found by it");
+    });
+
+    // The screen is offered on xprv wallets too, and an xprv wallet holds no
+    // recovery phrase — telling its owner to import one would send them
+    // looking for words that do not exist.
+    test("an xprv wallet is told about its extended private key", () => {
+        const text = recoveryPathText({ type: "xprv" });
+        expect(text).toContain("extended private key");
+        expect(text).not.toContain("recovery phrase");
+    });
+
+    test("an HD wallet is told about its recovery phrase", () => {
+        const text = recoveryPathText({ type: "hd" });
+        expect(text).toContain("recovery phrase");
+        expect(text).not.toContain("extended private key");
+    });
+});
+
+describe("whether an address holds anything", () => {
+    test("ETH counts", () => {
+        expect(addressHoldsFunds(ETH_ONLY)).toBe(true);
+    });
+
+    // The case that decides the screen: no ETH at all, and $2500 of a
+    // stablecoin sitting at the address.
+    test("an ERC-20 balance counts even with no ETH", () => {
+        expect(addressHoldsFunds(TOKEN_ONLY)).toBe(true);
+    });
+
+    // 0.00001 ETH renders as "0.0000" at four decimals. It is still money.
+    test("an ETH balance below the displayed precision counts", () => {
+        expect(addressHoldsFunds(DUST)).toBe(true);
+    });
+
+    test("an address holding nothing does not", () => {
+        expect(addressHoldsFunds(EMPTY)).toBe(false);
+        expect(addressHoldsFunds(ZERO_TOKEN)).toBe(false);
+    });
+
+    test("a missing address or missing fields do not", () => {
+        expect(addressHoldsFunds(undefined)).toBe(false);
+        expect(addressHoldsFunds({ address: "0x1" })).toBe(false);
+    });
+});
+
+describe("the balance warning on the removal confirmation", () => {
+    test("an address holding nothing gets a blank line, not a warning", () => {
+        expect(balanceWarningHtml(EMPTY)).toBe(" ");
+        expect(balanceWarningHtml(ZERO_TOKEN)).toBe(" ");
+    });
+
+    test("an ERC-20-only address is warned about, and its token listed", () => {
+        const html = balanceWarningHtml(TOKEN_ONLY);
+        expect(html).toContain("This address holds a balance.");
+        expect(html).toContain("does not move or spend anything");
+        expect(html).toContain("USDC");
+        expect(html).toContain("2500.0000");
+    });
+
+    // The rendered line says 0.0000 for this address — that is the display
+    // format, shared with Home and AddressDetail — and the warning is shown
+    // all the same, because the balance is not zero.
+    test("an ETH balance that renders as 0.0000 is warned about", () => {
+        const html = balanceWarningHtml(DUST);
+        expect(html).toContain("This address holds a balance.");
+        expect(html).toContain("<span>0.0000</span>");
+    });
+
+    // The sentence must not assert an amount, because any amount it could
+    // assert has been rounded: "This address holds 0.0000 ETH." is what the
+    // rounded form produces for an address that holds real money.
+    test("the warning sentence asserts no rounded amount", () => {
+        for (const addr of [DUST, ETH_ONLY, TOKEN_ONLY]) {
+            expect(balanceWarningHtml(addr)).not.toMatch(
+                /holds [\d.]+ (ETH|USDC)/,
+            );
+        }
+    });
+
+    test("the USD total is shown when prices are known", () => {
+        prices.ETH = 2000;
+        prices.USDC = 1;
+        expect(balanceWarningHtml(TOKEN_ONLY)).toContain("Total: $2,500.00");
+        expect(balanceWarningHtml(ETH_ONLY)).toContain("Total: $3,000.00");
+    });
+
+    // getAddressValueUsd() returns null on testnet and before the first
+    // price fetch. A "Total: $0.00" there would be a lie about the holdings.
+    test("no USD total is shown when prices are not known", () => {
+        expect(balanceWarningHtml(TOKEN_ONLY)).not.toContain("Total:");
+    });
+});

tests/e2e/run.js

@@ -398,6 +398,101 @@ test("reopening the popup never lands on the phrase screen (#161)", async (env)
     assertWiped(st, env.phrase, "after reopening the popup");
 });
 
+// -------------------------------------------- address removal (#162)
+
+// Number of address rows across every wallet in the list, counted in the DOM
+// whether or not Home is the screen on top.
+function addressRowCount(page) {
+    return page.locator("#wallet-list .btn-addr-info").count();
+}
+
+function waitForAddressRows(page, n) {
+    return page.waitForFunction(
+        (want) =>
+            document.querySelectorAll("#wallet-list .btn-addr-info").length ===
+            want,
+        n,
+        { timeout: 60000 },
+    );
+}
+
+// The suite arrives here with two wallets, an HD one and a key one, holding
+// one address each.
+test("only a wallet that can spare an address offers to remove one (#162)", async (env) => {
+    await visible(env.page, "#view-main");
+    const rows = await addressRowCount(env.page);
+    assert(rows === 2, "expected two address rows, got " + rows);
+    const offered = await env.page
+        .locator("#wallet-list .btn-remove-address")
+        .count();
+    assert(
+        offered === 0,
+        "a wallet holding its last address offered to remove it",
+    );
+
+    await env.page.click("#wallet-list .btn-add-address");
+    await waitForAddressRows(env.page, 3);
+
+    // Only the HD wallet's two rows; the key wallet still holds one address.
+    const nowOffered = await env.page
+        .locator("#wallet-list .btn-remove-address")
+        .count();
+    assert(
+        nowOffered === 2,
+        "expected the HD wallet's two rows to offer removal, got " + nowOffered,
+    );
+});
+
+// The gate itself: the control opens a confirmation, and leaving that
+// confirmation by "Back" removes nothing.
+test("leaving the removal confirmation removes nothing (#162)", async (env) => {
+    await env.page.locator("#wallet-list .btn-remove-address").nth(1).click();
+    await visible(env.page, "#view-delete-address-confirm");
+
+    const label = await env.page.locator("#delete-address-label").innerText();
+    assert(
+        label === "Address 2",
+        "the confirmation names the wrong address: " + JSON.stringify(label),
+    );
+
+    // The route back is written by the view, not by index.html, so an empty
+    // paragraph here means the user is confirming with no idea what it
+    // takes to undo. This wallet is an HD one, so it is told about its
+    // recovery phrase.
+    const recovery = await env.page
+        .locator("#delete-address-recovery")
+        .innerText();
+    assert(
+        recovery.includes("delete the whole wallet in Settings") &&
+            recovery.includes("recovery phrase"),
+        "the confirmation does not state the route back: " +
+            JSON.stringify(recovery),
+    );
+
+    // "Back" re-renders Home, so a count taken after it is a real
+    // measurement of the wallet rather than a stale screen.
+    await env.page.click("#btn-delete-address-back");
+    await visible(env.page, "#view-main");
+    const rows = await addressRowCount(env.page);
+    assert(rows === 3, "the address was removed without a confirmation");
+});
+
+test("confirming removes the address and returns Home (#162)", async (env) => {
+    await env.page.locator("#wallet-list .btn-remove-address").nth(1).click();
+    await visible(env.page, "#view-delete-address-confirm");
+    await env.page.click("#btn-delete-address-confirm");
+    await visible(env.page, "#view-main");
+
+    await waitForAddressRows(env.page, 2);
+    const offered = await env.page
+        .locator("#wallet-list .btn-remove-address")
+        .count();
+    assert(
+        offered === 0,
+        "the HD wallet still offers to remove its last address",
+    );
+});
+
 // ---------------------------------------------------------------- runner
 
 async function main() {

tests/walletDelete.test.js

@@ -1,4 +1,6 @@
 const {
+    canRemoveAddress,
+    removeAddressFromState,
     removeWalletFromState,
     broadcastActiveChanged,
 } = require("../src/shared/walletDelete");
@@ -6,6 +8,7 @@ const {
 // Fixed addresses — never used for anything but these tests.
 const A0 = "0x66133E8ea0f5D1d612D2502a968757D1048c214a";
 const A1 = "0xdAC17F958D2ee523a2206206994597C13D831ec7";
+const A2 = "0x514910771AF9Ca656af840dff83E8264EcF986CA";
 const B0 = "0x2260FAC5E5542a773Aa44fBCfeDf7C193bc2C599";
 const C0 = "0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48";
 
@@ -111,6 +114,219 @@ describe("removeWalletFromState", () => {
     });
 });
 
+// An HD wallet with three addresses next to a single-address key wallet.
+// `nextIndex` is the wallet's derivation high-water mark, three addresses in.
+function makeAddressState(overrides = {}) {
+    return {
+        hasWallet: true,
+        wallets: [
+            { ...wallet("A", [A0, A1, A2]), type: "hd", nextIndex: 3 },
+            { ...wallet("B", [B0]), type: "key" },
+        ],
+        selectedWallet: 0,
+        selectedAddress: 0,
+        activeAddress: A0,
+        allowedSites: { [A0]: ["a.example"], [A1]: ["b.example"] },
+        deniedSites: { [A1]: ["d.example"], [B0]: ["e.example"] },
+        ...overrides,
+    };
+}
+
+describe("canRemoveAddress", () => {
+    test("an HD wallet with more than one address may remove one", () => {
+        expect(canRemoveAddress({ type: "hd", addresses: [{}, {}] })).toBe(
+            true,
+        );
+    });
+
+    test("an xprv wallet with more than one address may too", () => {
+        expect(canRemoveAddress({ type: "xprv", addresses: [{}, {}] })).toBe(
+            true,
+        );
+    });
+
+    // The last address is what delete-wallet is for.
+    test("a wallet holding a single address may not", () => {
+        expect(canRemoveAddress({ type: "hd", addresses: [{}] })).toBe(false);
+    });
+
+    // A key wallet holds one bare private key and cannot derive more, so it
+    // has no "+" button and gets no remove control either.
+    test("a key wallet may not, whatever its address count", () => {
+        expect(canRemoveAddress({ type: "key", addresses: [{}] })).toBe(false);
+        expect(canRemoveAddress({ type: "key", addresses: [{}, {}] })).toBe(
+            false,
+        );
+    });
+
+    test("a missing or typeless wallet may not", () => {
+        expect(canRemoveAddress(undefined)).toBe(false);
+        expect(canRemoveAddress({})).toBe(false);
+    });
+});
+
+describe("removeAddressFromState", () => {
+    test("removing a non-selected address leaves the selection where it is", () => {
+        const state = makeAddressState({
+            selectedAddress: 2,
+            activeAddress: A2,
+        });
+
+        const { removed, activeAddressChanged } = removeAddressFromState(
+            state,
+            0,
+            0,
+        );
+
+        expect(removed).toBe(true);
+        // A2 moved from index 2 to index 1 by the splice.
+        expect(state.wallets[0].addresses.map((a) => a.address)).toEqual([
+            A1,
+            A2,
+        ]);
+        expect(state.selectedWallet).toBe(0);
+        expect(state.selectedAddress).toBe(1);
+        expect(state.activeAddress).toBe(A2);
+        expect(activeAddressChanged).toBe(false);
+        // The wallet list itself is untouched.
+        expect(state.wallets).toHaveLength(2);
+        expect(state.hasWallet).toBe(true);
+    });
+
+    test("removing an address after the selection does not shift it", () => {
+        const state = makeAddressState({
+            selectedAddress: 0,
+            activeAddress: A0,
+        });
+
+        const { removed, activeAddressChanged } = removeAddressFromState(
+            state,
+            0,
+            2,
+        );
+
+        expect(removed).toBe(true);
+        expect(state.selectedAddress).toBe(0);
+        expect(state.activeAddress).toBe(A0);
+        expect(activeAddressChanged).toBe(false);
+    });
+
+    test("a selection in another wallet is untouched", () => {
+        const state = makeAddressState({
+            selectedWallet: 1,
+            selectedAddress: 0,
+            activeAddress: B0,
+        });
+
+        const { removed, activeAddressChanged } = removeAddressFromState(
+            state,
+            0,
+            1,
+        );
+
+        expect(removed).toBe(true);
+        expect(state.selectedWallet).toBe(1);
+        expect(state.selectedAddress).toBe(0);
+        expect(state.activeAddress).toBe(B0);
+        expect(activeAddressChanged).toBe(false);
+    });
+
+    test("removing the selected address falls back to the wallet's first address", () => {
+        const state = makeAddressState({
+            selectedAddress: 1,
+            activeAddress: A1,
+        });
+
+        const { removed, activeAddressChanged } = removeAddressFromState(
+            state,
+            0,
+            1,
+        );
+
+        expect(removed).toBe(true);
+        expect(state.wallets[0].addresses.map((a) => a.address)).toEqual([
+            A0,
+            A2,
+        ]);
+        expect(state.selectedWallet).toBe(0);
+        expect(state.selectedAddress).toBe(0);
+        expect(state.activeAddress).toBe(A0);
+        expect(activeAddressChanged).toBe(true);
+    });
+
+    // The active address can be persisted in a different case than the
+    // wallet's copy of it, so the comparison must not be literal.
+    test("the active address is matched case-insensitively", () => {
+        const state = makeAddressState({
+            selectedAddress: 1,
+            activeAddress: A1.toLowerCase(),
+        });
+
+        const { activeAddressChanged } = removeAddressFromState(state, 0, 1);
+
+        expect(state.activeAddress).toBe(A0);
+        expect(activeAddressChanged).toBe(true);
+    });
+
+    test("site permissions are dropped for the removed address only", () => {
+        const state = makeAddressState();
+
+        removeAddressFromState(state, 0, 1);
+
+        expect(state.allowedSites).toEqual({ [A0]: ["a.example"] });
+        expect(state.deniedSites).toEqual({ [B0]: ["e.example"] });
+    });
+
+    // The derivation counter is a high-water mark, never rewound: "+" derives
+    // a fresh index rather than re-deriving the address just removed.
+    test("the wallet's derivation counter is not rewound", () => {
+        const state = makeAddressState();
+
+        removeAddressFromState(state, 0, 1);
+
+        expect(state.wallets[0].nextIndex).toBe(3);
+    });
+
+    test("the last address of a wallet is refused, and nothing changes", () => {
+        const state = makeAddressState({
+            selectedWallet: 1,
+            selectedAddress: 0,
+            activeAddress: B0,
+        });
+
+        const { removed, activeAddressChanged } = removeAddressFromState(
+            state,
+            1,
+            0,
+        );
+
+        expect(removed).toBe(false);
+        expect(activeAddressChanged).toBe(false);
+        expect(state.wallets[1].addresses.map((a) => a.address)).toEqual([B0]);
+        expect(state.activeAddress).toBe(B0);
+        expect(state.hasWallet).toBe(true);
+    });
+
+    // The same refusal reached the other way: an HD wallet worn down to one
+    // address is no more removable than a key wallet.
+    test("an HD wallet down to its last address is refused too", () => {
+        const state = makeAddressState();
+
+        expect(removeAddressFromState(state, 0, 2).removed).toBe(true);
+        expect(removeAddressFromState(state, 0, 1).removed).toBe(true);
+        expect(removeAddressFromState(state, 0, 0).removed).toBe(false);
+        expect(state.wallets[0].addresses.map((a) => a.address)).toEqual([A0]);
+    });
+
+    test("an out-of-range address index is refused", () => {
+        const state = makeAddressState();
+
+        expect(removeAddressFromState(state, 0, 7).removed).toBe(false);
+        expect(removeAddressFromState(state, 7, 0).removed).toBe(false);
+        expect(state.wallets[0].addresses).toHaveLength(3);
+    });
+});
+
 describe("broadcastActiveChanged", () => {
     afterEach(() => {
         delete global.chrome;