`;
const isActive = state.activeAddress === addr.address;
const infoBtn = `
[info]`;
+ // Only where a wallet can spare the address: a wallet holding a
+ // single address has no remove control, because its last address
+ // is never removable.
+ const removeBtn = canRemoveAddress(wallet)
+ ? `
[x]`
+ : "";
const dot = addressDotHtml(addr.address);
const titleBold = isActive ? "font-bold" : "";
html += `
Address ${ai + 1}
`;
@@ -246,7 +253,7 @@ function render(ctx) {
}
html += `
`;
html += `${addr.ensName ? "" : dot}${addr.address}`;
- html += `${infoBtn}`;
+ html += `${infoBtn}${removeBtn}`;
html += `
`;
const addrUsd = formatUsd(getAddressValueUsd(addr));
html += `
${addrUsd || " "}
`;
@@ -289,6 +296,16 @@ function render(ctx) {
});
});
+ container.querySelectorAll(".btn-remove-address").forEach((btn) => {
+ btn.addEventListener("click", (e) => {
+ e.stopPropagation();
+ ctx.showDeleteAddress(
+ parseInt(btn.dataset.wallet, 10),
+ parseInt(btn.dataset.address, 10),
+ );
+ });
+ });
+
container.querySelectorAll(".btn-add-address").forEach((btn) => {
btn.addEventListener("click", async (e) => {
e.stopPropagation();
diff --git a/src/shared/walletDelete.js b/src/shared/walletDelete.js
index dea26ee..81bc65a 100644
--- a/src/shared/walletDelete.js
+++ b/src/shared/walletDelete.js
@@ -1,5 +1,22 @@
-// Wallet deletion state transition, kept out of the view so the selection
-// and broadcast rules are testable without a DOM.
+// Wallet and address deletion state transitions, kept out of the views so the
+// selection and broadcast rules are testable without a DOM.
+
+// Two records of the same address can be stored in different cases, so
+// address equality is never a literal string comparison.
+function sameAddress(a, b) {
+ if (a === null || a === undefined || b === null || b === undefined) {
+ return false;
+ }
+ return String(a).toLowerCase() === String(b).toLowerCase();
+}
+
+// Forget every site permission held against the given addresses.
+function dropSitePermissions(state, addresses) {
+ for (const addr of addresses) {
+ delete state.allowedSites[addr];
+ delete state.deniedSites[addr];
+ }
+}
// Remove wallet `walletIdx` from `state` and repair the derived state.
//
@@ -18,19 +35,13 @@ function removeWalletFromState(state, walletIdx) {
const wallet = state.wallets[walletIdx];
const addresses = (wallet.addresses || []).map((a) => a.address);
const previousActive = state.activeAddress;
- const activeWasDeleted =
- previousActive !== null &&
- previousActive !== undefined &&
- addresses.some(
- (a) => a.toLowerCase() === String(previousActive).toLowerCase(),
- );
+ const activeWasDeleted = addresses.some((a) =>
+ sameAddress(a, previousActive),
+ );
state.wallets.splice(walletIdx, 1);
- for (const addr of addresses) {
- delete state.allowedSites[addr];
- delete state.deniedSites[addr];
- }
+ dropSitePermissions(state, addresses);
state.hasWallet = state.wallets.length > 0;
@@ -58,6 +69,77 @@ function removeWalletFromState(state, walletIdx) {
return { activeAddressChanged: state.activeAddress !== previousActive };
}
+// Whether a wallet may be offered a per-address remove control, and the same
+// gate the removal itself is held behind.
+//
+// Only a wallet that derives its addresses from an extended key can hold more
+// than one, so only those get the control — a key wallet has exactly one
+// address and no "+" button either. The last address of any wallet is never
+// removable: a wallet with no addresses is what delete-wallet is for.
+function canRemoveAddress(wallet) {
+ if (!wallet) return false;
+ if (wallet.type !== "hd" && wallet.type !== "xprv") return false;
+ return (wallet.addresses || []).length > 1;
+}
+
+// Remove address `addrIdx` of wallet `walletIdx` and repair the derived state.
+//
+// Nothing is destroyed here. The address stays derivable from the wallet's own
+// key material and any funds at it are untouched; this only stops the wallet
+// tracking it. `nextIndex` is deliberately left alone — it is a derivation
+// high-water mark, so "+" derives a fresh index rather than handing back the
+// address just removed, and the gap it leaves is within what
+// `scanForAddresses()` re-discovers on a later import.
+//
+// The rules mirror removeWalletFromState() one level down:
+// - The call is refused unless canRemoveAddress() allows it, so the last
+// address of a wallet always survives.
+// - Site permissions are dropped for the removed address.
+// - `selectedAddress` follows the splice, but only within the wallet that
+// lost the address: it is decremented when an earlier address was
+// removed, and falls back to that wallet's first address when the
+// selection itself was removed. `selectedWallet` never moves, because the
+// wallet list does not.
+// - `activeAddress` moves only when it was the removed address, and then to
+// the wallet's first remaining address.
+//
+// Returns whether the address was removed and whether `activeAddress`
+// changed, so the caller can broadcast it.
+function removeAddressFromState(state, walletIdx, addrIdx) {
+ const wallet = state.wallets[walletIdx];
+ const refused = { removed: false, activeAddressChanged: false };
+ if (!canRemoveAddress(wallet)) return refused;
+ if (!wallet.addresses[addrIdx]) return refused;
+
+ const address = wallet.addresses[addrIdx].address;
+ const previousActive = state.activeAddress;
+ const activeWasRemoved = sameAddress(address, previousActive);
+
+ wallet.addresses.splice(addrIdx, 1);
+
+ dropSitePermissions(state, [address]);
+
+ if (state.selectedWallet === walletIdx) {
+ if (state.selectedAddress === addrIdx) {
+ state.selectedAddress = 0;
+ } else if (
+ typeof state.selectedAddress === "number" &&
+ state.selectedAddress > addrIdx
+ ) {
+ state.selectedAddress -= 1;
+ }
+ }
+
+ if (activeWasRemoved) {
+ state.activeAddress = wallet.addresses[0].address;
+ }
+
+ return {
+ removed: true,
+ activeAddressChanged: state.activeAddress !== previousActive,
+ };
+}
+
// Tell the background the active address changed, so it re-emits
// accountsChanged to connected sites. Same call shape as the address
// switch in the home view.
@@ -67,4 +149,9 @@ function broadcastActiveChanged() {
runtime.sendMessage({ type: "AUTISTMASK_ACTIVE_CHANGED" });
}
-module.exports = { removeWalletFromState, broadcastActiveChanged };
+module.exports = {
+ canRemoveAddress,
+ removeAddressFromState,
+ removeWalletFromState,
+ broadcastActiveChanged,
+};
diff --git a/tests/e2e/run.js b/tests/e2e/run.js
index c3b7c46..ed8ab2b 100644
--- a/tests/e2e/run.js
+++ b/tests/e2e/run.js
@@ -376,6 +376,87 @@ 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),
+ );
+
+ // "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() {
diff --git a/tests/walletDelete.test.js b/tests/walletDelete.test.js
index dc592d6..201ddff 100644
--- a/tests/walletDelete.test.js
+++ b/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;