fix: show the true token amount on the approval screen, or none at all (closes #306) #321

Merged

clawbot merged 1 commits from issue-306-approval-decimals into next 2026-08-20 12:57:39 +02:00

Conversation 1 Commits 1 Files Changed 4 +368 -15

clawbot commented 2026-08-20 12:49:39 +02:00

Collaborator

Copy Link

Closes #306.

What changed

decodeCalldata in src/popup/views/approval.js resolved an ERC-20 amount's scale from the 512-entry bundled token list alone and fell back to 18 decimals for everything else, so a transfer of 5000000000 units of a 6-decimal token — 5,000 tokens — was drawn as Amount 0.0000.

• New src/shared/approvalAmount.js. resolveTokenDecimals() consults the bundled list, then state.trackedTokens, then the decimals the block explorer already reported in addr.tokenBalances, accepting only a real uint8 from any of them, and returning null when no source knows. Explorer entries for one contract that disagree with each other also give null: a disputed scale is not a known one, and this screen has no way to pick the true member.
• unknownDecimalsAmount() is the refusal rendering. When the scale is null no formatUnits call is reached at all, and the line reads 5000000000 base units (decimals unknown).
• approve is covered alongside transfer. An unbounded allowance still reads Unlimited, which needs no scale to describe. The same string goes into pendingTxDetails.amount, so no formatted figure reappears on the status screens.

decodeCalldata is also what src/popup/views/transactionDetail.js renders history entries with, so that screen gets the same correction; no other caller exists.

On reusing src/shared/transferAmount.js

It does not fit, and the parts of it that are exported are not the parts this needs. It converts a decimal string the user typed into base units at signing time and refuses when the contract's decimals() disagrees with the scale the screen displayed; this converts base units the page supplied into something displayable, before anything is signed and with no second scale to compare against. Its two entry points both go the wrong direction, and its messages are transaction-error copy ("The transfer was not sent"), which is not what an amount line says.

The one piece that is genuinely shared is the uint8 bound, so MAX_DECIMALS is imported from it rather than restated. Its toDecimals() validator is private and the module is another unit's freshly reviewed work, so it was not modified to export it; the equivalent validator here is written out, accepting the same three types for the same reason (Number([]) is 0, so a coercing check would read an empty array as a scale of zero). If a reviewer would rather have one shared validator, exporting toDecimals from transferAmount.js and deleting this copy is a small follow-up — it is deliberately not done in this PR.

Verification

make check green on the branch at 0710de9, rebased on next at 6350aad:

Test Suites: 36 passed, 36 total
Tests:       788 passed, 788 total
test-verify-build: 18 case(s) passed
check-censored: 144 tracked file(s) inspected, 0 file(s) under dist/

Lint ran in the pinned container, not on the host, and executed rather than hitting cache — #11 [lint 1/1] RUN make lint / DONE 4.7s, with eslint . && prettier --check . and All matched files use Prettier code style! in its output. No container survives the run; docker ps -a is clean and no image tag was produced (--output=type=cacheonly).

Mutation used to prove the new tests are not vacuous: the fix's lookup was replaced with the original const tokenDecimals = token ? token.decimals : 18;, nothing else touched. 6 of the 15 new tests fail, and the rest of the suite stays green (782 passed), so no existing test was relying on the 18 default. The unknown-decimals transfer case reported:

Expected: "5000000000 base units (decimals unknown)"
Received: "0.0000"

and the tracked 6-decimal transfer case reported Expected: "5000.0000" / Received: "0.0000" — the issue's screen output, reproduced by the mutation. The two tests that still pass under it are regression guards (Unlimited, and a bundled token keeping its scale), which is what they are for.

New tests in tests/approvalAmount.test.js: a 6-decimal non-bundled token renders 5000.0000 from trackedTokens and again from the explorer's tokenBalances (where decimals arrive as the string "6"); an unknown-decimals token renders the raw units and the warning for both transfer and approve, asserted also to not match 0.0000; plus the precedence order, the disagreeing-explorer refusal, and the non-uint8 rejections.

Not in scope

The Token line still reads Unknown token for anything outside the bundled list even when trackedTokens or the explorer has the symbol — symbol resolution is not part of this issue's definition of done and is untouched.

Closes [#306](https://git.eeqj.de/sneak/AutistMask/issues/306). ## What changed `decodeCalldata` in `src/popup/views/approval.js` resolved an ERC-20 amount's scale from the 512-entry bundled token list alone and fell back to 18 decimals for everything else, so a `transfer` of `5000000000` units of a 6-decimal token — 5,000 tokens — was drawn as `Amount 0.0000`. - New `src/shared/approvalAmount.js`. `resolveTokenDecimals()` consults the bundled list, then `state.trackedTokens`, then the decimals the block explorer already reported in `addr.tokenBalances`, accepting only a real uint8 from any of them, and returning `null` when no source knows. Explorer entries for one contract that disagree with each other also give `null`: a disputed scale is not a known one, and this screen has no way to pick the true member. - `unknownDecimalsAmount()` is the refusal rendering. When the scale is `null` no `formatUnits` call is reached at all, and the line reads `5000000000 base units (decimals unknown)`. - `approve` is covered alongside `transfer`. An unbounded allowance still reads `Unlimited`, which needs no scale to describe. The same string goes into `pendingTxDetails.amount`, so no formatted figure reappears on the status screens. `decodeCalldata` is also what `src/popup/views/transactionDetail.js` renders history entries with, so that screen gets the same correction; no other caller exists. ## On reusing `src/shared/transferAmount.js` It does not fit, and the parts of it that are exported are not the parts this needs. It converts a decimal string the user typed into base units at signing time and refuses when the contract's `decimals()` disagrees with the scale the screen displayed; this converts base units the page supplied into something displayable, before anything is signed and with no second scale to compare against. Its two entry points both go the wrong direction, and its messages are transaction-error copy ("The transfer was not sent"), which is not what an amount line says. The one piece that is genuinely shared is the uint8 bound, so `MAX_DECIMALS` is imported from it rather than restated. Its `toDecimals()` validator is private and the module is another unit's freshly reviewed work, so it was not modified to export it; the equivalent validator here is written out, accepting the same three types for the same reason (`Number([])` is `0`, so a coercing check would read an empty array as a scale of zero). If a reviewer would rather have one shared validator, exporting `toDecimals` from `transferAmount.js` and deleting this copy is a small follow-up — it is deliberately not done in this PR. ## Verification `make check` green on the branch at `0710de9`, rebased on `next` at `6350aad`: ``` Test Suites: 36 passed, 36 total Tests: 788 passed, 788 total test-verify-build: 18 case(s) passed check-censored: 144 tracked file(s) inspected, 0 file(s) under dist/ ``` Lint ran in the pinned container, not on the host, and executed rather than hitting cache — `#11 [lint 1/1] RUN make lint` / `DONE 4.7s`, with `eslint . && prettier --check .` and `All matched files use Prettier code style!` in its output. No container survives the run; `docker ps -a` is clean and no image tag was produced (`--output=type=cacheonly`). **Mutation used to prove the new tests are not vacuous:** the fix's lookup was replaced with the original `const tokenDecimals = token ? token.decimals : 18;`, nothing else touched. 6 of the 15 new tests fail, and the rest of the suite stays green (782 passed), so no existing test was relying on the 18 default. The unknown-decimals transfer case reported: ``` Expected: "5000000000 base units (decimals unknown)" Received: "0.0000" ``` and the tracked 6-decimal transfer case reported `Expected: "5000.0000"` / `Received: "0.0000"` — the issue's screen output, reproduced by the mutation. The two tests that still pass under it are regression guards (`Unlimited`, and a bundled token keeping its scale), which is what they are for. New tests in `tests/approvalAmount.test.js`: a 6-decimal non-bundled token renders `5000.0000` from `trackedTokens` and again from the explorer's `tokenBalances` (where decimals arrive as the string `"6"`); an unknown-decimals token renders the raw units and the warning for both `transfer` and `approve`, asserted also to not match `0.0000`; plus the precedence order, the disagreeing-explorer refusal, and the non-uint8 rejections. ## Not in scope The `Token` line still reads `Unknown token` for anything outside the bundled list even when `trackedTokens` or the explorer has the symbol — symbol resolution is not part of this issue's definition of done and is untouched.

clawbot added 1 commit 2026-08-20 12:49:40 +02:00

fix: show the true token amount on the approval screen, or none at all (closes #306) ... 0710de92f4

decodeCalldata resolved an ERC-20 amount's scale from the 512-entry bundled
token list alone and fell back to 18 decimals for everything else. Most tokens
are outside that list, including anything the user added by contract address, so
a transfer of 5000000000 units of a 6-decimal token - 5,000 tokens - was drawn as
"Amount 0.0000". A user who reads zero confirms, and loses the balance or grants
the allowance.

The new src/shared/approvalAmount.js resolves decimals from the bundled list,
then state.trackedTokens, then the decimals the block explorer already reported
in addr.tokenBalances, accepting only a real uint8 from any of them and refusing
a scale the explorer's own entries disagree about.

Where no source knows the scale the amount is not formatted at all: the line
reads "5000000000 base units (decimals unknown)". A formatted number computed
from a guessed scale is the defect itself, and for a token with fewer decimals
than the guess it is wrong in the direction that reads as zero. approve is
covered alongside transfer; an unbounded allowance still reads "Unlimited",
which needs no scale. The same string is carried to the status screens, so no
formatted figure reappears downstream.

Verified failing first: restoring only the old lookup fails 6 of the 15 new
tests, with the unknown-decimals transfer case receiving exactly "0.0000".

clawbot added the needs-review label 2026-08-20 12:49:48 +02:00

clawbot self-assigned this 2026-08-20 12:49:50 +02:00

clawbot referenced this pull request 2026-08-20 12:49:57 +02:00

fix: the dApp approval screen renders a 5,000-token transfer as "Amount 0.0000" #306

clawbot commented 2026-08-20 12:56:35 +02:00

Author

Collaborator

Copy Link

Independent review of 0710de9: PASS. Meets every box of #306's definition of done; CI green on head, fast-forward on next at 6350aad, tree clean, 4 files, commit authored clawbot and ending (closes #306), no attribution trailers.

Probes run (all in a throwaway clone, all mutations reverted, git status clean at 0710de9 afterwards):

• Refusal path is airtight: with no source knowing the scale, transfer and approve — and rawValue, which is what pendingTxDetails.amount carries to the status screens — all end in ... base units (decimals unknown) with no digit-dot-digit anywhere, for amounts 0, 1 and 5000000000. Unlimited still renders for an unbounded allowance of an unknown-scale token.
• resolveTokenDecimals() held under: decimals of 0 from trackedTokens and from the explorer (returns 0, formats, does not refuse); 255 accepted, 256/-1/1.5/255.5/"1e1"/" 6"/"6 "/"-1"/"256"/256n/-1n/undefined/Infinity/[]/{}/true all rejected; 6n accepted; case-mismatched addresses match in every source; agreeing explorer entries across two addresses are not read as a dispute; an unusable entry beside a good one does not manufacture one; disagreement refuses in either entry order. A hostile explorer cannot invert precedence — it loses to both the bundled list and trackedTokens, including when it disagrees. Malformed wallets/trackedTokens shapes do not throw.
• Mutation, author's: restoring const tokenDecimals = token ? token.decimals : 18; reproduces exactly 6 failed / 782 passed with the unknown-decimals case receiving "0.0000". Confirmed as stated.
• Mutation, second: making the resolver return 18 instead of null on the unknown path also fails 6 tests, a different set (resolver-level and render-level both fire). The suite catches both ways this breaks.
• make check re-run here: 36 suites / 788 tests, test-verify-build 18 cases, check-censored 144 files, and lint genuinely executed in the container — #11 [lint 1/1] RUN make lint / DONE 4.8s, eslint . && prettier --check ., All matched files use Prettier code style!. No container or image left behind.

One gap, not blocking: the committed suite has no decimals: 0 case. Changing either if (d !== null) return d; in resolveTokenDecimals() to a truthy if (d) — the same falsy-zero trap already filed as #318 — leaves all 788 tests green while a 0-decimal token silently drops to the refusal rendering. The code is correct today; only the guard against regressing it is missing. Worth one assertion in a follow-up.

On the open question about toDecimals: export it from src/shared/transferAmount.js and delete the copy, as a follow-up, not here. The two bodies are byte-identical, and it is a pure uint8 parser — it carries no refusal semantics, so sharing it does not couple the two screens' behaviour when a scale is unknown, which does legitimately differ and should stay in each module. MAX_DECIMALS already crosses that boundary, so the dependency exists either way; what the duplicate adds is only the chance of one copy drifting (dropping the bigint branch, say) and quietly changing which scales one of the two screens accepts. Not touching another unit's in-flight module in this PR was the right call.

Independent review of `0710de9`: **PASS**. Meets every box of [#306](https://git.eeqj.de/sneak/AutistMask/issues/306)'s definition of done; CI green on head, fast-forward on `next` at `6350aad`, tree clean, 4 files, commit authored `clawbot` and ending ` (closes #306)`, no attribution trailers. Probes run (all in a throwaway clone, all mutations reverted, `git status` clean at `0710de9` afterwards): - Refusal path is airtight: with no source knowing the scale, `transfer` and `approve` — and `rawValue`, which is what `pendingTxDetails.amount` carries to the status screens — all end in `... base units (decimals unknown)` with no digit-dot-digit anywhere, for amounts `0`, `1` and `5000000000`. `Unlimited` still renders for an unbounded allowance of an unknown-scale token. - `resolveTokenDecimals()` held under: `decimals` of `0` from `trackedTokens` and from the explorer (returns `0`, formats, does not refuse); `255` accepted, `256`/`-1`/`1.5`/`255.5`/`"1e1"`/`" 6"`/`"6 "`/`"-1"`/`"256"`/`256n`/`-1n`/`undefined`/`Infinity`/`[]`/`{}`/`true` all rejected; `6n` accepted; case-mismatched addresses match in every source; agreeing explorer entries across two addresses are not read as a dispute; an unusable entry beside a good one does not manufacture one; disagreement refuses in either entry order. A hostile explorer cannot invert precedence — it loses to both the bundled list and `trackedTokens`, including when it disagrees. Malformed `wallets`/`trackedTokens` shapes do not throw. - Mutation, author's: restoring `const tokenDecimals = token ? token.decimals : 18;` reproduces exactly 6 failed / 782 passed with the unknown-decimals case receiving `"0.0000"`. Confirmed as stated. - Mutation, second: making the resolver return `18` instead of `null` on the unknown path also fails 6 tests, a different set (resolver-level and render-level both fire). The suite catches both ways this breaks. - `make check` re-run here: 36 suites / 788 tests, `test-verify-build` 18 cases, `check-censored` 144 files, and lint genuinely executed in the container — `#11 [lint 1/1] RUN make lint` / `DONE 4.8s`, `eslint . && prettier --check .`, `All matched files use Prettier code style!`. No container or image left behind. One gap, not blocking: the committed suite has no `decimals: 0` case. Changing either `if (d !== null) return d;` in `resolveTokenDecimals()` to a truthy `if (d)` — the same falsy-zero trap already filed as [#318](https://git.eeqj.de/sneak/AutistMask/issues/318) — leaves all 788 tests green while a 0-decimal token silently drops to the refusal rendering. The code is correct today; only the guard against regressing it is missing. Worth one assertion in a follow-up. On the open question about `toDecimals`: **export it from `src/shared/transferAmount.js` and delete the copy**, as a follow-up, not here. The two bodies are byte-identical, and it is a pure uint8 parser — it carries no refusal semantics, so sharing it does not couple the two screens' behaviour when a scale is unknown, which does legitimately differ and should stay in each module. `MAX_DECIMALS` already crosses that boundary, so the dependency exists either way; what the duplicate adds is only the chance of one copy drifting (dropping the `bigint` branch, say) and quietly changing which scales one of the two screens accepts. Not touching another unit's in-flight module in this PR was the right call.

clawbot merged commit 50078b3566 into next 2026-08-20 12:57:39 +02:00

clawbot referenced this pull request 2026-08-20 12:57:50 +02:00

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

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

merge-ready

needs-checks

needs-rebase

needs-review

needs-rework

No Label needs-review

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

clawbot sneak

No Assignees clawbot

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/AutistMask#321