From bac1c23c62ca1ca0598d009fec243e39eec0fd22 Mon Sep 17 00:00:00 2001 From: clawbot Date: Wed, 12 Aug 2026 08:23:27 +0000 Subject: [PATCH] fix: treat an unreported holders_count as unknown, not as zero holders (closes #230) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- TODO.md | 4 + src/popup/views/send.js | 6 +- src/shared/balances.js | 16 +++- src/shared/holders.js | 32 +++++++ src/shared/transactions.js | 13 ++- tests/holders.test.js | 166 ++++++++++++++++++++++++++++++++++ tests/sendTokenSelect.test.js | 123 +++++++++++++++++++++++++ tests/transactions.test.js | 59 ++++++++++++ 8 files changed, 412 insertions(+), 7 deletions(-) create mode 100644 src/shared/holders.js create mode 100644 tests/holders.test.js create mode 100644 tests/sendTokenSelect.test.js diff --git a/TODO.md b/TODO.md index dcc905f..03d9eab 100644 --- a/TODO.md +++ b/TODO.md @@ -44,6 +44,10 @@ undefined identifiers, which is how # Completed Steps +- 2026-08-12: An unreported `holders_count` is now parsed as `null` rather than + `0`, so the low-holder rule declines to judge an unknown count instead of + hiding a legitimate token as spam, in both the transaction history and the + Send token selector ([#230](https://git.eeqj.de/sneak/AutistMask/issues/230)). - 2026-08-12: Bundled token list documentation no longer states a count. The four "top 250" claims in `README.md` and the "roughly 500" claim in `docs/README.md` are replaced with a description of how the list is actually diff --git a/src/popup/views/send.js b/src/popup/views/send.js index f91654f..6884764 100644 --- a/src/popup/views/send.js +++ b/src/popup/views/send.js @@ -13,6 +13,7 @@ const { state, currentAddress } = require("../../shared/state"); let ctx; const { getProvider } = require("../../shared/balances"); const { KNOWN_SYMBOLS, resolveSymbol } = require("../../shared/tokenList"); +const { isLowHolderCount } = require("../../shared/holders"); const { getAddress } = require("ethers"); const ZERO_ADDRESS = "0x0000000000000000000000000000000000000000"; @@ -132,7 +133,10 @@ function renderSendTokenSelect(addr) { for (const t of addr.tokenBalances || []) { if (isSpoofedToken(t)) continue; if (fraudSet.has(t.address.toLowerCase())) continue; - if (state.hideLowHolderTokens && (t.holders || 0) < 1000) continue; + // An unknown holder count does not withhold a token the user holds: + // only a count the explorer actually reported as below the threshold + // does. Otherwise a missing field makes a real asset unspendable. + if (state.hideLowHolderTokens && isLowHolderCount(t.holders)) continue; const opt = document.createElement("option"); opt.value = t.address; opt.textContent = t.symbol; diff --git a/src/shared/balances.js b/src/shared/balances.js index 46d5c5e..1ca5be5 100644 --- a/src/shared/balances.js +++ b/src/shared/balances.js @@ -12,6 +12,7 @@ const { ERC20_ABI } = require("./constants"); const { log, debugFetch } = require("./log"); const { deriveAddressFromXpub } = require("./wallet"); const { KNOWN_SYMBOLS, TOKEN_BY_ADDRESS } = require("./tokenList"); +const { LOW_HOLDER_THRESHOLD, parseHoldersCount } = require("./holders"); // Use a static network to skip auto-detection (which can fail and cause // "could not coalesce error" on some RPC endpoints like Cloudflare). @@ -70,10 +71,20 @@ async function fetchTokenBalances(address, blockscoutUrl, trackedTokens) { if (bal === "0.0") continue; const tokenAddr = (item.token.address_hash || "").toLowerCase(); - const holders = parseInt(item.token.holders_count || "0", 10); + // null means the explorer reported no count, which is not the + // same as a count of zero. This gate is not the low-holder + // display filter: it has no user-facing off switch and governs + // the whole balance list, so it stays strict and admits a token + // only on a reported count — an unreported one is no evidence. + // A legitimate token still reaches the list through the known + // token list or by the user tracking it, and the null is carried + // through to the views, where the two low-holder filters treat + // an unknown count as "do not judge" rather than as zero. + const holders = parseHoldersCount(item.token.holders_count); const isKnown = TOKEN_BY_ADDRESS.has(tokenAddr); const isTracked = trackedSet.has(tokenAddr); - const hasEnoughHolders = holders >= 1000; + const hasEnoughHolders = + holders !== null && holders >= LOW_HOLDER_THRESHOLD; // Skip spam tokens the user never asked to see if (!isKnown && !isTracked && !hasEnoughHolders) continue; @@ -278,6 +289,7 @@ async function scanForAddresses(xpub, rpcUrl, gapLimit = 5) { } module.exports = { + fetchTokenBalances, refreshBalances, lookupTokenInfo, getProvider, diff --git a/src/shared/holders.js b/src/shared/holders.js new file mode 100644 index 0000000..5038be1 --- /dev/null +++ b/src/shared/holders.js @@ -0,0 +1,32 @@ +// Holder counts, and the one rule that decides whether a count is "low". +// +// The block explorer's holders_count is optional: it is absent on a token it +// has only just indexed, and it goes missing on a degraded or changed API. +// Absent means the count is unknown. It does not mean the token has no +// holders, and collapsing the two hides a token the user really holds as if +// it were spam. Every call site reads the count through here so the +// distinction cannot be lost again in one place while holding in the others. + +const LOW_HOLDER_THRESHOLD = 1000; + +// Parse an explorer-supplied holders_count into a number, or null when the +// explorer did not report one. Anything unparseable is unknown too: a count +// we cannot read is not a count of zero. +function parseHoldersCount(raw) { + if (raw === null || raw === undefined || raw === "") return null; + const n = parseInt(raw, 10); + return Number.isFinite(n) ? n : null; +} + +// True only for a token the explorer reported as having fewer holders than +// the threshold. An unknown count is never low: showing a spam token the +// user can see is unusual costs less than hiding an asset they own. +function isLowHolderCount(holders) { + return holders != null && holders < LOW_HOLDER_THRESHOLD; +} + +module.exports = { + LOW_HOLDER_THRESHOLD, + parseHoldersCount, + isLowHolderCount, +}; diff --git a/src/shared/transactions.js b/src/shared/transactions.js index ee7a9fe..b4c9638 100644 --- a/src/shared/transactions.js +++ b/src/shared/transactions.js @@ -9,6 +9,7 @@ const { formatEther, formatUnits } = require("ethers"); const { log, debugFetch } = require("./log"); const { KNOWN_SYMBOLS, TOKEN_BY_ADDRESS } = require("./tokenList"); +const { parseHoldersCount, isLowHolderCount } = require("./holders"); // Ethereum addresses are case-insensitive: EIP-55 mixed case is a checksum // over the address, not part of its identity. Every address comparison in @@ -116,7 +117,10 @@ function parseTokenTransfer(tt, addrLower) { contractAddress: normalizeAddress( tt.token?.address_hash || tt.token?.address || "", ), - holders: parseInt(tt.token?.holders_count || "0", 10), + // null when the explorer reported no count: unknown, not zero. The + // low-holder filter declines to judge a null, so a legitimate token + // is not hidden because a field went missing upstream. + holders: parseHoldersCount(tt.token?.holders_count), }; } @@ -292,12 +296,13 @@ function filterTransactions(txs, filters = {}) { continue; } - // Filter low-holder tokens (<1000) if setting is on + // Filter low-holder tokens (<1000) if setting is on. A token whose + // holder count the explorer did not report is kept: only a reported + // count below the threshold is "low". if ( filters.hideLowHolderTokens && tx.contractAddress && - tx.holders !== null && - tx.holders < 1000 + isLowHolderCount(tx.holders) ) { continue; } diff --git a/tests/holders.test.js b/tests/holders.test.js new file mode 100644 index 0000000..48d7ce7 --- /dev/null +++ b/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(); + }); +}); diff --git a/tests/sendTokenSelect.test.js b/tests/sendTokenSelect.test.js new file mode 100644 index 0000000..d14af89 --- /dev/null +++ b/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(''); + 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([]); + }); +}); diff --git a/tests/transactions.test.js b/tests/transactions.test.js index d7c6043..962173b 100644 --- a/tests/transactions.test.js +++ b/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, -- 2.49.1