README: second rework after review

Fix the 23 findings of the second review and the two leftovers from the
first: SiteApproval's connection lifetime, the stateSchema.js module line,
the flash line and title bar, the six network-bound screens, the token-only
0.000001 cutoff, the 12-character symbol cap, the time display, the native
currency labels, the truncateAmountNeverZero screens, the browser suite
rules, the build receipt contents, the External services list, the worker
memory exceptions, POPUP_ONLY_TYPES, the undefined terms, the comma lists,
the Rationale, Entrypoints and TODO sections, and the dropped crypto comment
rule. A fact pass then checked every statement about the code and every
absolute against each case, and a newcomer read removed repeats.

Model: fable-5-1

Dockerfile

@@ -10,6 +10,7 @@ WORKDIR /app
 
 COPY script/ script/
 COPY package.json yarn.lock ./
+# yarn warns about the workspaces field in eslint and babel-plugin-istanbul; no newer release drops it.
 RUN script/bootstrap
 
 COPY . .

eslint.config.js

@@ -153,8 +153,7 @@ module.exports = [
         },
     },
 
-    // The helpers the script/ entrypoints call: plain node programs too, run
+    // The files in script/lib/ run on plain node too, and are never bundled.
-    // from a shell script rather than from yarn, and never bundled.
     {
         files: ["script/lib/**/*.js"],
         languageOptions: {

package.json

@@ -6,8 +6,8 @@
     "license": "GPL-3.0",
     "private": true,
     "scripts": {
-        "test": "jest --forceExit --maxWorkers=3",
+        "test": "node script/lib/jest.js --maxWorkers=3",
-        "test:verbose": "jest --forceExit --maxWorkers=3 --verbose",
+        "test:verbose": "node script/lib/jest.js --maxWorkers=3 --verbose",
         "build": "node build.js",
         "lint": "eslint . && prettier --check .",
         "fmt": "prettier --write .",

script/bootstrap

@@ -118,6 +118,7 @@ ensure_yarn() {
     fi
 }
 
+# yarn warns about the workspaces field in eslint and babel-plugin-istanbul; no newer release drops it.
 install_js_deps() {
     if missing yarn && [ -s "$HOME/.nvm/nvm.sh" ]; then
         nvm_sh "nvm use $NODE_VERSION >/dev/null && cd \"$ROOT\" && \

script/lib/jest.js

@@ -0,0 +1,40 @@
+// Runs jest with the arguments it is given, and fails the run when jest had to
+// force-stop the worker process that ran a test file. Jest itself only prints a
+// warning when that happens, and its run still passes. The warning means a test
+// left a timer or another open handle running. The test scripts in package.json
+// run jest through this file.
+//
+// Jest runs inside this process and writes to the terminal directly, so its
+// output looks the same as a plain jest run. This file only checks each write
+// to stderr for the warning below.
+
+"use strict";
+
+// The start of the warning that jest prints on stderr after it force-stops a
+// worker process.
+const WORKER_WARNING = "A worker process has failed to exit gracefully";
+
+let forceStopped = false;
+
+const writeStderr = process.stderr.write.bind(process.stderr);
+process.stderr.write = (chunk, ...rest) => {
+    if (String(chunk).includes(WORKER_WARNING)) {
+        forceStopped = true;
+    }
+    return writeStderr(chunk, ...rest);
+};
+
+// Runs after jest has finished, as the process exits.
+process.on("exit", () => {
+    if (forceStopped) {
+        console.error(
+            "script/lib/jest.js: failing the run, because jest force-stopped " +
+                "a worker process. A test left a timer or another open " +
+                "handle running.",
+        );
+        process.exitCode = 1;
+    }
+});
+
+// Jest's own command-line program. It reads its arguments from process.argv.
+require("jest/bin/jest");

src/popup/views/addToken.js

@@ -5,7 +5,12 @@ const { lookupTokenInfo } = require("../../shared/balances");
 const { isScamAddress } = require("../../shared/scamlist");
 const { log } = require("../../shared/log");
 
+// Counts the times AddToken has opened. A contract lookup started before
+// AddToken was last opened changes neither the screen nor the status line.
+let addTokenScreenOpenings = 0;
+
 function show() {
+    addTokenScreenOpenings++;
     $("add-token-address").value = "";
     $("add-token-info").textContent = "";
     $("add-token-info").style.visibility = "hidden";
@@ -55,6 +60,7 @@ function init(ctx) {
         // The network the lookup asks, which Settings may switch away from
         // before it answers, as for a transaction in confirmTx.js.
         const chainId = currentNetwork().chainId;
+        const opening = addTokenScreenOpenings;
         try {
             const info = await lookupTokenInfo(
                 contractAddr,
@@ -80,11 +86,15 @@ function init(ctx) {
             });
             await saveState();
             ctx.doRefreshAndRender();
+            // If the lookup answers after AddToken was opened again, the
+            // token is still added, but the screen and the status line stay
+            // as they are.
+            if (addTokenScreenOpenings !== opening) return;
             // The lookup is over, so a later Back onto AddToken shows no
             // status.
             infoEl.textContent = "";
             infoEl.style.visibility = "hidden";
-            // If the user left AddToken while the lookup ran, the token is
+            // If the lookup answers while another screen shows, the token is
             // still added but the screen does not change.
             if (state.currentView !== "add-token") return;
             // Pop the stack (back to address detail) and re-render it
@@ -107,6 +117,7 @@ function init(ctx) {
                     ? detail
                     : "Could not add the token.",
             );
+            if (addTokenScreenOpenings !== opening) return;
             infoEl.textContent = "";
             infoEl.style.visibility = "hidden";
         }

tests/addTokenLateAnswer.test.js

@@ -18,6 +18,9 @@ const TOKEN = {
     name: "Test Token",
 };
 
+// A second contract address, typed after AddToken was opened again.
+const OTHER_TOKEN_ADDRESS = "0xBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB";
+
 afterEach(() => {
     cleanupPopup();
 });
@@ -72,7 +75,7 @@ test("an answer after the user opened Settings adds the token and Settings stays
     expect(boot.storage.read("autistmask").trackedTokens).toEqual([TOKEN]);
 });
 
-test("Back onto AddToken after that answer shows no lookup status", async () => {
+test("Back from Settings onto AddToken after the lookup answered shows no lookup status", async () => {
     const boot = await openAddToken();
     const answerLookup = addWithPendingLookup(boot);
     expect(boot.text("add-token-info")).toBe("Looking up token...");
@@ -85,6 +88,44 @@ test("Back onto AddToken after that answer shows no lookup status", async () =>
     expect(boot.node("add-token-info").style.visibility).toBe("hidden");
 });
 
+test("an answer after AddToken was opened again adds the token and leaves AddToken and its status line as they are", async () => {
+    const boot = await openAddToken();
+    const answerFirstLookup = addWithPendingLookup(boot);
+    await boot.click("btn-add-token-back");
+    await boot.click("btn-add-token");
+    expect(boot.visibleViews()).toEqual(["add-token"]);
+    boot.node("add-token-address").value = OTHER_TOKEN_ADDRESS;
+    // The second lookup is never answered.
+    addWithPendingLookup(boot);
+
+    await answerFirstLookup();
+    expect(boot.visibleViews()).toEqual(["add-token"]);
+    expect(boot.text("add-token-info")).toBe("Looking up token...");
+    expect(boot.storage.read("autistmask").trackedTokens).toEqual([TOKEN]);
+});
+
+test("a lookup that fails after AddToken was opened again leaves the status line as it is", async () => {
+    const boot = await openAddToken();
+    const { lookupTokenInfo } = require("../src/shared/balances");
+    let failFirstLookup;
+    lookupTokenInfo.mockImplementationOnce(
+        () =>
+            new Promise((_, reject) => {
+                failFirstLookup = reject;
+            }),
+    );
+    const firstAdd = boot.click("btn-add-token-confirm");
+    await boot.click("btn-add-token-back");
+    await boot.click("btn-add-token");
+    boot.node("add-token-address").value = OTHER_TOKEN_ADDRESS;
+    // The second lookup is never answered.
+    addWithPendingLookup(boot);
+
+    failFirstLookup(new Error("lookup failed"));
+    await firstAdd;
+    expect(boot.text("add-token-info")).toBe("Looking up token...");
+});
+
 test("an answer while AddToken still shows goes to AddressDetail with the token listed", async () => {
     const boot = await openAddToken();
     const answerLookup = addWithPendingLookup(boot);

tests/addressKey.test.js

@@ -161,7 +161,7 @@ function click(element) {
 
 // show() starts the fee estimate without awaiting it; this lets it settle.
 async function settle() {
-    for (let i = 0; i < 10; i++) await new Promise((r) => setTimeout(r, 0));
+    for (let i = 0; i < 10; i++) await jest.advanceTimersByTimeAsync(0);
 }
 
 // ------------------------------------------------------------ harness
@@ -188,7 +188,11 @@ let sent;
 // handler to the buttons it drew, as the browser's fresh list would have.
 let plusButton;
 
+// showFlash() starts a timer that clears the message one or two seconds later.
+// Each test here runs on fake timers and drops them in afterEach, so that timer
+// never outlives the test.
 beforeEach(() => {
+    jest.useFakeTimers();
     approvalDetails = null;
     sent = [];
     mockNode.broadcast = [];
@@ -239,6 +243,7 @@ beforeEach(() => {
 });
 
 afterEach(() => {
+    jest.useRealTimers();
     jest.restoreAllMocks();
 });
 

tests/backgroundApproval.test.js

@@ -537,7 +537,18 @@ async function settleIncludingRejections() {
     await new Promise((resolve) => setImmediate(resolve));
 }
 
+// While an approval is pending, the background starts a 20-second timer to
+// keep itself running. The timer is stubbed and never starts, so no test in
+// this file leaves it running.
+let keepRunningTimer;
+beforeEach(() => {
+    keepRunningTimer = jest
+        .spyOn(global, "setInterval")
+        .mockImplementation(() => 1);
+});
+
 afterEach(() => {
+    keepRunningTimer.mockRestore();
     delete global.chrome;
     jest.resetModules();
 });
@@ -1883,18 +1894,6 @@ describe("a closed approval window leaves the request pending", () => {
         return bg.actionPopups[bg.actionPopups.length - 1];
     }
 
-    // The timer that keeps the worker running while an approval is pending is
-    // stubbed, so no test here leaves that timer running when it ends.
-    let keepRunningTimer;
-    beforeEach(() => {
-        keepRunningTimer = jest
-            .spyOn(global, "setInterval")
-            .mockImplementation(() => 1);
-    });
-    afterEach(() => {
-        keepRunningTimer.mockRestore();
-    });
-
     test("the toolbar popup opens a closed transaction approval, and approving it there sends", async () => {
         const bg = loadBackground();
         const pending = bg.requestTx();

tests/backgroundStateIsolation.test.js

@@ -103,7 +103,18 @@ function deferred() {
     return { promise, resolve };
 }
 
+// While an approval is pending, the background starts a 20-second timer to
+// keep itself running. The timer is stubbed and never starts, so no test in
+// this file leaves it running.
+let keepRunningTimer;
+beforeEach(() => {
+    keepRunningTimer = jest
+        .spyOn(global, "setInterval")
+        .mockImplementation(() => 1);
+});
+
 afterEach(() => {
+    keepRunningTimer.mockRestore();
     delete global.chrome;
 });
 

tests/chainSwitchGate.test.js

@@ -56,7 +56,18 @@ async function settle() {
     for (let i = 0; i < 50; i++) await Promise.resolve();
 }
 
+// While an approval is pending, the background starts a 20-second timer to
+// keep itself running. The timer is stubbed and never starts, so no test in
+// this file leaves it running.
+let keepRunningTimer;
+beforeEach(() => {
+    keepRunningTimer = jest
+        .spyOn(global, "setInterval")
+        .mockImplementation(() => 1);
+});
+
 afterEach(() => {
+    keepRunningTimer.mockRestore();
     delete global.chrome;
 });
 

tests/coldWorkerChainSwitch.test.js

@@ -64,7 +64,18 @@ async function settle() {
     for (let i = 0; i < 50; i++) await Promise.resolve();
 }
 
+// While an approval is pending, the background starts a 20-second timer to
+// keep itself running. The timer is stubbed and never starts, so no test in
+// this file leaves it running.
+let keepRunningTimer;
+beforeEach(() => {
+    keepRunningTimer = jest
+        .spyOn(global, "setInterval")
+        .mockImplementation(() => 1);
+});
+
 afterEach(() => {
+    keepRunningTimer.mockRestore();
     delete global.chrome;
 });
 

tests/coldWorkerSendTransaction.test.js

@@ -76,7 +76,18 @@ async function settle() {
     for (let i = 0; i < 60; i++) await Promise.resolve();
 }
 
+// While an approval is pending, the background starts a 20-second timer to
+// keep itself running. The timer is stubbed and never starts, so no test in
+// this file leaves it running.
+let keepRunningTimer;
+beforeEach(() => {
+    keepRunningTimer = jest
+        .spyOn(global, "setInterval")
+        .mockImplementation(() => 1);
+});
+
 afterEach(() => {
+    keepRunningTimer.mockRestore();
     delete global.chrome;
 });
 

tests/connectedSitesWorkerRestart.test.js

@@ -57,7 +57,18 @@ async function settle() {
     for (let i = 0; i < 50; i++) await Promise.resolve();
 }
 
+// While an approval is pending, the background starts a 20-second timer to
+// keep itself running. The timer is stubbed and never starts, so no test in
+// this file leaves it running.
+let keepRunningTimer;
+beforeEach(() => {
+    keepRunningTimer = jest
+        .spyOn(global, "setInterval")
+        .mockImplementation(() => 1);
+});
+
 afterEach(() => {
+    keepRunningTimer.mockRestore();
     delete global.chrome;
     jest.resetModules();
 });

tests/deleteWalletLostPassword.test.js

@@ -102,6 +102,17 @@ function makeDocument() {
 
 // ------------------------------------------------------------ harness
 
+// showFlash() starts a timer that clears the message two seconds later. Each
+// test here runs on fake timers and drops them in afterEach, so that timer
+// never outlives the test.
+beforeEach(() => {
+    jest.useFakeTimers();
+});
+
+afterEach(() => {
+    jest.useRealTimers();
+});
+
 function wallet(name, secret, addresses) {
     return {
         type: "hd",

tests/sendAmountRule.test.js

@@ -131,10 +131,14 @@ async function review(amount, token) {
 async function confirm(amount, token) {
     const txInfo = await review("0.5", token);
     confirmTx.show({ ...txInfo, amount });
-    for (let i = 0; i < 10; i++) await new Promise((r) => setTimeout(r, 0));
+    for (let i = 0; i < 10; i++) await jest.advanceTimersByTimeAsync(0);
 }
 
+// showFlash() starts a timer that clears the message two seconds later. Each
+// test here runs on fake timers and drops them in afterEach, so that timer
+// never outlives the test.
 beforeEach(() => {
+    jest.useFakeTimers();
     elements.clear();
     state.wallets = [
         {
@@ -167,6 +171,10 @@ beforeEach(() => {
     state.currentView = null;
 });
 
+afterEach(() => {
+    jest.useRealTimers();
+});
+
 describe.each([
     ["1e-3", "ETH"],
     ["1e-3", "TOK"],

tests/sendMax.test.js

@@ -277,14 +277,18 @@ async function review() {
 
 // show() starts the fee estimate without awaiting it; this lets it settle.
 async function settle() {
-    for (let i = 0; i < 10; i++) await new Promise((r) => setTimeout(r, 0));
+    for (let i = 0; i < 10; i++) await jest.advanceTimersByTimeAsync(0);
 }
 
 function canSend() {
     return !el("btn-confirm-send").disabled;
 }
 
+// showFlash() starts a timer that clears the message two seconds later. Each
+// test here runs on fake timers and drops them in afterEach, so that timer
+// never outlives the test.
 beforeEach(() => {
+    jest.useFakeTimers();
     elements.clear();
     log.errorf.mockClear();
     pageClosed = new AbortController();
@@ -297,6 +301,10 @@ beforeEach(() => {
     mockNode.broadcast = [];
 });
 
+afterEach(() => {
+    jest.useRealTimers();
+});
+
 describe("Max on an ETH send", () => {
     test("fills in the exact balance minus the fee reserve", async () => {
         await refreshWith(BALANCE_WEI);

tests/walletDefects.test.js

@@ -357,6 +357,11 @@ describe("every way to a signature or the private key refuses a defective wallet
     });
 
     beforeEach(() => {
+        // showFlash() starts a timer that clears the message two seconds
+        // later. Each test here runs on fake timers and drops them in
+        // afterEach, so that timer never outlives the test.
+        jest.useFakeTimers();
+
         const broken = brokenXprvWallet();
         // A balance, so that no Send button's zero-balance refusal can stand
         // in for the defect check.
@@ -398,6 +403,7 @@ describe("every way to a signature or the private key refuses a defective wallet
     });
 
     afterEach(() => {
+        jest.useRealTimers();
         state.wallets = [];
         state.activeAddress = null;
         state.selectedWallet = null;