From bcaf0cb1f56bcf9deac1ef3bb401d93f84e7f425 Mon Sep 17 00:00:00 2001 From: sneak Date: Tue, 11 Aug 2026 12:16:59 +0000 Subject: [PATCH] fix: repair wallet state on delete (closes #156) Deleting a wallet left three defects in the same path: - `hasWallet` stayed true after the last wallet was deleted, so the next popup open rendered Home with "No wallets yet" instead of Welcome. It now tracks whether any wallet remains. - The selection was reset to wallet 0 / address 0 unconditionally. `selectedWallet` now follows the splice (decremented when a wallet before it is removed), and only falls back to the first remaining wallet's first address when the selection itself was deleted; `activeAddress` only moves when it belonged to the deleted wallet. - No `AUTISTMASK_ACTIVE_CHANGED` was sent, so connected sites kept reporting a deleted address. The delete path now broadcasts it after the state is persisted, whenever the active address actually changed, and the background re-emits `accountsChanged` from there. The transition moved into `src/shared/walletDelete.js` so it can be tested without a DOM. Site-permission cleanup is unchanged and still covers every address of an HD wallet. --- README.md | 2 +- TODO.md | 4 + src/popup/views/deleteWallet.js | 35 ++++----- src/shared/walletDelete.js | 70 +++++++++++++++++ tests/walletDelete.test.js | 129 ++++++++++++++++++++++++++++++++ 5 files changed, 218 insertions(+), 22 deletions(-) create mode 100644 src/shared/walletDelete.js create mode 100644 tests/walletDelete.test.js diff --git a/README.md b/README.md index 4e5049f..7c10480 100644 --- a/README.md +++ b/README.md @@ -980,7 +980,7 @@ Currently supported: ### Wallet Management -- [ ] Delete wallet (with confirmation) +- [x] Delete wallet (with confirmation) - [ ] Delete address from HD wallet (with confirmation) - [ ] Show wallet's recovery phrase (requires password) diff --git a/TODO.md b/TODO.md index 1529c97..78c8ceb 100644 --- a/TODO.md +++ b/TODO.md @@ -44,6 +44,10 @@ undefined identifiers, which is how # Completed Steps +- 2026-08-11: Wallet deletion repairs its own state — `hasWallet` follows the + remaining wallets, the selection only moves when it was deleted, and the + active-address change is broadcast to connected sites + ([#156](https://git.eeqj.de/sneak/AutistMask/issues/156)). - 2026-08-11: `TODO.md` Workflow rewritten to the branch-and-PR-per-issue model on `next`, with Status and Next Step refreshed ([#191](https://git.eeqj.de/sneak/AutistMask/issues/191)). diff --git a/src/popup/views/deleteWallet.js b/src/popup/views/deleteWallet.js index f682e23..0f0381a 100644 --- a/src/popup/views/deleteWallet.js +++ b/src/popup/views/deleteWallet.js @@ -1,6 +1,10 @@ const { $, showView, showFlash, goBack, clearViewStack } = require("./helpers"); const { state, saveState } = require("../../shared/state"); const { decryptWithPassword } = require("../../shared/vault"); +const { + removeWalletFromState, + broadcastActiveChanged, +} = require("../../shared/walletDelete"); let deleteWalletIndex = null; let ctx = null; @@ -58,35 +62,24 @@ function init(_ctx) { return; } - // Collect addresses to clean up from allowedSites/deniedSites - const addresses = (wallet.addresses || []).map((a) => a.address); - - // Remove wallet - state.wallets.splice(walletIdx, 1); - - // Clean up site permissions for deleted addresses - for (const addr of addresses) { - delete state.allowedSites[addr]; - delete state.deniedSites[addr]; - } + // Remove the wallet and repair selection, permissions and hasWallet + const { activeAddressChanged } = removeWalletFromState( + state, + walletIdx, + ); deleteWalletIndex = null; - if (state.wallets.length === 0) { - // No wallets left — reset selection and show welcome - state.selectedWallet = null; - state.selectedAddress = null; - state.activeAddress = null; + if (!state.hasWallet) { clearViewStack(); await saveState(); + // Save before broadcasting: the background reads the active + // address back out of storage to build accountsChanged. + if (activeAddressChanged) broadcastActiveChanged(); showView("welcome"); } else { - // Switch to first wallet if deleted wallet was active - state.selectedWallet = 0; - state.selectedAddress = 0; - state.activeAddress = - state.wallets[0].addresses[0]?.address || null; await saveState(); + if (activeAddressChanged) broadcastActiveChanged(); // Reset stack to [main] so Settings back goes home. // Use require() lazily to avoid circular dependency // (settings.js requires deleteWallet.js). diff --git a/src/shared/walletDelete.js b/src/shared/walletDelete.js new file mode 100644 index 0000000..dea26ee --- /dev/null +++ b/src/shared/walletDelete.js @@ -0,0 +1,70 @@ +// Wallet deletion state transition, kept out of the view so the selection +// and broadcast rules are testable without a DOM. + +// Remove wallet `walletIdx` from `state` and repair the derived state. +// +// Rules: +// - `hasWallet` tracks whether any wallet remains. +// - Site permissions are dropped for every address of the deleted wallet. +// - `selectedWallet` follows the splice: it is decremented when a wallet +// before it was removed, and falls back to the first remaining wallet's +// first address only when the selection itself was deleted. +// - `activeAddress` is only moved when it belonged to the deleted wallet; +// the fallback is the first remaining wallet's first address, or null +// when no wallet remains. +// +// Returns whether `activeAddress` changed, so the caller can broadcast it. +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(), + ); + + state.wallets.splice(walletIdx, 1); + + for (const addr of addresses) { + delete state.allowedSites[addr]; + delete state.deniedSites[addr]; + } + + state.hasWallet = state.wallets.length > 0; + + const fallbackAddress = state.hasWallet + ? state.wallets[0].addresses[0]?.address || null + : null; + + if (!state.hasWallet) { + state.selectedWallet = null; + state.selectedAddress = null; + } else if (state.selectedWallet === walletIdx) { + state.selectedWallet = 0; + state.selectedAddress = 0; + } else if ( + typeof state.selectedWallet === "number" && + state.selectedWallet > walletIdx + ) { + state.selectedWallet -= 1; + } + + if (activeWasDeleted || !state.hasWallet) { + state.activeAddress = fallbackAddress; + } + + return { 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. +function broadcastActiveChanged() { + const runtime = + typeof browser !== "undefined" ? browser.runtime : chrome.runtime; + runtime.sendMessage({ type: "AUTISTMASK_ACTIVE_CHANGED" }); +} + +module.exports = { removeWalletFromState, broadcastActiveChanged }; diff --git a/tests/walletDelete.test.js b/tests/walletDelete.test.js new file mode 100644 index 0000000..dc592d6 --- /dev/null +++ b/tests/walletDelete.test.js @@ -0,0 +1,129 @@ +const { + removeWalletFromState, + broadcastActiveChanged, +} = require("../src/shared/walletDelete"); + +// Fixed addresses — never used for anything but these tests. +const A0 = "0x66133E8ea0f5D1d612D2502a968757D1048c214a"; +const A1 = "0xdAC17F958D2ee523a2206206994597C13D831ec7"; +const B0 = "0x2260FAC5E5542a773Aa44fBCfeDf7C193bc2C599"; +const C0 = "0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48"; + +function wallet(name, addresses) { + return { + name, + addresses: addresses.map((address) => ({ address })), + }; +} + +// A three-wallet state; wallet A is an HD wallet with two addresses. +function makeState(overrides = {}) { + return { + hasWallet: true, + wallets: [wallet("A", [A0, A1]), wallet("B", [B0]), wallet("C", [C0])], + selectedWallet: 0, + selectedAddress: 0, + activeAddress: A0, + allowedSites: { + [A0]: ["a.example"], + [A1]: ["b.example"], + [B0]: ["c.example"], + }, + deniedSites: { [A1]: ["d.example"], [C0]: ["e.example"] }, + ...overrides, + }; +} + +describe("removeWalletFromState", () => { + test("deleting the last wallet clears hasWallet", () => { + const state = makeState({ + wallets: [wallet("A", [A0])], + allowedSites: { [A0]: ["a.example"] }, + deniedSites: {}, + }); + + const { activeAddressChanged } = removeWalletFromState(state, 0); + + expect(state.hasWallet).toBe(false); + expect(state.wallets).toEqual([]); + expect(state.selectedWallet).toBeNull(); + expect(state.selectedAddress).toBeNull(); + expect(state.activeAddress).toBeNull(); + expect(activeAddressChanged).toBe(true); + }); + + test("deleting a non-selected wallet leaves the selection intact", () => { + const state = makeState({ + selectedWallet: 2, + selectedAddress: 0, + activeAddress: C0, + }); + + const { activeAddressChanged } = removeWalletFromState(state, 1); + + // Wallet C moved from index 2 to index 1 by the splice. + expect(state.wallets.map((w) => w.name)).toEqual(["A", "C"]); + expect(state.selectedWallet).toBe(1); + expect(state.selectedAddress).toBe(0); + expect(state.activeAddress).toBe(C0); + expect(activeAddressChanged).toBe(false); + expect(state.hasWallet).toBe(true); + }); + + test("deleting a wallet after the selection does not shift it", () => { + const state = makeState({ + selectedWallet: 1, + selectedAddress: 0, + activeAddress: B0, + }); + + const { activeAddressChanged } = removeWalletFromState(state, 2); + + expect(state.selectedWallet).toBe(1); + expect(state.activeAddress).toBe(B0); + expect(activeAddressChanged).toBe(false); + }); + + test("deleting the active wallet falls back to the first remaining address", () => { + const state = makeState({ + selectedWallet: 0, + selectedAddress: 1, + activeAddress: A1, + }); + + const { activeAddressChanged } = removeWalletFromState(state, 0); + + expect(state.wallets.map((w) => w.name)).toEqual(["B", "C"]); + expect(state.selectedWallet).toBe(0); + expect(state.selectedAddress).toBe(0); + expect(state.activeAddress).toBe(B0); + expect(activeAddressChanged).toBe(true); + expect(state.hasWallet).toBe(true); + }); + + test("site permissions are dropped for every address of the wallet", () => { + const state = makeState(); + + removeWalletFromState(state, 0); + + expect(state.allowedSites).toEqual({ [B0]: ["c.example"] }); + expect(state.deniedSites).toEqual({ [C0]: ["e.example"] }); + }); +}); + +describe("broadcastActiveChanged", () => { + afterEach(() => { + delete global.chrome; + }); + + test("sends AUTISTMASK_ACTIVE_CHANGED to the background", () => { + const sendMessage = jest.fn(); + global.chrome = { runtime: { sendMessage } }; + + broadcastActiveChanged(); + + expect(sendMessage).toHaveBeenCalledWith({ + type: "AUTISTMASK_ACTIVE_CHANGED", + }); + }); +});