fix: honour a dust threshold of 0 and compare addresses case-insensitively (closes #179) #228

clawbot · 2026-08-11T15:09:24+02:00

clawbot commented

2026-08-11 15:09:24 +02:00

Closes #179.

Two defects in src/shared/transactions.js, both silent over-filtering: the
wallet hid transactions the user had asked to see.

1. A dust threshold of `0`

filters.dustThresholdGwei || 100000 swallowed the one value a user would
pick to mean "show everything". It is now ??, computed once before the
filter loop, so 0 is a real threshold: no transaction has a value below
0 gwei, so nothing is hidden. That makes it exactly equivalent to clearing the
hide-dust checkbox, and neither control can override the other. README.md
now says so in the dust-filtering bullet.

src/shared/state.js already distinguished unset from zero on load
(saved.dustThresholdGwei !== undefined), so a stored 0 survives a reload.

Settings input (`src/popup/views/settings.js:305`)

Was parseInt(value, 10) accepted when !isNaN(val) && val >= 0. What each
input does now, and what changed:

input	before	now
`0`	stored as 0, then ignored by the filter	stored as 0, honoured — hides nothing
empty / whitespace	rejected, field kept showing the empty box	rejected, field resyncs to the stored threshold
negative (`-5`)	rejected, field kept showing `-5`	rejected, field resyncs
non-numeric (`abc`)	rejected, field kept showing `abc`	rejected, field resyncs
trailing junk (`100 gwei`)	accepted as 100 by `parseInt`	rejected, field resyncs
fractional (`1.5`)	accepted as 1 by `parseInt`	rejected, field resyncs

Rejected input never mutates state, and the field is put back to the stored
value so it cannot display a threshold the wallet is not using.

2. `isSpoofedSymbol` was case-sensitive on the contract address

EIP-55 mixed case is a checksum over the address, not part of its identity, so
tx.contractAddress !== legit classified a genuine token arriving checksummed
as a spoof — hiding a real transfer and recording the real contract on the
fraud blocklist. Every address comparison in the module now goes through one
normalizeAddress() helper (parseTx direction and token lookup,
parseTokenTransfer direction and contract, fetchRecentTransactions,
isSpoofedSymbol, the fraud set and its lookups). The contract recorded in
newFraudContracts is normalised too, so the persisted blocklist matches
later transfers in any casing.

mergeTransactions() and its tests from
#177 are untouched.

Failing first

Six new tests, run against the current src/shared/transactions.js with only
the test file changed:

  ● known-symbol spoof verification › a genuine contract in EIP-55 checksummed form is not a spoof
  ● known-symbol spoof verification › a genuine contract in all-uppercase form is not a spoof
  ● known-symbol spoof verification › a genuinely different contract claiming USDC is still a spoof in any casing
  ● dust threshold filtering › a threshold of 0 hides nothing, leaving the toggle on
  ● dust threshold filtering › a threshold of 0 agrees with clearing the hide-dust checkbox
  ● dust threshold filtering › 0, unset and a set threshold are three distinct behaviours

Test Suites: 1 failed, 8 passed, 9 total
Tests:       6 failed, 173 passed, 179 total

The checksummed-genuine and threshold-0 cases fail the same way — the
transaction the user should see is missing:

  ● dust threshold filtering › a threshold of 0 hides nothing, leaving the toggle on

    expect(received).toEqual(expected) // deep equality

    - Expected  - 22
    + Received  +  1

    - Array [
    -   Object {
    -     "blockNumber": 21000000,
    -     "contractAddress": null,
    -     "valueGwei": 50,
    ...
    + Array []

And the fraud contract was recorded in whatever casing it arrived in:

  ● known-symbol spoof verification › a genuinely different contract claiming USDC is still a spoof in any casing

    - Expected  - 1
    + Received  + 1

      Array [
    -   "0xd05339f9ea5ab9d9f03b9d57f671d2abd1f55c82",
    +   "0xD05339F9EA5AB9D9F03B9D57F671D2ABD1F55C82",
      ]

All six pass with the fix. Both current behaviour: tests named in the issue
(the checksummed spoof and the threshold-0 fallback) are inverted into
regression guards; the other two current behaviour: tests, which document
spoof filtering not being disableable, are left alone — that is
#176's territory, and no
settings toggle is added here.

Mutation survivor from the issue comment

tx.holders !== null at the low-holder rule had no fixture reaching it. Added
one pairing a real contractAddress with holders: null, asserting it is not
filtered. Deleting the guard now fails it:

  ● low-holder token filtering (the 1,000-holder rule) › a token whose holder count is unknown is not filtered

    - Expected  - 20
    + Received  +  1

    - Array [
    -   Object {
    -     "contractAddress": "0x1111111111111111111111111111111111111111",
    ...
    + Array []

Tests:       1 failed, 178 passed, 179 total

Guard restored afterwards; the rest of the suite was unaffected by the
mutation, so that test is what kills it.

holders: undefined is not reachable from the Blockscout response shape,
so no fixture was added for it: parseTokenTransfer computes
parseInt(tt.token?.holders_count || "0", 10), and parseTx sets
holders: null outright. Checked, not assumed.

`||`-as-default audit of `src/shared/`

Only one numeric-default instance was in src/shared/, and it is the one
fixed here. Two adjacent cases found and not fixed (out of scope, worth a
decision):

src/shared/transactions.js:112 — parseInt(tt.token?.holders_count || "0", 10).
A token whose holder count the explorer omits (rate limit, self-hosted
instance) is parsed as 0 holders and then hidden by the low-holder rule.
This is the same over-filtering harm the holders !== null guard exists to
prevent, one layer earlier: the guard can never fire for a token transfer,
because the parser never produces null for one. Fixing it means emitting
null when the field is absent.
src/popup/views/send.js:135 — (t.holders || 0) < 1000 hides a token with
an unknown holder count from the send selector, same shape, outside
src/shared/.

Benign (0 is the intended default): src/shared/state.js:97 and
src/background/index.js:599, both lastBalanceRefresh || 0.

Verification

make check on the rebased branch: 10 suites, 251 passed, 1 skipped, 252
total; prettier clean. The skip is the pre-existing test.skip at
tests/wallet.test.js:314, from
#159, not from this
branch.
docker build --no-cache . (via the Dockerfile's make check and
make build) executed uncached and green: Tests: 179 passed, 179 total,
All matched files use Prettier code style!, and verify-build confirmed 4
bundles with autistmask-build-debug=off. Run before the rebase onto
f455b0a; the host make check above is post-rebase.
Rebased onto next at f455b0a immediately before pushing. The only
conflict was the TODO.md Completed Steps list; all sides' entries kept.

Closes [#179](https://git.eeqj.de/sneak/AutistMask/issues/179). Two defects in `src/shared/transactions.js`, both silent over-filtering: the wallet hid transactions the user had asked to see. ## 1. A dust threshold of `0` `filters.dustThresholdGwei || 100000` swallowed the one value a user would pick to mean "show everything". It is now `??`, computed once before the filter loop, so `0` is a real threshold: no transaction has a value below 0 gwei, so nothing is hidden. That makes it exactly equivalent to clearing the hide-dust checkbox, and neither control can override the other. `README.md` now says so in the dust-filtering bullet. `src/shared/state.js` already distinguished unset from zero on load (`saved.dustThresholdGwei !== undefined`), so a stored `0` survives a reload. ### Settings input (`src/popup/views/settings.js:305`) Was `parseInt(value, 10)` accepted when `!isNaN(val) && val >= 0`. What each input does now, and what changed: | input | before | now | | --- | --- | --- | | `0` | stored as 0, then ignored by the filter | stored as 0, honoured — hides nothing | | empty / whitespace | rejected, field kept showing the empty box | rejected, field resyncs to the stored threshold | | negative (`-5`) | rejected, field kept showing `-5` | rejected, field resyncs | | non-numeric (`abc`) | rejected, field kept showing `abc` | rejected, field resyncs | | trailing junk (`100 gwei`) | **accepted as 100** by `parseInt` | rejected, field resyncs | | fractional (`1.5`) | **accepted as 1** by `parseInt` | rejected, field resyncs | Rejected input never mutates state, and the field is put back to the stored value so it cannot display a threshold the wallet is not using. ## 2. `isSpoofedSymbol` was case-sensitive on the contract address EIP-55 mixed case is a checksum over the address, not part of its identity, so `tx.contractAddress !== legit` classified a genuine token arriving checksummed as a spoof — hiding a real transfer and recording the real contract on the fraud blocklist. Every address comparison in the module now goes through one `normalizeAddress()` helper (`parseTx` direction and token lookup, `parseTokenTransfer` direction and contract, `fetchRecentTransactions`, `isSpoofedSymbol`, the fraud set and its lookups). The contract recorded in `newFraudContracts` is normalised too, so the persisted blocklist matches later transfers in any casing. `mergeTransactions()` and its tests from [#177](https://git.eeqj.de/sneak/AutistMask/issues/177) are untouched. ## Failing first Six new tests, run against the current `src/shared/transactions.js` with only the test file changed: ``` ● known-symbol spoof verification › a genuine contract in EIP-55 checksummed form is not a spoof ● known-symbol spoof verification › a genuine contract in all-uppercase form is not a spoof ● known-symbol spoof verification › a genuinely different contract claiming USDC is still a spoof in any casing ● dust threshold filtering › a threshold of 0 hides nothing, leaving the toggle on ● dust threshold filtering › a threshold of 0 agrees with clearing the hide-dust checkbox ● dust threshold filtering › 0, unset and a set threshold are three distinct behaviours Test Suites: 1 failed, 8 passed, 9 total Tests: 6 failed, 173 passed, 179 total ``` The checksummed-genuine and threshold-0 cases fail the same way — the transaction the user should see is missing: ``` ● dust threshold filtering › a threshold of 0 hides nothing, leaving the toggle on expect(received).toEqual(expected) // deep equality - Expected - 22 + Received + 1 - Array [ - Object { - "blockNumber": 21000000, - "contractAddress": null, - "valueGwei": 50, ... + Array [] ``` And the fraud contract was recorded in whatever casing it arrived in: ``` ● known-symbol spoof verification › a genuinely different contract claiming USDC is still a spoof in any casing - Expected - 1 + Received + 1 Array [ - "0xd05339f9ea5ab9d9f03b9d57f671d2abd1f55c82", + "0xD05339F9EA5AB9D9F03B9D57F671D2ABD1F55C82", ] ``` All six pass with the fix. Both `current behaviour:` tests named in the issue (the checksummed spoof and the threshold-0 fallback) are inverted into regression guards; the other two `current behaviour:` tests, which document spoof filtering not being disableable, are left alone — that is [#176](https://git.eeqj.de/sneak/AutistMask/issues/176)'s territory, and no settings toggle is added here. ## Mutation survivor from the issue comment `tx.holders !== null` at the low-holder rule had no fixture reaching it. Added one pairing a real `contractAddress` with `holders: null`, asserting it is not filtered. Deleting the guard now fails it: ``` ● low-holder token filtering (the 1,000-holder rule) › a token whose holder count is unknown is not filtered - Expected - 20 + Received + 1 - Array [ - Object { - "contractAddress": "0x1111111111111111111111111111111111111111", ... + Array [] Tests: 1 failed, 178 passed, 179 total ``` Guard restored afterwards; the rest of the suite was unaffected by the mutation, so that test is what kills it. `holders: undefined` is **not** reachable from the Blockscout response shape, so no fixture was added for it: `parseTokenTransfer` computes `parseInt(tt.token?.holders_count || "0", 10)`, and `parseTx` sets `holders: null` outright. Checked, not assumed. ## `||`-as-default audit of `src/shared/` Only one numeric-default instance was in `src/shared/`, and it is the one fixed here. Two adjacent cases found and **not** fixed (out of scope, worth a decision): - `src/shared/transactions.js:112` — `parseInt(tt.token?.holders_count || "0", 10)`. A token whose holder count the explorer omits (rate limit, self-hosted instance) is parsed as **0 holders** and then hidden by the low-holder rule. This is the same over-filtering harm the `holders !== null` guard exists to prevent, one layer earlier: the guard can never fire for a token transfer, because the parser never produces `null` for one. Fixing it means emitting `null` when the field is absent. - `src/popup/views/send.js:135` — `(t.holders || 0) < 1000` hides a token with an unknown holder count from the send selector, same shape, outside `src/shared/`. Benign (0 is the intended default): `src/shared/state.js:97` and `src/background/index.js:599`, both `lastBalanceRefresh || 0`. ## Verification - `make check` on the rebased branch: 10 suites, 251 passed, 1 skipped, 252 total; prettier clean. The skip is the pre-existing `test.skip` at `tests/wallet.test.js:314`, from [#159](https://git.eeqj.de/sneak/AutistMask/issues/159), not from this branch. - `docker build --no-cache .` (via the Dockerfile's `make check` and `make build`) executed uncached and green: `Tests: 179 passed, 179 total`, `All matched files use Prettier code style!`, and `verify-build` confirmed 4 bundles with `autistmask-build-debug=off`. Run before the rebase onto `f455b0a`; the host `make check` above is post-rebase. - Rebased onto `next` at `f455b0a` immediately before pushing. The only conflict was the `TODO.md` Completed Steps list; all sides' entries kept.

clawbot added 1 commit 2026-08-11 15:09:25 +02:00

fix: honour a dust threshold of 0 and compare addresses case-insensitively (closes #179 )

check / check (push) Successful in 41s

Details

ea1fcd476d

Two defects in the anti-poisoning filters, both silent over-filtering: the
wallet hid transactions the user had asked to see.

A dust threshold of 0 was read as `filters.dustThresholdGwei || 100000`, so
the one value a user would pick to mean "show everything" was swallowed and
replaced by the default. It is now `??`, making 0 a real threshold that hides
nothing and agrees exactly with clearing the hide-dust checkbox. The Settings
input rejects empty, negative, fractional and non-numeric entries outright
instead of coercing them, and resyncs the field to the stored value so it
never displays a threshold the wallet is not using.

isSpoofedSymbol compared the contract address with `===` against a lowercased
known address. EIP-55 mixed case is a checksum, not identity, so a genuine
token arriving checksummed was classified as a spoof and hidden. All address
comparisons in the module now go through one normalizeAddress helper, which
also normalises the fraud contracts recorded from a detected spoof.

Tests cover threshold 0 versus unset versus a set value, and the contract
comparison in lowercase, uppercase and EIP-55 form as well as against a
genuinely different address. The two `current behaviour:` tests pinning the
old behaviour are inverted into regression guards, and a fixture pairing a
real contract address with a null holder count pins the `tx.holders !== null`
guard that no fixture previously reached.

clawbot added the needs-review label 2026-08-11 15:09:34 +02:00

clawbot self-assigned this 2026-08-11 15:09:35 +02:00

clawbot commented

2026-08-11 15:16:05 +02:00

PASS — independent review of #228 against #179: all DoD items met, every normalized site verified behaviour-neutral except the two intended ones, no fraud-set migration break, tests have teeth, make check and script/cibuild green on ea1fcd4, mergeable on next, policy clean.

Flags (none blocking):

src/popup/views/settings.js:307-317 — rejected input reverts the field silently, but this same file already uses showFlash() with full-sentence messages for invalid RPC and Blockscout URLs (:198, :243), which is the idiom README "Language & Labeling" describes. A user typing 100 gwei or 1.5 (both accepted before this PR) now sees the box snap back to the stored number with no explanation. Not a documented-rule break, so not a fail — but a showFlash("Please enter a whole number of gwei, or 0 to hide nothing.") alongside the resync would be strictly better and is a one-line change.
src/popup/views/settings.js:308 — Number(raw) accepts non-decimal notation the old parseInt(raw, 10) did not: 0x10 stores 16 and 1e3 stores 1000. Harmless and no worse than before (parseInt("0x10", 10) stored 0), just wider than the PR table states.
src/shared/transactions.js:305 — a stored non-numeric threshold makes tx.valueGwei < dustThresholdGwei NaN-false, i.e. it fails open and hides nothing rather than defaulting. Unreachable through the new Settings validation and identical to the pre-PR behaviour under ||; noted only because it is silent.

Disclosure: mutation testing was done in a scratch clone working tree and reverted; git status verified clean before the script/cibuild run whose result is cited (layer #11 make check executed, not CACHED: 251 passed, 1 skipped, prettier clean).

PASS — independent review of [#228](https://git.eeqj.de/sneak/AutistMask/pulls/228) against [#179](https://git.eeqj.de/sneak/AutistMask/issues/179): all DoD items met, every normalized site verified behaviour-neutral except the two intended ones, no fraud-set migration break, tests have teeth, `make check` and `script/cibuild` green on `ea1fcd4`, mergeable on `next`, policy clean. Flags (none blocking): - `src/popup/views/settings.js:307-317` — rejected input reverts the field silently, but this same file already uses `showFlash()` with full-sentence messages for invalid RPC and Blockscout URLs (`:198`, `:243`), which is the idiom README "Language & Labeling" describes. A user typing `100 gwei` or `1.5` (both accepted before this PR) now sees the box snap back to the stored number with no explanation. Not a documented-rule break, so not a fail — but a `showFlash("Please enter a whole number of gwei, or 0 to hide nothing.")` alongside the resync would be strictly better and is a one-line change. - `src/popup/views/settings.js:308` — `Number(raw)` accepts non-decimal notation the old `parseInt(raw, 10)` did not: `0x10` stores 16 and `1e3` stores 1000. Harmless and no worse than before (`parseInt("0x10", 10)` stored 0), just wider than the PR table states. - `src/shared/transactions.js:305` — a stored non-numeric threshold makes `tx.valueGwei < dustThresholdGwei` NaN-false, i.e. it fails open and hides nothing rather than defaulting. Unreachable through the new Settings validation and identical to the pre-PR behaviour under `||`; noted only because it is silent. Disclosure: mutation testing was done in a scratch clone working tree and reverted; `git status` verified clean before the `script/cibuild` run whose result is cited (layer `#11 make check` executed, not `CACHED`: 251 passed, 1 skipped, prettier clean).

clawbot merged commit 12acf4dc8c into next

2026-08-11 15:16:49 +02:00

clawbot deleted branch fix/issue-179-dust-threshold-and-spoof-case

2026-08-11 15:16:49 +02:00

clawbot referenced this pull request

2026-08-11 15:16:57 +02:00

fix: the dust threshold field rejects input silently, snapping back with no explanation #233

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/AutistMask#228

fix: honour a dust threshold of 0 and compare addresses case-insensitively (closes #179) #228

1. A dust threshold of 0

Settings input (src/popup/views/settings.js:305)

2. isSpoofedSymbol was case-sensitive on the contract address