diff --git a/README.md b/README.md index 6e4e285..a374972 100644 --- a/README.md +++ b/README.md @@ -2080,8 +2080,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, that started before the switch + stores nothing when it returns, so the previous network's balances never + reach the new one; the next refresh fetches the new network's. 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..0ff900e 100644 --- a/src/popup/index.js +++ b/src/popup/index.js @@ -68,21 +68,48 @@ 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. Its results are copied onto +// the address records in `state` only if the network it ran on is still the +// active one: a network switch while it 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 refresh that returns +// after a switch is dropped and records nothing, so the next tick refreshes the +// new network. 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, ), ]); + if (state.networkId !== networkId) return; + const refreshed = new Map(); + for (const wallet of wallets) { + for (const addr of wallet.addresses) { + refreshed.set(addr.address, addr); + } + } + 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(); await saveState(); renderWalletList(); 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..37bd527 --- /dev/null +++ b/tests/balanceRefreshNetworkSwitch.test.js @@ -0,0 +1,79 @@ +// A balance refresh the popup started on one network, and that returns after +// the user switched to another in Settings, 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). 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"); + +afterEach(() => { + cleanupPopup(); +}); + +// 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(); +} + +test("a refresh that returns after a switch 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"); + + // The ten-second refresh, held open until answer() is called. It writes + // Ethereum Mainnet's balances in place, on the records handed in, as the + // real refreshBalances() does. + 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(); + + await page.click("btn-settings"); + await chooseNetwork(page, "sepolia"); + answer(); + await ticking; + + expect(state.wallets[0].addresses[0].balance).toBe("0"); + expect(state.wallets[0].addresses[0].tokenBalances).toEqual([]); + 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); + + 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, + ); +});