feat(spark): add Stable Balance and BTC - USD conversion flow#3761
Conversation
esaugomez31
changed the title
esaugomez31
changed the title
esaugomez31
force-pushed
the
feat--spark-stable-balance-and-usd-conversion
branch
2 times, most recently
from
3ad985d to
e28d173
Compare
esaugomez31
changed the base branch from
feat--spark-payments-and-transaction-history
to
graphite-base/3761
esaugomez31
force-pushed
the
feat--spark-stable-balance-and-usd-conversion
branch
from
e28d173 to
104549a
Compare
esaugomez31
changed the base branch from
graphite-base/3761
to
feat--spark-payments-and-transaction-history
grimen
changed the title
esaugomez31
force-pushed
the
feat--spark-payments-and-transaction-history
branch
from
d129a05 to
5e2acfa
Compare
esaugomez31
force-pushed
the
feat--spark-stable-balance-and-usd-conversion
branch
2 times, most recently
from
6503c1f to
c824aac
Compare
grimen
left a comment
grimen
left a comment
There was a problem hiding this comment.
Review — bugs + test coverage
Important
This PR (and its stack) needs a major refactor pass. The structural critique from #3758 is inherited and amplified here. Findings include:
-
SOLID violations —
useNonCustodialConversionis a parallel hook pathway gatedenabled: isSelfCustodialthat mirrors the existing custodial flow rather than abstracting it;useConversionQuoteexists alongside the existing custodial quote architecture; theConvertParamsshape change adds another adapter-contract knob (the PR description acknowledges "Custodial adapter updated to match the new ConvertParams shape so both providers share the same interface" — but the contract is leaky and mode-aware);bridge/convert.tsmixes pass-through wrappers (createConvertat lines 193-202 is dead code in this PR) with real abstraction (prepareConversion); presentation logic leaks into the bridge layer (formatUsdFromBaseUnitsatbridge/convert.ts:32-43hard-codes$prefix and decimal point inside a non-UI module). -
High cyclomatic complexity in the screen layer — verified branch counts on this PR's modified screens:
home-screen.tsxhas 19 mode branches (isSelfCustodial/isStableBalanceActive/stableBalanceEnabled/nonCustodialEnabled);conversion-details-screen.tsxhas 10 mode branches plus a 31-call hook prelude mixinguseState,useRef, and custom hooks;conversion-confirmation-screen.tsxhas 9 mode branches in 446 LOC and forks the entire submit pathway between custodial GraphQL mutations and SCnonCustodialConversion.execute();stable-balance-settings-screen.tsxis internally clean (3 mode branches) but composes a 7-hook prelude that owns state, fee-quote, refresh, AppState, and UI all in one component. -
Incomplete adapter pattern — mode leaks into call sites instead of being hidden behind a uniform Port.
app/types/payment.types.tsadds 8 new Convert-specific types (ConvertParams,ConversionLimits,ConvertDirection,ConvertErrorCode,ConvertAmountAdjustment,ConvertAdapter,ConvertQuote,GetConversionQuoteAdapter) — adapter-contract bloat; the SC convert path threads its owngetConversionQuoteseparately frompayments.convert(which is unused by the SC flow), so two parallel adapter shapes coexist; mode-prefixed exports proliferate (this PR adds 11+selfCustodial*/nonCustodial*/StableBalance*exports across hooks/bridge/components/screens —useStableBalanceFirstTime,useNonCustodialConversion,useNonCustodialConversionLimits,StableBalanceConfirmModal,useStableBalanceToggleQuote,activateStableBalance,deactivateStableBalance,findUsdbToken,fetchUsdbDecimals, etc.). -
Naming smells — "lying" names:
tokenBaseUnitsToCentssilently rounds (lossy) while the exact version is buried astokenBaseUnitsToCentsExact(amounts.ts:21-31) — the rounding lossiness is the actual root cause of Critical #5;formatUsdFromBaseUnitsis locale-unsafe (hard-coded$X.YY);createConvert(bridge/convert.ts:193) is unreferenced — lying API surface. Inconsistent fee vocabulary across the PR diff: at least 9 distinctfee*names —feeAmount,feeCurrency,feeError,feeInSettlementCurrency,feeInWalletCurrency,feeLabel,feeMajor,feeRowWrapper,feeText. Two type families for the same concept:BalanceMode.Btc/BalanceMode.Usd(string literals"btc"/"usd"inuse-balance-mode.ts) collides withWalletCurrency.Btc/WalletCurrency.Usd(graphql enum) used everywhere else — same concept, parallel vocabularies. Prefix proliferation:selfCustodial*/nonCustodial*/StableBalance*/usdb*— four prefix families for overlapping concerns.ConvertvsConversionused interchangeably in the same module (bridge/convert.tsexportsConversionLimitswhile the file isconvert.ts;ConvertDirectiondescribes aConversion).
Why this is not in scope for this PR: with #3761 sitting under #3762–#3769 (six stacked PRs above this one), doing the refactor here would force a multi-day restack across all of them, break the stack's momentum, and create combinatorial conflict pain — particularly the rename passes and adapter-shape unification. Refactoring against main after the stack settles is materially cheaper for everyone.
Decision: this review covers correctness only. The structural / SOLID / complexity / naming review will be shared as the same cleanup ticket opened against #3758 (this PR's findings extend it, not replace it). Cleanup will land as a sequence of small, focused PRs against main once the stack is approved — no restack pain, each independently mergeable. Production rollout (#3768) should be gated on the cleanup ticket reaching at least the adapter-pattern unification milestone, otherwise the structural debt ships and gets normalised.
Real strengths first: the bridge-layer DI on prepareConversion (bridge/convert.ts:71-149) is well-shaped and worth keeping when the wrapper is fixed; wallet-provider.spec.tsx (1128 LOC) genuinely exercises 10s polling, AppState transitions, sticky-stale, and refresh coalescing — model behavioural coverage; bridge/convert.spec.ts covers BelowMinimum / LimitsUnavailable / prepare/send error paths; transaction-mapper.ts's reportUnhandledEnum (records to crashlytics AND falls back deterministically) is the gold-standard pattern other mappers should follow; useConversionQuote's stale-resolve cancellation is correctly implemented and tested; the custodial regression risk from the ConvertParams shape change is real but actually mitigated — the custodial branch in conversion-confirmation-screen.tsx:244-327 uses intraLedgerPaymentSend directly and bypasses the new SC machinery. The shape of the integration is right; the gaps are concentrated at the toggle + Convert money paths.
Severity:
- Critical — fund-loss, mis-attribution, or silent failure that misleads the user. Must fix before merge.
- Important — correctness/reliability that doesn't lose funds but degrades UX or hides incidents.
- Test — coverage gap or test that passes while a confirmed bug is alive.
Critical
1. Stable Balance toggle has try/finally with no catch
app/screens/stable-balance-settings-screen/stable-balance-settings-screen.tsx:67-83 + bridge at app/self-custodial/bridge/stable-balance.ts:6-19 (no catch either)
try {
if (activate) await activateStableBalance(sdk, SparkToken.Label)
else await deactivateStableBalance(sdk)
await refreshStableBalanceActive()
await refreshWallets()
} finally { setBusy(false); setPendingValue(null) }If sdk.updateUserSettings(...) rejects (network, signing, persistence) the rejection bubbles to the event handler. Production: switch animates back to its previous position, no toast, no crashlytics, no message. The user has no signal whether it persisted server-side and the refresh failed, the SDK call rejected, or it silently succeeded but the UI rolled back. This is the central knob for the entire feature.
Fix: wrap in try/catch, record to crashlytics, show a toast, resync the switch. Also the confirm modal at stable-balance-confirm-modal.tsx:83 uses primaryButtonDisabled={isSubmitting || isLoading} — should also disable on hasError so a broken fee preview can't be Confirmed past.
2. createGetConversionQuote swallows every SDK error to null
app/self-custodial/bridge/convert.ts:182-191
async (params) => {
try {
const context = await prepareConversion(sdk, params)
return toConvertQuote(sdk, context)
} catch {
return null
}
}The author throws typed ConvertError(LimitsUnavailable) and ConvertError(BelowMinimum) from prepareConversion, then discards both in the wrapper. Slippage failures, signing failures, network errors, dust-limit issues all map to a generic gray "fee unavailable" UI. Knock-on: use-conversion-quote.ts:48-60's .catch arm becomes dead code because the inner promise never rejects → crashlytics().recordError is never invoked for quote failures.
Fix: keep the catch, record to crashlytics with breadcrumbs (direction, fromAmount), and return a discriminated { ok: false, code } so the UI can render "Conversion limits temporarily unavailable" vs "Amount below minimum" vs generic. Either that, or re-throw and let the hook log.
3. Min-conversion gate falls open when fetchConversionLimits fails
app/screens/conversion-flow/conversion-details-screen.tsx:161-165, 482-501 + hook app/self-custodial/hooks/use-non-custodial-conversion-limits.ts:31-39
const { limits: scConversionLimits } = useNonCustodialConversionLimits(convertDirection)
const scMinFromAmount = isSelfCustodial ? scConversionLimits?.minFromAmount ?? null : null
...
const belowMinimum =
isSelfCustodial && scMinFromAmount !== null && settlementSendAmount.amount > 0 &&
settlementSendAmount.amount < scMinFromAmountThe hook exposes error: Error | null but the screen never reads it. When limits load fails, scMinFromAmount === null → belowMinimum === false → the user can press Review on a 1-sat conversion. Confirmation will fail at prepareConversion (compounded by #2's swallowed error) and the user sees only a disabled slider with no actionable message. Combined with #2 they retry forever.
Fix: consume error from the hook; disable the Review button and toast a clear "Conversion temporarily unavailable" when non-null.
4. Confirmation re-runs full prepareSendPayment on every realtime-price tick — slider can fire a quote the user never saw
app/screens/conversion-flow/hooks/use-conversion-quote.ts:41-64, use-non-custodial-conversion.ts:43-51, conversion-confirmation-screen.tsx:110-114
quoteParams reference changes whenever priceOfCurrencyInCurrency updates (every realtime-price tick from use-price-conversion.ts:55-84). Each change re-runs createOwnSparkInvoice + prepareSendPayment, replacing quote.execute mid-screen. There is no "fee changed — reconfirm" guard, so the slider's visible fee text can be stale relative to the prepared payload that actually executes. Side effect: every keystroke on details and every price tick on confirmation leaks a freshly-minted Spark self-invoice + prepared payment session — dozens per minute on a slow typer.
Fix: snapshot the first Ready quote on entering confirmation; only re-quote on explicit retry. Debounce quoteParams upstream (≥ 500 ms) and abort cancelled prepares. Gate execute on a quote-id captured at slider-press, and re-confirm if quote regenerated since.
5. tokenBaseUnitsToCents rounds the minimum down — sub-cent gap lets UI green-light a sub-min amount
app/self-custodial/bridge/limits.ts:21-30 + app/utils/amounts.ts:21-26
toWalletUnit uses tokenBaseUnitsToCents = Math.round(...). If the SDK's minimum is 1_000_001 USDB base units (1.000001 ¢), it rounds to 1 ¢. The UI's belowMinimum check passes 1 ¢, the user submits, then the SDK rejects below-minimum at prepareSendPayment. Compounded by #2's swallowed errors, the user sees only a generic "fee unavailable" with no path forward.
Fix: Math.ceil for minimums (always conservative upward), Math.floor for maximums. Or carry the raw bigint through and compare in token base units.
6. Self-custodial transaction fees are unconditionally tagged as BTC sats
app/self-custodial/mappers/transaction-mapper.ts:174 + app/self-custodial/mappers/to-transaction-fragment.ts:128-140, 152-157
fee: toWalletMoneyAmount(Math.abs(toNumber(payment.fees)), WalletCurrency.Btc)For USDB token payments, if payment.fees is in token base units (USDB has 6 decimals — factor of 10⁴ vs cents), the mapper labels it as sats and feeInSettlementCurrency later runs that "sats" through the BTC/USD price. Off-by-10⁴ in transaction history. Even if the SDK happens to express token fees in sats today, there's no documentation pin and no test asserting the unit semantics.
Fix: branch on payment.method / payment.details.tag. For Token, use tokenBaseUnitsToCents with the token's decimals. Add a unit test asserting fee currency for a token payment.
7. Config.SPARK_TOKEN_IDENTIFIER collapses to empty string when unset
app/self-custodial/config.ts:35, used in bridge/convert.ts:136, 143 and bridge/limits.ts:18, 40
tokenIdentifier: Config.SPARK_TOKEN_IDENTIFIER ?? ""A misconfigured build silently passes "" to every conversion call. Best case the SDK errors and conversion fails mysteriously. Worst case it's interpreted as native asset and the conversion path no-ops (route reduces to a self-payment with no token side, executing differently than intended). The whole conversion feature can be broken with no warning.
Fix: throw at module-load if Config.SPARK_TOKEN_IDENTIFIER is missing in builds that ship the conversion feature, or guard each entry with a clear "Stable Balance not configured" error.
8. loadMore cursor wiped by background refresh — paginated transactions disappear
app/self-custodial/providers/use-sdk-lifecycle.ts:101-104, 213-220 + app/self-custodial/providers/wallet-snapshot.ts:85-118
refreshWallets (10s poll, AppState change, SDK event) calls getSelfCustodialWalletSnapshot which returns page 0 and resets rawTxOffsetRef.current = snapshot.rawTransactionCount (= 20). If the user has paginated to offset 60 (3 pages), the next refresh truncates the visible list to page 0 and resets the cursor — older txs the user just loaded vanish. Not money-loss, but data corruption / scroll-jumping while reading history.
Fix: refresh re-fetches up to current rawTxOffsetRef.current, merging by id; or keep loaded pages cached and reconcile new entries above offset 0.
9. executePrepared collapses every SDK error into a single message string
app/self-custodial/bridge/convert.ts:155-161
} catch (err) {
return failed(err instanceof Error ? err.message : `Conversion failed: ${err}`)
}Hidden distinct cases: slippage exceeded mid-flight (maxSlippageBps was passed to prepareSend, so the actual rejection happens at execute time), insufficient liquidity, signing key revoked, network mid-send. None tagged with a code, none reported to crashlytics. The base branch's recent send.ts work introduced crashlytics on send failures; convert is the equivalent money-losing path and reports nothing.
Fix: record to crashlytics with breadcrumbs (direction, fromAmount, toAmount); stamp a code (SlippageExceeded | InsufficientFunds | NetworkFailed) where parseable.
10. Token filter silently drops unknown tokens, no telemetry
app/self-custodial/providers/wallet-snapshot.ts:28-32 + matching findUsdbToken in bridge/token-balance.ts:14-17
const isKnownPayment = (payment: Payment): boolean => {
if (payment.method !== PaymentMethod.Token) return true
if (!payment.details || !PaymentDetails.Token.instanceOf(payment.details)) return false
return payment.details.inner.metadata.identifier === SparkConfig.tokenIdentifier
}If SparkConfig.tokenIdentifier ever differs from the on-chain identifier (case slip, regtest config leak, future USDB migration), every USDB payment silently disappears from history while getStableBalance still computes a balance from the same identifier comparison. The user sees a positive USD balance but zero conversion history and no warning — exactly the kind of bug that gets reported as "transactions disappeared after update."
Fix: when a token-method payment with an unrecognised identifier is dropped, emit a one-shot crashlytics breadcrumb (de-duped) so config mismatches surface in production. Same in findUsdbToken.
Important
| # | Item | Location |
|---|---|---|
| I1 | detect-balance-stale flags a legitimately-zero wallet as stale forever — heuristic is balance==0 AND any completed receive. Combined with the 10s poll + AppState reactivation, the toast spams on every refresh. |
app/self-custodial/providers/detect-balance-stale.ts:8-24 |
| I2 | Locale-blind deactivation warning — (usdBalanceAmount / 100).toFixed(2) injected into a translated message; no symbol, no thousands separator, no display-currency conversion. |
app/screens/stable-balance-settings-screen/stable-balance-settings-screen.tsx:153-157 |
| I3 | fetchUsdbDecimals silent fallback to SparkToken.DefaultDecimals if token absent — subsequent centsToTokenBaseUnits math may be wrong by orders of magnitude. No telemetry. |
app/self-custodial/bridge/token-balance.ts:19-22 |
| I4 | is-online.ts swallows every SDK error to a sentinel (ServiceStatus.Major / OnlineState.Unknown) without crashlytics().recordError(err); upstream only logs the sentinel, stack trace lost. |
app/self-custodial/providers/is-online.ts:11-17, 33-40 |
| I5 | refreshStableBalanceActive swallows error with only a debug log — uses logSdkEvent(SdkLogLevel.Error, …) rather than crashlytics().recordError(...) like the rest of the file. After a successful toggle, a refresh failure leaves UI showing stale value silently. |
app/self-custodial/providers/use-sdk-lifecycle.ts:237-245 |
| I6 | 10s polling has no back-off, no error recording; setInterval fire-and-forget with refreshWallets() rejection unguarded. Wedged SDK = silent permanent Offline screen with no crashlytics. |
app/self-custodial/providers/use-sdk-lifecycle.ts:212-220 |
| I7 | AppState "active" listener fires refreshWallets() with no .catch guard — same fire-and-forget shape as I6. |
app/self-custodial/providers/use-sdk-lifecycle.ts:205-210 |
| I8 | use-stable-balance-first-time re-shows modal on every launch if AsyncStorage.setItem fails — crashlytics is recorded but UX nags forever after a single flake. |
app/hooks/use-stable-balance-first-time.ts:19-29 |
| I9 | createConvert (bridge/convert.ts:193-202) is dead code in this PR — no caller routes through payments.convert for self-custodial; everything goes via getConversionQuote → quote.execute(). Future devs may use it and pay double-prepare cost (limits + full prepareSendPayment twice). |
app/self-custodial/bridge/convert.ts:193-202 |
| I10 | formatUsdFromBaseUnits hard-codes $ prefix and decimal point — locale-unsafe for the fee preview text. |
app/self-custodial/bridge/convert.ts:32-43 |
| I11 | Disconnect race: connectAndListen returns the SDK immediately, only checks mounted afterward; if initSdk resolves after unmount, the SDK is disconnected from a stale closure. Uncovered branch. |
app/self-custodial/providers/use-sdk-lifecycle.ts (init effect) |
Test coverage
Estimated behavioural coverage of the new Stable Balance + Convert layer: ~70%, concentrated on bridge / hooks; the screen-level submit and refresh-sequencing paths (where the money actually moves) are the gap.
Tier 1 — production code with no test file
Verified against __tests__/:
| File | LOC | Notes |
|---|---|---|
app/utils/amounts.ts (new helpers) |
(modified) | tokenBaseUnitsToCents, tokenBaseUnitsToCentsExact, centsToTokenBaseUnits are only exercised transitively (one wallet-snapshot assertion + limits.spec.ts). The existing __tests__/utils/amounts.spec.ts covers toSatsAmount only. Source of Critical #5. |
app/self-custodial/providers/use-sdk-lifecycle.ts |
(modified, ~250 LOC) | No __tests__/self-custodial/providers/use-sdk-lifecycle.spec.ts. Coverage is transitive via wallet-provider.spec.tsx; init/disconnect race (I11) and the back-off-on-poll-failure path (I6) are uncovered. |
app/screens/stable-balance-settings-screen/stable-balance-confirm-modal.tsx |
101 | No dedicated spec — exercised only via the parent screen's spec. The hasError → button-disabled gate (Critical #1) is not asserted anywhere. |
app/components/status-pill/status-pill.tsx |
62 | New component, no spec file. The "STALE" pill is the user-visible signal for the entire stale-balance heuristic. |
Tier 2 — tests that pass while a Critical bug is alive
Each should be made to fail on the current code first, then fixed alongside the production fix.
| Test file | What it should catch but doesn't | Bug |
|---|---|---|
__tests__/screens/conversion-flow/conversion-confirmation.spec.tsx |
Describe block named generically ("conversion-confirmation-screen") but only renders the custodial branch. The SC submit (isSelfCustodial && useNonCustodialConversion(...) → execute()) is never executed. The <ConversionFeeRow feeText adjustmentText/> placement and navigation-on-success in the SC branch are completely uncovered. |
#1, #2, #4 |
__tests__/screens/conversion-details-screen.spec.tsx |
useActiveWallet mocked with isSelfCustodial: false so scMinFromAmount is never tested. The 60-scenario it.each table for the gate runs against the custodial path. The SC-specific min-conversion check is dead in this spec. |
#3, #5 |
__tests__/screens/conversion-details-stable-balance-modal.spec.tsx |
Self-custodial tests only assert the first-time modal renders — no amount typed, no belowMinimum exercised. |
#3, #5 |
__tests__/screens/stable-balance-settings-screen.spec.tsx |
Asserts mockActivate/mockDeactivate called, but never asserts mockRefreshStableBalanceActive or mockRefresh are called, let alone the order. The PR description's "Refresh sequencing" claim is unverified. Always uses mockActivate.mockResolvedValue(undefined) — the failure path is never exercised; if activation fails in production, the user gets no feedback (Critical #1). |
#1 |
__tests__/self-custodial/providers/wallet-snapshot.spec.ts |
The pagination test "loadMoreTransactions returns hasMore=true even when filtering reduces transactions below page size" is the right shape, but loadMoreTransactions is called with 0 then 20 as offsets — the test never demonstrates that on a second loadMore, rawTxOffsetRef.current increments by result.rawCount rather than result.transactions.length. That increment is the bug-prevention mechanism. |
#8 |
__tests__/screens/conversion-flow/use-non-custodial-conversion.spec.ts |
Asserts execute() delegation; never re-quotes mid-test to verify the same quote returned at quote-time is the one executed at submit-time. A stale quote whose execute is still in state can fire at a price/fee the user didn't see. |
#4 |
__tests__/screens/conversion-details-stable-balance-modal.spec.tsx |
useStableBalanceFirstTime is mocked, so expect(mockMarkAsShown).toHaveBeenCalledTimes(1) proves the screen calls a callback — not that AsyncStorage receives stableBalanceExplanationShown=true. The hook's spec covers persistence, but no test verifies the storage keys match between writer and reader (classic "shown once" regression). |
(test quality) |
Tier 3 — Critical bugs with no test coverage anywhere
For each, no existing test exercises the behaviour. Add a regression test alongside the fix.
| Critical | Recommended test |
|---|---|
| #1 | stable-balance-settings-screen.spec.tsx: activate failure (mockActivate.mockRejectedValue(...)) → toast shown, switch resyncs to inactive, refreshWallets not called, crashlytics recorded. Plus it("after activate, calls refreshStableBalanceActive then refreshWallets in order") using mock.invocationCallOrder. |
| #2 | bridge/convert.spec.ts: each prepare-throw path (LimitsUnavailable, BelowMinimum, network, slippage) → getConversionQuote returns a tagged failure (or re-throws), and crashlytics().recordError is called. The current spec only asserts the happy path returns a quote. |
| #3 | conversion-details-screen.spec.tsx with isSelfCustodial: true and useNonCustodialConversionLimits returning { limits: null, error: new Error(...) } → Review button disabled, error message rendered. Mirror with { minFromAmount: 100 } → fromAmount=50 disables Next, fromAmount=100 enables. |
| #4 | use-conversion-quote.spec.ts: render with quote A (Ready), tick priceOfCurrencyInCurrency, await re-quote → previous prepare aborted (no leaked state); explicit "snapshot on confirmation entry" semantics. Plus a useNonCustodialConversion test: render with params A, await Ready, change params to B, await re-quote, call execute() and assert it was quote-B's execute. |
| #5 | New __tests__/utils/amounts.spec.ts cases: USDB 6-decimal min 1_000_001 → ≥ 2 cents (rounded up, never down); 0-decimal token; 1-decimal excess clamping; round-trip through centsToTokenBaseUnits; negative input behaviour; exact non-rounded tokenBaseUnitsToCentsExact. |
| #6 | transaction-mapper.spec.ts and to-transaction-fragment.spec.ts: a PaymentMethod.Token fixture asserts fee.currency === WalletCurrency.Usd and the value matches tokenBaseUnitsToCents(payment.fees, decimals) rather than being passed straight as sats. |
| #7 | bridge/convert.spec.ts (or a new config.spec.ts): with Config.SPARK_TOKEN_IDENTIFIER undefined, conversion entry points throw a clear configuration error rather than silently calling the SDK with "". |
| #8 | wallet-snapshot.spec.ts: refresh during pagination (offset=60) → after refresh, rawTxOffsetRef.current preserved at 60; subsequent loadMore requests offset 60. Plus the second-loadMore raw-count vs filtered-count assertion. |
| #9 | bridge/convert.spec.ts: each executePrepared failure type is recorded to crashlytics and surfaces a tagged code, not just err.message. |
| #10 | wallet-snapshot.spec.ts: a token-method payment with mismatched metadata.identifier is dropped AND emits a (mocked) crashlytics breadcrumb. |
Suggested order of operations
- Land the Critical fixes in #3761: all 10 critical items originated in this PR. The toggle no-catch (#1) and the
createGetConversionQuoteswallow (#2) are the "compose to silent failure" pair — fix together. #3 and #5 share root cause (limits-related under-check) — fix together. #6 and #7 are five-line fixes with high impact. #8 needs therawTxOffsetRefpreservation in the refresh path. - Write the Tier 3 regression tests alongside the production fixes. Particularly #1, #2, #3, #4 — these are the money-handling tests that should exist before this PR merges.
- Repair the Tier 2 false-confidence tests. Either rename
conversion-confirmation.spec.tsxto--custodialand add a--self-custodialsibling, or extend the existing file with the SCdescribeblock. Add the missingmock.invocationCallOrderassertions tostable-balance-settings-screen.spec.tsx. - Fill the Tier 1 gaps: new spec for
app/utils/amounts.ts(the Critical #5 testbed); dedicateduse-sdk-lifecycle.spec.tscovering disconnect race (I11) and poll-error back-off (I6); spec forstable-balance-confirm-modal.tsx(withhasError→ disabled assertion);status-pill.spec.tsx. - Important items I1–I11 — most are small (one-line crashlytics adds) and can fold into the same commits as the related Critical fixes. I1 (stale heuristic spam) and I2 (locale-blind deactivation warning) are user-visible and should ship in this PR.
Structural and naming feedback is held to the post-stack cleanup ticket noted in the IMPORTANT block above. Do not refactor in this PR — restack cost outweighs the cleanup benefit until #3768 is approved.
|
@grimen Feedback applied, here's the summary. Critical1. Stable Balance toggle has
|
…ep UI gate above SDK floor
…rt adapter, and expose fee as MoneyAmount
…ep fee preview in sync with execute
…lytics on token mismatches
…tching every page up to the active offset
… spark status errors, and guard against unmount mid-init
…d SC submit describe
…ot paths read instead of re-throwing
…ote (bridge already records with breadcrumbs)
…conversion-quote memo deps
grimen
force-pushed
the
graphite-base/3761
branch
from
f56c1c2 to
2f37ef5
Compare
grimen
force-pushed
the
feat--spark-stable-balance-and-usd-conversion
branch
from
10a742a to
1904857
Compare
…ument use-sdk-lifecycle race-condition disables
grimen
force-pushed
the
feat--spark-stable-balance-and-usd-conversion
branch
from
1904857 to
1af9609
Compare
2 participants
PR Summary — Self-Custodial Stable Balance and USD Conversion
Epic: 5 — Stable Balance on Spark
Base branch:
feat--spark-payments-and-transaction-historyThis PR delivers the full Stable Balance experience for self-custodial accounts: activation/deactivation settings, dual BTC + USD balance display on home, one-time trust/dual-balance explanation, conversion minimum handling, and the manual BTC ↔ USD Convert flow. It closes every task in the Epic 5 brief (Stories 5.1, 5.2, 5.3).
Task checklist
Requirements coverage
Balance · SATS/Balance · USDshowStableBalanceToggleprop-driven; feature flag is read only at the call site (home-screen.tsx:274)LL.StableBalance.minimumConversionisStableBalanceActiveselfCustodialBalanceMode(use-balance-mode.ts) +stableBalanceExplanationShown(use-stable-balance-first-time.ts)mapCurrencymapsPaymentDetails.Token → WalletCurrency.Usd; wallet-snapshot converts token base units to centsuseNonCustodialConversionactivateStableBalance+refreshStableBalanceActive+refreshWalletsin sequence; SDK event listener + AppState-active refresh keep snapshot freshSparkConfig.maxSlippageBps; bridge prepares a self Spark invoice and executes atomically; unit tests cover below-min / limits-unavailable / success pathsWhat's delivered
Story 5.1 — Activation, deactivation, balance display
nonCustodialEnabled && stableBalanceEnabled && active account is SC) opens StableBalanceSettingsScreen which owns the activation toggle.useStableBalanceToggleQuote).StableBalance.deactivateWarningBodycopy with the exact USD amount, plus the estimated conversion fee.updateUserSettings({ stableBalanceActiveLabel: Set | Unset }).isStableBalanceActive+refreshStableBalanceActiveexposed via wallet-provider.tsx; initial fetch + on-demand refresh after activation/deactivation.stableBalanceSettingsroute registered in root-navigator.tsx:769.showStableBalanceToggle,mode,onModeChangeprops; tapping the label persists the chosen mode via useBalanceMode.Story 5.2 — Dual-balance model + minimum handling
fetchUsdbDecimalsandtokenBaseUnitsToCents.loadMorecorrect.fetchConversionLimits(bridge/limits.ts) normalizes min amounts into wallet-display units (sats for BTC, cents for USD) based on direction.LL.StableBalance.minimumConversionwhen the source amount is below the per-direction minimum.paymentType: "conversion"and extracts conversion metadata so the entry is recognisable in history.Story 5.3 — Manual Convert flow
ConvertParams,ConversionLimits,ConvertDirection,ConvertAdapter,ConvertQuote,GetConversionQuoteAdapter,ConvertAmountAdjustment,ConvertErrorCode,convertDirectionFromCurrency,oppositeWalletCurrency.createGetConversionQuote(sdk)— prepares a self Spark invoice, runsprepareSendPaymentwithconversionOptions+maxSlippageBps, returns a formatted fee +execute().createConvert(sdk)— composes prepare + execute in one call, returningPaymentAdapterResultwith explicit error codes forBelowMinimum/LimitsUnavailable.ConvertParamsshape so both providers share the same interface.usePayments()now returnsconvert+getConversionQuote— SC wire-up at use-payments.ts:63.useActiveWallet()and applies the SC min check + first-time modal trigger.ConvertAmountAdjustmentto user-facing copy.i18n
StableBalance.*— home toggle labels, settings copy, activation modal, first-time modal (dual-balance + trust disclosure), minimum conversion message.SelfCustodialBalance.*— stale indicator label + toast.ConversionConfirmationScreen.amountFloored/amountDustBumped— surfacing SDK amount adjustments.Tests
New specs added (and existing ones updated for the new SDK mocks):
bridge/convert,bridge/limits,bridge/stable-balance,bridge/token-balance,bridge/status,bridge/wallet.providers/detect-balance-stale,providers/is-online, extendedproviders/wallet-provider(Stable Balance refresh + AppState handling), wallet-snapshot token filter.use-balance-mode,use-stable-balance-first-time,use-non-custodial-conversion-limits,use-conversion-quote,use-non-custodial-conversion,use-stable-balance-toggle-quote,use-payments(getConversionQuote wiring).balance-header,stable-balance-first-time-modal,stable-balance-settings-screen, settings row,conversion-details-screen,conversion-details-stable-balance-modal,conversion-fee-row,conversion-confirmation, home (Stable Balance toggle rendering).custodial/adapters/payment-adapterspec updated for the newConvertParamsshape.Out of scope (lives here but not in the Epic 5 brief)
A few items ship in this PR because they touch the same surfaces, but they are not part of Epic 5 per the spec. Flagged here for reviewer awareness:
StatusPill"STALE" badge in the balance header + warning toast. This is closer to FR44 (Epic 4.2) and FR65c (Epic 6.3) than Epic 5. Landed here so the Stable Balance UX doesn't silently show a stale USD/BTC split when Spark operators are unreachable.getServiceStatus()+refreshWallets()while the app is foregrounded; refresh on AppStateactive. Supports the stale detector above; also serves FR37 / FR64 from earlier epics.app/graphql/generated.tsregenerated via codegen (required by the CI committer check).Known deltas vs spec
components/stable-balance/stable-balance-settings.tsx; the actual file location is app/screens/stable-balance-settings-screen/ following the existing screens/components convention in the repo. Behaviour matches the acceptance criteria.