diff --git a/README.md b/README.md index 109a69b..0268c4b 100644 --- a/README.md +++ b/README.md @@ -397,10 +397,16 @@ is missing, a write fails. The popup's `saveState()` (`src/shared/state.js`) reads the stored record, merges this page's changes into it and writes it back. It writes only the fields -this page changed since its last save. Wallets, site lists and endpoints are -merged entry by entry, so two pages changing different things do not overwrite -each other. The background's `updateState()` reads the record, applies one -change and writes it back. +this page changed since its last save. It merges the fields below entry by +entry, so two pages changing different things do not overwrite each other: + +- wallets; +- site lists; +- endpoints; +- tracked tokens. + +The background's `updateState()` reads the record, applies one change and writes +it back. Every write stamps `schemaVersion` with `STATE_SCHEMA_VERSION` from `src/shared/stateSchema.js`, currently `1`. The version is raised only when the diff --git a/src/shared/state.js b/src/shared/state.js index 78cc9e6..dbe0903 100644 --- a/src/shared/state.js +++ b/src/shared/state.js @@ -404,6 +404,36 @@ function mergeNetworkEndpoints(base, ours, theirs) { return mergeMapByKey(base, ours, theirs, mergeEndpointEntry); } +// Merges one network's tracked tokens by contract address: +// +// - A token another page added or removed since this page's last save stays +// added or removed. +// - A token that this page and `theirs` both hold keeps this page's record. +// - The result holds one record per contract address. +// +// mergeListByIdentity() keeps records that share an identity, on purpose, for +// wallets. A contract address names one token, so this keeps one record for it. +// Without that, a token that `ours` and `theirs` each hold twice comes back four +// times, and each later merge multiplies it again. +// +// `theirs` takes this page's records first. mergeListByIdentity() would +// otherwise log an error for two different records of one token. +function mergeTrackedTokens(base, ours, theirs) { + const contract = (token) => token.address.toLowerCase(); + const oursByContract = new Map( + ours.map((token) => [contract(token), token]), + ); + const merged = mergeListByIdentity( + base, + ours, + theirs.map((token) => oursByContract.get(contract(token)) || token), + contract, + (baseToken, ourToken) => ourToken, + ); + const byContract = new Map(merged.map((token) => [contract(token), token])); + return [...byContract.values()]; +} + // Returns a copy of `stored.networkTrackedTokens` for when this page has not yet // taken the switch in `stored`. `page` is this page's state. A token that this // page tracked or removed since its last save is tracked or removed in the list @@ -411,12 +441,10 @@ function mergeNetworkEndpoints(base, ours, theirs) { function networkTrackedTokensWithChange(stored, page) { return { ...stored.networkTrackedTokens, - [page.networkId]: mergeListByIdentity( + [page.networkId]: mergeTrackedTokens( baseline.trackedTokens, page.trackedTokens, - stored.networkTrackedTokens[page.networkId], - (token) => token.address.toLowerCase(), - (base, ours) => ours, + stored.networkTrackedTokens[page.networkId] || [], ), }; } @@ -461,23 +489,27 @@ function networkTrackedTokensWithChange(stored, page) { // comment at mergeEndpointEntry() for why the collision this closes is // milder than the other two. // +// `trackedTokens` is merged by contract address (mergeTrackedTokens()) when this +// page has changed it, so a token another page added or removed since this +// page's last save stays added or removed. A token both lists hold keeps this +// page's record. Two saves differ: +// +// - When the stored record is on another network than this page's, a save keeps +// the stored `trackedTokens`. It merges this page's change into the list in +// `networkTrackedTokens` for this page's network, by contract address. +// - A save that switches networks writes this page's list whole. A switch in +// Settings first merges that list by contract address +// (takeStoredTrackedTokens()). +// // Every other persisted field stays a whole-field diff: // `fraudContracts`/`viewStack` are arrays of scalars with no per-element -// identity to merge by; a save diffs `trackedTokens` whole, and a switch in -// Settings first merges that list by contract address -// (takeStoredTrackedTokens()); `networkTrackedTokens` is diffed whole like -// `trackedTokens` and `networkId`, the two fields a network switch moves -// together with it; `tokenHolderCache` is a map shaped like the ones above, -// but nothing in src/ ever writes an entry into it — it is only ever reset -// wholesale to `{}` (applyChainSwitchFields()) — so there is no in-place -// mutation for a whole-field diff to collide with; `viewData` is this page's -// own UI scratch space, not data another page has any reason to share -// membership of. -// -// One exception: when the stored record is on another network than this -// page's, a save keeps the stored `trackedTokens`. It merges this page's change -// into the list in `networkTrackedTokens` for this page's network, by contract -// address. +// identity to merge by; `networkTrackedTokens` is diffed whole like `networkId`, +// which a network switch moves together with it; `tokenHolderCache` is a map +// shaped like the ones above, but nothing in src/ ever writes an entry into it — +// it is only ever reset wholesale to `{}` (applyChainSwitchFields()) — so there +// is no in-place mutation for a whole-field diff to collide with; `viewData` is +// this page's own UI scratch space, not data another page has any reason to +// share membership of. // // 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 @@ -564,6 +596,20 @@ async function saveStateOnce(beforeWrite) { fresh, current, ); + } else if ( + baseline !== null && + baseline.networkId === current.networkId && + !deepEqual(current.trackedTokens, baseline.trackedTokens) + ) { + // This page's network is the one it last saved on, and the stored + // record is on it too, so all three lists are of one network. This + // page's change goes into the stored list, and another page's add or + // removal stays (https://git.eeqj.de/sneak/AutistMask/issues/717). + merged.trackedTokens = mergeTrackedTokens( + baseline.trackedTokens, + current.trackedTokens, + fresh.trackedTokens, + ); } merged.hasWallet = Boolean(merged.wallets && merged.wallets.length > 0); // Stamped on every write, never merged or diffed: the record that goes to @@ -680,12 +726,10 @@ function takeStoredNetwork(stored) { // `baseline` takes the stored list, so the save stores the list in `state` // whenever the two differ. function takeStoredTrackedTokens(stored) { - rawState.trackedTokens = mergeListByIdentity( + rawState.trackedTokens = mergeTrackedTokens( baseline.trackedTokens, rawState.trackedTokens, stored.trackedTokens, - (token) => token.address.toLowerCase(), - (base, ours) => ours, ); baseline.trackedTokens = structuredClone(stored.trackedTokens); } diff --git a/tests/trackedTokensPerNetwork.test.js b/tests/trackedTokensPerNetwork.test.js index d15fd6e..e77089c 100644 --- a/tests/trackedTokensPerNetwork.test.js +++ b/tests/trackedTokensPerNetwork.test.js @@ -117,6 +117,17 @@ async function chooseNetwork(boot, networkId) { await boot.settle(); } +// Adds NEW_TOKEN_ADDRESS by contract address on SettingsAddToken, as the user +// does. Returns the record the add tracks. +async function addNewToken(boot) { + const { lookupTokenInfo } = require("../src/shared/balances"); + const info = { symbol: "NEW", decimals: 18, name: "New Token" }; + lookupTokenInfo.mockResolvedValueOnce(info); + boot.node("settings-addtoken-address").value = NEW_TOKEN_ADDRESS; + await boot.click("btn-settings-addtoken-manual"); + return { address: NEW_TOKEN_ADDRESS, ...info, balanceNotFetched: true }; +} + // The Settings gear, a switch to Sepolia, and Back. async function gearSwitchBack(boot) { await boot.click("btn-settings"); @@ -685,6 +696,113 @@ test("a token removed while this page waits to take another page's switch is gon expect(stored.networkTrackedTokens.mainnet).toEqual([]); }); +// Another popup page tracks DAI after this page's last save. This page then +// adds a token. Its save keeps DAI. +test("a token another page added stays when this page adds a token", async () => { + const boot = await bootOnAddToken("mainnet"); + expect(boot.pageErrors).toEqual([]); + boot.storage.write("autistmask", { + ...boot.storage.read("autistmask"), + trackedTokens: [MAINNET_TOKEN], + }); + + const added = await addNewToken(boot); + + expect(boot.storage.read("autistmask").trackedTokens).toEqual([ + MAINNET_TOKEN, + added, + ]); +}); + +// Another popup page tracks a new token after this page's last save. This page +// then removes DAI, as the `[x]` on a tracked token in Settings does. Its save +// keeps the new token. +test("a token another page added stays when this page removes a token", async () => { + const boot = await bootPopup( + unversionedValidProfile({ trackedTokens: [MAINNET_TOKEN] }), + ); + expect(boot.pageErrors).toEqual([]); + const otherPageToken = { + address: NEW_TOKEN_ADDRESS, + symbol: "NEW", + decimals: 18, + name: "New Token", + }; + boot.storage.write("autistmask", { + ...boot.storage.read("autistmask"), + trackedTokens: [MAINNET_TOKEN, otherPageToken], + }); + + const { state, saveState } = require("../src/shared/state"); + state.trackedTokens.splice(0, 1); + await saveState(); + + expect(boot.storage.read("autistmask").trackedTokens).toEqual([ + otherPageToken, + ]); +}); + +// Another popup page stops tracking DAI after this page's last save. This page +// still holds DAI and adds a token. Its save does not store DAI again. +test("a token another page removed stays removed when this page adds a token", async () => { + const boot = await bootOnAddToken("mainnet", { + trackedTokens: [MAINNET_TOKEN], + }); + expect(boot.pageErrors).toEqual([]); + boot.storage.write("autistmask", { + ...boot.storage.read("autistmask"), + trackedTokens: [], + }); + + const added = await addNewToken(boot); + + expect(boot.storage.read("autistmask").trackedTokens).toEqual([added]); +}); + +// Another popup page tracks DAI and the new token after this page's last save, +// and its refresh has read the new token's balance. This page adds the new token +// too, so the two pages hold different records for it. The save stores one +// record for the new token, this page's, and keeps DAI. +test("a token both pages added is stored once, with this page's record", async () => { + const boot = await bootOnAddToken("mainnet"); + expect(boot.pageErrors).toEqual([]); + boot.storage.write("autistmask", { + ...boot.storage.read("autistmask"), + trackedTokens: [ + MAINNET_TOKEN, + { + address: NEW_TOKEN_ADDRESS, + symbol: "NEW", + decimals: 18, + name: "New Token", + }, + ], + }); + + const added = await addNewToken(boot); + + expect(boot.storage.read("autistmask").trackedTokens).toEqual([ + MAINNET_TOKEN, + added, + ]); +}); + +// This page and the stored list each hold DAI twice. This page then adds a +// token. Its save stores DAI once. +test("a token held twice is stored once when this page adds a token", async () => { + const boot = await bootOnAddToken("mainnet", { + trackedTokens: [MAINNET_TOKEN, MAINNET_TOKEN], + }); + expect(boot.pageErrors).toEqual([]); + + const added = await addNewToken(boot); + + expect(boot.storage.read("autistmask").trackedTokens).toEqual([ + MAINNET_TOKEN, + added, + ]); +}); + describe("Back after a switch from the Settings gear", () => { test("SettingsAddToken no longer offers the common tokens", async () => { const boot = await bootOnAddToken("mainnet");