test: guard decimals:0 against the falsy trap, and de-duplicate toDecimals #325

New Issue

clawbot · 2026-08-20T12:57:50+02:00

clawbot commented

2026-08-20 12:57:50 +02:00

Found by the independent review of #321 (#321 (comment)). The code is correct today; the regression guard is missing.

The gap. resolveTokenDecimals() in src/shared/approvalAmount.js handles decimals: 0 correctly, but nothing asserts it. The reviewer mutated both if (d !== null) return d; guards to a truthy if (d) — the same falsy-zero trap already filed as #318 — and all 788 tests stayed green while a 0-decimal token silently fell through to the unknown-scale rendering. A zero-decimal ERC-20 is legitimate and would render as "decimals unknown" instead of its true quantity.

The duplication. toDecimals exists byte-identically in src/shared/approvalAmount.js and src/shared/transferAmount.js. It is a pure uint8 parser carrying no refusal semantics, so sharing it does not couple the two screens' deliberately different unknown-scale behaviour, and MAX_DECIMALS already crosses that module boundary. The only thing the duplicate adds is drift risk — one copy losing the bigint branch would silently change which scales one screen accepts, with no test noticing.

Both are one small PR. The author of #306 correctly declined to touch transferAmount.js while it was another unit's freshly reviewed work; it has since landed, so the coupling concern is gone.

Definition of done

toDecimals is exported from src/shared/transferAmount.js and the copy in src/shared/approvalAmount.js is deleted.
A decimals: 0 assertion at the resolver level and at the rendered amount-line level.
Mutating either d !== null guard to a truthy if (d) fails a test. State the mutation and the observed result.
make check green.

Found by the independent review of https://git.eeqj.de/sneak/AutistMask/pulls/321 (https://git.eeqj.de/sneak/AutistMask/pulls/321#issuecomment-67521). The code is correct today; the regression guard is missing. **The gap.** `resolveTokenDecimals()` in `src/shared/approvalAmount.js` handles `decimals: 0` correctly, but nothing asserts it. The reviewer mutated both `if (d !== null) return d;` guards to a truthy `if (d)` — the same falsy-zero trap already filed as https://git.eeqj.de/sneak/AutistMask/issues/318 — and **all 788 tests stayed green** while a 0-decimal token silently fell through to the unknown-scale rendering. A zero-decimal ERC-20 is legitimate and would render as "decimals unknown" instead of its true quantity. **The duplication.** `toDecimals` exists byte-identically in `src/shared/approvalAmount.js` and `src/shared/transferAmount.js`. It is a pure uint8 parser carrying no refusal semantics, so sharing it does not couple the two screens' deliberately different unknown-scale behaviour, and `MAX_DECIMALS` already crosses that module boundary. The only thing the duplicate adds is drift risk — one copy losing the `bigint` branch would silently change which scales one screen accepts, with no test noticing. Both are one small PR. The author of #306 correctly declined to touch `transferAmount.js` while it was another unit's freshly reviewed work; it has since landed, so the coupling concern is gone. ## Definition of done - [ ] `toDecimals` is exported from `src/shared/transferAmount.js` and the copy in `src/shared/approvalAmount.js` is deleted. - [ ] A `decimals: 0` assertion at the resolver level **and** at the rendered amount-line level. - [ ] Mutating either `d !== null` guard to a truthy `if (d)` fails a test. State the mutation and the observed result. - [ ] `make check` green.

clawbot referenced this issue

2026-08-20 14:26:18 +02:00

pre-1.0 security review: key handling, DEBUG-mode policy, RPC input validation #303

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/AutistMask#325