diff --git a/README.md b/README.md index 260306c..527778c 100644 --- a/README.md +++ b/README.md @@ -2092,8 +2092,11 @@ view would leave a wallet one click from deletion. none, and the tokens tracked on that network, and sends `chainChanged` with the new chain id to every open tab. A token is tracked only on the network it was added on, because a contract address names a token on one - network only. Choosing the network already active changes nothing and - sends nothing + network only. Switching also clears every address's balances. A balance + refresh, by the popup or the background, stores what it fetched only if + the stored network is still the one it fetched on when it returns, so the + previous network's balances never reach the new one. Choosing the network + already active changes nothing and sends nothing - Ethereum RPC: endpoint URL input + "Save" button (validated against `eth_chainId` before being saved) - Blockscout API: endpoint URL input + "Save" button (validated against diff --git a/src/background/index.js b/src/background/index.js index 09292de..f9d6b9d 100644 --- a/src/background/index.js +++ b/src/background/index.js @@ -1379,6 +1379,13 @@ const RECENT_BALANCE_REFRESH_MS = Math.floor(BALANCE_REFRESH_PERIOD_MS / 2); // balances that came back — by address, onto whatever storage holds NOW. // Applying by address rather than writing the array back is what keeps a // wallet or address added, renamed or deleted during the round trip. +// +// Nothing is applied when the network stored NOW is not the one the refresh +// ran on. A network switch during the round trip cleared every address's +// balances (applyChainSwitchFields()), and the previous network's must not come +// back over them (https://git.eeqj.de/sneak/AutistMask/issues/577). +// lastBalanceRefresh is left as the switch set it, so the next tick refreshes +// the new network. async function backgroundRefresh() { const s = await getState(); const now = Date.now(); @@ -1402,6 +1409,7 @@ async function backgroundRefresh() { } await updateState((fresh) => { + if (fresh.networkId !== s.networkId) return; for (const wallet of fresh.wallets) { for (const addr of wallet.addresses || []) { const got = refreshed.get(String(addr.address).toLowerCase()); diff --git a/src/popup/index.js b/src/popup/index.js index b49a1d6..191d374 100644 --- a/src/popup/index.js +++ b/src/popup/index.js @@ -68,23 +68,54 @@ let refreshInFlight = false; // the recovery screen: there is no profile left to refresh. let refreshTimer = null; +// The refresh writes into a copy of the wallets. Inside the save, under the +// lock, its results are copied onto the address records in `state` only if the +// network it ran on is still the active one, on this page and in the stored +// record; the background's switch, after the user approves a site's request, +// changes only the stored record. A switch while the refresh ran cleared every +// address's balances (applyChainSwitchFields() in +// src/shared/chainSwitchFields.js), and the previous network's must not come +// back over them (https://git.eeqj.de/sneak/AutistMask/issues/577). A dropped +// refresh is not recorded as done. Results are copied by address, so an address +// added or removed meanwhile is left as it is. async function doRefreshAndRender() { if (refreshInFlight) return; refreshInFlight = true; try { + const networkId = state.networkId; + const wallets = structuredClone(state.wallets); await Promise.all([ refreshPrices(), refreshBalances( - state.wallets, + wallets, state.rpcUrl, state.blockscoutUrl, state.trackedTokens, - state.networkId, + networkId, pageClosed.signal, ), ]); - state.lastBalanceRefresh = Date.now(); - await saveState(); + const refreshed = new Map(); + for (const wallet of wallets) { + for (const addr of wallet.addresses) { + refreshed.set(addr.address, addr); + } + } + await saveState((stored) => { + if (state.networkId !== networkId) return false; + if (stored.networkId !== networkId) return false; + for (const wallet of state.wallets) { + for (const addr of wallet.addresses) { + const got = refreshed.get(addr.address); + if (!got) continue; + for (const key of ["balance", "ensName", "tokenBalances"]) { + if (got[key] !== undefined) addr[key] = got[key]; + } + } + } + state.lastBalanceRefresh = Date.now(); + return true; + }); renderWalletList(); } catch (e) { // Every call site fires this and walks away — the boot below, the ten diff --git a/src/shared/state.js b/src/shared/state.js index 9a649e2..4134318 100644 --- a/src/shared/state.js +++ b/src/shared/state.js @@ -467,11 +467,7 @@ function mergeNetworkEndpoints(base, ours, theirs) { // (caught by tests/txStatus.test.js). A page's live picture of a field it // does not own goes on being whatever its last loadState() saw, same as // before this fix; only the persisted record is guaranteed current. -async function saveStateOnce() { - // A copy, so what this save compares and writes is the page's state as it - // stood when the save began. A change made while it waits on storage is - // left for the next save, which compares against this copy. - const current = structuredClone(snapshotPersisted()); +async function saveStateOnce(beforeWrite) { const result = await storageGet("autistmask"); // The record in storage right now is about to be merged into and written // back, so it is validated exactly like a load validates it. Without this, @@ -485,6 +481,12 @@ async function saveStateOnce() { // come from storage in its loaded (self-healed) shape. See // normalizePersisted() in persistedState.js. const fresh = normalizePersisted(result.autistmask); + if (beforeWrite && beforeWrite(fresh) === false) return; + + // A copy, so what this save compares and writes is the page's state as it + // stood once the stored record was read. A change made while it waits on + // the write is left for the next save, which compares against this copy. + const current = structuredClone(snapshotPersisted()); const merged = { ...fresh }; for (const key of PERSISTED_FIELDS) { @@ -579,10 +581,15 @@ function reportSaveFailure(err) { // The same lock orders this page's own saves. showView() calls saveState() on // every navigation without awaiting it, so two saves from one page can be // asked for at once; they take the lock in the order they were asked, and each -// compares the page's state as it stands when its turn begins against the -// baseline the save before it left. -function saveState() { - const turn = withLock("autistmask", saveStateOnce); +// compares the page's state, as it stands once the stored record has been read +// and `beforeWrite` has run, against the baseline the save before it left. +// +// `beforeWrite`, when given, is called inside the lock with the stored record +// as just read, before this page's state is compared against it. It may change +// `state`, and this save writes that change. When it returns false, nothing is +// written; any other return lets the save go ahead. +function saveState(beforeWrite) { + const turn = withLock("autistmask", () => saveStateOnce(beforeWrite)); // Every failed save is reported, whether or not the caller awaited this // one. The returned promise still rejects, so a caller that DOES await // keeps its own error handling. diff --git a/tests/backgroundStateIsolation.test.js b/tests/backgroundStateIsolation.test.js index 4592977..ee24cc7 100644 --- a/tests/backgroundStateIsolation.test.js +++ b/tests/backgroundStateIsolation.test.js @@ -19,7 +19,10 @@ // that followed persisted the pre-refresh values — while still stamping // lastBalanceRefresh, suppressing the redo. // -// Both use the real persistence path over a cloning storage stub. Nothing here +// A network switch landing inside a balance refresh's round trip is covered at +// the end (https://git.eeqj.de/sneak/AutistMask/issues/577). +// +// All use the real persistence path over a cloning storage stub. Nothing here // asserts the absence of a loadState() call; each asserts the OUTCOME, so it // holds against any implementation that gets the outcome right. @@ -395,3 +398,48 @@ describe("a balance refresh under another handler's state read", () => { expect(after.wallets[0].addresses[0].balance).toBe(REFRESHED_BALANCE); }); }); + +describe("a balance refresh under a network switch", () => { + // https://git.eeqj.de/sneak/AutistMask/issues/577. The switch clears every + // address's balances, and a refresh that started on the previous network + // must not write that network's back over them when it returns. The same + // refresh in the popup is in tests/balanceRefreshNetworkSwitch.test.js. + test("a refresh that returns after a switch writes nothing, and the next one runs on the new network", async () => { + const roundTrip = deferred(); + const reachedNetwork = deferred(); + const ranOn = []; + + const bg = loadWorker("sepolia", { + refreshBalances: async (wallets, rpcUrl, explorer, tokens, id) => { + ranOn.push(id); + reachedNetwork.resolve(); + await roundTrip.promise; + wallets[0].addresses[0].balance = REFRESHED_BALANCE; + }, + }); + + const refresh = bg.fireBalanceAlarm(); + await reachedNetwork.promise; + + // The user switches to Ethereum Mainnet in Settings while the round + // trip is out. + const during = bg.persisted(); + applyChainSwitchFields(during, MAINNET.id); + global.chrome.storage.write("autistmask", during); + + roundTrip.resolve(); + await refresh; + + const after = bg.persisted(); + expect(after.networkId).toBe("mainnet"); + expect(after.wallets[0].addresses[0].balance).toBe("0"); + expect(after.lastBalanceRefresh).toBe(0); + + await bg.fireBalanceAlarm(); + + expect(ranOn).toEqual(["sepolia", "mainnet"]); + expect(bg.persisted().wallets[0].addresses[0].balance).toBe( + REFRESHED_BALANCE, + ); + }); +}); diff --git a/tests/balanceRefreshNetworkSwitch.test.js b/tests/balanceRefreshNetworkSwitch.test.js new file mode 100644 index 0000000..9208ab5 --- /dev/null +++ b/tests/balanceRefreshNetworkSwitch.test.js @@ -0,0 +1,160 @@ +// A balance refresh the popup started on one network, and that returns after +// the network was switched to another, writes nothing: the switch cleared every +// address's balances, and the previous network's must not come back over them +// (https://git.eeqj.de/sneak/AutistMask/issues/577). The switch is made either +// by the user in this popup's Settings, or by the background after the user +// approved a site's request, which changes only the stored record. Driven +// through the real popup over a stored profile on Ethereum Mainnet. The same +// refresh in the background is in tests/backgroundStateIsolation.test.js. + +const { + bootPopup, + cleanupPopup, + unversionedValidProfile, + TOKEN_ADDRESS, +} = require("./support/popupBoot"); +const { applyChainSwitchFields } = require("../src/shared/chainSwitchFields"); + +afterEach(() => { + cleanupPopup(); +}); + +// Starts the ten-second refresh and holds it open. The balance stand-in writes +// Ethereum Mainnet's balances in place, on the records handed in, as the real +// refreshBalances() does, once the returned function is called. Resolves with +// that function and the refresh still running. +async function holdRefresh(page) { + const { refreshBalances } = require("../src/shared/balances"); + let answer; + refreshBalances.mockImplementationOnce(async (wallets) => { + await new Promise((resolve) => { + answer = resolve; + }); + wallets[0].addresses[0].balance = "7.0"; + wallets[0].addresses[0].tokenBalances = [ + { + address: TOKEN_ADDRESS, + name: "Test Token", + symbol: "TST", + decimals: 18, + balance: "5.0", + holders: null, + }, + ]; + }); + const ticking = page.tick(); + await page.settle(); + return { + finish: async () => { + answer(); + await ticking; + }, + }; +} + +// Picks `networkId` in the Settings network selector, as the user does. +async function chooseNetwork(page, networkId) { + const select = page.node("settings-network"); + select.value = networkId; + for (const fn of select.listeners.change) await fn(); + await page.settle(); +} + +// The stored record holds none of Ethereum Mainnet's balances, and no refresh +// recorded as done. +function expectNothingStored(page) { + const stored = page.storage.read("autistmask"); + expect(stored.networkId).toBe("sepolia"); + expect(stored.wallets[0].addresses[0].balance).toBe("0"); + expect(stored.wallets[0].addresses[0].tokenBalances).toEqual([]); + expect(stored.lastBalanceRefresh).toBe(0); +} + +test("a refresh that returns after a switch in Settings writes nothing, and the next one runs on the new network", async () => { + const page = await bootPopup(unversionedValidProfile()); + expect(page.pageErrors).toEqual([]); + const { refreshBalances } = require("../src/shared/balances"); + const { state } = require("../src/shared/state"); + + const refresh = await holdRefresh(page); + await page.click("btn-settings"); + await chooseNetwork(page, "sepolia"); + await refresh.finish(); + + expect(state.wallets[0].addresses[0].balance).toBe("0"); + expect(state.wallets[0].addresses[0].tokenBalances).toEqual([]); + expectNothingStored(page); + + await page.click("btn-settings-back"); + expect(page.visibleViews()).toEqual(["main"]); + expect(page.text("total-value")).toBe("0.0000 SepoliaETH"); + + await page.tick(); + + expect(refreshBalances.mock.lastCall[4]).toBe("sepolia"); + expect(page.storage.read("autistmask").lastBalanceRefresh).toBeGreaterThan( + 0, + ); +}); + +// The stored record still holds Ethereum Mainnet while the refresh's save reads +// it, so only this page's own network shows the switch. +test("a refresh whose save is reading the stored record when the user switches in Settings writes nothing", async () => { + const page = await bootPopup(unversionedValidProfile()); + expect(page.pageErrors).toEqual([]); + const { state } = require("../src/shared/state"); + + const refresh = await holdRefresh(page); + await page.click("btn-settings"); + + // The next read of the stored record, the refresh's save's, is held. + const get = page.storage.local.get; + const read = get.getMockImplementation(); + let releaseRead; + get.mockImplementationOnce(async (key) => { + await new Promise((resolve) => { + releaseRead = resolve; + }); + return read(key); + }); + const finishing = refresh.finish(); + await page.settle(); + + // The user picks Sepolia meanwhile; the switch's own save waits for the + // refresh's save to let the lock go. + const select = page.node("settings-network"); + select.value = "sepolia"; + const switching = Promise.all(select.listeners.change.map((fn) => fn())); + await page.settle(); + + releaseRead(); + await finishing; + await switching; + await page.settle(); + + expect(state.networkId).toBe("sepolia"); + expect(state.wallets[0].addresses[0].balance).toBe("0"); + expect(state.wallets[0].addresses[0].tokenBalances).toEqual([]); + expectNothingStored(page); +}); + +test("a refresh that returns after the background switched the network writes nothing", async () => { + const page = await bootPopup(unversionedValidProfile()); + expect(page.pageErrors).toEqual([]); + const { saveState } = require("../src/shared/state"); + + const refresh = await holdRefresh(page); + // The background writes the switch to Sepolia, as its updateState() does + // once the user approves a site's request. This page is not told. + const record = page.storage.read("autistmask"); + applyChainSwitchFields(record, "sepolia"); + page.storage.write("autistmask", record); + await refresh.finish(); + + expectNothingStored(page); + + // Nor does a later save by this page, such as every navigation makes. + await saveState(); + + expectNothingStored(page); +});