From 2c5364f7bf11424ac79f63aefe367ea8f54813c6 Mon Sep 17 00:00:00 2001 From: sneak Date: Tue, 10 Nov 2026 20:18:56 +0000 Subject: [PATCH] fix: every screen cuts amounts the same way (closes #757) The balance lists and Home's ETH line rounded with toFixed(4), so 0.99996 ETH read 1.0000 there and 0.9999 on Send, and 0.00004 USDC read 0.0000. They now show the stored decimal string through truncateAmountNeverZero(), as Send and the approval screens do. History rows take the same floor. truncateAmount() is no longer exported. balanceLine() now takes the stored balance string instead of a parsed number. Model: opus-5-5 --- README.md | 21 ++++---- src/popup/views/addressToken.js | 11 +++-- src/popup/views/helpers.js | 23 ++++----- src/popup/views/home.js | 4 +- src/shared/amountDisplay.js | 29 ++++------- src/shared/transactions.js | 11 ++--- tests/approvalDisplayFloor.test.js | 12 +---- tests/balanceLineEscaping.test.js | 10 ++-- tests/balanceTruncation.test.js | 78 ++++++++++++++++++++++++++++++ tests/deleteAddress.test.js | 15 +++--- tests/transactions.test.js | 25 ++++++++++ tests/unknownScaleDisplay.test.js | 4 +- 12 files changed, 164 insertions(+), 79 deletions(-) create mode 100644 tests/balanceTruncation.test.js diff --git a/README.md b/README.md index 6bb4aa9..3947868 100644 --- a/README.md +++ b/README.md @@ -1187,20 +1187,17 @@ These helpers write one kind of value on every screen that shows it: times; - `renderAddressHtml()` in the same file: most addresses. -Most screens show token and ETH amounts with four decimal places. History rows -cut amounts to four decimals and never round up (`truncateAmount()` in -`src/shared/amountDisplay.js`). The balance lists and Home's ETH balance round -to four places. The screens that state what is being authorized cut as the -history does, and extend an amount below 0.0001 to its first nonzero digit -(`truncateAmountNeverZero()`). So a nonzero amount never reads as zero there. -Those screens are: +Every screen shows token and ETH amounts the same way, through +`truncateAmountNeverZero()` in `src/shared/amountDisplay.js`: -- TxApproval; -- SignApproval's Permit warning; -- ConfirmTx, and Send's current balance; -- WaitTx, SuccessTx and ErrorTx. +- an amount is cut to four decimal places, never rounded, so 0.99996 reads + `0.9999`; +- an amount below 0.0001 is shown to its first nonzero digit, so 0.00004 reads + `0.00004`. -TransactionDetail shows the exact amount and its base units. +So an amount never reads as more than it is, and a nonzero amount never reads as +zero. TransactionDetail is the one exception: it shows the exact amount and its +base units. An amount to send is valid when it is a number above zero, written as digits with at most one decimal point. Nothing but zeros may follow the token's last diff --git a/src/popup/views/addressToken.js b/src/popup/views/addressToken.js index ec8e3d5..9bfec5e 100644 --- a/src/popup/views/addressToken.js +++ b/src/popup/views/addressToken.js @@ -159,10 +159,10 @@ function renderBalance() { const addr = state.wallets[state.selectedWallet].addresses[state.selectedAddress]; const tokenId = state.selectedToken; - let amount, price; + let balance, price; if (tokenId === "ETH") { // null when no refresh has read the balance. - amount = unknownableAmount(addr.balance); + balance = addr.balance; price = getPrice("ETH"); } else { const id = tokenId.toLowerCase(); @@ -175,12 +175,13 @@ function renderBalance() { // null when the scale is unknown, or when no refresh has read the // token's balance: no quantity to show, and none to price. // balanceLine() states that rather than printing 0.0000. - if (tb) amount = unknownableAmount(tb.balance); - else amount = tracked && tracked.balanceNotFetched ? null : 0; + if (tb) balance = tb.balance; + else balance = tracked && tracked.balanceNotFetched ? null : "0"; price = getPrice(tokenId); } // USD total for this token only + const amount = unknownableAmount(balance); const usdVal = price && amount !== null ? amount * price : null; const usdStr = formatUsd(usdVal); $("address-token-usd-total").innerHTML = escapeHtml(usdStr) || " "; @@ -188,7 +189,7 @@ function renderBalance() { // Single token balance line (no tokenId — not clickable here) $("address-token-balance").innerHTML = balanceLine( currentSymbol, - amount, + balance, price, ); } diff --git a/src/popup/views/helpers.js b/src/popup/views/helpers.js index ceb40db..b12dd16 100644 --- a/src/popup/views/helpers.js +++ b/src/popup/views/helpers.js @@ -394,13 +394,16 @@ function formatFee(wei, symbol, chainId) { // the row that issue #307 was reported against: every screen that lists a // holding renders through here. // -// `amount` is null for a holding whose scale nothing knows +// `balance` is the stored balance, a decimal string. It is null for a holding +// whose scale nothing knows // (https://git.eeqj.de/sneak/AutistMask/issues/349), and for a balance no // refresh has read. There is no quantity to 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 says so instead. -function balanceLine(symbol, amount, price, tokenId) { - const qty = amount === null ? "quantity unknown" : amount.toFixed(4); +function balanceLine(symbol, balance, price, tokenId) { + const amount = unknownableAmount(balance); + const qty = + amount === null ? "quantity unknown" : truncateAmountNeverZero(balance); const usd = price && amount !== null ? escapeHtml(formatUsd(amount * price)) || " " @@ -415,7 +418,7 @@ function balanceLine(symbol, amount, price, tokenId) { `
` + `` + `${escapeHtml(displaySymbol(symbol))}` + - `${qty}` + + `${escapeHtml(qty)}` + `` + `${usd}` + `
` @@ -441,7 +444,7 @@ function markTrackedTokensNotFetched() { function balanceLinesForAddress(addr, trackedTokens, showZero) { let html = balanceLine( nativeCurrency(), - unknownableAmount(addr.balance), + addr.balance, getPrice("ETH"), "ETH", ); @@ -459,7 +462,7 @@ function balanceLinesForAddress(addr, trackedTokens, showZero) { if (bal === 0 && !showZero) continue; html += balanceLine( t.symbol, - bal, + t.balance, getPrice(t.address), t.address.toLowerCase(), ); @@ -477,7 +480,7 @@ function balanceLinesForAddress(addr, trackedTokens, showZero) { } html += balanceLine( t.symbol, - t.balanceNotFetched ? null : 0, + t.balanceNotFetched ? null : "0", getPrice(t.address), t.address.toLowerCase(), ); @@ -488,10 +491,8 @@ function balanceLinesForAddress(addr, trackedTokens, showZero) { // Whether an address holds anything at all: ETH or any ERC-20 the wallet // knows about, except a token holding below 0.000001, which the balance list -// under the remove-address warning leaves out too. Deliberately unrounded — -// the rendered lines round to four decimals, so a dust balance displays as -// 0.0000 while still being real money at a real address. Callers that warn -// about holdings must ask this, not the rendered figure. +// under the remove-address warning leaves out too. Callers that warn about +// holdings must ask this, not the rendered lines. function addressHoldsFunds(addr) { if (!addr) return false; // A null or absent ETH balance reads as unknown, so the address may hold diff --git a/src/popup/views/home.js b/src/popup/views/home.js index e9cc2b2..2214eb4 100644 --- a/src/popup/views/home.js +++ b/src/popup/views/home.js @@ -31,6 +31,7 @@ const { resetSendValidation, } = require("./send"); const { deriveAddressFromXpub } = require("../../shared/wallet"); +const { truncateAmountNeverZero } = require("../../shared/amountDisplay"); const { canRemoveAddress } = require("../../shared/walletDelete"); const { walletDefect, @@ -81,7 +82,8 @@ function renderTotalValue() { if (ethBal === null) { el.textContent = "unknown (" + nativeCurrency() + ")"; } else { - const ethStr = ethBal.toFixed(4) + " " + nativeCurrency(); + const ethStr = + truncateAmountNeverZero(addr.balance) + " " + nativeCurrency(); const ethUsd = ethPrice ? " (" + formatUsd(ethBal * ethPrice) + ")" : ""; diff --git a/src/shared/amountDisplay.js b/src/shared/amountDisplay.js index 5232574..9ebebb5 100644 --- a/src/shared/amountDisplay.js +++ b/src/shared/amountDisplay.js @@ -1,15 +1,6 @@ -// The 4-decimal amount rule from README.md's Amounts section, and -// the one exception to it, in one place. Three call sites had grown their own -// copy of the truncation — the history and balance lists -// (`src/shared/transactions.js`), the approval screen's ERC-20 amount line -// (`src/popup/views/approval.js`) and its Uniswap swap detail lines -// (`src/shared/uniswap.js`) — and a fix applied to one of them left the other -// two showing a different number for the same value. -// -// The two truncation functions below are the two policies, not two -// implementations of one: summary lists truncate, and the screens that state -// what is being authorized truncate with a floor. Keeping them adjacent is the -// point, so a change to the rule cannot reach one screen and miss another. +// The amount rule from README.md's Amounts section. Every screen that shows a +// token or ETH amount, except TransactionDetail, shows it through +// truncateAmountNeverZero(), so one value reads the same on every screen. // Truncate to exactly four decimal places. Truncation, never rounding: an // amount must never be displayed as larger than it is, so 0.99999 stays @@ -20,12 +11,13 @@ function truncateAmount(val) { return parts[0] + "." + (parts[1] + "0000").slice(0, 4); } -// The same rule, plus the invariant the approval and confirmation screens -// hold: a nonzero amount never renders as zero. Truncating to four decimals -// does exactly that to an amount below 0.0001 — one base unit of an 18-decimal -// token, 500 of an 8-decimal one — and a real transfer or allowance then reads -// as "nothing is being moved" on the screen whose whole job is to say what is -// being authorized. +// Four decimal places, truncated, and a nonzero amount never renders as zero. +// Truncating alone would show an amount below 0.0001 as 0.0000, and a real +// holding, transfer or allowance would then read as nothing. +// +// `val` is the exact decimal string: a stored balance, or what formatEther() +// or formatUnits() returns. Never a JavaScript number, whose own rounding +// would happen before the truncation. // // When the truncated string carries no significant digit and the value does, // the amount is extended to its first significant digit instead. It stays in @@ -54,7 +46,6 @@ function isBelowOneMillionth(balance) { } module.exports = { - truncateAmount, truncateAmountNeverZero, isBelowOneMillionth, }; diff --git a/src/shared/transactions.js b/src/shared/transactions.js index 522458c..ff341c9 100644 --- a/src/shared/transactions.js +++ b/src/shared/transactions.js @@ -16,10 +16,7 @@ const { nativeCurrencyByChainId } = require("./networks"); // a scale is either reported or it is unknown, and "unknown" must mean the // same thing here as it does on the screens that refuse to format one. const { toDecimals } = require("./transferAmount"); -// The plain 4-decimal rule. The history and balance lists deliberately keep -// truncation without the approval screens' nonzero floor: the transaction -// detail view is the authoritative record and already shows exact precision. -const { truncateAmount: formatTxValue } = require("./amountDisplay"); +const { truncateAmountNeverZero } = require("./amountDisplay"); const { routerRelease } = require("./uniswap"); const { storedSettingIsOn } = require("./storedSettings"); @@ -40,7 +37,7 @@ function parseTx(tx, addrLower, chainId) { // For contract calls, produce a meaningful label instead of "0.0000 ETH" let symbol = nativeCurrencyByChainId(chainId); - let value = formatTxValue(formatEther(rawWei)); + let value = truncateAmountNeverZero(formatEther(rawWei)); let exactValue = formatEther(rawWei); let rawAmount = rawWei; let rawUnit = "wei"; @@ -122,7 +119,9 @@ function parseTokenTransfer(tt, addrLower, chainId) { // The exact figure is not lost — it is the base-unit line below, which is // the one number that needs no scale to be true. const formatted = - decimals === null ? "" : formatTxValue(formatUnits(rawVal, decimals)); + decimals === null + ? "" + : truncateAmountNeverZero(formatUnits(rawVal, decimals)); const exact = decimals === null ? "" : formatUnits(rawVal, decimals); return { hash: tt.transaction_hash, diff --git a/tests/approvalDisplayFloor.test.js b/tests/approvalDisplayFloor.test.js index 0fb67f2..50398c6 100644 --- a/tests/approvalDisplayFloor.test.js +++ b/tests/approvalDisplayFloor.test.js @@ -25,10 +25,7 @@ const { ERC20_ABI } = require("../src/shared/constants"); const { state } = require("../src/shared/state"); const { decodeCalldata } = require("../src/popup/views/approval"); const uniswap = require("../src/shared/uniswap"); -const { - truncateAmount, - truncateAmountNeverZero, -} = require("../src/shared/amountDisplay"); +const { truncateAmountNeverZero } = require("../src/shared/amountDisplay"); const iface = new Interface(ERC20_ABI); @@ -184,12 +181,7 @@ describe("the four-decimal rule is otherwise unchanged", () => { expect(amount(transferData(0n), DAI).rawValue).toBe("0.0000"); }); - // The three truncators now share one module. The floor is a policy of the - // approval and confirmation screens only: the history and balance lists - // keep plain truncation, because the transaction detail view is the - // authoritative record and already shows exact precision. - test("the list rule stays unfloored", () => { - expect(truncateAmount("0.000000000000000001")).toBe("0.0000"); + test("one base unit of an 18-decimal token reads as itself", () => { expect(truncateAmountNeverZero("0.000000000000000001")).toBe( "0.000000000000000001", ); diff --git a/tests/balanceLineEscaping.test.js b/tests/balanceLineEscaping.test.js index dc3268b..09467a6 100644 --- a/tests/balanceLineEscaping.test.js +++ b/tests/balanceLineEscaping.test.js @@ -41,13 +41,13 @@ describe("balanceLine", () => { // this payload before its id attribute, so an assertion about the // rest of the payload would pass on the cap and say nothing about // the escape. - const html = balanceLine(HOSTILE_SYMBOL, 1, null, null); + const html = balanceLine(HOSTILE_SYMBOL, "1", null, null); expect(html).not.toContain(" { - const html = balanceLine("A".repeat(4096), 1, null, null); + const html = balanceLine("A".repeat(4096), "1", null, null); expect(html).toContain("A".repeat(MAX_SYMBOL_LENGTH - 1) + "…"); expect(html).not.toContain("A".repeat(MAX_SYMBOL_LENGTH + 1)); }); @@ -55,13 +55,13 @@ describe("balanceLine", () => { // The token id lands inside data-token="...", so a quote in it is a // way out of the attribute and into a new one. test("keeps a quote-bearing token id inside its attribute", () => { - const html = balanceLine("TKN", 1, null, '" onclick="alert(1)'); + const html = balanceLine("TKN", "1", null, '" onclick="alert(1)'); expect(html).not.toContain('onclick="'); expect(html).toContain('data-token="" onclick="alert(1)"'); }); test("renders an ordinary holding unchanged", () => { - const html = balanceLine("USDC", 1.5, null, "0xabc"); + const html = balanceLine("USDC", "1.5", null, "0xabc"); expect(html).toContain("USDC"); expect(html).toContain("1.5000"); expect(html).toContain('data-token="0xabc"'); @@ -69,7 +69,7 @@ describe("balanceLine", () => { // formatUsd() writes a value under a cent as "< $0.01". test("escapes the USD value along with the symbol", () => { - const html = balanceLine("USDC", 0.001, 1, null); + const html = balanceLine("USDC", "0.001", 1, null); expect(html).toContain("< $0.01"); expect(html).not.toContain("< $0.01"); }); diff --git a/tests/balanceTruncation.test.js b/tests/balanceTruncation.test.js new file mode 100644 index 0000000..277ffe3 --- /dev/null +++ b/tests/balanceTruncation.test.js @@ -0,0 +1,78 @@ +// The balance lists and Home's ETH line show a balance as Send does (README.md, +// Amounts): cut to four decimal places, never rounded up, and a nonzero amount +// never shown as zero +// (https://git.eeqj.de/sneak/AutistMask/issues/757). +// +// Driven through the real popup over a stored profile on Ethereum Mainnet. + +const { + ADDRESS, + bootPopup, + cleanupPopup, + unversionedValidProfile, +} = require("./support/popupBoot"); +const { balanceLine } = require("../src/popup/views/helpers"); + +const USDC = "0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48"; + +afterEach(() => { + cleanupPopup(); +}); + +// The quantity cell of a balance row. +function quantity(row) { + return row.match(/([^<]*)<\/span><\/span>/)[1]; +} + +// The popup on Home over one address holding `eth` ETH and `usdc` USDC. +function bootHolding(eth, usdc) { + const profile = unversionedValidProfile({ + trackedTokens: [{ address: USDC, symbol: "USDC", decimals: 6 }], + }); + profile.wallets[0].addresses[0] = { + address: ADDRESS, + balance: eth, + tokenBalances: [ + { address: USDC, symbol: "USDC", decimals: 6, balance: usdc }, + ], + }; + return bootPopup(profile); +} + +// The balance row for `token` in Home's wallet list. +function homeRow(page, token) { + return page + .node("wallet-list") + .innerHTML.split("
row.includes('data-token="' + token + '"')); +} + +describe("balanceLine", () => { + test("cuts 0.99996 to 0.9999, never rounding it up to 1.0000", () => { + expect(quantity(balanceLine("ETH", "0.99996", null, "ETH"))).toBe( + "0.9999", + ); + }); + + test("shows 0.00004 as 0.00004, not 0.0000", () => { + expect(quantity(balanceLine("USDC", "0.00004", null, USDC))).toBe( + "0.00004", + ); + }); +}); + +describe("Home", () => { + test("the ETH line and the balance list read 0.99996 ETH as 0.9999", async () => { + const page = await bootHolding("0.99996", "0.00004"); + + expect(page.visibleViews()).toEqual(["main"]); + expect(page.text("total-value")).toBe("0.9999 ETH"); + expect(quantity(homeRow(page, "ETH"))).toBe("0.9999"); + }); + + test("the balance list reads 0.00004 USDC as 0.00004", async () => { + const page = await bootHolding("0.99996", "0.00004"); + + expect(quantity(homeRow(page, USDC.toLowerCase()))).toBe("0.00004"); + }); +}); diff --git a/tests/deleteAddress.test.js b/tests/deleteAddress.test.js index 42b157a..a12675b 100644 --- a/tests/deleteAddress.test.js +++ b/tests/deleteAddress.test.js @@ -4,8 +4,8 @@ // the copy is the substance and is tested as such. Two things it must not // get wrong: what it takes to get the address back — the app refuses both // obvious routes — and what counts as holding something, which is any -// ERC-20 as well as ETH, at any size, including a balance that rounds to -// zero at the four decimals the balance lines render. The DOM behaviour +// ERC-20 as well as ETH, at any size, including a balance below the four +// decimals the balance lines render. The DOM behaviour // around them is driven against the real popup by tests/e2e/run.js. // helpers.js pulls in state.js, which reads chrome.storage.local at load. @@ -102,7 +102,7 @@ describe("whether an address holds anything", () => { expect(addressHoldsFunds(TOKEN_ONLY)).toBe(true); }); - // 0.00001 ETH renders as "0.0000" at four decimals. It is still money. + // 0.00001 ETH is below four decimals. It is still money. test("an ETH balance below the displayed precision counts", () => { expect(addressHoldsFunds(DUST)).toBe(true); }); @@ -141,13 +141,12 @@ describe("the balance warning on the removal confirmation", () => { expect(html).toContain("2500.0000"); }); - // The rendered line says 0.0000 for this address — that is the display - // format, shared with Home and AddressDetail — and the warning is shown - // all the same, because the balance is not zero. - test("an ETH balance that renders as 0.0000 is warned about", () => { + // The rendered line shows the balance to its first nonzero digit, as Home + // and AddressDetail do (README.md, Amounts). + test("an ETH balance below 0.0001 is warned about, and reads as itself", () => { const html = balanceWarningHtml(DUST); expect(html).toContain("This address holds a balance."); - expect(html).toContain("0.0000"); + expect(html).toContain("0.00001"); }); test("an ETH balance no refresh has read is warned about, and reads as unknown", () => { diff --git a/tests/transactions.test.js b/tests/transactions.test.js index 631aa70..4f550c3 100644 --- a/tests/transactions.test.js +++ b/tests/transactions.test.js @@ -1361,6 +1361,31 @@ describe("fetchRecentTransactions merge and dedup", () => { expect(txs[0].holders).toBe(3500000); }); + // README.md, Amounts: a nonzero amount never reads as zero. + test("a token transfer below 0.0001 shows its first nonzero digit", async () => { + respondWith( + [], + [ + { + transaction_hash: "0x" + "4".repeat(64), + block_number: 21000040, + timestamp: TS, + from: { hash: ORDINARY_PEER }, + to: { hash: VICTIM }, + total: { value: "40", decimals: "6" }, + token: { + symbol: "USDC", + address_hash: USDC_CONTRACT, + holders_count: "3500000", + }, + }, + ], + ); + + const txs = await fetchRecentTransactions(VICTIM, BLOCKSCOUT, "0x1"); + expect(txs[0].value).toBe("0.00004"); + }); + test("two transfers of the same token in one transaction collapse to one entry", async () => { const hash = "0x" + "3".repeat(64); const leg = (value) => ({ diff --git a/tests/unknownScaleDisplay.test.js b/tests/unknownScaleDisplay.test.js index e83be6e..e054f0a 100644 --- a/tests/unknownScaleDisplay.test.js +++ b/tests/unknownScaleDisplay.test.js @@ -96,7 +96,7 @@ afterEach(() => { describe("balanceLine", () => { test("an unknown quantity and a zero one render differently", () => { const unknown = balanceLine("NOVEL", null, null, NOVEL); - const zero = balanceLine("NOVEL", 0, null, NOVEL); + const zero = balanceLine("NOVEL", "0", null, NOVEL); expect(unknown).not.toBe(zero); expect(quantities(unknown)).toEqual(["NOVEL", "quantity unknown"]); expect(quantities(zero)).toEqual(["NOVEL", "0.0000"]); @@ -105,7 +105,7 @@ describe("balanceLine", () => { test("an unknown quantity produces no fiat figure, a zero one does", () => { prices[NOVEL] = 3; const unknown = balanceLine("NOVEL", null, 3, NOVEL); - const zero = balanceLine("NOVEL", 0, 3, NOVEL); + const zero = balanceLine("NOVEL", "0", 3, NOVEL); // A price times an unknown quantity is not $0.00: that is the same // claim of "nothing here" the quantity cell just refused to make. expect(unknown).toContain(