Rework round 2 (2026-08-23, head 6b156b3) — the round-1 fix closed half the regression, and the PR body said otherwise
The half that was still open
send.js sourced the balance and the scale from two different resolutions of the same value, and nothing reconciled them:
src/shared/balances.js resolves withoutwallets, so its explorer leg is the row it is formatting.
src/popup/views/send.js resolves withwallets, so its explorer leg is explorerDecimals(), which answers null when two addresses report different scales for one contract.
For a token that is neither bundled nor tracked, whose explorer rows disagree across two addresses, that produced tokenBalance non-null alongside tokenDecimals === null — the exact state round 1 was about. The unknown-balance path is gated on tokenBalance, not on the scale, so INSUFFICIENT_TOKEN never fired and the only thing on the confirmation screen was confirm-fee-unknown-error again.
The round-1 PR body's claim "Still null when nothing knows: no fallback, and the unknown path below is then the real one" was false for that input. It is deleted, not softened.
Are the two call sites supposed to resolve identically?
No, and making them identical would be the wrong fix in either direction:
Passing wallets into balances.js would have it consult, mid-fetch, the very state.wallets that refreshBalances() is about to overwrite wholesale — including this address's own outgoing row. The displayed scale would then depend on refresh order and on stale state.
Dropping wallets from send.js would remove explorerDecimals()'s cross-address check from the one value that goes on to encode a transfer. That check exists precisely so a disputed scale is not picked from.
They answer different questions legitimately. The reconciliation belongs on the Send screen: a quantity computed at a scale Send has just refused is not a balance Send may state.
// after resolveTokenDecimals(...)
if(tb&&tokenDecimals===null)tokenBalance=null;
The tb && guard is deliberate and is the one deviation from the reviewer's suggested one-liner: tokenBalance is "0" when the token has no row at all, and that zero is an absence of holdings, true at every scale and not derived from one. Only a stored quantity is withdrawn.
What the user now sees, executed
Fixture: token NOVEL (neither bundled nor tracked), address A row decimals: "6" / 5000000 units, address B row decimals: "18" / 5000000000000000000 units — both format to 5.0, so only the scale is in dispute. Driven through the real fetchTokenBalances(), the real Send review handler and the real confirmTx.show().
before
after
stored A
{ decimals: 6, balance: "5.0" }
unchanged — storage holds the explorer's own answer
txInfo.tokenDecimals
null
null
txInfo.tokenBalance
"5.0"
null
confirm-balance
"5.0 NOVEL"
"unknown (NOVEL)"
confirm-errors
""
"This token's balance is unknown, because nothing this wallet can consult reports how many decimal places it uses…"
btn-confirm-send
disabled
disabled
Stated precisely, because the round-1 body was not:confirm-fee-amount still reads Unable to estimate and confirm-fee-unknown-error is still visible. The fee genuinely is unavailable — displayedDecimals() refuses the same missing scale inside estimateGas(). What changed is that it is no longer the only thing on the screen and no longer the only offered explanation. This is exactly how the already-accepted genuinely-unknown token (NOVEL with no decimals anywhere) already behaved. Both facts are now asserted in the test rather than described.
Test, and its fail-first
tests/unknownScaleSend.test.js, new describe block "a scale the explorer's own rows disagree about", 4 tests:
confirm-errors non-empty and carrying the unknown-balance sentence; confirm-balance === "unknown (NOVEL)"; Send disabled; fee line and fee-unknown element as described above
Mutation: delete the single line if (tb && tokenDecimals === null) tokenBalance = null; from src/popup/views/send.js, leaving everything else — including the resolution itself — in place. yarn run test:verbose tests/unknownScaleSend.test.js. 2 of the 4 fail; the control and the storage test pass. Verbatim:
resolves to null on the Send screen, and takes the balance with it — expect(received).toBeNull(), Received: "5.0"
so the user is told the balance is unknown, not only that the fee failed — expect(received).not.toBe(expected), Expected: not "" — i.e. confirm-errors was empty, reproducing the review's observation exactly
The line was restored with an editor and the file diffed byte-for-byte against its pre-mutation copy before continuing.
TODO.md and the commit body said "The only 18s left in src/ are native ETH's real scale in uniswap.js and the fixed-point comparison scale in txValidation.js."grep -c "decimals: 18" src/shared/tokenList.js → 432. Both now read:
> No || 18 or ?? 18 fallback remains anywhere in src/. The literal 18s that do remain are real data rather than defaults: 432 per-token decimals: 18 entries in the bundled src/shared/tokenList.js, and, outside that file, only native ETH's protocol-defined scale in src/shared/uniswap.js and the fixed-point comparison scale in src/shared/txValidation.js.
Checked before writing, and each clause is separately re-runnable:
grep -rnE '(\|\||\?\?)\s*"?18"?' --include=*.js src/ → 2 hits, both comments naming the pattern.
The compressed sentence was written from the sweep table's conclusion rather than from the sweep. Writing the claim last, after re-running the check, is what would have caught it.
Finding 3 — helpers.js comment placement
The balanceLine() doc comment (the #307 escaping rationale plus the amount-is-null paragraph) sat above unknownableAmount(). unknownableAmount() and its own two-line comment moved above it, so each comment is on the function it describes. No code change.
Not folded in
src/popup/views/addressToken.js:189's inverted precedence versus resolveTokenDecimals(), per the reviewer's own instruction. Display-only, cannot feed an amount, and improved rather than worsened by this PR.
Rework round 1 (head 041f5dc) — the capability regression this PR introduced
Disclosed rather than quietly corrected, because it was a regression and not an inherited defect.
What was wrong. This PR made tokenBalances[].decimals the explorer's own answer alone (null when it reported none), while the scale a balance is displayed at is resolved separately through resolveTokenDecimals() — bundled list, then tracked tokens, then the explorer. Those are two different questions. src/popup/views/send.js was still reading the stored field raw and carrying it onto the pending transaction:
tokenDecimals=tb?tb.decimals:null;
For a bundled or tracked token whose explorer row omits decimals — WETH, DAI — that put null on a transaction whose balance and amount were both displayed correctly. validateTransfer() had nothing to object to, so the new "this token's balance is unknown" message never fired; instead confirmTx.js reached displayedDecimals(null) inside estimateGas(), which throws, is caught as FEE_UNAVAILABLE, and disabled Send behind "The network fee could not be estimated, so this transaction cannot be checked against your balance. Please go back and try again." Untrue, unactionable, and nothing on the screen mentioned decimals. Before this PR the fabricated 18 was that token's real scale and the send completed, so this was a loss of capability. Fail-closed, so no money was at risk.
The fix.send.js resolves the scale through resolveTokenDecimals() with no fallback of 18:
transferAmountUnits()'s displayed-vs-on-chain comparison is untouched and still guards the encode. Round 2 above adds what this did not: what happens when that resolution answers null while the stored balance does not.
Nit also taken: tb.balance != null ? tb.balance : null is now tb.balance ?? null.
Other consumers of tokenBalances[].decimals, audited. Exactly two sites in src/ dereference the field, plus the callers that reach it through resolution:
Nothing in src/background/ or src/content/ reads decimals at all; src/background/index.js:1175 copies tokenBalances as an opaque field.
The reader half is tested.grep -rn "balance: null" tests/ returned nothing before this rework. Two new files, one assertion per pair at each reader site — that null and 0 produce different output, not merely that null does something reasonable:
tests/unknownScaleDisplay.test.js (7 tests)
reader
null
0
balanceLine() quantity
quantity unknown
0.0000
balanceLine() fiat cell
 
$0.00
balanceLinesForAddress(), show-zero off
row kept
row dropped
balanceLinesForAddress(), show-zero on
quantity unknown
0.0000
addressHoldsFunds()
true
false
getAddressValue()
{ usd: 0, partial: true }
{ usd: 0, partial: false }
tests/unknownScaleSend.test.js (9 tests after round 2) drives the realfetchTokenBalances(), the real Send review handler and the realconfirmTx.show() against a stub DOM and a stub provider.
Fail-first, executed.
git checkout 12190ba -- src/popup/views/send.js (the pre-rework head), make test → 3 of the 5 round-1 Send tests fail, reproducing the regression exactly: expected 18, received null; confirm-fee-amount received "Unable to estimate"; displayedDecimals threw.
reverting src/popup/views/helpers.js, src/shared/prices.js and src/popup/views/confirmTx.js to their next versions, make test → all 10 reader-site tests fail. Restored in both cases; working tree clean.
The defect
fetchTokenBalances() did parseInt(item.token.decimals || "18", 10)before writing to state.wallets[].addresses[].tokenBalances[].decimals. A token whose decimals() reverts — one the explorer reports no scale for — was stored with a fabricated 18 that no reader could tell from a real one.
That is upstream of a rule already merged. #306 made the ERC-20 approval amount line resolve the real scale or refuse to format, and #340 extended it to the swap lines. Both read this stored value as an authoritative source, so the guess walked straight past refusals that were intact and simply never fired.
What changed
src/shared/balances.js stores the explorer's own answer or null, never a default. Both approval paths then reach the existing unknownDecimalsAmount(); no new refusal mechanism was built.
The zero-balance filter moved onto the base-unit integer, where it needs no scale: zero base units is zero at every scale. The dust filter proper (rounds to zero at six places) still applies, but only where a scale exists.
A holding whose scale nothing knows carries balance: null — unknown, never zero. The balance list, the address USD total (partial, the existing vocabulary), the Send screen and the confirmation screen each say so rather than printing 0.0000 for money that is really there.
The bundled list and the user's tracked tokens already outrank the explorer in resolveTokenDecimals(), so a token either of them knows still displays its real quantity when the explorer's entry omits decimals. The storeddecimals stays the explorer's own answer either way — copying another source into it would make explorerDecimals()'s disagreement check compare something other than explorer values. Consequently every screen that needs a display scale asks resolveTokenDecimals(), including Send.
toDecimals() is now one shared export from transferAmount.js instead of three copies. approvalAmount.js's copy was byte-identical; deduping it is behaviour-neutral.
Out of scope, deliberately: the issue's open design question — whether an explorer-sourced scale should size an approval at all. Untouched. An explorer that does report a scale is treated exactly as it is today.
Fail-first evidence (writer half)
tests/fabricatedDecimals.test.js drives a real Blockscout response through the real fetchTokenBalances() and asserts on the real approval screens (a test that hand-writes decimals: null onto state would pass on the broken build, because the fabrication is in the writer).
Mutation: git stash push -- src/ — revert src/ to head, keep the new test — then make check. 9 of the 11 new tests fail. Observed output, verbatim:
assertion
Expected
Received
ERC-20 transfer Amount line
"1000000000 base units (decimals unknown)"
"0.000000001"
ERC-20 approve Amount line
"1000000000 base units (decimals unknown)"
"0.000000001"
swap Amount line
"1000000000 base units (decimals unknown)"
"0.000000001"
stored decimals, absent
null
18
stored decimals, explicit null
null
18
stored decimals, real 0
0
18
stored balance, unknown scale
null
"5.0"
A finding from that fail-first run, disclosed because it changed the test. My first fixture used a small holding, and the two approval-line tests passed against head — for the wrong reason. Formatted at the fabricated 18 the balance came out "0.0", the dust filter dropped the row entirely, and the approval screens then found no source at all and refused by luck. The laundering only bites where the holding survives the dust filter at 18. The fixture now uses HOLDING = 5000000000000000000 base units so it does. Read that as: the defect's blast radius is holdings large enough not to round to zero at 18, which for a token of true scale 6 is anything from about a millionth of a token upward.
The || 18 sweep
grep -rnE '(\|\||\?\?)\s*"?18"?' --include=*.js src/ plus a manual read of every decimals site in src/:
site
what it was
what I did
src/shared/balances.js
parseInt(item.token.decimals || "18", 10) before storage
replaced with toDecimals(); stores the answer or null
src/shared/transactions.js
parseInt(tt.total?.decimals || "18", 10) in parseTokenTransfer()
replaced with toDecimals(); with no scale the row states no quantity (value/exactValue blank, as the contract-call rows in the same file already do) and rawUnit reads SYM base units (decimals unknown). The exact figure is not lost — it is the base-unit line, the one number that needs no scale to be true
src/shared/uniswap.js:124, :567
{ symbol: "ETH", decimals: 18 }
left alone — native ETH's real, protocol-defined scale, not a fallback
src/shared/txValidation.js:15
SCALE_DECIMALS = 18
left alone — the fixed-point scale two human decimal strings are compared at, explicitly independent of any token's scale
src/shared/tokenList.js (432 entries)
decimals: 18 per bundled token
left alone — the bundled list's real per-token data, verified against the contracts, not a default
Re-run on 6b156b3: no || "18", || 18 or ?? 18 remains anywhere in src/ — the only hits are two comments naming the pattern.
Every check is presence/type, never truthiness. toDecimals() enumerates accepted types and answers 0 for a real scale of zero, null for absence — two different answers, which is the whole point. Test: a real scale of zero is stored as zero, not collapsed asserts both "0" and 0 store as 0 and format the balance at scale 0 ("5000000000000000000.0"). Against head it received 18.
Migration
Existing installs already hold fabricated 18s that cannot be told apart from real ones retroactively. That is the defect itself; no migration can undo it, and this PR does not pretend to.
Chosen behaviour: they are left exactly as they are and display exactly as they do today, until the next balance refresh replaces them. This is safe because refreshBalances() writes addr.tokenBalanceswholesale (addr.tokenBalances = balances), so the first refresh after upgrade replaces every row with one built by the fixed code — no partial state, no per-row migration to get wrong. That refresh needs no user action: the popup runs one on open (doRefreshAndRender()), and the background alarm runs one on its own schedule (backgroundRefresh()). So the exposure window is at most one refresh, and until then the user sees precisely the pre-upgrade behaviour rather than a new one.
The schema version is deliberately NOT bumped. Version 1 records remain fully valid and are read exactly as before; the change widens what a field may hold (adds null), and every reader in this build handles it. A bump would gain nothing — no migration is possible by construction — and would only break downgrades.
Composition with the recent work in this area
#311's tokenRefs() floor drops a tokenBalances entry only if it is not a record or has no text address. A decimals: null / balance: null entry therefore survives it — verified, and the tokenRefs() comment now says so explicitly, because flooring those nulls to a default would put the guess back one layer below where it was removed.
The src/shared/stateSchema.js field-by-field categorisation: tokenBalances[].decimals does not move between categories — tokenBalances was and remains "type-checked, container AND entries". What I added is a precision the header was missing and which my change makes load-bearing: what is checked on an entry is address alone, and the rest of an entry is taken verbatim — so an entry's decimals/balance may be null, and readers handle that rather than being defended from it here.
#340's resolveTokenDecimals() is unchanged and is the mechanism this fix relies on; balances.js and send.js now call it too, for display scale only.
Verification (on 6b156b3, rebased onto current next at 75a5fa9)
make check: green, exit 0. Test Suites: 59 passed, Tests: 1052 passed, 1052 total (1048 before round 2; +4 disagreement tests). Lint ran in Docker — #11 [lint 1/1] RUN make lint, DONE 5.4s, notCACHED, on this tree — All matched files use Prettier code style!. check-censored: 185 tracked file(s) inspected. test-verify-build: 46 case(s) passed.
make build: exit 0. verify-build: 15 emitted file(s) verified against the receipt, 4 bundle(s) autistmask-build-debug=off. dist/ removed afterwards with make clean.
make fmt run; prettier clean. git fetch re-run immediately before pushing: next still at 75a5fa9, so the branch is a fast-forward and no conflict arose. No containers created or left behind (docker build only; docker ps -a empty). No cache pruned.
Closes https://git.eeqj.de/sneak/AutistMask/issues/349.
## Rework round 2 (2026-08-23, head `6b156b3`) — the round-1 fix closed half the regression, and the PR body said otherwise
### The half that was still open
`send.js` sourced the balance and the scale from **two different resolutions of the same value**, and nothing reconciled them:
- `src/shared/balances.js` resolves **without** `wallets`, so its explorer leg is the row it is formatting.
- `src/popup/views/send.js` resolves **with** `wallets`, so its explorer leg is `explorerDecimals()`, which answers `null` when two addresses report different scales for one contract.
For a token that is neither bundled nor tracked, whose explorer rows disagree across two addresses, that produced `tokenBalance` non-null alongside `tokenDecimals === null` — the exact state round 1 was about. The unknown-balance path is gated on `tokenBalance`, not on the scale, so `INSUFFICIENT_TOKEN` never fired and the only thing on the confirmation screen was `confirm-fee-unknown-error` again.
**The round-1 PR body's claim *"Still null when nothing knows: no fallback, and the unknown path below is then the real one"* was false for that input.** It is deleted, not softened.
### Are the two call sites supposed to resolve identically?
No, and making them identical would be the wrong fix in either direction:
- Passing `wallets` into `balances.js` would have it consult, mid-fetch, the very `state.wallets` that `refreshBalances()` is about to overwrite wholesale — including this address's own outgoing row. The displayed scale would then depend on refresh order and on stale state.
- Dropping `wallets` from `send.js` would remove `explorerDecimals()`'s cross-address check from the one value that goes on to **encode a transfer**. That check exists precisely so a disputed scale is not picked from.
They answer different questions legitimately. The reconciliation belongs on the Send screen: a quantity computed at a scale Send has just refused is not a balance Send may state.
```js
// after resolveTokenDecimals(...)
if (tb && tokenDecimals === null) tokenBalance = null;
```
The `tb &&` guard is deliberate and is the one deviation from the reviewer's suggested one-liner: `tokenBalance` is `"0"` when the token has **no row at all**, and that zero is an absence of holdings, true at every scale and not derived from one. Only a stored quantity is withdrawn.
### What the user now sees, executed
Fixture: token `NOVEL` (neither bundled nor tracked), address A row `decimals: "6"` / `5000000` units, address B row `decimals: "18"` / `5000000000000000000` units — both format to `5.0`, so only the scale is in dispute. Driven through the real `fetchTokenBalances()`, the real Send review handler and the real `confirmTx.show()`.
| | before | after |
| --- | --- | --- |
| stored A | `{ decimals: 6, balance: "5.0" }` | unchanged — storage holds the explorer's own answer |
| `txInfo.tokenDecimals` | `null` | `null` |
| `txInfo.tokenBalance` | `"5.0"` | `null` |
| `confirm-balance` | `"5.0 NOVEL"` | `"unknown (NOVEL)"` |
| `confirm-errors` | `""` | *"This token's balance is unknown, because nothing this wallet can consult reports how many decimal places it uses…"* |
| `btn-confirm-send` | disabled | disabled |
**Stated precisely, because the round-1 body was not:** `confirm-fee-amount` still reads `Unable to estimate` and `confirm-fee-unknown-error` is still visible. The fee genuinely is unavailable — `displayedDecimals()` refuses the same missing scale inside `estimateGas()`. What changed is that it is no longer the *only* thing on the screen and no longer the only offered explanation. This is exactly how the already-accepted genuinely-unknown token (`NOVEL` with no `decimals` anywhere) already behaved. Both facts are now asserted in the test rather than described.
### Test, and its fail-first
`tests/unknownScaleSend.test.js`, new describe block *"a scale the explorer's own rows disagree about"*, 4 tests:
| test | asserts |
| --- | --- |
| stored per row | `a[0].decimals === 6`, `a[0].balance === "5.0"`, `b[0].decimals === 18` |
| resolves to null, and takes the balance with it | `tokenDecimals === null` **and** `tokenBalance === null` |
| the user is told the balance is unknown | `confirm-errors` non-empty and carrying the unknown-balance sentence; `confirm-balance === "unknown (NOVEL)"`; Send disabled; fee line and fee-unknown element as described above |
| the control: **agreeing** rows | `tokenDecimals === 6`, `tokenBalance === "5.0"`, `confirm-balance === "5.0 NOVEL"`, `confirm-errors === ""`, Send **enabled** |
**Mutation:** delete the single line `if (tb && tokenDecimals === null) tokenBalance = null;` from `src/popup/views/send.js`, leaving everything else — including the resolution itself — in place. `yarn run test:verbose tests/unknownScaleSend.test.js`. **2 of the 4 fail; the control and the storage test pass.** Verbatim:
- *resolves to null on the Send screen, and takes the balance with it* — `expect(received).toBeNull()`, `Received: "5.0"`
- *so the user is told the balance is unknown, not only that the fee failed* — `expect(received).not.toBe(expected)`, `Expected: not ""` — i.e. `confirm-errors` was empty, reproducing the review's observation exactly
The line was restored with an editor and the file diffed byte-for-byte against its pre-mutation copy before continuing.
### Still behaving, re-run after the change
- **Resolvable token** (bundled WETH, explorer row omits `decimals`): `tokenDecimals === 18`, `tokenBalance === "5.0"`, fee estimated, `confirm-errors` empty, **Send enabled**, `transferAmountUnits("1.5", 18, 18n) === parseUnits("1.5", 18)`. All 4 tests pass.
- **Genuinely unknown token**: `tokenDecimals === null`, `confirm-balance === "unknown (NOVEL)"`, unknown-balance message, **Send disabled**. Passes.
### Finding 2 — the sweep sentence
`TODO.md` and the commit body said *"The only `18`s left in `src/` are native ETH's real scale in `uniswap.js` and the fixed-point comparison scale in `txValidation.js`."* `grep -c "decimals: 18" src/shared/tokenList.js` → **432**. Both now read:
> No `|| 18` or `?? 18` fallback remains anywhere in `src/`. The literal `18`s that do remain are real data rather than defaults: 432 per-token `decimals: 18` entries in the bundled `src/shared/tokenList.js`, and, outside that file, only native ETH's protocol-defined scale in `src/shared/uniswap.js` and the fixed-point comparison scale in `src/shared/txValidation.js`.
Checked before writing, and each clause is separately re-runnable:
- `grep -rnE '(\|\||\?\?)\s*"?18"?' --include=*.js src/` → 2 hits, both comments naming the pattern.
- `grep -c "decimals: 18" src/shared/tokenList.js` → 432.
- `grep -rn "18" --include=*.js src/ | grep -v '^src/shared/tokenList.js' | grep -vi '0x\|//'` → exactly `uniswap.js:124`, `uniswap.js:567`, `txValidation.js:15`.
The compressed sentence was written from the sweep table's *conclusion* rather than from the sweep. Writing the claim last, after re-running the check, is what would have caught it.
### Finding 3 — `helpers.js` comment placement
The `balanceLine()` doc comment (the https://git.eeqj.de/sneak/AutistMask/issues/307 escaping rationale plus the `amount`-is-null paragraph) sat above `unknownableAmount()`. `unknownableAmount()` and its own two-line comment moved above it, so each comment is on the function it describes. No code change.
### Not folded in
`src/popup/views/addressToken.js:189`'s inverted precedence versus `resolveTokenDecimals()`, per the reviewer's own instruction. Display-only, cannot feed an amount, and improved rather than worsened by this PR.
---
## Rework round 1 (head `041f5dc`) — the capability regression this PR introduced
Disclosed rather than quietly corrected, because it was a regression and not an inherited defect.
**What was wrong.** This PR made `tokenBalances[].decimals` the explorer's own answer alone (`null` when it reported none), while the scale a balance is *displayed* at is resolved separately through `resolveTokenDecimals()` — bundled list, then tracked tokens, then the explorer. Those are two different questions. `src/popup/views/send.js` was still reading the stored field raw and carrying it onto the pending transaction:
```js
tokenDecimals = tb ? tb.decimals : null;
```
For a **bundled or tracked** token whose explorer row omits `decimals` — WETH, DAI — that put `null` on a transaction whose balance and amount were both displayed correctly. `validateTransfer()` had nothing to object to, so the new "this token's balance is unknown" message never fired; instead `confirmTx.js` reached `displayedDecimals(null)` inside `estimateGas()`, which throws, is caught as `FEE_UNAVAILABLE`, and disabled Send behind *"The network fee could not be estimated, so this transaction cannot be checked against your balance. Please go back and try again."* Untrue, unactionable, and nothing on the screen mentioned decimals. **Before this PR the fabricated `18` was that token's real scale and the send completed**, so this was a loss of capability. Fail-closed, so no money was at risk.
**The fix.** `send.js` resolves the scale through `resolveTokenDecimals()` with **no fallback of 18**:
```js
tokenDecimals = resolveTokenDecimals(token, {
trackedTokens: state.trackedTokens,
wallets: state.wallets,
});
```
`transferAmountUnits()`'s displayed-vs-on-chain comparison is untouched and still guards the encode. Round 2 above adds what this did not: what happens when that resolution answers `null` while the stored balance does not.
Nit also taken: `tb.balance != null ? tb.balance : null` is now `tb.balance ?? null`.
**Other consumers of `tokenBalances[].decimals`, audited.** Exactly two sites in `src/` dereference the field, plus the callers that reach it through resolution:
| site | how it reads the field | disposition |
| --- | --- | --- |
| `src/shared/approvalAmount.js:44` (`explorerDecimals()`) | raw, via `toDecimals()`, disagreement across addresses answers `null` | **correct by design** — this *is* the explorer leg of `resolveTokenDecimals()`, and it must compare explorer values only |
| `src/popup/views/addressToken.js:189` | its own chain: row, then tracked, then bundled | **left alone** — display-only contract metadata (the `Decimals:` info line); it never formats an amount |
| `src/popup/views/send.js` | `resolveTokenDecimals()` | fixed here |
| `src/popup/views/approval.js:95`, `src/shared/uniswap.js:82` | `resolveTokenDecimals()` | already correct |
| `src/popup/views/confirmTx.js:338`, `:488` | `txInfo.tokenDecimals`, which only `send.js` sets | correct once `send.js` is |
Nothing in `src/background/` or `src/content/` reads `decimals` at all; `src/background/index.js:1175` copies `tokenBalances` as an opaque field.
**The reader half is tested.** `grep -rn "balance: null" tests/` returned nothing before this rework. Two new files, one assertion **per pair** at each reader site — that `null` and `0` produce *different* output, not merely that `null` does something reasonable:
`tests/unknownScaleDisplay.test.js` (7 tests)
| reader | `null` | `0` |
| --- | --- | --- |
| `balanceLine()` quantity | `quantity unknown` | `0.0000` |
| `balanceLine()` fiat cell | ` ` | `$0.00` |
| `balanceLinesForAddress()`, show-zero **off** | row kept | row dropped |
| `balanceLinesForAddress()`, show-zero **on** | `quantity unknown` | `0.0000` |
| `addressHoldsFunds()` | `true` | `false` |
| `getAddressValue()` | `{ usd: 0, partial: true }` | `{ usd: 0, partial: false }` |
`tests/unknownScaleSend.test.js` (9 tests after round 2) drives the **real** `fetchTokenBalances()`, the **real** Send review handler and the **real** `confirmTx.show()` against a stub DOM and a stub provider.
**Fail-first, executed.**
- `git checkout 12190ba -- src/popup/views/send.js` (the pre-rework head), `make test` → **3 of the 5 round-1 Send tests fail**, reproducing the regression exactly: `expected 18, received null`; `confirm-fee-amount` received `"Unable to estimate"`; `displayedDecimals` threw.
- reverting `src/popup/views/helpers.js`, `src/shared/prices.js` and `src/popup/views/confirmTx.js` to their `next` versions, `make test` → **all 10 reader-site tests fail**. Restored in both cases; working tree clean.
---
## The defect
`fetchTokenBalances()` did `parseInt(item.token.decimals || "18", 10)` **before writing** to `state.wallets[].addresses[].tokenBalances[].decimals`. A token whose `decimals()` reverts — one the explorer reports no scale for — was stored with a fabricated `18` that no reader could tell from a real one.
That is upstream of a rule already merged. https://git.eeqj.de/sneak/AutistMask/issues/306 made the ERC-20 approval amount line resolve the real scale or refuse to format, and https://git.eeqj.de/sneak/AutistMask/issues/340 extended it to the swap lines. Both read this stored value as an authoritative source, so the guess walked straight past refusals that were intact and simply never fired.
## What changed
- `src/shared/balances.js` stores the explorer's own answer or `null`, never a default. Both approval paths then reach the existing `unknownDecimalsAmount()`; no new refusal mechanism was built.
- The zero-balance filter moved onto the **base-unit integer**, where it needs no scale: zero base units is zero at every scale. The dust filter proper (rounds to zero at six places) still applies, but only where a scale exists.
- A holding whose scale nothing knows carries `balance: null` — unknown, never zero. The balance list, the address USD total (`partial`, the existing vocabulary), the Send screen and the confirmation screen each say so rather than printing `0.0000` for money that is really there.
- The bundled list and the user's tracked tokens already outrank the explorer in `resolveTokenDecimals()`, so a token either of them knows still displays its real quantity when the explorer's entry omits `decimals`. The **stored** `decimals` stays the explorer's own answer either way — copying another source into it would make `explorerDecimals()`'s disagreement check compare something other than explorer values. Consequently every screen that needs a *display* scale asks `resolveTokenDecimals()`, including Send.
- `toDecimals()` is now one shared export from `transferAmount.js` instead of three copies. `approvalAmount.js`'s copy was byte-identical; deduping it is behaviour-neutral.
**Out of scope, deliberately:** the issue's open design question — whether an explorer-sourced scale should size an approval at all. Untouched. An explorer that *does* report a scale is treated exactly as it is today.
## Fail-first evidence (writer half)
`tests/fabricatedDecimals.test.js` drives a real Blockscout response through the real `fetchTokenBalances()` and asserts on the real approval screens (a test that hand-writes `decimals: null` onto state would pass on the broken build, because the fabrication is in the writer).
Mutation: `git stash push -- src/` — revert `src/` to head, keep the new test — then `make check`. **9 of the 11 new tests fail.** Observed output, verbatim:
| assertion | Expected | Received |
| --- | --- | --- |
| ERC-20 `transfer` Amount line | `"1000000000 base units (decimals unknown)"` | `"0.000000001"` |
| ERC-20 `approve` Amount line | `"1000000000 base units (decimals unknown)"` | `"0.000000001"` |
| swap `Amount` line | `"1000000000 base units (decimals unknown)"` | `"0.000000001"` |
| stored `decimals`, absent | `null` | `18` |
| stored `decimals`, explicit `null` | `null` | `18` |
| stored `decimals`, real `0` | `0` | `18` |
| stored `balance`, unknown scale | `null` | `"5.0"` |
**A finding from that fail-first run, disclosed because it changed the test.** My first fixture used a *small* holding, and the two approval-line tests passed against head — for the wrong reason. Formatted at the fabricated 18 the balance came out `"0.0"`, the dust filter dropped the row entirely, and the approval screens then found no source at all and refused by luck. The laundering only bites where the holding survives the dust filter at 18. The fixture now uses `HOLDING = 5000000000000000000` base units so it does. Read that as: the defect's blast radius is holdings large enough not to round to zero at 18, which for a token of true scale 6 is anything from about a millionth of a token upward.
## The `|| 18` sweep
`grep -rnE '(\|\||\?\?)\s*"?18"?' --include=*.js src/` plus a manual read of every `decimals` site in `src/`:
| site | what it was | what I did |
| --- | --- | --- |
| `src/shared/balances.js` | `parseInt(item.token.decimals \|\| "18", 10)` before storage | replaced with `toDecimals()`; stores the answer or `null` |
| `src/shared/transactions.js` | `parseInt(tt.total?.decimals \|\| "18", 10)` in `parseTokenTransfer()` | replaced with `toDecimals()`; with no scale the row states no quantity (`value`/`exactValue` blank, as the contract-call rows in the same file already do) and `rawUnit` reads `SYM base units (decimals unknown)`. The exact figure is not lost — it is the base-unit line, the one number that needs no scale to be true |
| `src/shared/uniswap.js:124`, `:567` | `{ symbol: "ETH", decimals: 18 }` | **left alone** — native ETH's real, protocol-defined scale, not a fallback |
| `src/shared/txValidation.js:15` | `SCALE_DECIMALS = 18` | **left alone** — the fixed-point scale two human decimal strings are compared at, explicitly independent of any token's scale |
| `src/shared/tokenList.js` (432 entries) | `decimals: 18` per bundled token | **left alone** — the bundled list's real per-token data, verified against the contracts, not a default |
Re-run on `6b156b3`: no `|| "18"`, `|| 18` or `?? 18` remains anywhere in `src/` — the only hits are two comments naming the pattern.
## Falsy collapse (https://git.eeqj.de/sneak/AutistMask/issues/246)
Every check is presence/type, never truthiness. `toDecimals()` enumerates accepted types and answers `0` for a real scale of zero, `null` for absence — two different answers, which is the whole point. Test: `a real scale of zero is stored as zero, not collapsed` asserts both `"0"` and `0` store as `0` and format the balance at scale 0 (`"5000000000000000000.0"`). Against head it received `18`.
## Migration
**Existing installs already hold fabricated 18s that cannot be told apart from real ones retroactively.** That is the defect itself; no migration can undo it, and this PR does not pretend to.
Chosen behaviour: **they are left exactly as they are and display exactly as they do today, until the next balance refresh replaces them.** This is safe because `refreshBalances()` writes `addr.tokenBalances` **wholesale** (`addr.tokenBalances = balances`), so the first refresh after upgrade replaces every row with one built by the fixed code — no partial state, no per-row migration to get wrong. That refresh needs no user action: the popup runs one on open (`doRefreshAndRender()`), and the background alarm runs one on its own schedule (`backgroundRefresh()`). So the exposure window is at most one refresh, and until then the user sees precisely the pre-upgrade behaviour rather than a new one.
**The schema version is deliberately NOT bumped.** Version 1 records remain fully valid and are read exactly as before; the change widens what a field may hold (adds `null`), and every reader in this build handles it. A bump would gain nothing — no migration is possible by construction — and would only break downgrades.
## Composition with the recent work in this area
- https://git.eeqj.de/sneak/AutistMask/issues/311's `tokenRefs()` floor drops a `tokenBalances` entry only if it is not a record or has no text `address`. A `decimals: null` / `balance: null` entry therefore **survives it** — verified, and the `tokenRefs()` comment now says so explicitly, because flooring those nulls to a default would put the guess back one layer below where it was removed.
- The `src/shared/stateSchema.js` field-by-field categorisation: `tokenBalances[].decimals` does **not** move between categories — `tokenBalances` was and remains "type-checked, container AND entries". What I added is a precision the header was missing and which my change makes load-bearing: what is checked on an *entry* is `address` alone, and the rest of an entry is taken verbatim — so an entry's `decimals`/`balance` may be `null`, and readers handle that rather than being defended from it here.
- https://git.eeqj.de/sneak/AutistMask/issues/340's `resolveTokenDecimals()` is unchanged and is the mechanism this fix relies on; `balances.js` and `send.js` now call it too, for display scale only.
## Verification (on `6b156b3`, rebased onto current `next` at `75a5fa9`)
- `make check`: **green, exit 0**. `Test Suites: 59 passed`, `Tests: 1052 passed, 1052 total` (1048 before round 2; +4 disagreement tests). Lint ran **in Docker** — `#11 [lint 1/1] RUN make lint`, `DONE 5.4s`, **not** `CACHED`, on this tree — `All matched files use Prettier code style!`. `check-censored: 185 tracked file(s) inspected`. `test-verify-build: 46 case(s) passed`.
- `make build`: **exit 0**. `verify-build: 15 emitted file(s) verified against the receipt, 4 bundle(s) autistmask-build-debug=off`. `dist/` removed afterwards with `make clean`.
- `make fmt` run; prettier clean. `git fetch` re-run immediately before pushing: `next` still at `75a5fa9`, so the branch is a fast-forward and no conflict arose. No containers created or left behind (`docker build` only; `docker ps -a` empty). No cache pruned.
fetchTokenBalances() did parseInt(item.token.decimals || "18", 10) before writing to state.wallets[].addresses[].tokenBalances[].decimals, so a token whose decimals() reverts -- one the block explorer reports no scale for -- was stored with a fabricated 18 that no reader could tell from a real one.
That is upstream of a rule already merged. #306 made the ERC-20 approval amount line resolve the real scale or refuse to format, and #340 extended it to the swap lines; both read this stored value as an authoritative source, so the guess walked straight past refusals that were intact and simply never fired. A 1,000-unit approval of such a token rendered 0.000000001 on the one screen whose job is to state what is being authorized.
The stored value is now the explorer's own answer or null, never a default. Both approval paths reach unknownDecimalsAmount() on a null, using the refusal that was already there. The history list's token transfers carried the same || "18" and now state exact base units with the scale unknown rather than a quantity at a guessed one.
A holding whose scale nothing knows has no quantity either, so its balance is stored as null -- unknown, never zero -- and the balance list, the address USD total, the Send screen and the confirmation screen each say so rather than printing 0.0000 for money that is really there. The zero-balance filter moved onto the base-unit integer, where it needs no scale at all. The bundled token list and the user's tracked tokens already outrank the explorer, so a token either of them knows still displays its real quantity when the explorer's entry omits decimals; only what none of the three knows is unknown.
The uint8 check is one shared toDecimals() rather than three copies of it, and it answers 0 for a real scale of zero: || "18" collapsed that to eighteen, the falsy-collapse trap of #246.
Existing installs hold 18s that cannot be told apart retroactively -- that is the defect, and no migration can undo it. They display exactly as they do today until the next balance refresh, which rewrites tokenBalances wholesale and needs no user action. The schema version is not bumped: version 1 records stay valid and are read exactly as before.
The only 18s left in src/ are native ETH's real scale in uniswap.js and the fixed-point comparison scale in txValidation.js.
clawbot
self-assigned this 2026-08-23 20:23:35 +02:00
1. BLOCKING: a bundled or tracked token whose explorer row omits decimals becomes unsendable, with a wrong error message
src/popup/views/send.js:248 still reads the raw stored value:
tokenDecimals=tb?tb.decimals:null;
This PR taught balances.js to resolve the DISPLAY scale through resolveTokenDecimals() (bundled list, then tracked tokens, then the explorer) while deliberately storing the explorer's own answer — null — in tokenBalances[].decimals. send.js was not taught the same thing, so the two halves now disagree for a token the wallet actually knows the scale of.
The PR's own test proves the wallet reaches that state: tests/fabricatedDecimals.test.js, the bundled list still supplies a quantity the explorer omitted asserts decimals === null and balance === "5.0" for WETH.
Walk it for a bundled 18-decimal token (WETH, DAI) whose explorer row omits decimals:
Balance list and Send screen show the real quantity, e.g. Current balance: 5.0 WETH.
send.js:248 sets tokenDecimals = null; tokenBalance = "5.0" (non-null), so validateTransfer() raises nothing and the new "This token's balance is unknown" message never appears.
confirmTx.js:338displayedDecimals(txInfo.tokenDecimals) throws inside estimateGas(), which is caught into FEE_UNAVAILABLE.
The user sees Unable to estimate and, from src/popup/index.html:693, "The network fee could not be estimated, so this transaction cannot be checked against your balance. Please go back and try again." Send is disabled permanently — going back and trying again can never work, and nothing on the screen mentions decimals.
Before this PR the stored 18 was coincidentally correct for such a token and the send completed. So this is a capability regression introduced here, it is undisclosed in the PR body, and it degrades to an unactionable and untrue message. The direction is fail-closed so no money is at risk, but "cannot send WETH, told it is a fee problem" is a worse outcome than the fabricated 18 was in exactly this case.
Acceptable: send.js resolves the scale the same way balances.js now does — resolveTokenDecimals(token, { trackedTokens: state.trackedTokens, wallets: state.wallets }) — and only carries null forward when that answers null. Then the unknown-scale path is reached only when the scale really is unknown, and confirmTx's balance/validation messaging (which already says "unknown") is what the user sees instead of a fee error. transferAmountUnits()'s displayed-vs-on-chain comparison is unchanged and still guards the encode.
2. The display half of this change has no test coverage at all
grep -rn "balance: null" tests/ returns nothing. Five of the six reader changes are asserted nowhere:
helpers.jsbalanceLine() rendering quantity unknown instead of 0.0000
helpers.jsbalanceLinesForAddress() keeping a null row regardless of the show-zero setting
helpers.jsaddressHoldsFunds() answering true for a null balance (tests/deleteAddress.test.js covers ETH-only, token-only, dust, empty and zero-token — not null)
prices.jsgetAddressValue() setting partial
send.js / confirmTx.js stating unknown (SYM), and confirmTx.js's new INSUFFICIENT_TOKEN wording
The new suite is genuinely fail-first and covers the storage fix and both approval lines well (verified: 9 of 11 fail with src/ reverted to next). But a wallet-wide null-vs-zero display change is precisely the class that has recurred through #246, #306, #322, #359 and #364, and it should not land with the reader side untested. Each of the five needs one assertion that null and 0 produce different output.
3. Nit
src/popup/views/send.js:247 — tb.balance != null ? tb.balance : null is a no-op ternary. tb.balance ?? null states the same normalization.
Verified and passing: fail-first reproduced exactly (9/11, src/ reverted to next); || 18 / ?? 18 sweep clean, and every disposition in the PR table checks out; null and 0 are distinguished at all six sites; addressHoldsFunds() errs toward warning and its only consumer is deleteAddress.js; the migration claim holds (addr.tokenBalances = balances wholesale at balances.js:268, and mergeAddress() treats tokenBalances as a leaf, so nothing merges old rows forward); toDecimals() dedupe is byte-identical and behaviour-neutral; tokenRefs() filters on isRecord && typeof address === "string" alone, so the new stateSchema.js claim is accurate and an unknown-scale entry survives the #311 floor; transactions.js's blank value/exactValue are guarded at every consumer and the base-unit line still carries the exact figure; no Claude/Anthropic reference or attribution trailer; one commit titled (closes #349); base next; mergeable; prettier clean. make check and make build both exit 0 in my own clone, lint executed in Docker (#11 [lint 1/1] RUN make lint, 5.2s, not CACHED), Tests: 1027 passed. CI: check / check success on 12190ba; both e2e jobs still queued at review time, neither red.
Raised rather than filed: the dust filter no longer masks an UNKNOWN scale (the row is kept). It still masks a scale the explorer reports but reports wrong — the row formats to "0.0", is dropped, and the holding then vanishes from the balance list, the Send dropdown and explorerDecimals() alike. That follows from the trusted-explorer decision settled in #349 (comment) and is not a defect of this diff.
FAIL — `needs-rework`.
### 1. BLOCKING: a bundled or tracked token whose explorer row omits `decimals` becomes unsendable, with a wrong error message
`src/popup/views/send.js:248` still reads the raw stored value:
```js
tokenDecimals = tb ? tb.decimals : null;
```
This PR taught `balances.js` to resolve the DISPLAY scale through `resolveTokenDecimals()` (bundled list, then tracked tokens, then the explorer) while deliberately storing the explorer's own answer — `null` — in `tokenBalances[].decimals`. `send.js` was not taught the same thing, so the two halves now disagree for a token the wallet actually knows the scale of.
The PR's own test proves the wallet reaches that state: `tests/fabricatedDecimals.test.js`, `the bundled list still supplies a quantity the explorer omitted` asserts `decimals === null` and `balance === "5.0"` for WETH.
Walk it for a bundled 18-decimal token (WETH, DAI) whose explorer row omits `decimals`:
- Balance list and Send screen show the real quantity, e.g. `Current balance: 5.0 WETH`.
- `send.js:248` sets `tokenDecimals = null`; `tokenBalance = "5.0"` (non-null), so `validateTransfer()` raises nothing and the new "This token's balance is unknown" message never appears.
- `confirmTx.js:338` `displayedDecimals(txInfo.tokenDecimals)` throws inside `estimateGas()`, which is caught into `FEE_UNAVAILABLE`.
- The user sees `Unable to estimate` and, from `src/popup/index.html:693`, "The network fee could not be estimated, so this transaction cannot be checked against your balance. Please go back and try again." Send is disabled permanently — going back and trying again can never work, and nothing on the screen mentions decimals.
Before this PR the stored `18` was coincidentally correct for such a token and the send completed. So this is a capability regression introduced here, it is undisclosed in the PR body, and it degrades to an unactionable and untrue message. The direction is fail-closed so no money is at risk, but "cannot send WETH, told it is a fee problem" is a worse outcome than the fabricated 18 was in exactly this case.
Acceptable: `send.js` resolves the scale the same way `balances.js` now does — `resolveTokenDecimals(token, { trackedTokens: state.trackedTokens, wallets: state.wallets })` — and only carries `null` forward when that answers `null`. Then the unknown-scale path is reached only when the scale really is unknown, and `confirmTx`'s balance/validation messaging (which already says "unknown") is what the user sees instead of a fee error. `transferAmountUnits()`'s displayed-vs-on-chain comparison is unchanged and still guards the encode.
### 2. The display half of this change has no test coverage at all
`grep -rn "balance: null" tests/` returns nothing. Five of the six reader changes are asserted nowhere:
- `helpers.js` `balanceLine()` rendering `quantity unknown` instead of `0.0000`
- `helpers.js` `balanceLinesForAddress()` keeping a null row regardless of the show-zero setting
- `helpers.js` `addressHoldsFunds()` answering `true` for a null balance (`tests/deleteAddress.test.js` covers ETH-only, token-only, dust, empty and zero-token — not null)
- `prices.js` `getAddressValue()` setting `partial`
- `send.js` / `confirmTx.js` stating `unknown (SYM)`, and `confirmTx.js`'s new `INSUFFICIENT_TOKEN` wording
The new suite is genuinely fail-first and covers the storage fix and both approval lines well (verified: 9 of 11 fail with `src/` reverted to `next`). But a wallet-wide null-vs-zero display change is precisely the class that has recurred through https://git.eeqj.de/sneak/AutistMask/issues/246, https://git.eeqj.de/sneak/AutistMask/issues/306, https://git.eeqj.de/sneak/AutistMask/issues/322, https://git.eeqj.de/sneak/AutistMask/issues/359 and https://git.eeqj.de/sneak/AutistMask/issues/364, and it should not land with the reader side untested. Each of the five needs one assertion that `null` and `0` produce different output.
### 3. Nit
`src/popup/views/send.js:247` — `tb.balance != null ? tb.balance : null` is a no-op ternary. `tb.balance ?? null` states the same normalization.
---
Verified and passing: fail-first reproduced exactly (9/11, `src/` reverted to `next`); `|| 18` / `?? 18` sweep clean, and every disposition in the PR table checks out; `null` and `0` are distinguished at all six sites; `addressHoldsFunds()` errs toward warning and its only consumer is `deleteAddress.js`; the migration claim holds (`addr.tokenBalances = balances` wholesale at `balances.js:268`, and `mergeAddress()` treats `tokenBalances` as a leaf, so nothing merges old rows forward); `toDecimals()` dedupe is byte-identical and behaviour-neutral; `tokenRefs()` filters on `isRecord && typeof address === "string"` alone, so the new `stateSchema.js` claim is accurate and an unknown-scale entry survives the https://git.eeqj.de/sneak/AutistMask/issues/311 floor; `transactions.js`'s blank `value`/`exactValue` are guarded at every consumer and the base-unit line still carries the exact figure; no Claude/Anthropic reference or attribution trailer; one commit titled ` (closes #349)`; base `next`; mergeable; prettier clean. `make check` and `make build` both exit 0 in my own clone, lint executed in Docker (`#11 [lint 1/1] RUN make lint`, 5.2s, not `CACHED`), `Tests: 1027 passed`. CI: `check / check` success on `12190ba`; both e2e jobs still queued at review time, neither red.
Raised rather than filed: the dust filter no longer masks an UNKNOWN scale (the row is kept). It still masks a scale the explorer reports but reports wrong — the row formats to `"0.0"`, is dropped, and the holding then vanishes from the balance list, the Send dropdown and `explorerDecimals()` alike. That follows from the trusted-explorer decision settled in https://git.eeqj.de/sneak/AutistMask/issues/349#issuecomment-69308 and is not a defect of this diff.
FAIL — needs-rework. Independent re-review of 041f5dc.
1. The Finding-1 regression is closed for the resolvable case but survives when resolution answers null while the stored balance does not
src/popup/views/send.js:248 and :260 now answer two different questions from two different sources, and nothing reconciles them:
tokenBalance=tb?(tb.balance??null):"0";// scale: bundled/tracked/THIS ROW's explorer value
tokenDecimals=resolveTokenDecimals(token,{trackedTokens,wallets});// scale: bundled/tracked/AGREED-ACROSS-ADDRESSES explorer value
balances.js:148 resolves without wallets, so its explorer leg is this row's own decimals. send.js resolves with wallets, so its explorer leg is explorerDecimals(), which answers null when two addresses report different values for one contract. For a token that is neither bundled nor tracked, that produces tokenBalance non-null with tokenDecimals === null — the exact input state Finding 1 was about.
Executed, in my clone, through the real fetchTokenBalances() / real Send handler / real confirmTx.show(): token NOVEL, address A row decimals: "6", address B row decimals: "18", sending from A.
stored A: {"decimals":6,"balance":"5.0"}
handed on: tokenBalance: "5.0", tokenDecimals: null
confirm-balance: "5.0 NOVEL"
confirm-fee-amount: "Unable to estimate"
confirm-errors: "" — empty. validateTransfer() sees a usable tokenBalance, so INSUFFICIENT_TOKEN never fires and the new "this token's balance is unknown" sentence never appears.
btn-confirm-send.disabled: true, and the only thing on screen is confirm-fee-unknown-error (src/popup/index.html:697): "The network fee could not be estimated, so this transaction cannot be checked against your balance. Please go back and try again."
Same symptom, same untrue and unactionable message, nothing naming decimals — narrower, and fail-closed, but it is this diff that makes tokenDecimals: null reachable alongside a non-null balance. The PR body's claim "Still null when nothing knows: no fallback, and the unknown path below is then the real one" is false here: the unknown path is gated on tokenBalance, not on tokenDecimals. Untested; neither new suite covers disagreeing explorer values.
Acceptable: a balance whose scale the wallet will not stand behind is not a balance the confirmation screen should state either — e.g. in send.js, if (tokenDecimals === null) tokenBalance = null; so the existing unknown-balance wording fires; or a distinct validation code for a missing displayed scale so renderValidation() says what is actually wrong. Either way with a test on the disagreement fixture.
2. TODO.md and the commit message both state something false about the sweep
TODO.md:151 and the last line of the commit body: "The only 18s left in src/ are native ETH's real scale in src/shared/uniswap.js and the fixed-point comparison scale in src/shared/txValidation.js."
grep -c "decimals: 18" src/shared/tokenList.js → 432. The PR body's own sweep table gets this right (tokenList.js — bundled list's real per-token data, left alone); the compressed sentence that landed in the tree and in history contradicts it. A future reader re-running this sweep hits 432 hits and has to work out whether a regression occurred. Acceptable: say fallback18s, or name tokenList.js alongside the other two.
3. src/popup/views/helpers.js — the balanceLine() doc comment now documents unknownableAmount()
The #307 escaping rationale plus the new "amount is null for a holding whose scale nothing knows" paragraph sit immediately above function unknownableAmount(balance), whose parameter is balance and which renders nothing. balanceLine() — the function both paragraphs are about, and the one #307 was reported against — is left with no comment at all.
Verified and passing, by execution in my own clone at 041f5dc: fail-first both halves reproduced exactly (raw tb.decimals restored in send.js → 3 of 5 Send tests fail with Received: null, "Unable to estimate", and the displayedDecimals throw; helpers.js / prices.js / confirmTx.js reverted to next → exactly 10 reader-site tests fail). The 12 new tests are non-vacuous — each asserts the null/0 pair, so a collapse fails them, not merely a broken site. Unknown-scale refusal intact (NOVEL: tokenDecimals === null, unknown (NOVEL), Send disabled) and no 18 fallback was reintroduced. My own audit of raw tokenBalances[].decimals dereferences agrees with the PR's: exactly src/shared/approvalAmount.js:44 and src/popup/views/addressToken.js:189, nothing in src/background/ or src/content/. addressToken.js:189 is display-only (Decimals: info line, never feeds an amount) and this PR improves it — a fabricated 18 used to short-circuit its first branch, now a null falls through to the tracked/bundled real value; toDecimals() keeps it numeric so the unescaped interpolation is not reachable. README.md's claims check out against the code. make check and make build both exit 0 in my clone; lint ran in Docker, #11 [lint 1/1] RUN make lintDONE 5.7s, notCACHED, All matched files use Prettier code style!; Test Suites: 59 passed, Tests: 1048 passed, zero cached-test markers. CI green on 041f5dc — check, e2e-chrome, e2e-firefox all success. One commit, title ends (closes #349), base next, fast-forward onto current next (75a5fa9), no Claude/Anthropic reference or attribution trailer.
Raised, not filed: addressToken.js:189's chain still has inverted precedence versus resolveTokenDecimals() and no toDecimals() validation, so the info panel can print a scale other than the one every other screen computes with. Display-only and pre-existing; my view is that it should be folded into resolveTokenDecimals() eventually, but not in this PR.
FAIL — `needs-rework`. Independent re-review of `041f5dc`.
### 1. The Finding-1 regression is closed for the resolvable case but survives when resolution answers `null` while the stored balance does not
`src/popup/views/send.js:248` and `:260` now answer two different questions from two different sources, and nothing reconciles them:
```js
tokenBalance = tb ? (tb.balance ?? null) : "0"; // scale: bundled/tracked/THIS ROW's explorer value
tokenDecimals = resolveTokenDecimals(token, { trackedTokens, wallets }); // scale: bundled/tracked/AGREED-ACROSS-ADDRESSES explorer value
```
`balances.js:148` resolves without `wallets`, so its explorer leg is this row's own `decimals`. `send.js` resolves with `wallets`, so its explorer leg is `explorerDecimals()`, which answers `null` when two addresses report different values for one contract. For a token that is neither bundled nor tracked, that produces `tokenBalance` non-null with `tokenDecimals === null` — the exact input state Finding 1 was about.
Executed, in my clone, through the real `fetchTokenBalances()` / real Send handler / real `confirmTx.show()`: token `NOVEL`, address A row `decimals: "6"`, address B row `decimals: "18"`, sending from A.
- stored A: `{"decimals":6,"balance":"5.0"}`
- handed on: `tokenBalance: "5.0"`, `tokenDecimals: null`
- `confirm-balance`: `"5.0 NOVEL"`
- `confirm-fee-amount`: `"Unable to estimate"`
- `confirm-errors`: `""` — empty. `validateTransfer()` sees a usable `tokenBalance`, so `INSUFFICIENT_TOKEN` never fires and the new "this token's balance is unknown" sentence never appears.
- `btn-confirm-send.disabled`: `true`, and the only thing on screen is `confirm-fee-unknown-error` (`src/popup/index.html:697`): *"The network fee could not be estimated, so this transaction cannot be checked against your balance. Please go back and try again."*
Same symptom, same untrue and unactionable message, nothing naming decimals — narrower, and fail-closed, but it is this diff that makes `tokenDecimals: null` reachable alongside a non-null balance. The PR body's claim *"Still null when nothing knows: no fallback, and the unknown path below is then the real one"* is false here: the unknown path is gated on `tokenBalance`, not on `tokenDecimals`. Untested; neither new suite covers disagreeing explorer values.
Acceptable: a balance whose scale the wallet will not stand behind is not a balance the confirmation screen should state either — e.g. in `send.js`, `if (tokenDecimals === null) tokenBalance = null;` so the existing unknown-balance wording fires; or a distinct validation code for a missing displayed scale so `renderValidation()` says what is actually wrong. Either way with a test on the disagreement fixture.
### 2. `TODO.md` and the commit message both state something false about the sweep
`TODO.md:151` and the last line of the commit body: *"The only `18`s left in `src/` are native ETH's real scale in `src/shared/uniswap.js` and the fixed-point comparison scale in `src/shared/txValidation.js`."*
`grep -c "decimals: 18" src/shared/tokenList.js` → **432**. The PR body's own sweep table gets this right (`tokenList.js` — bundled list's real per-token data, left alone); the compressed sentence that landed in the tree and in history contradicts it. A future reader re-running this sweep hits 432 hits and has to work out whether a regression occurred. Acceptable: say *fallback* `18`s, or name `tokenList.js` alongside the other two.
### 3. `src/popup/views/helpers.js` — the `balanceLine()` doc comment now documents `unknownableAmount()`
The `#307` escaping rationale plus the new *"`amount` is null for a holding whose scale nothing knows"* paragraph sit immediately above `function unknownableAmount(balance)`, whose parameter is `balance` and which renders nothing. `balanceLine()` — the function both paragraphs are about, and the one `#307` was reported against — is left with no comment at all.
---
Verified and passing, by execution in my own clone at `041f5dc`: fail-first both halves reproduced exactly (raw `tb.decimals` restored in `send.js` → 3 of 5 Send tests fail with `Received: null`, `"Unable to estimate"`, and the `displayedDecimals` throw; `helpers.js` / `prices.js` / `confirmTx.js` reverted to `next` → exactly 10 reader-site tests fail). The 12 new tests are non-vacuous — each asserts the `null`/`0` pair, so a collapse fails them, not merely a broken site. Unknown-scale refusal intact (`NOVEL`: `tokenDecimals === null`, `unknown (NOVEL)`, Send disabled) and no `18` fallback was reintroduced. My own audit of raw `tokenBalances[].decimals` dereferences agrees with the PR's: exactly `src/shared/approvalAmount.js:44` and `src/popup/views/addressToken.js:189`, nothing in `src/background/` or `src/content/`. `addressToken.js:189` is display-only (`Decimals:` info line, never feeds an amount) and this PR improves it — a fabricated `18` used to short-circuit its first branch, now a `null` falls through to the tracked/bundled real value; `toDecimals()` keeps it numeric so the unescaped interpolation is not reachable. `README.md`'s claims check out against the code. `make check` and `make build` both exit 0 in my clone; lint ran in Docker, `#11 [lint 1/1] RUN make lint` `DONE 5.7s`, **not** `CACHED`, `All matched files use Prettier code style!`; `Test Suites: 59 passed`, `Tests: 1048 passed`, zero cached-test markers. CI green on `041f5dc` — `check`, `e2e-chrome`, `e2e-firefox` all success. One commit, title ends ` (closes #349)`, base `next`, fast-forward onto current `next` (`75a5fa9`), no Claude/Anthropic reference or attribution trailer.
Raised, not filed: `addressToken.js:189`'s chain still has inverted precedence versus `resolveTokenDecimals()` and no `toDecimals()` validation, so the info panel can print a scale other than the one every other screen computes with. Display-only and pre-existing; my view is that it should be folded into `resolveTokenDecimals()` eventually, but not in this PR.
PASS — merge-ready. Verified at 6b156b3 in my own clone: make check and make build exit 0, lint executed in Docker (#11 [lint 1/1] RUN make lint, DONE 5.1s, not CACHED), Tests: 1052 passed; CI green on the head commit (check, e2e-chrome, e2e-firefox); one commit ending (closes #349), base next, fast-forward onto 75a5fa9; fail-first re-derived through make test (deleting if (tb && tokenDecimals === null) tokenBalance = null; fails exactly 2 of 1052, Received: "5.0" and Expected: not ""); the corrected sweep sentence holds clause by clause (432 decimals: 18 in src/shared/tokenList.js; outside that file the only literal 18s in code are src/shared/txValidation.js:15, src/shared/uniswap.js:124 and :567 — every other occurrence is inside a comment).
Anomalies and disclosures, none blocking:
The round-3 evidence run invoked yarn run test:verbose tests/unknownScaleSend.test.js directly, which REPO_POLICIES.md:78 forbids ("Always use Makefile targets ... instead of invoking the underlying tools directly"); the PR body shows the command but does not name it as a deviation. Re-derived through make test: the reported numbers are exact, and the tree carries no artifact of the bypass.
The tb && narrowing is behaviourally untested — replacing it with the broader if (tokenDecimals === null) tokenBalance = null; leaves all 1052 tests passing. Still correct (tb falsy means no row, and the Send dropdown is built from tokenBalances, so that path is barely reachable) and fail-closed either way.
updateSendBalance() was not reconciled: on the disagreement fixture the Send screen reads Current balance: 5.0 NOVEL while the confirmation screen it leads to reads unknown (NOVEL). Consistent with the balance list — the deliberate asymmetry — but two consecutive screens state different things about one holding.
Asymmetry judged correct on its merits: refreshBalances() fetches every address concurrently and assigns addr.tokenBalances per resolved promise, so a wallets-consulting resolution inside fetchTokenBalances() would read a nondeterministic mix of old and new rows. No input yields a stored quantity alongside a null scale, and the two resolutions can never both be non-null and different (a disagreeing row makes explorerDecimals() answer null), so a displayed quantity and an encoded amount cannot use different scales.
Finding 3 verified in its end state only: the round-2 head 041f5dc is unreachable after the force-push, so "no code change rode along" is not independently diffable.
Nit: TODO.md reads "the user is told that rather than that the fee could not be estimated" — the fee message is in fact still shown alongside. The PR body states this precisely; the entry reads as substitution in isolation.
PASS — `merge-ready`. Verified at `6b156b3` in my own clone: `make check` and `make build` exit 0, lint executed in Docker (`#11 [lint 1/1] RUN make lint`, `DONE 5.1s`, not `CACHED`), `Tests: 1052 passed`; CI green on the head commit (`check`, `e2e-chrome`, `e2e-firefox`); one commit ending ` (closes #349)`, base `next`, fast-forward onto `75a5fa9`; fail-first re-derived through `make test` (deleting `if (tb && tokenDecimals === null) tokenBalance = null;` fails exactly 2 of 1052, `Received: "5.0"` and `Expected: not ""`); the corrected sweep sentence holds clause by clause (432 `decimals: 18` in `src/shared/tokenList.js`; outside that file the only literal `18`s in code are `src/shared/txValidation.js:15`, `src/shared/uniswap.js:124` and `:567` — every other occurrence is inside a comment).
Anomalies and disclosures, none blocking:
- The round-3 evidence run invoked `yarn run test:verbose tests/unknownScaleSend.test.js` directly, which `REPO_POLICIES.md:78` forbids ("Always use Makefile targets ... instead of invoking the underlying tools directly"); the PR body shows the command but does not name it as a deviation. Re-derived through `make test`: the reported numbers are exact, and the tree carries no artifact of the bypass.
- The `tb &&` narrowing is behaviourally untested — replacing it with the broader `if (tokenDecimals === null) tokenBalance = null;` leaves all 1052 tests passing. Still correct (`tb` falsy means no row, and the Send dropdown is built from `tokenBalances`, so that path is barely reachable) and fail-closed either way.
- `updateSendBalance()` was not reconciled: on the disagreement fixture the Send screen reads `Current balance: 5.0 NOVEL` while the confirmation screen it leads to reads `unknown (NOVEL)`. Consistent with the balance list — the deliberate asymmetry — but two consecutive screens state different things about one holding.
- Asymmetry judged correct on its merits: `refreshBalances()` fetches every address concurrently and assigns `addr.tokenBalances` per resolved promise, so a `wallets`-consulting resolution inside `fetchTokenBalances()` would read a nondeterministic mix of old and new rows. No input yields a stored quantity alongside a null scale, and the two resolutions can never both be non-null and different (a disagreeing row makes `explorerDecimals()` answer `null`), so a displayed quantity and an encoded amount cannot use different scales.
- Finding 3 verified in its end state only: the round-2 head `041f5dc` is unreachable after the force-push, so "no code change rode along" is not independently diffable.
- Nit: `TODO.md` reads "the user is told that rather than that the fee could not be estimated" — the fee message is in fact still shown alongside. The PR body states this precisely; the entry reads as substitution in isolation.
clawbot
merged commit 1b52aa1723 into next2026-08-23 21:19:05 +02:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #349.
Rework round 2 (2026-08-23, head
6b156b3) — the round-1 fix closed half the regression, and the PR body said otherwiseThe half that was still open
send.jssourced the balance and the scale from two different resolutions of the same value, and nothing reconciled them:src/shared/balances.jsresolves withoutwallets, so its explorer leg is the row it is formatting.src/popup/views/send.jsresolves withwallets, so its explorer leg isexplorerDecimals(), which answersnullwhen two addresses report different scales for one contract.For a token that is neither bundled nor tracked, whose explorer rows disagree across two addresses, that produced
tokenBalancenon-null alongsidetokenDecimals === null— the exact state round 1 was about. The unknown-balance path is gated ontokenBalance, not on the scale, soINSUFFICIENT_TOKENnever fired and the only thing on the confirmation screen wasconfirm-fee-unknown-erroragain.The round-1 PR body's claim "Still null when nothing knows: no fallback, and the unknown path below is then the real one" was false for that input. It is deleted, not softened.
Are the two call sites supposed to resolve identically?
No, and making them identical would be the wrong fix in either direction:
walletsintobalances.jswould have it consult, mid-fetch, the verystate.walletsthatrefreshBalances()is about to overwrite wholesale — including this address's own outgoing row. The displayed scale would then depend on refresh order and on stale state.walletsfromsend.jswould removeexplorerDecimals()'s cross-address check from the one value that goes on to encode a transfer. That check exists precisely so a disputed scale is not picked from.They answer different questions legitimately. The reconciliation belongs on the Send screen: a quantity computed at a scale Send has just refused is not a balance Send may state.
The
tb &&guard is deliberate and is the one deviation from the reviewer's suggested one-liner:tokenBalanceis"0"when the token has no row at all, and that zero is an absence of holdings, true at every scale and not derived from one. Only a stored quantity is withdrawn.What the user now sees, executed
Fixture: token
NOVEL(neither bundled nor tracked), address A rowdecimals: "6"/5000000units, address B rowdecimals: "18"/5000000000000000000units — both format to5.0, so only the scale is in dispute. Driven through the realfetchTokenBalances(), the real Send review handler and the realconfirmTx.show().{ decimals: 6, balance: "5.0" }txInfo.tokenDecimalsnullnulltxInfo.tokenBalance"5.0"nullconfirm-balance"5.0 NOVEL""unknown (NOVEL)"confirm-errors""btn-confirm-sendStated precisely, because the round-1 body was not:
confirm-fee-amountstill readsUnable to estimateandconfirm-fee-unknown-erroris still visible. The fee genuinely is unavailable —displayedDecimals()refuses the same missing scale insideestimateGas(). What changed is that it is no longer the only thing on the screen and no longer the only offered explanation. This is exactly how the already-accepted genuinely-unknown token (NOVELwith nodecimalsanywhere) already behaved. Both facts are now asserted in the test rather than described.Test, and its fail-first
tests/unknownScaleSend.test.js, new describe block "a scale the explorer's own rows disagree about", 4 tests:a[0].decimals === 6,a[0].balance === "5.0",b[0].decimals === 18tokenDecimals === nullandtokenBalance === nullconfirm-errorsnon-empty and carrying the unknown-balance sentence;confirm-balance === "unknown (NOVEL)"; Send disabled; fee line and fee-unknown element as described abovetokenDecimals === 6,tokenBalance === "5.0",confirm-balance === "5.0 NOVEL",confirm-errors === "", Send enabledMutation: delete the single line
if (tb && tokenDecimals === null) tokenBalance = null;fromsrc/popup/views/send.js, leaving everything else — including the resolution itself — in place.yarn run test:verbose tests/unknownScaleSend.test.js. 2 of the 4 fail; the control and the storage test pass. Verbatim:expect(received).toBeNull(),Received: "5.0"expect(received).not.toBe(expected),Expected: not ""— i.e.confirm-errorswas empty, reproducing the review's observation exactlyThe line was restored with an editor and the file diffed byte-for-byte against its pre-mutation copy before continuing.
Still behaving, re-run after the change
decimals):tokenDecimals === 18,tokenBalance === "5.0", fee estimated,confirm-errorsempty, Send enabled,transferAmountUnits("1.5", 18, 18n) === parseUnits("1.5", 18). All 4 tests pass.tokenDecimals === null,confirm-balance === "unknown (NOVEL)", unknown-balance message, Send disabled. Passes.Finding 2 — the sweep sentence
TODO.mdand the commit body said "The only18s left insrc/are native ETH's real scale inuniswap.jsand the fixed-point comparison scale intxValidation.js."grep -c "decimals: 18" src/shared/tokenList.js→ 432. Both now read:> No
|| 18or?? 18fallback remains anywhere insrc/. The literal18s that do remain are real data rather than defaults: 432 per-tokendecimals: 18entries in the bundledsrc/shared/tokenList.js, and, outside that file, only native ETH's protocol-defined scale insrc/shared/uniswap.jsand the fixed-point comparison scale insrc/shared/txValidation.js.Checked before writing, and each clause is separately re-runnable:
grep -rnE '(\|\||\?\?)\s*"?18"?' --include=*.js src/→ 2 hits, both comments naming the pattern.grep -c "decimals: 18" src/shared/tokenList.js→ 432.grep -rn "18" --include=*.js src/ | grep -v '^src/shared/tokenList.js' | grep -vi '0x\|//'→ exactlyuniswap.js:124,uniswap.js:567,txValidation.js:15.The compressed sentence was written from the sweep table's conclusion rather than from the sweep. Writing the claim last, after re-running the check, is what would have caught it.
Finding 3 —
helpers.jscomment placementThe
balanceLine()doc comment (the #307 escaping rationale plus theamount-is-null paragraph) sat aboveunknownableAmount().unknownableAmount()and its own two-line comment moved above it, so each comment is on the function it describes. No code change.Not folded in
src/popup/views/addressToken.js:189's inverted precedence versusresolveTokenDecimals(), per the reviewer's own instruction. Display-only, cannot feed an amount, and improved rather than worsened by this PR.Rework round 1 (head
041f5dc) — the capability regression this PR introducedDisclosed rather than quietly corrected, because it was a regression and not an inherited defect.
What was wrong. This PR made
tokenBalances[].decimalsthe explorer's own answer alone (nullwhen it reported none), while the scale a balance is displayed at is resolved separately throughresolveTokenDecimals()— bundled list, then tracked tokens, then the explorer. Those are two different questions.src/popup/views/send.jswas still reading the stored field raw and carrying it onto the pending transaction:For a bundled or tracked token whose explorer row omits
decimals— WETH, DAI — that putnullon a transaction whose balance and amount were both displayed correctly.validateTransfer()had nothing to object to, so the new "this token's balance is unknown" message never fired; insteadconfirmTx.jsreacheddisplayedDecimals(null)insideestimateGas(), which throws, is caught asFEE_UNAVAILABLE, and disabled Send behind "The network fee could not be estimated, so this transaction cannot be checked against your balance. Please go back and try again." Untrue, unactionable, and nothing on the screen mentioned decimals. Before this PR the fabricated18was that token's real scale and the send completed, so this was a loss of capability. Fail-closed, so no money was at risk.The fix.
send.jsresolves the scale throughresolveTokenDecimals()with no fallback of 18:transferAmountUnits()'s displayed-vs-on-chain comparison is untouched and still guards the encode. Round 2 above adds what this did not: what happens when that resolution answersnullwhile the stored balance does not.Nit also taken:
tb.balance != null ? tb.balance : nullis nowtb.balance ?? null.Other consumers of
tokenBalances[].decimals, audited. Exactly two sites insrc/dereference the field, plus the callers that reach it through resolution:src/shared/approvalAmount.js:44(explorerDecimals())toDecimals(), disagreement across addresses answersnullresolveTokenDecimals(), and it must compare explorer values onlysrc/popup/views/addressToken.js:189Decimals:info line); it never formats an amountsrc/popup/views/send.jsresolveTokenDecimals()src/popup/views/approval.js:95,src/shared/uniswap.js:82resolveTokenDecimals()src/popup/views/confirmTx.js:338,:488txInfo.tokenDecimals, which onlysend.jssetssend.jsisNothing in
src/background/orsrc/content/readsdecimalsat all;src/background/index.js:1175copiestokenBalancesas an opaque field.The reader half is tested.
grep -rn "balance: null" tests/returned nothing before this rework. Two new files, one assertion per pair at each reader site — thatnulland0produce different output, not merely thatnulldoes something reasonable:tests/unknownScaleDisplay.test.js(7 tests)null0balanceLine()quantityquantity unknown0.0000balanceLine()fiat cell $0.00balanceLinesForAddress(), show-zero offbalanceLinesForAddress(), show-zero onquantity unknown0.0000addressHoldsFunds()truefalsegetAddressValue(){ usd: 0, partial: true }{ usd: 0, partial: false }tests/unknownScaleSend.test.js(9 tests after round 2) drives the realfetchTokenBalances(), the real Send review handler and the realconfirmTx.show()against a stub DOM and a stub provider.Fail-first, executed.
git checkout 12190ba -- src/popup/views/send.js(the pre-rework head),make test→ 3 of the 5 round-1 Send tests fail, reproducing the regression exactly:expected 18, received null;confirm-fee-amountreceived"Unable to estimate";displayedDecimalsthrew.src/popup/views/helpers.js,src/shared/prices.jsandsrc/popup/views/confirmTx.jsto theirnextversions,make test→ all 10 reader-site tests fail. Restored in both cases; working tree clean.The defect
fetchTokenBalances()didparseInt(item.token.decimals || "18", 10)before writing tostate.wallets[].addresses[].tokenBalances[].decimals. A token whosedecimals()reverts — one the explorer reports no scale for — was stored with a fabricated18that no reader could tell from a real one.That is upstream of a rule already merged. #306 made the ERC-20 approval amount line resolve the real scale or refuse to format, and #340 extended it to the swap lines. Both read this stored value as an authoritative source, so the guess walked straight past refusals that were intact and simply never fired.
What changed
src/shared/balances.jsstores the explorer's own answer ornull, never a default. Both approval paths then reach the existingunknownDecimalsAmount(); no new refusal mechanism was built.balance: null— unknown, never zero. The balance list, the address USD total (partial, the existing vocabulary), the Send screen and the confirmation screen each say so rather than printing0.0000for money that is really there.resolveTokenDecimals(), so a token either of them knows still displays its real quantity when the explorer's entry omitsdecimals. The storeddecimalsstays the explorer's own answer either way — copying another source into it would makeexplorerDecimals()'s disagreement check compare something other than explorer values. Consequently every screen that needs a display scale asksresolveTokenDecimals(), including Send.toDecimals()is now one shared export fromtransferAmount.jsinstead of three copies.approvalAmount.js's copy was byte-identical; deduping it is behaviour-neutral.Out of scope, deliberately: the issue's open design question — whether an explorer-sourced scale should size an approval at all. Untouched. An explorer that does report a scale is treated exactly as it is today.
Fail-first evidence (writer half)
tests/fabricatedDecimals.test.jsdrives a real Blockscout response through the realfetchTokenBalances()and asserts on the real approval screens (a test that hand-writesdecimals: nullonto state would pass on the broken build, because the fabrication is in the writer).Mutation:
git stash push -- src/— revertsrc/to head, keep the new test — thenmake check. 9 of the 11 new tests fail. Observed output, verbatim:transferAmount line"1000000000 base units (decimals unknown)""0.000000001"approveAmount line"1000000000 base units (decimals unknown)""0.000000001"Amountline"1000000000 base units (decimals unknown)""0.000000001"decimals, absentnull18decimals, explicitnullnull18decimals, real0018balance, unknown scalenull"5.0"A finding from that fail-first run, disclosed because it changed the test. My first fixture used a small holding, and the two approval-line tests passed against head — for the wrong reason. Formatted at the fabricated 18 the balance came out
"0.0", the dust filter dropped the row entirely, and the approval screens then found no source at all and refused by luck. The laundering only bites where the holding survives the dust filter at 18. The fixture now usesHOLDING = 5000000000000000000base units so it does. Read that as: the defect's blast radius is holdings large enough not to round to zero at 18, which for a token of true scale 6 is anything from about a millionth of a token upward.The
|| 18sweepgrep -rnE '(\|\||\?\?)\s*"?18"?' --include=*.js src/plus a manual read of everydecimalssite insrc/:src/shared/balances.jsparseInt(item.token.decimals || "18", 10)before storagetoDecimals(); stores the answer ornullsrc/shared/transactions.jsparseInt(tt.total?.decimals || "18", 10)inparseTokenTransfer()toDecimals(); with no scale the row states no quantity (value/exactValueblank, as the contract-call rows in the same file already do) andrawUnitreadsSYM base units (decimals unknown). The exact figure is not lost — it is the base-unit line, the one number that needs no scale to be truesrc/shared/uniswap.js:124,:567{ symbol: "ETH", decimals: 18 }src/shared/txValidation.js:15SCALE_DECIMALS = 18src/shared/tokenList.js(432 entries)decimals: 18per bundled tokenRe-run on
6b156b3: no|| "18",|| 18or?? 18remains anywhere insrc/— the only hits are two comments naming the pattern.Falsy collapse (#246)
Every check is presence/type, never truthiness.
toDecimals()enumerates accepted types and answers0for a real scale of zero,nullfor absence — two different answers, which is the whole point. Test:a real scale of zero is stored as zero, not collapsedasserts both"0"and0store as0and format the balance at scale 0 ("5000000000000000000.0"). Against head it received18.Migration
Existing installs already hold fabricated 18s that cannot be told apart from real ones retroactively. That is the defect itself; no migration can undo it, and this PR does not pretend to.
Chosen behaviour: they are left exactly as they are and display exactly as they do today, until the next balance refresh replaces them. This is safe because
refreshBalances()writesaddr.tokenBalanceswholesale (addr.tokenBalances = balances), so the first refresh after upgrade replaces every row with one built by the fixed code — no partial state, no per-row migration to get wrong. That refresh needs no user action: the popup runs one on open (doRefreshAndRender()), and the background alarm runs one on its own schedule (backgroundRefresh()). So the exposure window is at most one refresh, and until then the user sees precisely the pre-upgrade behaviour rather than a new one.The schema version is deliberately NOT bumped. Version 1 records remain fully valid and are read exactly as before; the change widens what a field may hold (adds
null), and every reader in this build handles it. A bump would gain nothing — no migration is possible by construction — and would only break downgrades.Composition with the recent work in this area
tokenRefs()floor drops atokenBalancesentry only if it is not a record or has no textaddress. Adecimals: null/balance: nullentry therefore survives it — verified, and thetokenRefs()comment now says so explicitly, because flooring those nulls to a default would put the guess back one layer below where it was removed.src/shared/stateSchema.jsfield-by-field categorisation:tokenBalances[].decimalsdoes not move between categories —tokenBalanceswas and remains "type-checked, container AND entries". What I added is a precision the header was missing and which my change makes load-bearing: what is checked on an entry isaddressalone, and the rest of an entry is taken verbatim — so an entry'sdecimals/balancemay benull, and readers handle that rather than being defended from it here.resolveTokenDecimals()is unchanged and is the mechanism this fix relies on;balances.jsandsend.jsnow call it too, for display scale only.Verification (on
6b156b3, rebased onto currentnextat75a5fa9)make check: green, exit 0.Test Suites: 59 passed,Tests: 1052 passed, 1052 total(1048 before round 2; +4 disagreement tests). Lint ran in Docker —#11 [lint 1/1] RUN make lint,DONE 5.4s, notCACHED, on this tree —All matched files use Prettier code style!.check-censored: 185 tracked file(s) inspected.test-verify-build: 46 case(s) passed.make build: exit 0.verify-build: 15 emitted file(s) verified against the receipt, 4 bundle(s) autistmask-build-debug=off.dist/removed afterwards withmake clean.make fmtrun; prettier clean.git fetchre-run immediately before pushing:nextstill at75a5fa9, so the branch is a fast-forward and no conflict arose. No containers created or left behind (docker buildonly;docker ps -aempty). No cache pruned.d1e1e7858dto12190ba428FAIL —
needs-rework.1. BLOCKING: a bundled or tracked token whose explorer row omits
decimalsbecomes unsendable, with a wrong error messagesrc/popup/views/send.js:248still reads the raw stored value:This PR taught
balances.jsto resolve the DISPLAY scale throughresolveTokenDecimals()(bundled list, then tracked tokens, then the explorer) while deliberately storing the explorer's own answer —null— intokenBalances[].decimals.send.jswas not taught the same thing, so the two halves now disagree for a token the wallet actually knows the scale of.The PR's own test proves the wallet reaches that state:
tests/fabricatedDecimals.test.js,the bundled list still supplies a quantity the explorer omittedassertsdecimals === nullandbalance === "5.0"for WETH.Walk it for a bundled 18-decimal token (WETH, DAI) whose explorer row omits
decimals:Current balance: 5.0 WETH.send.js:248setstokenDecimals = null;tokenBalance = "5.0"(non-null), sovalidateTransfer()raises nothing and the new "This token's balance is unknown" message never appears.confirmTx.js:338displayedDecimals(txInfo.tokenDecimals)throws insideestimateGas(), which is caught intoFEE_UNAVAILABLE.Unable to estimateand, fromsrc/popup/index.html:693, "The network fee could not be estimated, so this transaction cannot be checked against your balance. Please go back and try again." Send is disabled permanently — going back and trying again can never work, and nothing on the screen mentions decimals.Before this PR the stored
18was coincidentally correct for such a token and the send completed. So this is a capability regression introduced here, it is undisclosed in the PR body, and it degrades to an unactionable and untrue message. The direction is fail-closed so no money is at risk, but "cannot send WETH, told it is a fee problem" is a worse outcome than the fabricated 18 was in exactly this case.Acceptable:
send.jsresolves the scale the same waybalances.jsnow does —resolveTokenDecimals(token, { trackedTokens: state.trackedTokens, wallets: state.wallets })— and only carriesnullforward when that answersnull. Then the unknown-scale path is reached only when the scale really is unknown, andconfirmTx's balance/validation messaging (which already says "unknown") is what the user sees instead of a fee error.transferAmountUnits()'s displayed-vs-on-chain comparison is unchanged and still guards the encode.2. The display half of this change has no test coverage at all
grep -rn "balance: null" tests/returns nothing. Five of the six reader changes are asserted nowhere:helpers.jsbalanceLine()renderingquantity unknowninstead of0.0000helpers.jsbalanceLinesForAddress()keeping a null row regardless of the show-zero settinghelpers.jsaddressHoldsFunds()answeringtruefor a null balance (tests/deleteAddress.test.jscovers ETH-only, token-only, dust, empty and zero-token — not null)prices.jsgetAddressValue()settingpartialsend.js/confirmTx.jsstatingunknown (SYM), andconfirmTx.js's newINSUFFICIENT_TOKENwordingThe new suite is genuinely fail-first and covers the storage fix and both approval lines well (verified: 9 of 11 fail with
src/reverted tonext). But a wallet-wide null-vs-zero display change is precisely the class that has recurred through #246, #306, #322, #359 and #364, and it should not land with the reader side untested. Each of the five needs one assertion thatnulland0produce different output.3. Nit
src/popup/views/send.js:247—tb.balance != null ? tb.balance : nullis a no-op ternary.tb.balance ?? nullstates the same normalization.Verified and passing: fail-first reproduced exactly (9/11,
src/reverted tonext);|| 18/?? 18sweep clean, and every disposition in the PR table checks out;nulland0are distinguished at all six sites;addressHoldsFunds()errs toward warning and its only consumer isdeleteAddress.js; the migration claim holds (addr.tokenBalances = balanceswholesale atbalances.js:268, andmergeAddress()treatstokenBalancesas a leaf, so nothing merges old rows forward);toDecimals()dedupe is byte-identical and behaviour-neutral;tokenRefs()filters onisRecord && typeof address === "string"alone, so the newstateSchema.jsclaim is accurate and an unknown-scale entry survives the #311 floor;transactions.js's blankvalue/exactValueare guarded at every consumer and the base-unit line still carries the exact figure; no Claude/Anthropic reference or attribution trailer; one commit titled(closes #349); basenext; mergeable; prettier clean.make checkandmake buildboth exit 0 in my own clone, lint executed in Docker (#11 [lint 1/1] RUN make lint, 5.2s, notCACHED),Tests: 1027 passed. CI:check / checksuccess on12190ba; both e2e jobs still queued at review time, neither red.Raised rather than filed: the dust filter no longer masks an UNKNOWN scale (the row is kept). It still masks a scale the explorer reports but reports wrong — the row formats to
"0.0", is dropped, and the holding then vanishes from the balance list, the Send dropdown andexplorerDecimals()alike. That follows from the trusted-explorer decision settled in #349 (comment) and is not a defect of this diff.12190ba428toe3f3b331f9e3f3b331f9to041f5dc39fFAIL —
needs-rework. Independent re-review of041f5dc.1. The Finding-1 regression is closed for the resolvable case but survives when resolution answers
nullwhile the stored balance does notsrc/popup/views/send.js:248and:260now answer two different questions from two different sources, and nothing reconciles them:balances.js:148resolves withoutwallets, so its explorer leg is this row's owndecimals.send.jsresolves withwallets, so its explorer leg isexplorerDecimals(), which answersnullwhen two addresses report different values for one contract. For a token that is neither bundled nor tracked, that producestokenBalancenon-null withtokenDecimals === null— the exact input state Finding 1 was about.Executed, in my clone, through the real
fetchTokenBalances()/ real Send handler / realconfirmTx.show(): tokenNOVEL, address A rowdecimals: "6", address B rowdecimals: "18", sending from A.{"decimals":6,"balance":"5.0"}tokenBalance: "5.0",tokenDecimals: nullconfirm-balance:"5.0 NOVEL"confirm-fee-amount:"Unable to estimate"confirm-errors:""— empty.validateTransfer()sees a usabletokenBalance, soINSUFFICIENT_TOKENnever fires and the new "this token's balance is unknown" sentence never appears.btn-confirm-send.disabled:true, and the only thing on screen isconfirm-fee-unknown-error(src/popup/index.html:697): "The network fee could not be estimated, so this transaction cannot be checked against your balance. Please go back and try again."Same symptom, same untrue and unactionable message, nothing naming decimals — narrower, and fail-closed, but it is this diff that makes
tokenDecimals: nullreachable alongside a non-null balance. The PR body's claim "Still null when nothing knows: no fallback, and the unknown path below is then the real one" is false here: the unknown path is gated ontokenBalance, not ontokenDecimals. Untested; neither new suite covers disagreeing explorer values.Acceptable: a balance whose scale the wallet will not stand behind is not a balance the confirmation screen should state either — e.g. in
send.js,if (tokenDecimals === null) tokenBalance = null;so the existing unknown-balance wording fires; or a distinct validation code for a missing displayed scale sorenderValidation()says what is actually wrong. Either way with a test on the disagreement fixture.2.
TODO.mdand the commit message both state something false about the sweepTODO.md:151and the last line of the commit body: "The only18s left insrc/are native ETH's real scale insrc/shared/uniswap.jsand the fixed-point comparison scale insrc/shared/txValidation.js."grep -c "decimals: 18" src/shared/tokenList.js→ 432. The PR body's own sweep table gets this right (tokenList.js— bundled list's real per-token data, left alone); the compressed sentence that landed in the tree and in history contradicts it. A future reader re-running this sweep hits 432 hits and has to work out whether a regression occurred. Acceptable: say fallback18s, or nametokenList.jsalongside the other two.3.
src/popup/views/helpers.js— thebalanceLine()doc comment now documentsunknownableAmount()The
#307escaping rationale plus the new "amountis null for a holding whose scale nothing knows" paragraph sit immediately abovefunction unknownableAmount(balance), whose parameter isbalanceand which renders nothing.balanceLine()— the function both paragraphs are about, and the one#307was reported against — is left with no comment at all.Verified and passing, by execution in my own clone at
041f5dc: fail-first both halves reproduced exactly (rawtb.decimalsrestored insend.js→ 3 of 5 Send tests fail withReceived: null,"Unable to estimate", and thedisplayedDecimalsthrow;helpers.js/prices.js/confirmTx.jsreverted tonext→ exactly 10 reader-site tests fail). The 12 new tests are non-vacuous — each asserts thenull/0pair, so a collapse fails them, not merely a broken site. Unknown-scale refusal intact (NOVEL:tokenDecimals === null,unknown (NOVEL), Send disabled) and no18fallback was reintroduced. My own audit of rawtokenBalances[].decimalsdereferences agrees with the PR's: exactlysrc/shared/approvalAmount.js:44andsrc/popup/views/addressToken.js:189, nothing insrc/background/orsrc/content/.addressToken.js:189is display-only (Decimals:info line, never feeds an amount) and this PR improves it — a fabricated18used to short-circuit its first branch, now anullfalls through to the tracked/bundled real value;toDecimals()keeps it numeric so the unescaped interpolation is not reachable.README.md's claims check out against the code.make checkandmake buildboth exit 0 in my clone; lint ran in Docker,#11 [lint 1/1] RUN make lintDONE 5.7s, notCACHED,All matched files use Prettier code style!;Test Suites: 59 passed,Tests: 1048 passed, zero cached-test markers. CI green on041f5dc—check,e2e-chrome,e2e-firefoxall success. One commit, title ends(closes #349), basenext, fast-forward onto currentnext(75a5fa9), no Claude/Anthropic reference or attribution trailer.Raised, not filed:
addressToken.js:189's chain still has inverted precedence versusresolveTokenDecimals()and notoDecimals()validation, so the info panel can print a scale other than the one every other screen computes with. Display-only and pre-existing; my view is that it should be folded intoresolveTokenDecimals()eventually, but not in this PR.041f5dc39ftofb260ddf20fb260ddf20to6b156b32ecPASS —
merge-ready. Verified at6b156b3in my own clone:make checkandmake buildexit 0, lint executed in Docker (#11 [lint 1/1] RUN make lint,DONE 5.1s, notCACHED),Tests: 1052 passed; CI green on the head commit (check,e2e-chrome,e2e-firefox); one commit ending(closes #349), basenext, fast-forward onto75a5fa9; fail-first re-derived throughmake test(deletingif (tb && tokenDecimals === null) tokenBalance = null;fails exactly 2 of 1052,Received: "5.0"andExpected: not ""); the corrected sweep sentence holds clause by clause (432decimals: 18insrc/shared/tokenList.js; outside that file the only literal18s in code aresrc/shared/txValidation.js:15,src/shared/uniswap.js:124and:567— every other occurrence is inside a comment).Anomalies and disclosures, none blocking:
yarn run test:verbose tests/unknownScaleSend.test.jsdirectly, whichREPO_POLICIES.md:78forbids ("Always use Makefile targets ... instead of invoking the underlying tools directly"); the PR body shows the command but does not name it as a deviation. Re-derived throughmake test: the reported numbers are exact, and the tree carries no artifact of the bypass.tb &&narrowing is behaviourally untested — replacing it with the broaderif (tokenDecimals === null) tokenBalance = null;leaves all 1052 tests passing. Still correct (tbfalsy means no row, and the Send dropdown is built fromtokenBalances, so that path is barely reachable) and fail-closed either way.updateSendBalance()was not reconciled: on the disagreement fixture the Send screen readsCurrent balance: 5.0 NOVELwhile the confirmation screen it leads to readsunknown (NOVEL). Consistent with the balance list — the deliberate asymmetry — but two consecutive screens state different things about one holding.refreshBalances()fetches every address concurrently and assignsaddr.tokenBalancesper resolved promise, so awallets-consulting resolution insidefetchTokenBalances()would read a nondeterministic mix of old and new rows. No input yields a stored quantity alongside a null scale, and the two resolutions can never both be non-null and different (a disagreeing row makesexplorerDecimals()answernull), so a displayed quantity and an encoded amount cannot use different scales.041f5dcis unreachable after the force-push, so "no code change rode along" is not independently diffable.TODO.mdreads "the user is told that rather than that the fee could not be estimated" — the fee message is in fact still shown alongside. The PR body states this precisely; the entry reads as substitution in isolation.