Compare commits

..

1 Commits

Author SHA1 Message Date
fb260ddf20 fix: store an absent explorer decimals as unknown instead of fabricating 18 (closes #349)
Some checks failed
check / check (push) Successful in 31s
e2e / e2e-firefox (push) Has been cancelled
e2e / e2e-chrome (push) Has been cancelled
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.

Which makes the stored field the explorer's answer alone, and NOT the scale a screen renders at. Those are two questions, and every screen that needs the second one asks resolveTokenDecimals(). The Send screen did not: it read tokenBalances[].decimals raw and carried it onto the pending transaction, so a bundled or tracked token whose explorer row omits decimals reached displayedDecimals(null) inside estimateGas(). That throws, is caught as an unavailable fee, and disables Send behind "The network fee could not be estimated ... Please go back and try again" -- untrue, unactionable, and for a token such as WETH whose scale was never in doubt. The balance and the amount on the same screen were correct throughout, and validateTransfer() had nothing to object to, so nothing named the real reason. Before this change the fabricated 18 happened to be that token's real scale and the send completed, so this is a capability regression and not an inherited one. Send now resolves the scale through resolveTokenDecimals(), with no fallback.

The two resolutions are deliberately not identical, and where they differ the balance follows the scale. balances.js resolves without wallets, because it is formatting one explorer row during a fetch that is about to replace the very state it would be consulting; its explorer leg is therefore that row's own value. send.js resolves with wallets, which adds explorerDecimals()'s cross-address check, so a contract two addresses report different scales for answers null rather than picking one -- a check that must apply to a value which goes on to encode a transfer. For a token neither bundled nor tracked whose explorer rows disagree, that leaves a stored quantity computed at a scale Send has just refused. Stating it would leave validateTransfer() checking the amount against a number the wallet does not vouch for, and, since the unknown-balance path is gated on the balance rather than on the scale, would again leave the fee-estimate failure as the only thing on the confirmation screen. So Send withdraws the stored quantity along with the scale: an unknown scale is an unknown balance. Only a stored quantity is withdrawn -- the "0" for a token with no row at all is an absence of holdings, which is true at every scale.

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.

The reader half is asserted, not just the writer half. Each of the six sites that now distinguishes an unknown quantity from a zero one -- balanceLine(), balanceLinesForAddress(), addressHoldsFunds(), getAddressValue()'s partial flag, the Send balance line and the confirmation screen's balance and insufficient-balance wording -- is tested on the PAIR, because an assertion about null alone still passes on a build that renders both as zero. The Send and confirmation cases run the real explorer response through the real fetcher, the real review handler and the real confirmation screen, so they show which of the two scale questions each screen is asking, including a two-address fixture whose explorer rows report 6 and 18 for one contract.

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.

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.
2026-08-23 19:03:05 +00:00
4 changed files with 149 additions and 19 deletions

24
TODO.md
View File

@@ -145,14 +145,22 @@ but the review is broader than any of them.
[#246](https://git.eeqj.de/sneak/AutistMask/issues/246). Existing installs [#246](https://git.eeqj.de/sneak/AutistMask/issues/246). Existing installs
hold `18`s that cannot be told apart retroactively; they display exactly as hold `18`s that cannot be told apart retroactively; they display exactly as
they do today until the next balance refresh, which rewrites `tokenBalances` they do today until the next balance refresh, which rewrites `tokenBalances`
wholesale and needs no user action. The only `18`s left in `src/` are native wholesale and needs no user action. No `|| 18` or `?? 18` fallback remains
ETH's real scale in `src/shared/uniswap.js` and the fixed-point comparison anywhere in `src/`; the literal `18`s that do remain are real data, not
scale in `src/shared/txValidation.js`. `tokenBalances[].decimals` is the defaults — 432 per-token `decimals: 18` entries in the bundled
explorer's answer alone and not the scale a screen renders at, so the Send `src/shared/tokenList.js`, and, outside that file, only native ETH's
screen resolves through `resolveTokenDecimals()` like every other consumer: protocol-defined scale in `src/shared/uniswap.js` and the fixed-point
reading the stored field raw carried a `null` into `estimateGas()` for a comparison scale in `src/shared/txValidation.js`. `tokenBalances[].decimals`
bundled token such as WETH, which reported an unestimable network fee and left is the explorer's answer alone and not the scale a screen renders at, so the
Send disabled behind a message no retry could clear. Send screen resolves through `resolveTokenDecimals()` like every other
consumer: reading the stored field raw carried a `null` into `estimateGas()`
for a bundled token such as WETH, which reported an unestimable network fee
and left Send disabled behind a message no retry could clear. Send resolves
with `wallets`, which adds the cross-address disagreement check the balance
list does not make, so the two can differ; where they do, the stored quantity
was computed at a scale Send has refused, and it is withdrawn with it. An
unknown scale is an unknown balance, and the user is told that rather than
that the fee could not be estimated.
- 2026-08-23: The background no longer reads or writes the shared `state` - 2026-08-23: The background no longer reads or writes the shared `state`
singleton ([#324](https://git.eeqj.de/sneak/AutistMask/issues/324)), which singleton ([#324](https://git.eeqj.de/sneak/AutistMask/issues/324)), which

View File

@@ -197,6 +197,15 @@ function showFlash(msg, duration = 2000) {
}, duration); }, duration);
} }
// A stored token balance as a number, or null when there is no number in it.
// balances.js writes null for a holding whose scale nothing knows, and this
// keeps that null from becoming a zero one dereference later.
function unknownableAmount(balance) {
if (balance == null) return null;
const n = parseFloat(balance);
return Number.isFinite(n) ? n : null;
}
// One row of the balance list: symbol, quantity, fiat value. // One row of the balance list: symbol, quantity, fiat value.
// //
// `symbol` is the ERC-20's own symbol() as the block explorer reported it, // `symbol` is the ERC-20's own symbol() as the block explorer reported it,
@@ -210,15 +219,6 @@ function showFlash(msg, duration = 2000) {
// print for it and no fiat value to derive from one, and printing 0.0000 for // print for it and no fiat value to derive from one, and printing 0.0000 for
// a real holding is the failure this whole rule exists to prevent, so the row // a real holding is the failure this whole rule exists to prevent, so the row
// says so instead. // says so instead.
// A stored token balance as a number, or null when there is no number in it.
// balances.js writes null for a holding whose scale nothing knows, and this
// keeps that null from becoming a zero one dereference later.
function unknownableAmount(balance) {
if (balance == null) return null;
const n = parseFloat(balance);
return Number.isFinite(n) ? n : null;
}
function balanceLine(symbol, amount, price, tokenId) { function balanceLine(symbol, amount, price, tokenId) {
const qty = amount === null ? "quantity unknown" : amount.toFixed(4); const qty = amount === null ? "quantity unknown" : amount.toFixed(4);
const usd = const usd =

View File

@@ -255,12 +255,31 @@ function init(_ctx) {
// inside estimateGas(), which the confirmation screen reports as // inside estimateGas(), which the confirmation screen reports as
// an unestimable fee. Unsendable, over a scale that was never in // an unestimable fee. Unsendable, over a scale that was never in
// doubt (https://git.eeqj.de/sneak/AutistMask/issues/349). // doubt (https://git.eeqj.de/sneak/AutistMask/issues/349).
// Still null when nothing knows: no fallback, and the unknown // Still null when nothing knows: no fallback.
// path below is then the real one. //
// Resolved WITH `wallets`, which balances.js does not pass: that
// adds explorerDecimals()'s cross-address check, so a contract two
// addresses report different scales for answers null rather than
// picking one. That check has to apply here, because this value
// encodes a transfer; balances.js is formatting one explorer row
// at fetch time and cannot consult a state it is in the middle of
// replacing.
tokenDecimals = resolveTokenDecimals(token, { tokenDecimals = resolveTokenDecimals(token, {
trackedTokens: state.trackedTokens, trackedTokens: state.trackedTokens,
wallets: state.wallets, wallets: state.wallets,
}); });
// The two resolutions can therefore differ, and where they do, the
// stored `balance` is a quantity computed at a scale this screen
// has just declined to stand behind. Stating it would leave
// validateTransfer() checking the amount against a number the
// wallet does not vouch for, and — since the unknown-balance path
// is gated on the balance, not on the scale — would leave the
// fee-estimate failure as the only thing on the confirmation
// screen, which says nothing about decimals. Unknown scale means
// unknown balance. Only a stored quantity is withdrawn: the "0"
// for a token that has no row at all is an absence of holdings,
// which is true at every scale.
if (tb && tokenDecimals === null) tokenBalance = null;
} }
ctx.showConfirmTx({ ctx.showConfirmTx({

View File

@@ -127,6 +127,7 @@ const confirmTx = require("../src/popup/views/confirmTx");
const { TOKEN_BY_ADDRESS } = require("../src/shared/tokenList"); const { TOKEN_BY_ADDRESS } = require("../src/shared/tokenList");
const HOLDER = "0x" + "a".repeat(40); const HOLDER = "0x" + "a".repeat(40);
const SECOND_HOLDER = "0x" + "b".repeat(40);
const RECIPIENT = "0xC0FfEE0000000000000000000000000000c0fFEe"; const RECIPIENT = "0xC0FfEE0000000000000000000000000000c0fFEe";
const BLOCKSCOUT = "https://blockscout.example/api/v2"; const BLOCKSCOUT = "https://blockscout.example/api/v2";
// Bundled, 18 decimals. The wallet knows this token's scale without asking // Bundled, 18 decimals. The wallet knows this token's scale without asking
@@ -176,6 +177,38 @@ async function fetchOnto(items) {
return balances; return balances;
} }
// The same, for two addresses of one wallet holding the same contract. Sending
// is from the first. Two addresses is what it takes to reach
// explorerDecimals()'s disagreement check, which is only reachable across rows.
async function fetchOntoBoth(itemsA, itemsB) {
debugFetch.mockImplementation(async () => ({
ok: true,
status: 200,
statusText: "OK",
json: async () => itemsA,
}));
const a = await fetchTokenBalances(HOLDER, BLOCKSCOUT, []);
debugFetch.mockImplementation(async () => ({
ok: true,
status: 200,
statusText: "OK",
json: async () => itemsB,
}));
const b = await fetchTokenBalances(SECOND_HOLDER, BLOCKSCOUT, []);
state.wallets = [
{
name: "Wallet 1",
addresses: [
{ address: HOLDER, balance: "1.0", tokenBalances: a },
{ address: SECOND_HOLDER, balance: "1.0", tokenBalances: b },
],
},
];
state.selectedWallet = 0;
state.selectedAddress = 0;
return { a, b };
}
function el(id) { function el(id) {
return global.document.getElementById(id); return global.document.getElementById(id);
} }
@@ -283,6 +316,76 @@ describe("the Send screen resolves the scale rather than reading the stored one"
}); });
}); });
// balances.js resolves the display scale WITHOUT `wallets`, so its explorer leg
// is the row it is formatting. send.js resolves WITH `wallets`, so its explorer
// leg is explorerDecimals(), which answers null when two addresses report
// different scales for one contract — the check that must apply before a scale
// encodes a transfer. The two therefore disagree exactly here, and a stored
// balance formatted at a scale the Send screen just refused is not a balance it
// may state: it would leave validateTransfer() satisfied, the unknown-balance
// sentence unfired, and the fee-estimate failure as the only thing on screen.
describe("a scale the explorer's own rows disagree about", () => {
// 5000000 units at the "6" address A reports, 5e18 at the "18" address B
// reports: both format to "5.0", so the disagreement is in the scale alone
// and not in the quantity.
function novel(decimals, value) {
return row(
{
address_hash: NOVEL,
symbol: "NOVEL",
name: "Novel Token",
decimals,
},
value,
);
}
test("is stored per row, because storage holds the explorer's own answer", async () => {
const { a, b } = await fetchOntoBoth(
[novel("6", 5000000n)],
[novel("18", FIVE_WETH)],
);
expect(a[0].decimals).toBe(6);
expect(a[0].balance).toBe("5.0");
expect(b[0].decimals).toBe(18);
});
test("resolves to null on the Send screen, and takes the balance with it", async () => {
await fetchOntoBoth([novel("6", 5000000n)], [novel("18", FIVE_WETH)]);
const txInfo = await reviewSend(NOVEL, "1.5");
expect(txInfo.tokenDecimals).toBeNull();
// The regression this closes: null scale alongside a non-null balance.
expect(txInfo.tokenBalance).toBeNull();
});
test("so the user is told the balance is unknown, not that the fee failed", async () => {
await fetchOntoBoth([novel("6", 5000000n)], [novel("18", FIVE_WETH)]);
const txInfo = await reviewSend(NOVEL, "1.5");
confirmTx.show(txInfo);
await settle();
// Before the fix: "5.0 NOVEL", an empty confirm-errors, and
// confirm-fee-unknown-error — "the network fee could not be
// estimated... please go back and try again" — as the only explanation
// for a screen that can never proceed.
expect(errors()).not.toBe("");
expect(errors()).toContain("This token's balance is unknown");
expect(text("confirm-balance")).toBe("unknown (NOVEL)");
expect(sendDisabled()).toBe(true);
});
test("while agreeing rows leave the scale usable", async () => {
await fetchOntoBoth([novel("6", 5000000n)], [novel("6", 5000000n)]);
const txInfo = await reviewSend(NOVEL, "1.5");
expect(txInfo.tokenDecimals).toBe(6);
expect(txInfo.tokenBalance).toBe("5.0");
confirmTx.show(txInfo);
await settle();
expect(text("confirm-balance")).toBe("5.0 NOVEL");
expect(errors()).toBe("");
expect(sendDisabled()).toBe(false);
});
});
describe("the confirmation screen tells an unknown balance from a zero one", () => { describe("the confirmation screen tells an unknown balance from a zero one", () => {
function txInfo(tokenBalance) { function txInfo(tokenBalance) {
return { return {