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(