fix: treat an unreported holders_count as unknown, not as zero holders (closes #230)

The block explorer's holders_count is optional. Reading it as
`holders_count || "0"` recorded a token the explorer said nothing about
as a token with no holders at all, which is the strongest spam signal the
wallet has: the low-holder rule then hid a legitimate transfer from the
history and withheld a token the user actually holds from the Send
selector. It also made the `tx.holders !== null` guard in
filterTransactions unreachable for token transfers, since the coercion
guaranteed a number.

The null-versus-zero rule and the 1,000-holder threshold now live in one
place, src/shared/holders.js, because the rule was open-coded at three
call sites and got it wrong at all three.

An unknown count is shown rather than hidden in both user-facing
filters: hiding an asset the user owns costs more than showing a spam row
they can see is unusual, and both filters have a setting behind them.
The balance-list spam gate in fetchTokenBalances keeps its strict
behaviour — it has no off switch and governs the whole balance list, so
an unreported count is no evidence for admission — but it now records the
unknown as null, so a token that reaches the list by being known or
tracked is no longer hidden downstream by a zero it never reported.

A reported count of zero still parses to 0 and is still filtered
everywhere; that is covered by tests alongside the unknown-count ones.

tests/holders.test.js

@@ -0,0 +1,166 @@
+// Tests for src/shared/holders.js and the balance-list spam gate that reads
+// it (issue #230).
+//
+// The rule these pin down: an explorer that reports no holders_count has told
+// us nothing, and "nothing" must not be recorded as "zero holders".  Zero is
+// the strongest spam signal the wallet has, so handing it out for free turns
+// a missing field into a hidden asset.
+
+jest.mock("../src/shared/log", () => ({
+    log: {
+        debugf: () => {},
+        infof: () => {},
+        warnf: () => {},
+        errorf: () => {},
+    },
+    debugFetch: jest.fn(),
+    setRuntimeDebug: () => {},
+    isDebug: () => false,
+}));
+
+global.fetch = jest.fn(() => {
+    throw new Error("tests must not perform network requests");
+});
+global.chrome = { storage: { local: {} } };
+
+const {
+    LOW_HOLDER_THRESHOLD,
+    parseHoldersCount,
+    isLowHolderCount,
+} = require("../src/shared/holders");
+const { fetchTokenBalances } = require("../src/shared/balances");
+const { debugFetch } = require("../src/shared/log");
+
+const BLOCKSCOUT = "https://eth.blockscout.com/api/v2";
+const HOLDER = "0x66133e8ea0f5d1d612d2502a968757d1048c214a";
+const USDC_CONTRACT = "0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48";
+const NOVEL_TOKEN = "0x1111111111111111111111111111111111111111";
+
+describe("parseHoldersCount", () => {
+    test("a reported count parses to that number", () => {
+        expect(parseHoldersCount("3500000")).toBe(3500000);
+        expect(parseHoldersCount(3500000)).toBe(3500000);
+    });
+
+    test('a reported "0" parses to 0, which is not null', () => {
+        expect(parseHoldersCount("0")).toBe(0);
+        expect(parseHoldersCount(0)).toBe(0);
+    });
+
+    test("an omitted, null or empty count is unknown", () => {
+        expect(parseHoldersCount(undefined)).toBeNull();
+        expect(parseHoldersCount(null)).toBeNull();
+        expect(parseHoldersCount("")).toBeNull();
+    });
+
+    test("an unparseable count is unknown rather than zero", () => {
+        expect(parseHoldersCount("many")).toBeNull();
+        expect(parseHoldersCount(NaN)).toBeNull();
+    });
+});
+
+describe("isLowHolderCount", () => {
+    test("the threshold is the documented 1,000 holders", () => {
+        expect(LOW_HOLDER_THRESHOLD).toBe(1000);
+    });
+
+    test("a reported count below the threshold is low", () => {
+        expect(isLowHolderCount(0)).toBe(true);
+        expect(isLowHolderCount(999)).toBe(true);
+    });
+
+    test("a reported count at or above the threshold is not low", () => {
+        expect(isLowHolderCount(1000)).toBe(false);
+        expect(isLowHolderCount(1001)).toBe(false);
+    });
+
+    test("an unknown count is not low", () => {
+        expect(isLowHolderCount(null)).toBe(false);
+        expect(isLowHolderCount(undefined)).toBe(false);
+    });
+});
+
+// fetchTokenBalances applies its own spam gate, which is not the low-holder
+// display filter: it has no setting behind it and decides what the balance
+// list contains at all.  It stays strict on an unknown count — see the
+// comment at the gate — but must stop recording that unknown as zero.
+describe("the balance-list spam gate", () => {
+    function respondWith(items) {
+        debugFetch.mockImplementation(async () => ({
+            ok: true,
+            status: 200,
+            statusText: "OK",
+            json: async () => items,
+        }));
+    }
+
+    function item(overrides = {}) {
+        const { token, ...rest } = overrides;
+        return {
+            value: "12500000",
+            ...rest,
+            token: {
+                type: "ERC-20",
+                address_hash: NOVEL_TOKEN,
+                symbol: "SPAMTKN",
+                name: "Spam Token",
+                decimals: "6",
+                holders_count: "50000",
+                ...token,
+            },
+        };
+    }
+
+    beforeEach(() => {
+        debugFetch.mockReset();
+    });
+
+    test("a token with plenty of reported holders is listed", async () => {
+        respondWith([item()]);
+        const balances = await fetchTokenBalances(HOLDER, BLOCKSCOUT, []);
+        expect(balances).toHaveLength(1);
+        expect(balances[0].holders).toBe(50000);
+    });
+
+    test("a token reporting zero holders is still excluded", async () => {
+        respondWith([item({ token: { holders_count: "0" } })]);
+        expect(await fetchTokenBalances(HOLDER, BLOCKSCOUT, [])).toEqual([]);
+    });
+
+    test("an unknown holder count does not admit an unvouched token", async () => {
+        respondWith([item({ token: { holders_count: null } })]);
+        expect(await fetchTokenBalances(HOLDER, BLOCKSCOUT, [])).toEqual([]);
+    });
+
+    // The path that reaches the send selector and the history filter: a token
+    // the user vouched for by tracking it is listed whatever the explorer
+    // says, and it must carry the unknown count through as null, not as the
+    // zero that would then hide it downstream.
+    test("a tracked token with an unknown count is listed with holders null", async () => {
+        respondWith([item({ token: { holders_count: undefined } })]);
+        const balances = await fetchTokenBalances(HOLDER, BLOCKSCOUT, [
+            { address: NOVEL_TOKEN.toUpperCase() },
+        ]);
+        expect(balances).toHaveLength(1);
+        expect(balances[0].holders).toBeNull();
+    });
+
+    test("a known-list token with an unknown count is listed with holders null", async () => {
+        respondWith([
+            item({
+                token: {
+                    address_hash: USDC_CONTRACT,
+                    symbol: "USDC",
+                    holders_count: null,
+                },
+            }),
+        ]);
+        const balances = await fetchTokenBalances(HOLDER, BLOCKSCOUT, []);
+        expect(balances).toHaveLength(1);
+        expect(balances[0].holders).toBeNull();
+    });
+
+    test("no test in this file performed a network request", () => {
+        expect(global.fetch).not.toHaveBeenCalled();
+    });
+});

tests/sendTokenSelect.test.js

@@ -0,0 +1,123 @@
+// Tests for the token filtering in the Send view's token selector
+// (src/popup/views/send.js).
+//
+// The selector decides which of the user's tokens can be spent at all, so
+// over-filtering here is worse than in the history list: the asset is not
+// merely hidden, it becomes unspendable through the UI.  Issue #230: an
+// explorer that omits holders_count was read as "zero holders" and the token
+// disappeared from this list.
+//
+// renderSendTokenSelect only ever touches getElementById, createElement,
+// innerHTML, value, textContent and appendChild, so a small stub document is
+// enough to drive it; the real DOM behaviour of the view is covered by
+// tests/e2e/run.js.
+
+globalThis.chrome = {
+    storage: { local: { get: async () => ({}), set: async () => {} } },
+};
+
+const { state } = require("../src/shared/state");
+const { renderSendTokenSelect } = require("../src/popup/views/send");
+
+const USDC_CONTRACT = "0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48";
+const NOVEL_TOKEN = "0x1111111111111111111111111111111111111111";
+
+let select;
+
+function installStubDocument() {
+    select = { innerHTML: "", children: [] };
+    select.appendChild = (child) => select.children.push(child);
+    globalThis.document = {
+        getElementById: (id) => (id === "send-token" ? select : null),
+        createElement: () => ({ value: "", textContent: "" }),
+    };
+}
+
+// The symbols offered for sending, excluding the hardcoded ETH option that
+// renderSendTokenSelect writes straight into innerHTML.
+function offeredTokens() {
+    return select.children.map((opt) => opt.value.toLowerCase());
+}
+
+function tokenBalance(overrides) {
+    return {
+        address: NOVEL_TOKEN,
+        symbol: "SPAMTKN",
+        decimals: 18,
+        balance: "12.5",
+        holders: 50000,
+        ...overrides,
+    };
+}
+
+function render(tokenBalances) {
+    installStubDocument();
+    renderSendTokenSelect({ address: "0x" + "a".repeat(40), tokenBalances });
+}
+
+beforeEach(() => {
+    state.fraudContracts = [];
+    state.hideLowHolderTokens = true;
+});
+
+describe("the low-holder rule in the send token selector", () => {
+    test("ETH is always offered", () => {
+        render([]);
+        expect(select.innerHTML).toBe('<option value="ETH">ETH</option>');
+        expect(offeredTokens()).toEqual([]);
+    });
+
+    test("a token with plenty of holders is offered", () => {
+        render([tokenBalance()]);
+        expect(offeredTokens()).toEqual([NOVEL_TOKEN]);
+    });
+
+    test("a token reporting zero holders is withheld", () => {
+        render([tokenBalance({ holders: 0 })]);
+        expect(offeredTokens()).toEqual([]);
+    });
+
+    test("boundary: 999 holders is withheld, 1000 is offered", () => {
+        render([tokenBalance({ holders: 999 })]);
+        expect(offeredTokens()).toEqual([]);
+        render([tokenBalance({ holders: 1000 })]);
+        expect(offeredTokens()).toEqual([NOVEL_TOKEN]);
+    });
+
+    // Issue #230: an unknown holder count must not read as zero.  A token the
+    // user demonstrably holds — it has a balance — cannot be made unspendable
+    // by a field the block explorer failed to report.
+    test("a token whose holder count is unknown is still offered", () => {
+        render([tokenBalance({ holders: null })]);
+        expect(offeredTokens()).toEqual([NOVEL_TOKEN]);
+    });
+
+    test("a token balance carrying no holders field at all is offered", () => {
+        const t = tokenBalance();
+        delete t.holders;
+        render([t]);
+        expect(offeredTokens()).toEqual([NOVEL_TOKEN]);
+    });
+
+    test("the rule is bypassed entirely when the setting is off", () => {
+        state.hideLowHolderTokens = false;
+        render([tokenBalance({ holders: 0 })]);
+        expect(offeredTokens()).toEqual([NOVEL_TOKEN]);
+    });
+});
+
+describe("the other send-selector rules are unaffected", () => {
+    test("a token spoofing a known symbol from a wrong address is withheld", () => {
+        render([
+            tokenBalance({ symbol: "USDC", holders: null }),
+            tokenBalance({ address: USDC_CONTRACT, symbol: "USDC" }),
+        ]);
+        expect(offeredTokens()).toEqual([USDC_CONTRACT.toLowerCase()]);
+    });
+
+    test("a blocklisted fraud contract is withheld even with an unknown count", () => {
+        state.fraudContracts = [NOVEL_TOKEN.toUpperCase()];
+        render([tokenBalance({ holders: null })]);
+        expect(offeredTokens()).toEqual([]);
+    });
+});

tests/transactions.test.js

@@ -1473,6 +1473,65 @@ describe("fetchRecentTransactions merge and dedup", () => {
         expect(result.newFraudContracts).toEqual([FAKE_ETH_CONTRACT]);
     });
 
+    // Regression guards (#230): the explorer's holders_count is optional.  A
+    // missing field means the count is unknown; it does not mean the token
+    // has no holders.  Recording the two as the same number both hides a
+    // legitimate token and makes the `holders !== null` guard in
+    // filterTransactions unreachable for token transfers.
+    describe("an unreported holders_count is unknown, not zero", () => {
+        function spamTransferWithToken(token) {
+            return [
+                {
+                    transaction_hash: "0x" + "9".repeat(64),
+                    block_number: 21000070,
+                    timestamp: TS,
+                    from: { hash: ORDINARY_PEER },
+                    to: { hash: VICTIM },
+                    total: { value: "1500500000", decimals: "6" },
+                    token: token,
+                },
+            ];
+        }
+
+        const OMITTED = {
+            symbol: NOVEL_SPAM_SYMBOL,
+            address_hash: NOVEL_SPAM_CONTRACT,
+        };
+        const NULLED = { ...OMITTED, holders_count: null };
+        const ZERO = { ...OMITTED, holders_count: "0" };
+
+        test("an omitted holders_count parses to null", async () => {
+            respondWith([], spamTransferWithToken(OMITTED));
+            const txs = await fetchRecentTransactions(VICTIM, BLOCKSCOUT);
+            expect(txs[0].holders).toBeNull();
+        });
+
+        test("a null holders_count parses to null", async () => {
+            respondWith([], spamTransferWithToken(NULLED));
+            const txs = await fetchRecentTransactions(VICTIM, BLOCKSCOUT);
+            expect(txs[0].holders).toBeNull();
+        });
+
+        test("the transfer survives the low-holder filter", async () => {
+            respondWith([], spamTransferWithToken(OMITTED));
+            const txs = await fetchRecentTransactions(VICTIM, BLOCKSCOUT);
+            expect(filterTransactions(txs, filters()).transactions).toEqual(
+                txs,
+            );
+        });
+
+        // The regression this fix could cause: a token that genuinely
+        // reports zero holders must keep being filtered.  Unlike the fake
+        // "ETH" fixture above, this symbol is not in the token list, so the
+        // holder count is the only rule that can catch it.
+        test('a reported holders_count of "0" still parses to 0 and is filtered', async () => {
+            respondWith([], spamTransferWithToken(ZERO));
+            const txs = await fetchRecentTransactions(VICTIM, BLOCKSCOUT);
+            expect(txs[0].holders).toBe(0);
+            expect(filterTransactions(txs, filters()).transactions).toEqual([]);
+        });
+    });
+
     test("failed responses yield an empty list rather than throwing", async () => {
         debugFetch.mockImplementation(async () => ({
             ok: false,