From af9568db903a316a1fbbea9e7c27d39afdd8ac54 Mon Sep 17 00:00:00 2001 From: sneak Date: Thu, 20 Aug 2026 14:14:29 +0000 Subject: [PATCH] fix: merge wallets by identity, not whole-field, in saveState() (closes #304) backgroundRefresh() mutates state.wallets in place (addr.balance/ensName/ tokenBalances via refreshBalances()), so a whole-field diff on `wallets` marked the entire array "changed" the moment any balance moved and wrote back background's own copy -- loaded before its multi-second network round trip -- clobbering a wallet another page added, or resurrecting one another page deleted, in that window. That is DoD item 2 on the issue, still unmet by the prior whole-field merge. `wallets` is now merged structurally: by wallet identity (xpub for hd/xprv wallets, address for key wallets, both already enforced unique), then by address identity within each wallet. A leaf background actually changed applies on top of storage's current copy; membership added or removed by another page applies independently, since it no longer collides with `wallets` as a single field. Every other persisted field stays a whole-field diff -- no code path mutates them the way backgroundRefresh() mutates wallets, so there is no matching defect to fix there. --- src/shared/state.js | 146 +++++++++++++++++++++++++++++++++++++-- tests/stateMerge.test.js | 77 +++++++++++++++++++++ 2 files changed, 218 insertions(+), 5 deletions(-) diff --git a/src/shared/state.js b/src/shared/state.js index 34b9e7d..3ae5d5d 100644 --- a/src/shared/state.js +++ b/src/shared/state.js @@ -235,6 +235,112 @@ function deepEqual(a, b) { return true; } +// Stable identity for a wallet, independent of its position in the array +// (which shifts under a concurrent add/delete elsewhere) and independent of +// its mutable fields (name is user-editable; addresses gains/loses entries +// via scanning and deleteAddress.js). An "hd"/"xprv" wallet's xpub never +// changes for its lifetime and is already enforced unique +// (findWalletByXpub() in addWallet.js). A "key" wallet has no xpub, exactly +// one address for its whole lifetime (nothing ever adds to or removes from +// a key wallet's address list), and that address is already enforced +// unique (findWalletByAddress()) — so it stands in for identity there. +function walletIdentity(wallet) { + if (wallet.xpub) return "xpub:" + wallet.xpub; + const first = wallet.addresses && wallet.addresses[0]; + return "addr:" + (first ? String(first.address).toLowerCase() : ""); +} + +// Stable identity for an address within one wallet's address list. An +// address is unique within its wallet and, once derived or imported, never +// changes — only whether it is present. +function addressIdentity(addr) { + return String(addr.address).toLowerCase(); +} + +// Merge one array of identity-bearing objects (wallets, or the addresses +// inside one wallet) by identity rather than by array index — an index +// shifts under a concurrent insert/delete elsewhere, which would merge the +// wrong pair of objects entirely. +// +// `theirs` (fresh storage) sets the membership baseline and the order: +// - An item this page never had baseline knowledge of, but that is in +// `theirs`, was added by someone else — kept as-is. +// - An item `base` had and `ours` no longer has was deleted by THIS page +// — dropped even though `theirs` still has it (this page's own delete +// must win over a background save that only touched leaf fields). +// - An item present in both `ours` and `theirs` is merged leaf-by-leaf via +// `mergeItem`, so a leaf this page changed (e.g. a renamed wallet) lands +// on top of `theirs`' otherwise-current copy (e.g. a refreshed balance). +// Anything left in `ours` that `base` never had and `theirs` does not have +// yet is this page's own new addition — appended. +function mergeListByIdentity(base, ours, theirs, identityOf, mergeItem) { + base = base || []; + ours = ours || []; + theirs = theirs || []; + const baseIndex = new Map(base.map((item) => [identityOf(item), item])); + const oursIndex = new Map(ours.map((item) => [identityOf(item), item])); + const result = []; + const seen = new Set(); + + for (const theirItem of theirs) { + const id = identityOf(theirItem); + seen.add(id); + if (baseIndex.has(id) && !oursIndex.has(id)) continue; + if (oursIndex.has(id)) { + result.push( + mergeItem(baseIndex.get(id), oursIndex.get(id), theirItem), + ); + } else { + result.push(theirItem); + } + } + + for (const item of ours) { + const id = identityOf(item); + if (seen.has(id)) continue; + if (!baseIndex.has(id)) result.push(item); + } + + return result; +} + +// Merge one wallet's scalar/leaf fields (name, encryptedSecret, nextIndex, +// ...) against base, then recurse into its address list by identity. `base` +// is null when this page created the wallet itself (never had a baseline to +// diff against) — nothing to merge in that case, this page's own copy wins +// outright. +function mergeWallet(base, ours, theirs) { + if (!base) return ours; + const merged = { ...theirs }; + for (const key of Object.keys(ours)) { + if (key === "addresses") continue; + if (!deepEqual(ours[key], base[key])) merged[key] = ours[key]; + } + merged.addresses = mergeListByIdentity( + base.addresses, + ours.addresses, + theirs.addresses, + addressIdentity, + mergeAddress, + ); + return merged; +} + +// Merge one address's leaf fields (balance, ensName, tokenBalances, ...). +// tokenBalances is itself an array, but only backgroundRefresh() ever +// writes it and always wholesale (refreshBalances() in +// src/shared/balances.js), so there is no membership to reconcile within +// it — it is a leaf like balance or ensName, not a list with its own +// identity. +function mergeAddress(base, ours, theirs) { + if (!base) return ours; + const merged = { ...theirs }; + for (const key of Object.keys(ours)) { + if (!deepEqual(ours[key], base[key])) merged[key] = ours[key]; + } + return merged; +} + // Read-modify-write, merged per field, rather than one full-blob write. // // Every extension page (the toolbar popup, a dApp approval window, the @@ -249,10 +355,29 @@ function deepEqual(a, b) { // are written; every other field is carried forward from whatever is in // storage right now, which may already be a value another page wrote. // -// This does not make two pages that both change the SAME field concurrently -// safe: last write wins on that one field, same as before. What it removes -// is the cross-field clobber — a page that only navigated overwriting a -// wallet list it never touched. +// `wallets` is merged structurally (mergeListByIdentity(), by wallet +// identity and then by address identity within each wallet), not as one +// whole field: backgroundRefresh() mutates wallets IN PLACE (addr.balance / +// ensName / tokenBalances, via refreshBalances()), so a whole-field diff +// would mark all of `wallets` "changed" the moment any balance moved and +// write back background's own copy — loaded before its multi-second network +// round trip — clobbering a wallet another page added, or resurrecting one +// another page deleted, in that window. Merging by identity lets +// background's leaf changes and another page's membership changes +// (add/delete a wallet or an address) apply independently instead of +// colliding as the same field. Every other persisted field stays a +// whole-field diff: `trackedTokens`/`fraudContracts`/`viewStack` are arrays +// of scalars with no per-element identity to merge by, and no code path +// mutates `networkEndpoints`/`allowedSites`/`deniedSites`/`tokenHolderCache` +// /`viewData` the way backgroundRefresh() mutates wallets (in place, +// concurrently with another page's membership change to the same field), so +// there is no known defect there for the added complexity to earn. +// +// This does not make two pages that both change the SAME leaf concurrently +// safe: last write wins on that one leaf, same as before. What it removes +// is the cross-field (and now cross-membership-vs-leaf) clobber — a page +// that only navigated, or only refreshed a balance, overwriting a wallet or +// address list it never touched the membership of. // // This page's own live `state` is deliberately NOT rehydrated from a field // another page changed — only the record written to storage is merged. @@ -276,7 +401,18 @@ async function saveStateOnce() { const merged = { ...fresh }; for (const key of PERSISTED_FIELDS) { - if (baseline === null || !deepEqual(current[key], baseline[key])) { + if (key === "wallets") { + merged.wallets = mergeListByIdentity( + baseline ? baseline.wallets : [], + current.wallets, + fresh.wallets, + walletIdentity, + mergeWallet, + ); + } else if ( + baseline === null || + !deepEqual(current[key], baseline[key]) + ) { merged[key] = current[key]; } } diff --git a/tests/stateMerge.test.js b/tests/stateMerge.test.js index 066833d..eec09a9 100644 --- a/tests/stateMerge.test.js +++ b/tests/stateMerge.test.js @@ -212,3 +212,80 @@ describe("the approval-window reproduction", () => { ]); }); }); + +// backgroundRefresh() (src/background/index.js) loads state, spends seconds +// on network I/O in refreshBalances() (src/shared/balances.js) mutating +// addr.balance/ensName/tokenBalances IN PLACE on the wallets it already +// knew about, then saves. Precondition 2 on the issue: that refresh window +// overlapping a membership change (add or delete) on another page must not +// clobber or resurrect a wallet — a whole-field diff on `wallets` failed +// this, because "background changed a balance" and "another page changed +// membership" collided as the same field. +describe("background refresh racing a wallet added on another page", () => { + test("the wallet added elsewhere survives background's stale balance save", async () => { + const storage = makeStorage(); + await storage.set({ autistmask: { wallets: [W1] } }); + + // "background": loads first, and its save is the one that lands + // last, modeling the multi-second network round trip in between. + const background = loadPage(storage); + await background.state.loadState(); + background.state.state.wallets[0].addresses[0].balance = "1.2345"; + + // A second page, loaded after, adds a wallet while background's + // refresh is still in flight. + const popup = loadPage(storage); + await popup.state.loadState(); + popup.state.state.wallets.push(W2); + popup.state.state.hasWallet = true; + await popup.state.saveState(); + expect( + (await storage.get("autistmask")).autistmask.wallets, + ).toHaveLength(2); + + // background's save lands last, carrying only its balance update. + await background.state.saveState(); + + const persisted = (await storage.get("autistmask")).autistmask; + expect(persisted.wallets.map((w) => w.name)).toEqual([ + "Wallet 1", + "Wallet 2", + ]); + expect(persisted.wallets.map((w) => w.encryptedSecret)).toEqual([ + "secret-one", + "secret-two", + ]); + // The balance update itself must not be lost either — this is a + // merge, not deletion-always-wins. + expect(persisted.wallets[0].addresses[0].balance).toBe("1.2345"); + }); +}); + +describe("background refresh racing a wallet deleted on another page", () => { + test("the wallet deleted elsewhere stays deleted after background's stale balance save", async () => { + const storage = makeStorage(); + await storage.set({ autistmask: { wallets: [W1, W2] } }); + + const background = loadPage(storage); + await background.state.loadState(); + background.state.state.wallets[0].addresses[0].balance = "1.2345"; + + // A second page deletes Wallet 2 while background's refresh is in + // flight — the same splice deleteWallet.js's removeWalletFromState() + // does. + const popup = loadPage(storage); + await popup.state.loadState(); + popup.state.state.wallets.splice(1, 1); + popup.state.state.hasWallet = popup.state.state.wallets.length > 0; + await popup.state.saveState(); + expect( + (await storage.get("autistmask")).autistmask.wallets, + ).toHaveLength(1); + + await background.state.saveState(); + + const persisted = (await storage.get("autistmask")).autistmask; + expect(persisted.wallets.map((w) => w.name)).toEqual(["Wallet 1"]); + expect(persisted.wallets[0].addresses[0].balance).toBe("1.2345"); + }); +});