fix: a failed lookup is not a zero — builders, send form, swap dialog - #415
Merged
Conversation
#414 swept for the #411 bug and stopped at the EVM builders. Two more sites live outside them, and the UTXO one is the worst instance found so far. ## utxo.ts — a dropped xpub silently shrinks the wallet buildUtxoTx aggregates across accounts with Promise.allSettled and warns on a rejected ListUnspent ("Finding 5: tolerate individual xpub failures"). The tolerance is right; what follows it is not. `utxos` is then a strict subset, and three statements assert things about the whole balance anyway: - MAX built a valid tx spending only the visible accounts and called it a sweep. Not a wrong message — a wrong AMOUNT, signed by the user, no error shown. This is the one that can lose money. - "Insufficient funds: have 0.005, need 0.008" on a wallet holding 0.01, when 1 of 2 accounts failed. Verbatim the #411 report, on the UTXO path. - An all-fail landed on "No confirmed UTXOs found ... the transaction may still be confirming — please wait and try again": a fabricated explanation for a server outage, sending the user to wait for a tx that isn't pending. Now a failed-lookup count gates all three. MAX refuses outright, and the two messages name the gap instead of quoting a subtotal as if it were the total. estimateUtxoFee had the same partial set behind its fee/net quote; it already returns null on error, so a partial set returns null too. ## cosmos.ts — the ?? '0' that #414 fixed for EVM Three MAX reads used `(balResp?.data?.balances || [])[0]?.balance ?? '0'`. A missing balance became 0 - fee, clamped to 0, then "Amount must be greater than zero" — user hits MAX on a funded account and is told their amount is zero. Fails closed, wrong cause, server fault hidden. All three now go through readCosmosBalance(), mirroring readPioneerBalance(): throw when the field is absent, pass a real numeric 0 through as the verified empty account it is. Checked and left alone: ton.ts `data.result.balance || '0'` sits behind an `ok` check and the caller only reads `.initialized`, so no funds decision depends on it. ## Dashboard "All Chains" total Follow-up to #410, which taught the rows to say "—" but left the total summing those chains as 0 and printing a confident $X above them. It can't spin like a pending row — a chain the backend keeps failing would spin forever — so it now renders "≥ $X" with a tooltip when any row is unknown. ## Tests __tests__/failed-fetch-not-zero.test.ts — 13 cases. Reverting utxo.ts alone fails exactly 4 (MAX, subtotal message, outage message, fee estimate) while the reachable-path controls stay green, so they pin the behaviour and not the wording. Also wires src/bun/txbuilder/cosmos.test.ts into make test-unit. It was green and unreferenced — 10 assertions that had never guarded a release, and the regression net for the cosmos change above. It is script-style (own runner + process.exit), so it gets its own line rather than joining the `bun test` list, where the exit would cut the run short. make test-unit 456 pass, 0 fail across 31 files, +35 btc-backend, +10 cosmos tsc --noEmit 627 errors vs baseline 629 — none added, none in the new code
…the fix
Sweeping the layer above the builders turned up the send form making the same
claims from the same placeholder zero — plus one path where the form's value
walked straight past the guard added in the previous commit.
## The bypass (this is the one that mattered)
buildCosmosTx's token MAX read:
params.tokenBalance ?? readCosmosBalance(await pioneer.GetPortfolioBalances(...))
`??` only falls through on null/undefined. The send form passes
`tokenBalance: token.balance`, and for a chain whose fetch failed that value is
the string '0' — truthy, so it won. readCosmosBalance never ran, and the
previous commit's guard was defeated by the single input most likely to be
wrong. buildEvmTx (evm.ts:299) and the Solana path (index.ts:196) both gate on
`parseFloat(...) > 0` and re-fetch otherwise; cosmos now matches them.
## The claims
A `degraded` entry carries balance '0'. SendForm read it as a figure:
- The "Low BTC for Gas" banner gates on `!(nativeBal > 0)`, so a chain Pioneer
failed produced "You need ETH to pay network fees. Deposit ETH..." on an
account holding plenty. The ponytail note above that line was already
fighting a different false positive on the same banner.
- The "Available" readout and the small balance row printed a confident 0,
and the USD conversion multiplied it into a confident $0.00.
All three now render "—" with a tooltip. isBalanceUnverified() lives beside
balanceDisplayState() so the 'degraded' rule stays in one module — the send
form has no global load state, so it needs the predicate, not the three-state
machine.
Deliberately unchanged: the form still lets the send proceed. The builder
re-fetches and either succeeds or throws a specific error, which is the right
gate; blocking in the UI would strand a user whenever one chain was degraded.
`exceedsBalance` already required `balanceNum > 0`, so it never fired on a
placeholder zero.
Noted, not touched: SendForm sends `nativeBalance` in the buildTx payload and
no builder reads it — `params.nativeBalance` has no consumer anywhere in
src/bun. Dead field, pre-existing, left alone.
## Tests
Four more cases in the same file (17 total). Reverting only the cosmos change
fails exactly the two frontend-'0' cases while the rest stay green.
make test-unit 460 pass, 0 fail across 31 files, +35 btc-backend, +10 cosmos
tsc --noEmit 627, unchanged by this commit, 2 below the 629 baseline
All three confirmed against the source before fixing. Two are bypasses of the guards added in the first two commits; one is an inconsistency I introduced. ## 1. BTC MAX bypassed the completeness guard from upstream btc-accounts.ts filtered xpubs on `parseFloat(xp.balance) > 0` — the CACHED balance — before the builder ever saw them. A cached zero is not proof of an empty account; it is also exactly what a degraded chain looks like. So the degraded account was dropped, every surviving ListUnspent succeeded, `unreachableXpubs` stayed 0, and MAX swept a subset of the wallet believing it had swept all of it. The guard from commit 1 never saw the account it exists to catch. Same shape as the Cosmos `tokenBalance: '0'` bypass, one layer further up: a caller pre-filtering on an untrustworthy zero, so the callee's check has nothing left to catch. getFundedXpubs → getSpendableXpubs, filter dropped, three call sites updated. The name asserted a property it could not establish. Cost is one ListUnspent per genuinely empty xpub, which returns [] and adds nothing; the builder decides what is spendable, this method only says what exists. Behaviour change worth knowing: a wallet with an unfunded xpub whose lookup now ERRORS will block MAX where it previously proceeded on a subset. That is the point — but it does widen what can block a MAX. ## 2. isBalanceUnverified said a missing entry was verified It returned false for `undefined`, so SendForm's `balance?.balance || '0'` printed a confident zero. That contradicts balanceDisplayState, which never returns 'known' without an entry — my own predicate disagreed with the state machine it was written to mirror. Reachable since #410: the chain list is always visible, so Send opens for a chain that has no entry yet. A missing entry is now unverified. An entry with no syncState stays known — cached and legacy rows are real numbers, and blanking those would wipe every balance on cold start, the exact opposite of the bug being fixed. ## 3. SwapDialog kept the claims SendForm stopped making fromBalance ignored syncState, so a degraded '0' drove the displayed amount, the USD conversion, the MAX arithmetic, and the low-gas warning. Worse than SendForm's version: `sendAmount` fell back to `fromBalance || '0'`, so MAX on an unverifiable balance submitted a 0-amount swap. fromBalance and fromChainLowGas now yield null when the from-chain entry is degraded, MAX resolves to empty rather than '0', and the amount field carries a one-line explanation. Only a present-but-degraded entry counts — fromBalance already returns null when no entry exists, so that path needed no help. Preserved as the audit asked: isBalanceUnverified takes an optional assetCaip and honours confirmedAssetCaips, so an SPL token proven directly over RPC keeps its real figure while its parent chain is degraded — which is precisely what mergeTrustedBalanceSnapshot merges through. CAIP comparison reuses normalizeAssetCaip, so EVM contracts match case-insensitively and base58 token ids stay byte-exact. ## Tests 24 cases now (was 17). Reverting only the btc-accounts filter fails exactly the 2 new getSpendableXpubs cases; the earlier utxo.ts and cosmos proofs are untouched by this commit. make test-unit 467 pass, 0 fail across 31 files, +35 btc-backend, +10 cosmos tsc --noEmit 627, unchanged across all three commits, 2 below baseline 629
…or gas Two selector bugs from the re-audit. Both confirmed against the source first. ## 1. SwapDialog checked one source and read from another fromBalanceUnverified examined only the internal `balances` array — I wrote `!!cb && ...` to avoid destabilising the dialog, and that guard created the hole it was meant to close. fromBalance then fell through to the `balance` prop, so a degraded prop cleared a check that never looked at it: - confidence resolves as verified; - the prop's placeholder zero drives Available / USD / MAX; - an account-model balance gets pre-clamped and submitted with sendIsMax=false, past the builder's own MAX verification. Reachable whenever getCachedBalances returns a non-empty cache that lacks the selected chain, since the dialog then skips its live fetch. Both now resolve through selectBalanceEntry(balances, balance, chainId), and fromBalance's own tail reads that same entry instead of re-deriving the fallback. One object, judged and read. ## 2. A verified token vouched for the native gas balance isBalanceUnverified is asset-aware, and both screens passed the ACTIVE asset's caip — then reused that one boolean to gate the native low-gas check. For a degraded Solana chain with a directly-confirmed SPL token: the token is correctly verified, SOL is still a placeholder zero, the shared flag lets the gas verdict run, and "deposit SOL to send tokens" comes back. Exactly the false warning this PR set out to remove, re-entering through the exception that makes the token case correct. Split per screen: SendForm activeAssetUnverified (Available/USD) + nativeBalanceUnverified (gas) SwapDialog fromBalanceUnverified (Available/MAX) + fromNativeUnverified (gas) The native verdict uses the chain's native caip, so a confirmed token can never answer for the coin that pays the fee. ## Test coverage, honestly 30 cases. selectBalanceEntry is real shared code, so finding 1 is pinned: reverting it to the cache-only lookup fails exactly the one case. Finding 2's split is per-screen wiring. There is no React render harness in this repo and adding one is not this PR's job, so the tests pin the predicate contract the split depends on (token verified / native not, from one entry) while the wiring itself is verified by reading. The test file header says so rather than implying coverage it does not have. Also noted while verifying: fromBalance's EVM branch reads evmAddresses[].chainBalances, a third source with no syncState of its own. It is judged by the chain-level entry, which is right — but if `balances` has no entry for that chain at all, that branch can still return a figure. Out of scope here: that is the missing-entry case, not the degraded one. make test-unit 473 pass, 0 fail across 31 files, +35 btc-backend, +10 cosmos tsc --noEmit 627, unchanged across all four commits, 2 below baseline 629
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#414 swept for the #411 bug and stopped at the EVM builders. Three more sites live outside them. Two commits: the builders, then the layer above them — which turned out to contain a bypass of the first commit's own fix.
Commit 1 — the builders
utxo.ts — a dropped xpub silently shrinks the wallet
buildUtxoTxaggregates across accounts withPromise.allSettledand warns on a rejectedListUnspent("Finding 5: tolerate individual xpub failures"). The tolerance is right; what follows it is not.utxosis then a strict subset, and three statements describe the whole balance anyway:Insufficient funds: have 0.005, need 0.008on a wallet holding 0.01No confirmed UTXOs found ... may still be confirming — please waitThe MAX case is the one that can lose money.
estimateUtxoFeehad the same partial set behind its fee/net quote; it already returnsnullon error, so a partial set returnsnulltoo.cosmos.ts — the
?? '0'that #414 fixed for EVMThree MAX reads used
(balResp?.data?.balances || [])[0]?.balance ?? '0'. A missing balance became0 - fee, clamped to 0, then "Amount must be greater than zero" — user hits MAX on a funded account and is told their amount is zero. Fails closed, wrong cause, server fault hidden. All three now go throughreadCosmosBalance(), mirroringreadPioneerBalance(): throw when the field is absent, pass a real numeric0through as the verified empty account it is.Checked and left alone:
ton.ts'sdata.result.balance || '0'sits behind anokcheck and the caller only reads.initialized.Commit 2 — the send form, and the hole it opened
The bypass.
buildCosmosTx's token MAX readparams.tokenBalance ?? readCosmosBalance(...).??only falls through on null/undefined. SendForm passestokenBalance: token.balance, and for a degraded chain that value is the string'0'— truthy, so it won, and commit 1's guard never ran. Defeated by the single input most likely to be wrong.buildEvmTx(evm.ts:299) and the Solana path (index.ts:196) both gate onparseFloat(...) > 0; cosmos now matches.The claims. A
degradedentry carries balance'0', and SendForm read it as a figure:!(nativeBal > 0), so a chain Pioneer failed produced "You need ETH to pay network fees. Deposit ETH…" on an account holding plenty. Theponytail:note above that line was already fighting a different false positive on the same banner.0, and the USD conversion multiplied it into a confident$0.00.All three now render
—with a tooltip.isBalanceUnverified()sits besidebalanceDisplayState()so the'degraded'rule stays in one module — the send form has no global load state, so it needs the predicate, not the three-state machine.Deliberately unchanged: the form still lets the send proceed. The builder re-fetches and either succeeds or throws something specific, which is the right gate; blocking in the UI would strand a user whenever one chain was degraded.
Noted, not touched: SendForm sends
nativeBalancein thebuildTxpayload and no builder reads it —params.nativeBalancehas no consumer anywhere insrc/bun. Dead field, pre-existing.Commit 3 — the integration paths (post-audit)
An audit of the first two commits found two bypasses and one inconsistency. All three confirmed against the source before fixing.
1. BTC MAX bypassed the completeness guard from upstream.
btc-accounts.tsfiltered xpubs onparseFloat(xp.balance) > 0— the cached balance — before the builder saw them. A cached zero is not proof of an empty account; it is also exactly what a degraded chain looks like. The degraded account was dropped, every survivingListUnspentsucceeded,unreachableXpubsstayed 0, and MAX swept a subset believing it had swept the wallet. Commit 1's guard never saw the account it exists to catch — the same shape as the CosmostokenBalance: '0'bypass, a layer further up.getFundedXpubs→getSpendableXpubs, filter dropped, three call sites updated. The name asserted a property it could not establish. Cost is oneListUnspentper genuinely empty xpub, which returns[]. Behaviour change worth knowing: an unfunded xpub whose lookup now errors will block MAX where it previously proceeded on a subset. That is the point, but it widens what can block a MAX.2.
isBalanceUnverifiedsaid a missing entry was verified. It returnedfalseforundefined, so SendForm'sbalance?.balance || '0'printed a confident zero — contradictingbalanceDisplayState, which never returnsknownwithout an entry. My own predicate disagreed with the state machine it was written to mirror. Reachable since #410: the chain list is always visible, so Send opens for a chain with no entry. A missing entry is now unverified; an entry with nosyncStatestays known, since blanking cached and legacy rows would wipe every balance on cold start.3. SwapDialog kept the claims SendForm stopped making.
fromBalanceignoredsyncState, so a degraded'0'drove the displayed amount, the USD conversion, the MAX arithmetic, and the low-gas warning — andsendAmountfell back tofromBalance || '0', so MAX on an unverifiable balance submitted a 0-amount swap.fromBalanceandfromChainLowGasnow yieldnullfor a degraded from-chain, MAX resolves to empty, and the amount field carries a one-line explanation.The
confirmedAssetCaipsexception is preserved, as the audit asked:isBalanceUnverifiedtakes an optionalassetCaipand honoursconfirmedAssetCaips, so an SPL token proven directly over RPC keeps its real figure while its parent chain is degraded — precisely whatmergeTrustedBalanceSnapshotmerges through. Comparison reusesnormalizeAssetCaip, so EVM contracts match case-insensitively and base58 ids stay byte-exact.Commit 4 — two selector bugs (re-audit)
1. Checked one source, read from another.
fromBalanceUnverifiedexamined only the internalbalancesarray — the!!cb &&guard I added to avoid destabilising the dialog created the hole it was meant to close.fromBalancethen fell through to thebalanceprop, so a degraded prop cleared a check that never looked at it: confidence resolves verified, the prop's placeholder zero drives Available/USD/MAX, and an account-model balance gets pre-clamped and submitted withsendIsMax=false— past the builder's own MAX verification. Reachable whenevergetCachedBalancesreturns a non-empty cache lacking the selected chain, since the dialog then skips its live fetch.Both now resolve through
selectBalanceEntry(balances, balance, chainId), andfromBalance's tail reads that same entry instead of re-deriving the fallback. One object, judged and read.2. A verified token vouched for native gas. Both screens passed the active asset's caip to
isBalanceUnverified, then reused that one boolean to gate the native low-gas check. Degraded Solana + directly-confirmed SPL token ⇒ token correctly verified, SOL still a placeholder zero, shared flag lets the gas verdict run, and "deposit SOL to send tokens" returns — the exact false warning this PR removes, re-entering through the exception that makes the token case correct.activeAssetUnverifiednativeBalanceUnverifiedfromBalanceUnverifiedfromNativeUnverifiedThe native verdict uses the chain's native caip, so a confirmed token can never answer for the coin that pays the fee.
Coverage, stated plainly
selectBalanceEntryis real shared code, so finding 1 is pinned — reverting it to the cache-only lookup fails exactly one case. Finding 2's split is per-screen wiring; there is no React render harness in this repo and adding one is not this PR's job, so the tests pin the predicate contract the split depends on while the wiring itself is verified by reading. The test file header says so rather than implying coverage it does not have.Also noted while verifying:
fromBalance's EVM branch readsevmAddresses[].chainBalances, a third source with nosyncState. It is judged by the chain-level entry, which is right — but with no entry for that chain at all, that branch can still return a figure. Out of scope here: that is the missing-entry case, not the degraded one.Also: the "All Chains" total
Follow-up to #410, which taught the rows to say
—but left the total summing those chains as 0 and printing a confident$Xabove them. It can't spin like a pending row — a chain the backend keeps failing would spin forever — so it renders≥ $Xwith a tooltip when any row is unknown.Tests
__tests__/failed-fetch-not-zero.test.ts— 30 cases. They pin behaviour, not wording:utxo.tsfails exactly 4 (MAX, subtotal message, outage message, fee estimate)??fails exactly the 2 frontend-'0'casesgetSpendableXpubscasesselectBalanceEntryto a cache-only lookup fails exactly 1Also wires
src/bun/txbuilder/cosmos.test.tsintomake test-unit. It was green and unreferenced — 10 assertions that had never guarded a release, and the regression net for the cosmos changes here. It is script-style (own runner +process.exit), so it gets its own line rather than joining thebun testlist, where the exit would cut the run short.Not covered
Cold-start hardware validation, still outstanding from #410 and not made worse here.