feat(breez): surface the Spark USDT token balance (ENG-473) (#704) - #712
Merged
Conversation
…an overstated gate comment (#704) ENG-473 was blocked on the SDK upgrade; 0.22.3 is on main, so getInfo() now carries tokenBalances and the read costs nothing extra — it rides the same round-trip as the sat balance and refreshes on the same cadence. Two decisions worth arguing with: 1. It is NOT WalletCurrency.Usdt. Spark USDT is a self-custodial token; WalletCurrency.Usdt is the custodial wallet on the Flash backend. Same name, different money. Kept out of that vocabulary so no `walletCurrency === "USDT"` lookup can ever pick up the wrong one. 2. The balance stays a bigint in MINOR units with decimals carried alongside — never pre-divided. A source that rounds before the spend path is exactly what caused the MAX overdraw in flash#480. Selection refuses to guess: any Spark issuer can label a token "USDT", so one ticker match resolves, two or more resolve to undefined rather than risking a spoofed issuer's balance being shown as the user's money. issuerPublicKey is carried so the selection can be pinned to the canonical issuer before any UI or spend path consumes this — that pubkey is not recorded anywhere in the repo yet, which is why it is documented rather than enforced. Not persisted: unlike breezBalance this is in-memory, so it is absent until the first getInfo() rather than stale on cold start. Persisting a bigint needs a persistent-state schema change of its own. Also #704: AppUpdateBoundary's docblock claimed the gate is pinned last so nothing paints over it. True only inside its own subtree — NotificationsProvider wraps the boundary and its CustomModal takes the default coverScreen, so it goes through the native host and paints above regardless of sibling order. Verified, not assumed. Comment now says what the code delivers; hoisting the gate is a structural change that deserves its own argument. 9 new tests. 92 suites / 906 tests green, tsc clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NEoz7nBtdtsHyuYG5wNPQV
…e from lying Review fixes on #712. The headline defect: nothing could read the field the PR added. All 19 Breez consumers go through useBreez(), and useBreez() annotated the context result with a hand-copied ContextProps that predated sparkUsdtWallet — so `const { sparkUsdtWallet } = useBreez()` was a TS2339, and CI stayed green only because no consumer tried. The value was provided and unreachable; "met at the context layer" was not true for anyone. BreezInterface is now exported and the hook returns it directly, so the duplication that caused this is gone rather than patched, and the next field added to the provider is reachable by default. A new spec asserts it, and it is the compile step that enforces it: reverting the hook reproduces the exact TS2339 on that spec's two reads. The safety story was inverted, too. "Refuses to guess" only protects a user who already holds the real USDT — airdrop one spoofed token to a user holding none and it is the sole match, returned as their money with no warning. So the guarantee moves out of prose and into the type: UnverifiedSparkUsdtWallet, with a required `issuerVerified: false`. Pinning CANONICAL_USDT_ISSUER_PUBKEY is now a compile-level prerequisite for any consumer, not a docblock request. Two more holes in a module whose entire job is holding invariants: - A non-Map tokenBalances returned undefined silently, indistinguishable from "holds no USDT". That is the precise drift the module defends against, and its production symptom was a permanently missing balance with nothing in the logs. It warns now; plain absent stays quiet. - `balance` was copied onto balanceMinor untyped-checked while tokenMetadata was guarded. Today the SDK lifts u128 via BigInt(), but the repo just moved 0.13.6 -> 0.22.3; a bump lowering it as a string leaves balanceMinor typed bigint without being one, and the first `balanceMinor / 10n ** BigInt(decimals)` throws. Missing beats lying. Also #704: the corrected comment prescribed a remedy that does not work. CustomModal goes through RCTModalHostView and paints above the whole inline root regardless of tree position, so hoisting AppUpdateBoundary above NotificationsProvider changes nothing — a follow-up PR would have shipped a no-op refactor. It now names the two fixes that would work (coverScreen={false} on the notification modal, or gating notifyModal on the gate). The new paragraph also no longer splits the First/Second enumeration it was dropped into. 5 new tests. 93 suites / 911 tests green, tsc clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NEoz7nBtdtsHyuYG5wNPQV
The bigint guard in selectUsdtBalance protected one of the two fields the module's own documented consumer expression uses. `decimals` was copied straight out of `tokenMetadata` with no check, under a doc comment saying "Never assume 6" — and nothing upstream vouches for it, since the ticker filter only reads `tokenMetadata.ticker`. The static shape is safe (Map<string, TokenBalance> assignability at BreezContext makes a renamed or retyped field a tsc error), so the only way through is the exact scenario the balance guard was added for: an FFI bridge that lowers a value the .d.ts says is a `number` as something else. An absent or null `decimals` got past the guard, and the first consumer running `balanceMinor / 10n ** BigInt(decimals)` threw "Cannot convert undefined to a BigInt". Same failure class, same module, one field over. Fold it into the existing guard as `!Number.isInteger(decimals) || decimals < 0` — negative passes BigInt() but makes `10n ** -2n` a RangeError, so it fails the same way — and name the failing field in the warning so the log distinguishes the two causes. Spec adds undefined/null/string/float/NaN/negative decimals alongside the existing string/number balance cases, plus a 0-decimals case so the guard cannot start rejecting a legitimate integer token. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NEoz7nBtdtsHyuYG5wNPQV
Review residuals on #712, and they compound into one real failure: `ticker` was the only one of the three FFI-lowered fields read with optional chaining rather than a typeof check. `?.` short-circuits on null/undefined only, so a bridge lowering the ticker as a number makes `.toUpperCase` undefined and throws inside Array.filter — before either value guard can turn it into a clean undefined. That throw escapes selectUsdtBalance into updateBalance(), which runs in a try/finally with no catch. On the init path it lands in getBreezInfo's catch, so `loading` stays true for the entire session and the Lightning-address registration never runs; on refresh it is an uncaught rejection on every pull-to-refresh. An additive, best-effort, not-yet-consumed USDT balance must not be able to take the sat wallet down with it. So: type-check the ticker like the other two fields, and move the selector call below the breezBalance persist inside its own try/catch, so the sat balance is written before anything token-related runs. 15 selector tests (a numeric-ticker case added). Full suite green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NEoz7nBtdtsHyuYG5wNPQV
islandbitcoin
changed the title
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
2 participants
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.
Two v0.7.0 slate items. ENG-473 was explicitly blocked on the SDK upgrade — 0.22.3 is on main,
getInfo()now carriestokenBalances, and the read rides the same round-trip as the sat balance, so it refreshes on the same cadence at no extra call.ENG-473 — two decisions worth arguing with
1. This is deliberately NOT
WalletCurrency.Usdt.Spark USDT is a self-custodial token on Spark.
WalletCurrency.Usdtis the custodial wallet on the Flash backend (the one ENG-544 self-heals). Same name, different money, different spend path. Putting Spark's balance into that vocabulary means anywalletCurrency === "USDT"lookup could pick the wrong one — so it's exposed assparkUsdtWalletwith its own type instead. This resolves parent ENG-471's open-decision #1 in the conservative direction.2. The balance stays a
bigintin MINOR units, withdecimalscarried alongside — never pre-divided.Dividing at the source forces a float and hands every consumer a rounded balance. A balance field that was rounded before it reached the spend path is exactly what caused the MAX overdraw in flash#480. The display layer can format; the source must not.
Selection refuses to guess. Any Spark issuer can label a token
USDT— the ticker is a label, not an identity. One ticker match resolves; two or more resolve toundefinedrather than risk showing a spoofed issuer's balance as the user's money.issuerPublicKeyis carried on the result so selection can be pinned to the canonical issuer — that pubkey isn't recorded anywhere in this repo yet, which is why it's documented rather than enforced. Pinning it is a prerequisite before any UI presents this as USDT or any spend path consumes it.Not persisted. Unlike
breezBalance, this is in-memory: absent until the firstgetInfo()rather than stale on cold start. Persisting a bigint needs a persistent-state schema change of its own.Acceptance ("readable in-app, updates on sync") is met at the context layer; no UI consumes it yet by design.
#704 — comment narrowed to what the code delivers
AppUpdateBoundaryclaimed the gate is pinned last so nothing paints over it. Verified, not assumed: that holds only inside its own subtree.NotificationsProviderwraps the boundary inapp.tsx, and itsCustomModalpasses nocoverScreen— so it takes the default, goes through the native modal host, and paints above inline content regardless of sibling order. It's dismissible and the gate survives underneath, which is why this is a comment fix. Hoisting the gate aboveNotificationsProviderwould be a structural change deserving its own PR.Both the boundary docblock and the matching
app.tsxcomment are corrected.9 new tests (precision beyond
MAX_SAFE_INTEGER, decimals ≠ 6, spoofed-ticker refusal, malformed/absent inputs). 92 suites / 906 tests green, tsc clean, changed-lines lint gate clean.Note: the repo targets below ES2020, so the specs use
BigInt("…")calls rather than123nliterals — and the huge-value case builds from a string, since a numeric literal would lose precision beforeBigIntever saw it.