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.
This commit is contained in:
@@ -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");
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user