fix: a save merges this page's tracked token change into the stored list (closes #717)

A save wrote `trackedTokens` whole, so two pages on one network dropped each
other's adds and brought back each other's removals.

A save that keeps the network now merges this page's change into the stored
list by contract address. A token both lists hold keeps this page's record.
Two pages that add one token can hold different records for it, and
`mergeListByIdentity()` alone would store both.

A save that switches networks is unchanged.

Model: opus-5-5

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
+this page changed since its last save. It merges the fields below entry by
-merged entry by entry, so two pages changing different things do not overwrite
+entry, so two pages changing different things do not overwrite each other:
-each other. The background's `updateState()` reads the record, applies one
+
-change and writes it back.
+- 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

src/shared/state.js

@@ -461,23 +461,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 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
+// identity to merge by; `networkTrackedTokens` is diffed whole like `networkId`,
-// Settings first merges that list by contract address
+// which a network switch moves together with it; `tokenHolderCache` is a map
-// (takeStoredTrackedTokens()); `networkTrackedTokens` is diffed whole like
+// shaped like the ones above, but nothing in src/ ever writes an entry into it —
-// `trackedTokens` and `networkId`, the two fields a network switch moves
+// it is only ever reset wholesale to `{}` (applyChainSwitchFields()) — so there
-// together with it; `tokenHolderCache` is a map shaped like the ones above,
+// is no in-place mutation for a whole-field diff to collide with; `viewData` is
-// but nothing in src/ ever writes an entry into it — it is only ever reset
+// this page's own UI scratch space, not data another page has any reason to
-// wholesale to `{}` (applyChainSwitchFields()) — so there is no in-place
+// share membership of.
-// 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.
 //
 // 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 +568,33 @@ 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).
+        //
+        // Two pages that add one token can hold different records for it, and
+        // mergeListByIdentity() keeps both of two such records. A contract
+        // address names one token, so the stored list is given this page's
+        // record for each token both hold.
+        const contract = (token) => token.address.toLowerCase();
+        const oursByContract = new Map(
+            current.trackedTokens.map((token) => [contract(token), token]),
+        );
+        merged.trackedTokens = mergeListByIdentity(
+            baseline.trackedTokens,
+            current.trackedTokens,
+            fresh.trackedTokens.map(
+                (token) => oursByContract.get(contract(token)) || token,
+            ),
+            contract,
+            (base, ours) => ours,
+        );
     }
     merged.hasWallet = Boolean(merged.wallets && merged.wallets.length > 0);
     // Stamped on every write, never merged or diffed: the record that goes to

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,97 @@ 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,
+    ]);
+});
+
 describe("Back after a switch from the Settings gear", () => {
     test("SettingsAddToken no longer offers the common tokens", async () => {
         const boot = await bootOnAddToken("mainnet");