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

clawbot · 2026-08-12T10:24:57+02:00

clawbot commented

2026-08-12 10:24:57 +02:00

Closes #230.

holders_count is optional in the explorer's response. Reading it as
holders_count || "0" recorded "the explorer said nothing" as "this token has
no holders" — the strongest spam signal the wallet has. Consequences: a
legitimate transfer vanished from history, a token the user holds vanished from
the Send selector, and the tx.holders !== null guard in filterTransactions
was unreachable for token transfers, because the coercion guaranteed a number.

What changed

New src/shared/holders.js owns the null-versus-zero rule and the
1,000-holder threshold. The rule was open-coded at three call sites and was
wrong at all three; now there is one place to be wrong.

parseHoldersCount(raw) -> number, or null when the count is omitted,
null, empty, or unparseable. A count we cannot read is not a count of zero.
isLowHolderCount(holders) -> true only for a reported count below the
threshold.

Call sites:

parseTokenTransfer (src/shared/transactions.js) emits holders: null
instead of 0.
filterTransactions low-holder rule now uses isLowHolderCount, which makes
the previously dead holders !== null guard live.
renderSendTokenSelect (src/popup/views/send.js:135) uses the same
predicate instead of (t.holders || 0) < 1000.
fetchTokenBalances (src/shared/balances.js) stops recording an unreported
count as 0 — see the decision below for why its own gate is unchanged.
fetchTokenBalances is now exported so the gate can be tested.

The decision: an unknown count shows the token

In the two user-facing low-holder filters (history, Send selector): unknown
means shown. Hiding an asset the user owns is a worse failure than showing a
spam row they can see is unusual — in the Send selector it is strictly worse,
since the token becomes unspendable through the UI rather than merely hidden.
Both filters sit behind the "Hide tokens with fewer than 1,000 holders"
setting, so a user who wants maximum strictness is not being overruled; and the
holders !== null guard already in filterTransactions shows this was the
intended policy all along, defeated by the coercion upstream of it.

In the balance-list spam gate (fetchTokenBalances): unknown stays
excluded. This one is deliberately the other way, and it is not the same
filter. It has no off switch, it governs what the balance list contains at all
rather than what a filter hides, and the tokens most likely to be missing their
holder count are exactly the newly-indexed ones — which is what a spam airdrop
is. Admitting unknowns there would put unfilterable spam on the home screen,
with the setting powerless over it. A legitimate token still reaches the list
through the bundled token list or by the user tracking it, and it now carries
holders: null through, so the downstream filters no longer hide it on the
strength of a zero it never reported. That is the case the Send-selector fix
actually rescues.

Other `|| 0` / `|| "0"` coercions

Swept src/ for the same pattern. Nothing else collapses a meaningful absent
into zero:

balance || "0" (prices.js, home.js, helpers.js, addressToken.js,
send.js, confirmTx.js) — feeds numeric display and arithmetic, never a
hide/show decision. An address with no fetched balance has nothing to show
and "0" is the honest placeholder.
txParams.value || "0" (approval.js) — EIP-1193 omits value for a
zero-value call, so absent is zero by spec.
tx.value || "0", total.value || "0", rawAmount || "0"
(transactions.js) — the value of a transaction is not optional in the API,
and a call that moves nothing is genuinely zero-valued.
lastBalanceRefresh || 0 (state.js, background/index.js) — absent means
"never refreshed" and 0 (the epoch) produces the identical outcome: the
staleness check passes and a refresh runs. Fail-open in the right direction.
.replace(/0+$/, "") || "0" (balances.js, confirmTx.js) — string
formatting of an all-zero fraction, not a value decision.
decimals || "18" (transactions.js, balances.js) — worth naming because
0 decimals is a legal ERC-20 value: Blockscout sends decimals as a string,
and "0" is truthy, so absent and zero stay distinct. A numeric 0 would
collapse; left alone rather than hardened, as it is a different field and out
of this issue's scope.

Verification

Tests were written first and confirmed failing on unmodified next — 5 failed
(3 in tests/transactions.test.js, 2 in tests/sendTokenSelect.test.js), with
the zero-holder regression assertions passing before and after, so they pin the
old behaviour rather than the new.

tests/transactions.test.js — omitted and null holders_count parse to
null and survive the low-holder filter end-to-end through
fetchRecentTransactions; a reported "0" still parses to 0 and is still
filtered. The zero fixture uses a symbol absent from the token list, so the
holder rule is the only rule that can catch it.
tests/sendTokenSelect.test.js (new) — drives renderSendTokenSelect
against a stub document: unknown and missing counts are offered, 0 and 999
are withheld, 1000 is offered, the setting still bypasses the rule, and the
spoof and fraud-contract rules still withhold a token with an unknown count.
tests/holders.test.js (new) — the parse and threshold rules directly, plus
the balance-list gate: zero and unknown both stay excluded for an unvouched
token, while a known-list or tracked token is listed with holders: null.

make check green after the rebase onto ba35282: 16 suites, 388 tests
passed, prettier --check clean. Rebased onto current next immediately
before pushing; the TODO.md conflict against
#239 was resolved keeping
both entries, and make check was re-run after resolving.

Closes [#230](https://git.eeqj.de/sneak/AutistMask/issues/230). `holders_count` is optional in the explorer's response. Reading it as `holders_count || "0"` recorded "the explorer said nothing" as "this token has no holders" — the strongest spam signal the wallet has. Consequences: a legitimate transfer vanished from history, a token the user holds vanished from the Send selector, and the `tx.holders !== null` guard in `filterTransactions` was unreachable for token transfers, because the coercion guaranteed a number. ## What changed New `src/shared/holders.js` owns the null-versus-zero rule and the 1,000-holder threshold. The rule was open-coded at three call sites and was wrong at all three; now there is one place to be wrong. - `parseHoldersCount(raw)` -> number, or `null` when the count is omitted, `null`, empty, or unparseable. A count we cannot read is not a count of zero. - `isLowHolderCount(holders)` -> true only for a *reported* count below the threshold. Call sites: - `parseTokenTransfer` (`src/shared/transactions.js`) emits `holders: null` instead of `0`. - `filterTransactions` low-holder rule now uses `isLowHolderCount`, which makes the previously dead `holders !== null` guard live. - `renderSendTokenSelect` (`src/popup/views/send.js:135`) uses the same predicate instead of `(t.holders || 0) < 1000`. - `fetchTokenBalances` (`src/shared/balances.js`) stops recording an unreported count as `0` — see the decision below for why its own gate is unchanged. `fetchTokenBalances` is now exported so the gate can be tested. ## The decision: an unknown count shows the token **In the two user-facing low-holder filters (history, Send selector): unknown means shown.** Hiding an asset the user owns is a worse failure than showing a spam row they can see is unusual — in the Send selector it is strictly worse, since the token becomes unspendable through the UI rather than merely hidden. Both filters sit behind the "Hide tokens with fewer than 1,000 holders" setting, so a user who wants maximum strictness is not being overruled; and the `holders !== null` guard already in `filterTransactions` shows this was the intended policy all along, defeated by the coercion upstream of it. **In the balance-list spam gate (`fetchTokenBalances`): unknown stays excluded.** This one is deliberately the other way, and it is not the same filter. It has no off switch, it governs what the balance list contains at all rather than what a filter hides, and the tokens most likely to be missing their holder count are exactly the newly-indexed ones — which is what a spam airdrop is. Admitting unknowns there would put unfilterable spam on the home screen, with the setting powerless over it. A legitimate token still reaches the list through the bundled token list or by the user tracking it, and it now carries `holders: null` through, so the downstream filters no longer hide it on the strength of a zero it never reported. That is the case the Send-selector fix actually rescues. ## Other `|| 0` / `|| "0"` coercions Swept `src/` for the same pattern. Nothing else collapses a meaningful absent into zero: - `balance || "0"` (`prices.js`, `home.js`, `helpers.js`, `addressToken.js`, `send.js`, `confirmTx.js`) — feeds numeric display and arithmetic, never a hide/show decision. An address with no fetched balance has nothing to show and "0" is the honest placeholder. - `txParams.value || "0"` (`approval.js`) — EIP-1193 omits `value` for a zero-value call, so absent *is* zero by spec. - `tx.value || "0"`, `total.value || "0"`, `rawAmount || "0"` (`transactions.js`) — the value of a transaction is not optional in the API, and a call that moves nothing is genuinely zero-valued. - `lastBalanceRefresh || 0` (`state.js`, `background/index.js`) — absent means "never refreshed" and 0 (the epoch) produces the identical outcome: the staleness check passes and a refresh runs. Fail-open in the right direction. - `.replace(/0+$/, "") || "0"` (`balances.js`, `confirmTx.js`) — string formatting of an all-zero fraction, not a value decision. - `decimals || "18"` (`transactions.js`, `balances.js`) — worth naming because 0 decimals is a legal ERC-20 value: Blockscout sends `decimals` as a string, and `"0"` is truthy, so absent and zero stay distinct. A numeric `0` would collapse; left alone rather than hardened, as it is a different field and out of this issue's scope. ## Verification Tests were written first and confirmed failing on unmodified `next` — 5 failed (3 in `tests/transactions.test.js`, 2 in `tests/sendTokenSelect.test.js`), with the zero-holder regression assertions passing before and after, so they pin the old behaviour rather than the new. - `tests/transactions.test.js` — omitted and `null` `holders_count` parse to `null` and survive the low-holder filter end-to-end through `fetchRecentTransactions`; a reported `"0"` still parses to `0` and is still filtered. The zero fixture uses a symbol absent from the token list, so the holder rule is the only rule that can catch it. - `tests/sendTokenSelect.test.js` (new) — drives `renderSendTokenSelect` against a stub document: unknown and missing counts are offered, 0 and 999 are withheld, 1000 is offered, the setting still bypasses the rule, and the spoof and fraud-contract rules still withhold a token with an unknown count. - `tests/holders.test.js` (new) — the parse and threshold rules directly, plus the balance-list gate: zero and unknown both stay excluded for an unvouched token, while a known-list or tracked token is listed with `holders: null`. `make check` green after the rebase onto `ba35282`: **16 suites, 388 tests passed**, `prettier --check` clean. Rebased onto current `next` immediately before pushing; the `TODO.md` conflict against [#239](https://git.eeqj.de/sneak/AutistMask/issues/239) was resolved keeping both entries, and `make check` was re-run after resolving.

clawbot added the needs-review label 2026-08-12 10:24:57 +02:00

clawbot added 1 commit 2026-08-12 10:24:58 +02:00

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

check / check (push) Successful in 30s

Details

bac1c23c62

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.

clawbot referenced this pull request

2026-08-12 10:25:07 +02:00

fix: an omitted holders_count is coerced to 0, so a legitimate token gets filtered as spam #230

clawbot referenced this pull request

2026-08-12 10:26:40 +02:00

harden: decimals || "18" collapses absent and zero if a backend ever sends numeric 0 #246

clawbot commented

2026-08-12 10:34:01 +02:00

PASS — independent review of bac1c23. Every DoD item in #230 verified by execution: the 5 claimed pre-change failures reproduce exactly (3 tests/transactions.test.js, 2 tests/sendTokenSelect.test.js) at merge base ba35282, and the zero-holder regression assertions are non-vacuous — mutating isLowHolderCount to treat a reported 0 as unknown kills 6 tests, mutating parseHoldersCount to return null for 0 kills 3, and reverting it to return 0 for an unknown kills 6. make check and containerized script/cibuild both green, 16 suites / 388 tests, executed not cached.

Non-blocking notes:

src/shared/holders.js:13-18 — the comment says "Anything unparseable is unknown too", but parseInt partial-parses: "1,000" -> 1 and "0x10" -> 0, i.e. a malformed count beginning with a digit becomes a reported low count and triggers the exact hide this PR exists to prevent. Not a regression (pre-change used the same parseInt) and no Blockscout output looks like this, so it is a contract overstatement rather than a live bug. A strict form (/^\s*\d+\s*$/ before parsing) would make the code match the comment.
src/shared/balances.js:87 — the holders !== null && conjunct is inert; null >= 1000 is already false. Removing it passes all 388 tests. Defensive explicitness, not an untested guard.
Undisclosed but benign side effect: src/popup/views/addressToken.js:189 now omits the "Holders:" row for an unreported count instead of displaying "Holders: 0". An improvement; worth a line in the PR body.
The decimals || "18" numeric-0 collapse named and deferred in the PR body is a real latent bug with no tracker issue behind it. Worth filing.

Split decision: coherent. tokenBalances is the single source for both the balance list and the Send selector, and the balance list applies no filter of its own (src/popup/views/helpers.js:195), so the "no off switch" claim holds and "spendable but invisible" is impossible by construction; the change strictly reduces the pre-existing visible-but-unspendable divergence.

#235: unchanged, neither better nor worse. The balance gate's admission outcome is identical pre/post (0 and null both fail >= 1000) and the null-mapped-symbol check at src/shared/balances.js:95-100 is untouched. In the Send selector an unknown-count fake ETH is no longer withheld by the low-holder rule, but isSpoofedToken catches it first — pinned by the new test at tests/sendTokenSelect.test.js:106.

next moved to bf1dbec after this was pushed. The branch is no longer a fast-forward but merges cleanly, and the merged tree is green (18 suites / 394 tests), so no rebase is required for correctness.

**PASS** — independent review of `bac1c23`. Every DoD item in [#230](https://git.eeqj.de/sneak/AutistMask/issues/230) verified by execution: the 5 claimed pre-change failures reproduce exactly (3 `tests/transactions.test.js`, 2 `tests/sendTokenSelect.test.js`) at merge base `ba35282`, and the zero-holder regression assertions are non-vacuous — mutating `isLowHolderCount` to treat a reported 0 as unknown kills 6 tests, mutating `parseHoldersCount` to return `null` for 0 kills 3, and reverting it to return `0` for an unknown kills 6. `make check` and containerized `script/cibuild` both green, 16 suites / 388 tests, executed not cached. Non-blocking notes: - `src/shared/holders.js:13-18` — the comment says "Anything unparseable is unknown too", but `parseInt` partial-parses: `"1,000"` -> `1` and `"0x10"` -> `0`, i.e. a malformed count beginning with a digit becomes a *reported low* count and triggers the exact hide this PR exists to prevent. Not a regression (pre-change used the same `parseInt`) and no Blockscout output looks like this, so it is a contract overstatement rather than a live bug. A strict form (`/^\s*\d+\s*$/` before parsing) would make the code match the comment. - `src/shared/balances.js:87` — the `holders !== null &&` conjunct is inert; `null >= 1000` is already `false`. Removing it passes all 388 tests. Defensive explicitness, not an untested guard. - Undisclosed but benign side effect: `src/popup/views/addressToken.js:189` now omits the "Holders:" row for an unreported count instead of displaying "Holders: 0". An improvement; worth a line in the PR body. - The `decimals || "18"` numeric-`0` collapse named and deferred in the PR body is a real latent bug with no tracker issue behind it. Worth filing. Split decision: coherent. `tokenBalances` is the single source for both the balance list and the Send selector, and the balance list applies no filter of its own (`src/popup/views/helpers.js:195`), so the "no off switch" claim holds and "spendable but invisible" is impossible by construction; the change strictly *reduces* the pre-existing visible-but-unspendable divergence. [#235](https://git.eeqj.de/sneak/AutistMask/issues/235): **unchanged**, neither better nor worse. The balance gate's admission outcome is identical pre/post (`0` and `null` both fail `>= 1000`) and the null-mapped-symbol check at `src/shared/balances.js:95-100` is untouched. In the Send selector an unknown-count fake `ETH` is no longer withheld by the low-holder rule, but `isSpoofedToken` catches it first — pinned by the new test at `tests/sendTokenSelect.test.js:106`. `next` moved to `bf1dbec` after this was pushed. The branch is no longer a fast-forward but merges cleanly, and the merged tree is green (18 suites / 394 tests), so no rebase is required for correctness.

clawbot merged commit ce4a0d7b8d into next

2026-08-12 10:34:46 +02:00

clawbot deleted branch fix/issue-230-unknown-holders-count

2026-08-12 10:34:46 +02:00

clawbot referenced this pull request

2026-08-12 10:35:02 +02:00

harden: parseHoldersCount partial-parses a malformed count into a reported low count, hiding the token #251

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/AutistMask#244

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

What changed

The decision: an unknown count shows the token

Other || 0 / || "0" coercions

Verification

Other `|| 0` / `|| "0"` coercions