Skip to content

feat(spark): stable-sats receive auto-convert and send-flow fixes#3764

Merged
grimen merged 28 commits into
mainfrom
feat--spark-stable-receive-and-send-fixes
May 20, 2026
Merged

feat(spark): stable-sats receive auto-convert and send-flow fixes#3764
grimen merged 28 commits into
mainfrom
feat--spark-stable-receive-and-send-fixes

Conversation

@esaugomez31

@esaugomez31 esaugomez31 commented Apr 25, 2026

Copy link
Copy Markdown
Collaborator

Spark: stable-sats receive auto-convert and send-flow fixes

What this PR does

Completes the USD wallet experience for self-custodial Spark. Users can now receive payments as stable sats (incoming BTC is automatically converted to USDB right after a Lightning receive) and send from a USD wallet through the Spark SDK's built-in USDB→BTC swap. It also fixes several UX bugs on the confirmation and transaction-detail screens that misrepresented Spark transactions, and introduces a structured SDK-error pipeline so failures show clear, translated messages instead of raw SDK strings or "Something went wrong".

Receive side

Added a new auto-convert module (app/self-custodial/auto-convert/) with an executor, a persistent pending-conversion storage, a listener hook, and a mount component wired into app.tsx. It watches for completed Lightning receives and, when the active wallet is USD, converts the incoming BTC to USDB using the exact received amount. The storage makes the queue durable across app restarts so a conversion that fails mid-flight is retried on next launch. A post-convert sync is triggered so the new USDB balance shows up immediately rather than waiting for the next wallet poll, and the older balance-stale heuristic in the wallet provider has been removed in favor of this deterministic path.

Smaller receive polish: the lightning: URI prefix is stripped from the displayed invoice, the receive toggle icon hides at opacity 0 when the wallet is locked (keeps the layout from shifting), and a new useReceiveAssetMode hook centralizes the decision of whether to receive BTC or USDB.

Convert flow (BTC ↔ USDB)

Reworked the convert bridge to support exact-input amounts and to surface the fee as a DisplayAmount so the UI can render it in the user's display currency cleanly. The useNonCustodialConversionLimits hook now returns a typed min/max shape that the UI uses to validate before submitting. Existing tests were updated and new ones added for the convert bridge, stable-balance bridge, limits bridge, and token-balance bridge.

Send side — three bugs fixed

  1. Sending from a USD wallet over Lightning was routing wrong because the code was passing tokenIdentifier: "usdb-token-id" to the SDK, which is for the destination asset. For a USD→BTC Lightning send the SDK expects conversionOptions: ToBitcoin({ fromTokenIdentifier }) instead. The lightning payment-details now passes conversionOptions for USD wallets and nothing for BTC wallets; tokenIdentifier is no longer used on this path.

  2. The fee returned by the SDK is always in sats, but the confirmation screen was summing it into the settlement amount without conversion — so a BTC fee got added to a USD balance, producing wrong totals and false "amount exceeds balance" warnings. The screen now converts the fee to the settlement currency before adding it, and the send-helpers always return the fee as a BTC money amount so the conversion boundary is explicit.

  3. During a successful send, the SDK briefly reports a zero balance while the settlement broadcasts, which flagged the amount as exceeding balance mid-flight and blocked the confirm button. Added a skipBalanceCheck = isSendingMax || hasAttemptedSend guard so balance validation is bypassed once the user has committed to sending.

Structured SDK errors

A new classifySdkError function maps all twelve SdkError tags (SparkError, InsufficientFunds, NetworkError, ChainServiceError, MaxDepositClaimFeeExceeded, InvalidInput, InvalidUuid, LnurlError, MissingUtxo, StorageError, Signer, Generic) to a small stable set of SelfCustodialErrorCode values (InsufficientFunds, BelowMinimum, NetworkError, InvalidInput, Generic). Wrapper tags like SparkError and Generic are further refined by a case-insensitive .includes() check on the inner string for hints like "insufficient", "minimum", "network", "timeout", with the more specific hints evaluated first. The TAG_TO_CODE map is typed as an exhaustive Record<SdkErrorTags, …> so any future SDK tag will fail to compile until it's mapped.

A new useTranslateSdkError hook translates those codes into user-facing strings via a new SelfCustodialError i18n namespace (insufficientFunds, belowMinimum, networkError, invalidInput, generic), added in all 28 locales with the appropriate diacritics. The confirmation screen uses the hook for any error message coming back from the send mutation, falling back to the generic "something went wrong" string only when the code isn't recognized. The old "Send failed: …" raw string fallback is gone.

UI polish

The confirmation screen header was showing "Destination - " with an empty label for Spark sends because transactionType() had no case for paymentType === "spark". Added the case; it now reads LL.common.spark().

The transaction detail screen was labeling self-custodial Spark transactions as "Lightning" because the self-custodial→GraphQL fragment mapper routes Spark through SettlementViaLn. The typeDisplay helper is now exported and takes an optional selfCustodialPaymentType; when the active wallet is self-custodial and the transaction is Spark, the detail screen correctly shows "Spark". Custodial transactions are unchanged.

Breaking notes

None for consumers. The self-custodial createGetFee, createGetFeeOnchain, and related send-helpers lost their currency parameter — the fee is now always returned in sats and the UI reconverts. All in-tree callers were updated in the same commits. The custodial send path is untouched.

Tests

Full suite passes: 326 suites, 3403 tests. New specs cover the classifier (every tag branch plus inner-string refinement), the translator hook, the conversionOptions path on both prepareSend and the send mutations, the auto-convert executor and its storage, the auto-convert listener hook and mount component, the receive-asset-mode hook, and the typeDisplay Spark override. Existing payment-details, bridge, and mapper specs were updated for the new signatures and removed-mock surface.

@esaugomez31 esaugomez31 marked this pull request as ready for review April 25, 2026 02:01
@esaugomez31 esaugomez31 requested review from Copilot and removed request for Copilot April 25, 2026 02:01
@esaugomez31 esaugomez31 self-assigned this Apr 25, 2026
@esaugomez31 esaugomez31 force-pushed the feat--spark-stable-receive-and-send-fixes branch from 1d89661 to 8e3715b Compare April 26, 2026 18:14
@esaugomez31 esaugomez31 force-pushed the fix--spark-settings-and-onboarding-ui branch from 6ac3787 to ece95dc Compare April 28, 2026 16:27
@esaugomez31 esaugomez31 force-pushed the feat--spark-stable-receive-and-send-fixes branch from 8e3715b to c930670 Compare April 28, 2026 16:27
@grimen grimen changed the title Spark: stable-sats receive auto-convert and send-flow fixes feat(spark): stable-sats receive auto-convert and send-flow fixes Apr 28, 2026
@esaugomez31 esaugomez31 force-pushed the feat--spark-stable-receive-and-send-fixes branch from c930670 to 8646588 Compare May 6, 2026 02:05
@esaugomez31 esaugomez31 force-pushed the fix--spark-settings-and-onboarding-ui branch 2 times, most recently from a918f3a to fffa4ca Compare May 7, 2026 15:54
@esaugomez31 esaugomez31 force-pushed the feat--spark-stable-receive-and-send-fixes branch from 8646588 to 5bf699b Compare May 7, 2026 15:54

@grimen grimen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — bugs + test coverage

Important

This PR (and its stack) needs a major refactor pass. Findings include:

  • SOLID violationsAutoConvertOutcome is a 5-status discriminated union (Converted | AlreadyConverted | SkippedStableBalanceActive | SkippedBelowMin | Failed) where the listener (use-auto-convert-listener.ts:145-156) and executor (auto-convert/executor.ts:131-176) both encode which statuses mean "remove record" vs "retry" vs "drop silently." A new status requires lockstep updates across both files. Open/Closed violation. The executor's own comment — "Failed falls through so the next trigger retries" — is the leak made explicit. runAutoConvert (use-auto-convert-listener.ts:115-160) takes a 10-field RunAutoConvertParams bag — Parameter Coupling smell, and the function is doing attempt-cap, polling, fee-discovery, dispatch, and outcome-routing in one body.
  • High cyclomatic complexity in the bridge layerprepareConversion in bridge/convert.ts:181-263 is an 80-line function with three nested try/catch blocks, four early-return discovery-fallback paths, two empty } catch {} swallows (lines 252, 261), and a discovery-then-correction-then-fallback sequence with no testable seam between phases. McCabe ~12. The silent-half-convert bug (Critical #3) is a direct symptom of this density.
  • Listener over-scopeuse-auto-convert-listener.ts is 308 LOC for a single hook, containing 8 internal helpers (reportError, extractLightningInvoiceFromPayment, fetchPaymentById, convertSatsToUsdCents, isRetryableNow, showConvertedToast, runAutoConvert, findPaidAmountForInvoice), three useRefs coordinating cross-effect state, and at least three interleaved effects (live trigger, mount-replay, prune). The concurrency bugs (Critical #1, I1, I2) are direct symptoms — the inFlightInvoicesRef set is the only barrier between live and replay, and ad-hoc refs are how you spell "missing state machine."
  • Magic numbers scattered across filesRETRY_COOLDOWN_MS = 30_000 and ORPHAN_TIMEOUT_MS = 2 * 60 * 1000 (use-auto-convert-listener.ts:39-43); DEFAULT_AMOUNT_MATCH_TOLERANCE_BPS = 500, AMOUNT_MATCH_TOLERANCE_FLOOR = 50 (auto-convert/executor.ts:91-94); DISCOVERY_MIN_INPUT_MARGIN_BPS = 1000 (bridge/convert.ts:160). All knobs of the same auto-convert subsystem, declared as inline constants in three different files instead of a single config module. Some are remote-config-overridable (autoConvertMaxAttempts, autoConvertPollMaxAttempts), some aren't — no rationale for the split.
  • Duplicated crashlytics().recordError(err instanceof Error ? err : new Error(…)) pattern at executor.ts:84-86, use-auto-convert-listener.ts:48-50, bridge/convert.ts:289-291 and several more sites. Same shape every time; should be a single reportError(err, context) utility in app/utils/crashlytics.ts.
  • Linear-scan classifier where a switch belongsINNER_HINTS: Array<[string, SelfCustodialErrorCode]> (sdk-error.ts:38-43) is a 4-entry order-dependent array of substring tuples. The order is load-bearing ("minimum" before "insufficient") but nothing in the code documents this. A switch over a single regex match, or an explicit precedence comment + a test that locks the order, would be safer (this is the source of test-coverage gap #9 and Critical #9).
  • Naming smells — the listener's branch at use-auto-convert-listener.ts:148-156 has to enumerate three of the five AutoConvertStatus names in one if to express "remove the record on any non-retry outcome" — a sign the enum shape doesn't match how callers consume it. executeAutoConvert vs runAutoConvert are adjacent functions with synonymous verbs. useNonCustodialConversionLimits ("non-custodial" = "self-custodial" elsewhere — same concept, opposite naming convention). RETRY_COOLDOWN_MS and ORPHAN_TIMEOUT_MS are both timeouts; the relationship between them isn't surfaced. Hook prefix is inconsistent: useReceiveAssetMode (verb-noun) vs useAutoConvertListener (compound-noun) vs useTranslateSdkError (verb-noun) vs usePaymentRequest (noun). Three refs for related dedup state in one hook with no shared pattern: processedPaymentIdsRef, inFlightInvoicesRef, initialReplayDoneRef.
  • Status-enum proliferationPaymentResultStatus.Success | Failed (the bridge return) and AutoConvertStatus.Converted | Failed | SkippedStableBalanceActive | SkippedBelowMin | AlreadyConverted (the executor return) are two separate enums for "outcome of one auto-convert attempt." The executor unwraps the first into the second (executor.ts:166-176) and the listener then unwraps the second into "remove vs retry vs drop." Three enums where one tagged union of { kind: "success" } | { kind: "skip"; retry: false } | { kind: "fail"; retry: true } would do the job and remove the cross-file coupling.

Why this is not in scope for this PR: this branch is stacked on fix--spark-settings-and-onboarding-ui with downstream PRs likely depending on it. Refactoring the auto-convert module's shape, the listener's effects, and the cross-file enum/naming surface would force a multi-day restack across the stack and create combinatorial conflict pain — particularly the rename passes. Refactoring against main after the stack settles is materially cheaper.

Decision: this review covers correctness only. The structural / SOLID / complexity / naming review will be tracked as a separate cleanup ticket to land once both:

  1. The full stack is approved for functionality, and
  2. All Critical bugs and missing tests below are resolved.

Cleanup will then land as a sequence of small, focused PRs against main — no restack pain, each independently mergeable. Production rollout should be gated on the cleanup ticket reaching at least the auto-convert state-machine + adapter-shape unification milestones, otherwise the structural debt ships with the feature and gets normalised.


Real strengths first: the SDK error classifier's exhaustive Record<SdkErrorTags, …> shape is genuinely typed (sdk-error.ts:18-31); the persistent-queue design in auto-convert/storage.ts correctly handles schema-widening for records persisted before attempts/lastAttemptAtMs existed; executor.spec.ts covers the tolerance-band edges (4% match, 100% no-match) and the listPayments rejection fail-open; use-payment-request.spec.ts drives the full Created→Paid→re-mount lifecycle with the baseline paymentId ref guard. The shape is right — the issues below are concentrated in the listener's concurrency/replay paths and in the screen-level integration tests for the headline send-flow fixes.

Severity:

  • Critical — fund-stranding, mis-conversion, 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. Auto-convert attempts counter incremented before convert runs

app/self-custodial/hooks/use-auto-convert-listener.ts:122-132

await recordAutoConvertAttempt(record.paymentRequest, Date.now())
const settled = await waitForPaymentCompleted(sdk, paymentId, waitOptions)
if (!settled) return

waitForPaymentCompleted exhausting its bounded poll without observing Completed (slow operator, network hiccup longer than autoConvertPollMaxAttempts * autoConvertPollIntervalMs) bumps the attempt counter on a phantom failure where the convert never even ran. After autoConvertMaxAttempts such phantoms, the record.attempts >= maxAttempts branch (line 123) silently removes the record. The receive succeeded, the user's BTC stays as BTC, no UI feedback (silent-by-design).

Fix: stamp recordAutoConvertAttempt only after settled === true. The polling failure mode is a transient SDK condition unrelated to the convert's own retry budget.

2. Dedup collision on equal-amount invoices silently skips legitimate convert

app/self-custodial/auto-convert/executor.ts:124-164

hasAlreadyConverted matches against the SDK's payment history on (timestamp >= recordCreatedAtMs, amount within ±5% / 50-sat tolerance). Timeline:

  • T0: invoice A created, recordCreatedAtMs_A = T0
  • T1: invoice A paid
  • T2: invoice B created, recordCreatedAtMs_B = T2
  • T3: invoice B paid
  • T4: convert for A completes — payment.timestamp ≈ T4 (which is > T2)
  • T5: listener fires for B. hasAlreadyConverted({recordCreatedAtMs: T2, satsAmount: amount_B, toleranceBps: 500}) finds convert_A (timestamp ≥ T2, amount within tolerance) → returns true → B's record removed without converting.

User receives twice, only one auto-convert runs; the second receive's BTC stays as BTC silently. The 5% / 50-sat tolerance window makes the collision likely for round-number invoices users tend to issue.

Fix: correlate by SDK paymentId or per-attempt session id rather than amount window. The tolerance check is the only safety net against retrying a successful prior attempt, so it can't be removed — replace it with a stronger key.

3. Convert pipeline silently executes a half-convert on correction failure

app/self-custodial/bridge/convert.ts:198-262

When prepareConversionWithDestination overshoots (finalAmountIn > inputAmount), the code computes correctedTarget = (initialTarget * inputAmount) / finalAmountIn and re-quotes. If the corrected re-quote throws (line 259 catch), the function returns the discovery prepared response — but discovery's amountIn was constructed for halfDestination, which may be roughly half of inputAmount. The SDK then converts ~half of the user's funds and reports Success. For auto-convert this is invisible — the executor sees Success and removes the pending record.

Fix: in the correction catch, fall back to prepared (the overshoot quote, which executePrepared would reject) or rethrow. Never silently execute the discovery quote.

4. Confirmation-screen fee-currency conversion is the actual bug — and untested

app/screens/send-bitcoin-screen/send-bitcoin-confirmation-screen.tsx:295-302

const feeInSettlementCurrency = fee.amount
  ? paymentDetail.convertMoneyAmount(fee.amount, settlementAmount.currency)
  : zeroSettlementAmount
const totalAmount = addMoneyAmounts({ a: settlementAmount, b: feeInSettlementCurrency })

The PR description identifies this as a fix: previously a BTC fee got summed into a USD settlement, breaking the balance check. The fix is correct in code, but no test renders the screen with a USD settlementAmount and a BTC fee.amount and asserts the balance check outcome. The existing send-confirmation.spec.tsx only exercises the LNURL story, never crossing the settlement-currency-≠-fee-currency boundary. A regression here silently re-introduces the original bug.

Fix: screen-level test: USD($9.99) + BTC(50 sats) at $10.00 balance → no amountExceed; USD($9.999) + BTC(500 sats) at a rate that pushes total > balance → amountExceed renders.

5. skipBalanceCheck guard has no regression test

app/screens/send-bitcoin-screen/send-bitcoin-confirmation-screen.tsx:304

skipBalanceCheck = isSendingMax || hasAttemptedSend is the right guard, but no test verifies (a) over-balance with hasAttemptedSend=false still blocks, (b) isSendingMax=true legitimately allows over-balance, (c) hasAttemptedSend=true correctly bypasses. The path is currently safe at the UI layer (<GaloySliderButton> is disabled={!validAmount || hasAttemptedSend}, useSendPayment short-circuits with hasAttemptedSend && return undefined) — but neither of those guards is asserted in this PR. Any future refactor of either layer can silently open a real over-spend hole.

Fix: drive the screen through the three states above and assert (a) slider disabled state and (b) errorMessage text.

6. Listener replay can loop forever on busy wallets

app/self-custodial/hooks/use-auto-convert-listener.ts:260-296

processRecord calls findPaidAmountForInvoice; if it returns undefined (e.g. the matching record has aged off the most-recent 50 in listPayments), the function returns without ever calling recordAutoConvertAttempt and without removing the record. The same record gets picked up by every cold-start replay until pruned at TTL=24h. Combined with findPaidAmountForInvoice only scanning offset:0, limit:50, a wallet processing >50 transactions in 24h leaves a paid-but-listener-missed invoice in a permanent loop where every app launch re-scans payments and never converts. Funds stay BTC despite user opting in.

Fix: in the no-payment-found branch, either page through listPayments until the record's createdAtMs falls off the cursor, or recordAutoConvertAttempt so the cap engages.

7. useReceiveAssetMode returns Bitcoin during SDK boot window

app/self-custodial/hooks/use-receive-asset-mode.ts:23-35

useSelfCustodialWallet().isStableBalanceActive defaults to false until getUserSettings resolves in use-sdk-lifecycle.ts:128-137. The hook initialises assetMode = Bitcoin on this default. A stable-balance-active user opening receive during boot generates a BTC invoice with no auto-convert record persisted (use-payment-request.ts:101 if (assetMode === Dollar) is false). When isStableBalanceActive resolves to true afterwards, the useEffect re-aligns the toggle to Dollar but the existing invoice isn't regenerated and isn't queued. The user sees Dollar in the UI yet the receive lands as BTC (without auto-convert) once paid.

Fix: expose a loading state from useReceiveAssetMode and have use-payment-request.ts defer invoice creation until settings are known.

8. Active-wallet flip USD→BTC between receive and execute strands funds

app/self-custodial/hooks/use-auto-convert-listener.ts:229 + app/self-custodial/auto-convert/executor.ts (SkippedStableBalanceActive branch)

The listener captures isStableBalanceActive per-effect-run and forwards it to executeAutoConvert. If the user receives a Dollar invoice (record persisted), then toggles stable-balance off before the convert fires, the executor's first guard returns SkippedStableBalanceActive and the record is removed (use-auto-convert-listener.ts:153). Funds remain in BTC despite the user having opted in at receive time.

Fix: decouple the convert-time gate from receive-time intent — either re-check at execute time using a fresh wallet snapshot, or treat SkippedStableBalanceActive as keep-record-and-skip rather than remove-record.

9. SDK error classifier doesn't refine InvalidInput / InsufficientFunds

app/self-custodial/sdk-error.ts:32-61

Only SparkError and Generic are in INNER_REFINEMENT_TAGS. The SDK can return InvalidInput with inner text "amount below minimum" — exactly the pattern the hint set was designed to catch. Today this classifies as InvalidInput and shows "Invalid input" instead of "Below minimum". This isn't only a UX issue: BelowMinimum triggers SkippedBelowMin in executeAutoConvert (record deleted, convert abandoned), while InvalidInput yields Failed (record retried). Misclassification diverges queue state from user reality.

Fix: add InvalidInput and InsufficientFunds to INNER_REFINEMENT_TAGS, or redesign the refinement to apply unconditionally when an inner string exists and a tag-level mapping isn't already specific enough.

10. Convert reads pre-receive balance via ensureSynced:false

app/self-custodial/bridge/convert.ts:162-197

fetchFromBalanceBaseUnits calls getInfo({ ensureSynced: false }). The auto-convert listener invokes executeAutoConvert immediately after waitForPaymentCompleted returns true — there's a real window where getInfo(ensureSynced:false).balanceSats returns the pre-receive balance (the same SDK staleness skipBalanceCheck exists to mitigate on the send side). If actualBalance is non-zero but lower than requestedInput (the just-received sats), the convert proceeds with the smaller actualBalance — converting only the prior balance and leaving freshly-received sats unconverted.

Fix: ensureSynced: true for auto-convert callers, or accept requestedInput whenever it equals the just-completed payment amount.


Important

# Item Location
I1 Live-effect + mount-replay can race the same paymentRequest past the inFlightInvoicesRef check (live adds after findPendingAutoConvert, replay adds after findPaidAmountForInvoice — both async gaps) use-auto-convert-listener.ts:204, 220-222, 255, 267-273
I2 recordAutoConvertAttempt cap is checked against snapshotted record.attempts, not live storage value — two concurrent runAutoConvert invocations both pass the cap and both attempt convert use-auto-convert-listener.ts:123-126
I3 Mount-replay enforces sequential processing via reduce. Comment explains it; no test asserts non-parallelism. A "let's Promise.all this" optimisation would hammer the SDK and double-process use-auto-convert-listener.ts:292
I4 findPaidAmountForInvoice does exact-string match on invoice. SDK normalising case/padding silently breaks every replay use-auto-convert-listener.ts:169
I5 useReceiveAssetMode re-aligns to Dollar on isStableBalanceActive flipping ON but does nothing on OFF (by design — user keeps the mode they were in). Not asserted; a future "fix" that resets to BTC would silently regress intent use-receive-asset-mode.ts:31-35
I6 Storage withWriteLock chain absorbs both fulfilled and rejected outcomes; not exercised with a write that throws synchronously before producing a promise app/self-custodial/auto-convert/storage.ts:41-48
I7 SkippedBelowMin correctly removes the record without a toast, but no test guards that — a refactor that adds a misleading "converted" toast would not be caught use-auto-convert-listener.ts:156 (mount-replay path)
I8 prepareConversion discovery clamp tested only against minFromAmount; no test where minToAmount dominates bridge/convert.ts:211
I9 waitForPaymentCompleted maxAttempts:1 (degenerate case) — no setTimeout should fire, behaviour not asserted executor.ts:67-69
I10 auto-convert-listener-mount.spec.tsx is shallow — asserts mount and null render only; if the wrapper ever swallowed errors from the hook, the test wouldn't notice __tests__/self-custodial/components/auto-convert-listener-mount.spec.tsx

Test coverage

Estimated behavioural coverage of the new auto-convert + send-fee + SDK-error layer: ~70%, concentrated on storage, executor unit logic, and the SDK classifier; sparse on the listener's concurrency/replay branches and on the screen-level integration of the headline send-flow fixes.

Tier 1 — production code with no test file

File LOC Notes
app/self-custodial/auto-convert/types.ts 42 Type-only; no runtime tests needed, but ensures shape integrity if treated as schema
app/self-custodial/auto-convert/index.ts 16 Barrel; minor
app/self-custodial/components/index.ts 1 Barrel only
(none of the new hooks/executor/storage are uncovered — this PR adds tests for each)

The new units are individually covered. The gaps below are at the integration / branch-combination level, not at the unit level.

Tier 2 — tests that pass while a Critical bug is alive

These create false confidence. 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__/self-custodial/payment-details/lightning.spec.ts:266-279 Asserts only tag: "ToBitcoin" via expect.objectContaining. If buildConversionType were silently changed to new ConversionType.ToBitcoin({}) (no token id), the test still passes. The assertion the user-asked-for fix needs is inner.fromTokenIdentifier === "usdb-token-id" regression of the original tokenIdentifier bug
__tests__/self-custodial/sdk-error.spec.ts:37-42 Substring precedence ["minimum", BelowMinimum] precedes ["insufficient", InsufficientFunds]. No test asserts this is intentional; reorder = silent semantic flip and queue-state divergence #9
__tests__/self-custodial/sdk-error.spec.ts:54 if (inner) falsy-check means "" falls through to tag-level mapping. sdkError("SparkError", [""]) is a separate path, not asserted. A refactor to if (inner !== undefined) would silently flip behaviour #9 (defensive)
__tests__/self-custodial/bridge/convert.spec.ts Never asserts sdk.syncWallet is called on success (line 273-279 in production). The TODO comment explains the call is load-bearing for getInfo alignment until Breez materialises token balances on insert. A refactor that drops the call breaks every USD balance after convert #3 (adjacent)
__tests__/self-custodial/hooks/use-auto-convert-listener.spec.ts:292 "Deduplicates the same paymentId across rerenders" only varies the live effect's dependency. Doesn't simulate live + replay racing the same paymentRequest I1
__tests__/self-custodial/hooks/use-payment-request.spec.ts:401-421 Drives Dollar mode by mocking useReceiveAssetMode. Production wiring depends on the real hook reading isStableBalanceActive from the wallet provider — the boot-window race (#7) is invisible because the mock skips that path #7
__tests__/screens/send-confirmation.spec.tsx Only LNURL story; never enters settlement-currency-≠-fee-currency path; never asserts validAmount outcome. The headline fee-currency fix is unverified #4

Tier 3 — Critical bugs with no test coverage anywhere

Critical Recommended test
#1 runAutoConvert invoked with waitForPaymentCompleted returning false → assert recordAutoConvertAttempt is not called
#2 Two equal-amount invoices, A converts then B's listener fires → assert executeAutoConvert runs for both, removePendingAutoConvert is not called for B without a convert
#3 prepareConversionWithDestination overshoots, corrected re-quote rejects → assert function rethrows or returns the overshoot quote, not the discovery quote
#4 Render SendBitcoinConfirmationScreen with USD settlement + BTC fee at boundary balance → assert balance check fires/does-not-fire correctly
#5 Three-state matrix on (isSendingMax, hasAttemptedSend) → assert slider disabled state and errorMessage
#6 Listener mount with persisted record whose payment isn't in the most-recent 50 → assert recordAutoConvertAttempt is called (so the cap engages) or paging is performed
#7 useReceiveAssetMode mounted with isStableBalanceActive initially undefined/loading → assert assetMode is not committed to Bitcoin and no invoice is generated until settings resolve
#8 Pending Dollar record + isStableBalanceActive flips true between receive and execute → assert record is not removed (or that an explicit user notification fires)
#9 classifySdkError(sdkError("InvalidInput", ["amount below minimum"])) → assert result is BelowMinimum, not InvalidInput
#10 Auto-convert immediately post-receive with getInfo(ensureSynced:false) returning stale balance → assert requestedInput is honoured rather than truncated to stale balance

Suggested order of operations

  1. Land Critical #1 first — it's a one-line move (recordAutoConvertAttempt after the settled check) and prevents silent fund-stranding. Add the regression test from Tier 3.
  2. Critical #3 — the bridge/convert.ts correction-catch fallback. One-line behavioural change; high-value test.
  3. Critical #2 — replace the amount-tolerance dedup key with a per-attempt session id or SDK paymentId correlation. Requires a small schema bump to the persisted record (acceptable: storage already handles schema-widening).
  4. Critical #4 + #5 — add the screen-level fee-currency and skipBalanceCheck regression tests. These lock in the PR's headline fixes.
  5. Critical #7 — expose a loading state from useReceiveAssetMode; defer invoice creation until settings resolve.
  6. Critical #10ensureSynced: true for the auto-convert callers of fetchFromBalanceBaseUnits.
  7. Critical #6, #8, #9 — replay-loop bound, active-wallet-flip handling, classifier refinement table.
  8. Tier 2 tightenings — un-loose the lightning.spec.ts assertion, lock substring precedence in sdk-error.spec.ts, add syncWallet assertion in convert.spec.ts. These are cheap and prevent silent regressions.
  9. Important I1–I10 — most fold into the same commits as the Criticals; race tests (I1, I2) are worth adding once the Critical fixes are in.

Structural / state-machine refactor for the listener and the convert pipeline will follow as a separate note against main after this stack settles, so it doesn't restack-bomb downstream PRs.

@esaugomez31 esaugomez31 force-pushed the feat--spark-stable-receive-and-send-fixes branch from 5bf699b to 77345a0 Compare May 9, 2026 01:35
@esaugomez31 esaugomez31 force-pushed the fix--spark-settings-and-onboarding-ui branch from fffa4ca to 617d25b Compare May 9, 2026 01:35
@esaugomez31 esaugomez31 force-pushed the feat--spark-stable-receive-and-send-fixes branch from 77345a0 to c655288 Compare May 9, 2026 18:14
@esaugomez31

esaugomez31 commented May 10, 2026

Copy link
Copy Markdown
Collaborator Author

@grimen

Critical

1. Auto-convert attempts counter incremented before convert runs

recordAutoConvertAttempt is now called only after waitForPaymentCompleted returns true. A bounded poll exhausting on a slow operator no longer counts as a convert attempt — the cooldown stamp and the cap budget are both reserved for runs where the convert actually had a chance to execute. The if (!settled) return short-circuits before the stamp, so phantom poll-failures can no longer accumulate up to autoConvertMaxAttempts and silently strand funds. Covered by __tests__/self-custodial/hooks/use-auto-convert-listener.spec.ts ("does not bump the attempt counter on poll exhaustion") which mocks waitForPaymentCompleted to resolve false and asserts recordAutoConvertAttempt is not called.

2. Dedup collision on equal-amount invoices silently skips legitimate convert

Replaced the amount-tolerance fallback as the only dedup key with a persistent receive→conversion pairing. Each successful convert now writes { receivePaymentId, conversionPaymentId, pairedAtMs } to selfCustodialAutoConvertPairings (7-day TTL, dedup'd by either id, schema-widening preserved). The executor's hasAlreadyConverted accepts a claimedConversionIds set and excludes those payments from the amount-tolerance match, so receive A's conversion can no longer be mistaken for receive B's "already converted" state when their amounts fall within the 5%/50-sat window. The listener also pre-flights pairedReceiveIds.has(paymentId) before re-dispatching: a receive whose pairing is already on disk gets its pending record dropped and skipped — no double-convert on replay tras crash. Pairing TTL prune lives next to the existing record prune in the mount-replay effect. Covered by 12 new tests across executor.spec.ts (claimed-id exclusion, findRecentConversionId resolution + skip + non-completed filter + fail-open), storage.spec.ts (list/mark dedup-by-either-id/prune), and use-auto-convert-listener.spec.ts (persists pairing on success, excludes paired conversions from executor, skips and removes record on already-paired receive).

3. Convert pipeline silently executes a half-convert on correction failure

Both silent fallback } catch { return { prepared: discovery, tokenDecimals } } blocks in prepareConversionWithDestination are gone — the corrected re-quote's failure now propagates up to the caller. createGetConversionQuote already records the rejection to crashlytics and rethrows; the executor's executeAutoConvert classifies the rethrow as Failed, leaving the pending record intact for the cooldown-driven retry. The discovery quote (whose amountIn was constructed for halfDestination, ~half of requestedInput) can no longer reach executePrepared. Covered by a new test in convert.spec.ts ("rethrows when the corrected re-quote rejects after an overshoot, never executing the discovery quote") that drives prepareConversion through the overshoot path with the corrected re-quote rejecting, and asserts the function rethrows and never returns the discovery prepared response.

4. Confirmation-screen fee-currency conversion is the actual bug — and untested

The production fix at lines 295-302 was already in this PR's diff; what was missing was screen-level coverage. __tests__/screens/send-confirmation.spec.tsx now has a "Critical #4 fee-currency conversion" describe block that drives the SendBitcoinConfirmationScreen with a USD settlementAmount, a BTC fee.amount, and a real-ish convertMoneyAmount mock pegged at 1 BTC = $20,000 (50 sats per USD cent). Two cases lock the contract: USD($9.99) + BTC(50 sats) at $10.00 balance — fee converts to 1 cent, total $10.00, no amountExceed rendered; USD($9.99) + BTC(500 sats) at $10.00 balance — fee converts to 10 cents, total $10.09, amountExceed renders. Anyone who silently regresses the conversion (e.g. by summing fee.amount raw) trips both assertions because the unit-mixed totals would always exceed.

5. skipBalanceCheck guard has no regression test

Same spec file gained a "Critical #5 skipBalanceCheck matrix" describe that exercises all three states with a settlement of $11.00 over a $10.00 USD balance (always over-balance, so the guard's behaviour is the only thing that varies). The GaloySliderButton test mock was extended to forward its disabled prop via accessibilityState.disabled so the screen's disabled={!validAmount || hasAttemptedSend} decision is observable from tests. The three asserted cases: (isSendingMax=false, hasAttemptedSend=false) → slider disabled + amountExceed rendered; (isSendingMax=true, hasAttemptedSend=false) → slider enabled + no error (sending-max bypass legítimo); (isSendingMax=false, hasAttemptedSend=true) → slider disabled + no error (post-attempt re-render). A future refactor that flips any of skipBalanceCheck's OR-arms or the slider's disabled predicate fails the matrix immediately.

6. Listener replay can loop forever on busy wallets

processRecord in the mount-replay effect (use-auto-convert-listener.ts) used to short-circuit silently when findPaidAmountForInvoice returned undefined, leaving the record stuck in the queue until the 24h TTL pruned it. On wallets with >50 transactions in 24h the matching payment ages off the recent listPayments page (offset: 0, limit: 50) and the replay re-scans every cold start with no progress and no eviction. The no-paid branch now stamps an attempt via recordAutoConvertAttempt so the same retry-cooldown that gates live triggers also throttles the replay path. Crucially, when record.attempts + 1 >= autoConvertMaxAttempts the record is removed in-place: the cap normally lives inside runAutoConvert, but runAutoConvert is unreachable here (the function returns early on !paid), so the eviction is hoisted into this branch to keep the attempts → eviction invariant. Picked this over paginating listPayments until the record's createdAtMs falls off the cursor — paging would add SDK calls per cold start on every busy wallet for the same eventual outcome (eviction), and the cap-driven path is what live triggers already trust. Covered by two new tests in use-auto-convert-listener.spec.ts: "stamps an attempt when the matching payment is missing so the cap eventually evicts" (drives attempts: 0 → assert stamp + no removal) and "removes the record on the no-payment branch once attempts reach the cap" (drives attempts: 2 against the default autoConvertMaxAttempts: 3 → assert removal + no stamp).

7. useReceiveAssetMode returns Bitcoin during SDK boot window

isStableBalanceActive is now boolean | undefined (modeled as ?: on the wallet provider context). useSdkLifecycle initializes the state to undefined and only flips it to true | false once getUserSettings resolves; the previous default of false is gone. useReceiveAssetMode derives a loading flag from this sentinel (isStableBalanceActive === undefined) and exposes it on the hook's return shape. usePaymentRequest.generateRequest adds if (isAssetModeLoading) return to its early-exit guard and includes isAssetModeLoading in the useCallback deps, so the auto-fire useEffect defers invoice creation until settings resolve. Once they do, the existing dep-driven re-run picks up the real mode and generates the correct invoice (including the addPendingAutoConvert record on Dollar). The other two production callers of isStableBalanceActive were updated to handle undefined explicitly: the listener coerces with ?? false (treats unknown as "not active" so existing pending records still process), the conversion-details screen wraps the modal-visibility predicate in Boolean(...), and the stable-balance settings screen passes ?? false to the toggle hook. Truthy checks elsewhere already short-circuit correctly because undefined is falsy. Covered by ten new tests across use-receive-asset-mode.spec.ts (six on the loading-flag transitions, including pre-selection mid-load and rail-mode availability while loading), use-payment-request.spec.ts (two on the deferred-then-flipped invoice generation), use-auto-convert-listener.spec.ts (one asserting the listener still calls the executor with false when the context value is undefined), and stable-balance-settings-screen.spec.tsx (one smoke test that the screen renders without crashing during boot).

8. Active-wallet flip USD→BTC between receive and execute strands funds

SkippedStableBalanceActive no longer evicts the pending record. The terminal-outcome branch in runAutoConvert (use-auto-convert-listener.ts) used to remove on Converted | AlreadyConverted | SkippedStableBalanceActive | SkippedBelowMin; it now removes on AlreadyConverted | SkippedBelowMin only. SkippedStableBalanceActive falls through alongside Failed, so the next listener trigger retries until either (a) the user's stable-balance state matches the receive-time intent and the convert proceeds, or (b) the attempt cap kicks in and runAutoConvert's top-of-function check evicts the record cleanly. Picked the keep-record-and-skip shape over re-checking with a fresh wallet snapshot at execute time — the recordAutoConvertAttempt stamp already throttles the loop (cooldown + cap) and a fresh snapshot would couple the listener to an extra SDK round-trip without changing the eventual outcome. Covered by a new test in use-auto-convert-listener.spec.ts ("keeps the record on SkippedStableBalanceActive so a later toggle-off retries") that drives the live path with executeAutoConvert.mockResolvedValue({ status: SkippedStableBalanceActive }) and asserts removePendingAutoConvert is not called.

9. SDK error classifier doesn't refine InvalidInput / InsufficientFunds

INNER_REFINEMENT_TAGS (sdk-error.ts) now includes InvalidInput and InsufficientFunds alongside the existing SparkError and Generic. The classifier still walks INNER_HINTS only when the tag is in the set, so the addition is purely additive — tags without inner-string refinement (storage, signer, network, etc.) keep their direct tag-to-code mapping. Picked the conservative shape (extend the allowlist) over the alternative redesign (apply inner refinement unconditionally) to avoid behaviour drift on tags whose inner strings haven't been audited; the broader redesign is the right move for the post-stack cleanup ticket. Two real-world consequences of the misclassification are now correct: an SDK InvalidInput carrying "amount below minimum" resolves to BelowMinimum so the executor returns SkippedBelowMin (record cleared, no pointless retry), and the user-facing translator surfaces the right message instead of the generic "Invalid input". Covered by three new tests in sdk-error.spec.ts: InvalidInput + below-minimum → BelowMinimum; InsufficientFunds + below-minimum → BelowMinimum (the wallet-empty-due-to-pool-floor case); and InvalidInput with no matching hint → falls through to the tag mapping (InvalidInput).

10. Convert reads pre-receive balance via ensureSynced:false

Setting ensureSynced: true on the auto-convert callers was the first thing tried, and after confirming the behavior with the Breez team, it doesn't solve this case for long-running apps. The flag only gates the initial post-connect sync — after that first getInfo in a session it becomes a no-op, so once the SDK is up the call is silently ignored. Their official docs corroborate this: ensure_synced = true is described as "useful for short-lived scripts that connect, read the balance once, and disconnect," and for long-running clients the recommended pattern is ensure_synced = false plus subscribing to SdkEvent::Synced. Token payments/funds are only materialized at sync time, which is why getInfo alone — even with the flag — keeps returning the pre-receive view. The workaround the Breez team recommends today is an explicit syncWallet() call between payment-detected and balance-read.

That's what the listener does now. runAutoConvert (use-auto-convert-listener.ts) opens with triggerBackgroundSync(sdk) — fire-and-forget so the sync overlaps with the bounded waitForPaymentCompleted poll rather than stacking latency on top of it. By the time polling resolves and executeAutoConvert reads the balance via prepareConversion, the SDK has typically materialized the just-received sats (or the token balance on Unset flows). Fire-and-forget rather than awaited: awaiting syncWallet adds user-visible delay on slow operator responses, and the listener's outcome (success / retry-on-stale) is robust to a slightly-late sync, so blocking the pipeline on it isn't worth the latency. The actual SDK call goes through a small wrapper in bridge/wallet.ts (syncSelfCustodialWallet) so the listener stays free of SyncWalletRequest.create({}) boilerplate and the bridge stays the single seam where the SDK contract is described. Sync errors route to crashlytics via reportError and don't block the convert — if the balance is still stale at read time, the next listener trigger re-quotes. The existing post-send syncWallet in executePrepared (bridge/convert.ts) is a separate concern (post-convert balance alignment) and stays as-is. Covered by a new test in use-auto-convert-listener.spec.ts ("triggers a background syncWallet so the SDK materializes token balances before the convert reads them") that mocks the bridge module and asserts syncSelfCustodialWallet is invoked as part of the run-pipeline once a pending record matches a received payment.

Important

I1. Live-effect + mount-replay race past inFlightInvoicesRef

The async gap between the inFlightInvoicesRef.current.has(invoice) check and the add(invoice) call let two concurrent paths (live event + mount replay) both pass the dedup gate before either reserved. Both paths now reserve the invoice synchronously right after the has check (and before any further await), wrapped in try { ... } finally { inFlightInvoicesRef.current.delete(...) } so a pairedReceiveIds.has short-circuit still releases the slot. Live and replay use the same Set instance, so whichever lands first publishes the reservation; the other observes it on the synchronous has check and returns cleanly. Covered by a new test in use-auto-convert-listener.spec.ts ("dedups live and replay racing the same paymentRequest via inFlightInvoicesRef") that drives both effects with the same paymentRequest (live lastReceivedPaymentId + matching pending record + matching listPayments entry) and asserts executeAutoConvert is called exactly once after letting all replay-path microtasks flush.

I2. recordAutoConvertAttempt cap snapshots record.attempts instead of live storage

runAutoConvert now re-reads the record from storage at the cap check (findPendingAutoConvert(record.paymentRequest)) instead of trusting the in-memory snapshot it received as a parameter. Two concurrent invocations of the same paymentRequest (live event + replay racing past the inFlightInvoicesRef window) now agree on the post-stamp attempts value, so only one of them progresses past the cap when storage already says the budget is exhausted. Falls back to the snapshot's attempts if findPendingAutoConvert returns undefined (record evicted between the listener's lookup and the runAutoConvert entry — already a clean exit). Covered by a new test in use-auto-convert-listener.spec.ts ("re-reads attempts from storage so concurrent invocations agree on the cap") that drives mockFindPendingAutoConvert.mockResolvedValueOnce(record-attempts-2).mockResolvedValueOnce(record-attempts-3) and asserts removePendingAutoConvert is called and executeAutoConvert is not.

I3. Mount-replay sequential reduce — non-parallelism not asserted

use-auto-convert-listener.spec.ts now drives the mount-replay path with two pending records and gates the first record's waitForPaymentCompleted on a manual promise (mockImplementationOnce). The test asserts the second record does NOT begin processing until the first resolves — a future "let's Promise.all this" optimisation would call waitForPaymentCompleted twice immediately and trip the count-of-1 expectation. Locks the existing records.reduce((prev, record) => prev.then(() => processRecord(record)), Promise.resolve()) chain.

I4. findPaidAmountForInvoice exact-string match brittle on SDK normalisation

findPaidAmountForInvoice now lowercases both paymentRequest and the SDK-returned invoice before comparing. Bolt11 is case-insensitive per BIP-0027/Bolt-11 spec, so any future SDK pass that case-folds (or any caller that persists the record with a different casing than the SDK ultimately returns) no longer silently breaks the replay match. Covered by a new test in use-auto-convert-listener.spec.ts ("matches the Lightning payment case-insensitively") that persists a record with lnbc1MIXEDcase and exposes the SDK's payment as lnbc1mixedcase, asserting executeAutoConvert is still called.

I5. useReceiveAssetMode re-aligns ON but not OFF — direction not asserted

use-receive-asset-mode.spec.ts gained a directional asymmetry test: starts with isStableBalanceActive: true, asserts assetMode === "dollar", then re-renders with isStableBalanceActive: false and asserts assetMode STAYS at "dollar" (not reset to Bitcoin). The intentional asymmetry is now asserted: ON re-aligns to enforce the new sweep policy, OFF preserves the user's last explicit choice. A future "fix" that resets to BTC would silently regress receive intent and the test trips it immediately.

I6. Storage withWriteLock chain not exercised with synchronous-throw write

storage.spec.ts now exercises a rejection in the chain: mockSetItem.mockRejectedValueOnce(...) for the first write, then a second addPendingAutoConvert call. Asserts both writes reach setItem (count-of-2), proving the writeQueue.then(undefined, undefined) chain absorbs the rejection without deadlocking subsequent writes. The dual undefined resolve/reject handlers are now load-bearing AND tested.

I7. SkippedBelowMin no-toast contract not asserted

use-auto-convert-listener.spec.ts (mount-replay path) drives mockExecuteAutoConvert.mockResolvedValue({ status: SkippedBelowMin }) and asserts removePendingAutoConvert is called AND mockToastShow is NOT — pinning the silent-cleanup contract. A refactor that adds a misleading "converted" toast on this branch is now caught immediately.

I8. prepareConversion discovery clamp — minToAmount dominance untested

convert.spec.ts now exercises the to-side floor: drives the discovery clamp with toAmount=2000, minToAmount=1500, minFromAmount=0. With halfDestination=1000 and fromMinInDestUnits=0, minToAmount dominates Math.max(...) and the test asserts prepareSendPayment receives amount: BigInt(1500 * 10 ** 4) — the to-side floor in token base units at 6 decimals. Locks the third operand of the discovery Math.max.

I9. waitForPaymentCompleted maxAttempts:1 degenerate case not asserted

executor.spec.ts spies on global.setTimeout and runs waitForPaymentCompleted with maxAttempts: 1. Asserts setTimeout is never called — the if (attempt < options.maxAttempts - 1) guard correctly skips the sleep on the only attempt. Locks the loop's degenerate-case behaviour against a future refactor that drops the guard.

I10. auto-convert-listener-mount.spec.tsx shallow — wrapper-error path uncovered

The wrapper spec gained a hook-error propagation test: mocks useAutoConvertListener to throw synchronously and asserts render(<AutoConvertListenerMount />) re-throws the same Error("listener crashed") rather than absorbing it. Confirms the wrapper has no try/catch around the hook — if a future refactor adds one and silently swallows hook crashes, this test fails immediately.

Tier 2 (tests that pass while a Critical bug is alive)

Each load-bearing assertion is now locked. __tests__/self-custodial/payment-details/lightning.spec.ts:267-279 ("passes a USDB→BTC conversionOptions when sending from a USD wallet") now also asserts conversionType.inner.fromTokenIdentifier === "usdb-token-id", so a regression to new ConversionType.ToBitcoin({}) (no token id) trips the test instead of slipping through. __tests__/self-custodial/sdk-error.spec.ts gained two tightenings: a precedence test that drives sdkError("SparkError", ["amount below minimum due to insufficient funds"]) and asserts the result is BelowMinimum (locks the load-bearing ["minimum", "insufficient"] order — a future reorder produces queue-state divergence and now fails CI), and a falsy-check test that drives sdkError("SparkError", [""]) and asserts the empty-inner case falls through to the tag-level Generic mapping (covers the if (inner) branch that a defensive if (inner !== undefined) rewrite would silently flip). __tests__/self-custodial/bridge/convert.spec.ts ("execute() sends the prepared payment and returns success") now asserts sdk.syncWallet is called on success — the post-send sync is load-bearing for getInfo alignment until Breez materializes token balances on payment insert (per the production TODO), and a refactor that drops it now breaks the test instead of every USD balance after a convert. The global breez SDK mock (__mocks__/@breeztech/breez-sdk-spark-react-native.js) gained SyncWalletRequest: { create: p => p } so the assertion path actually reaches sdk.syncWallet without the inner try/catch swallowing the call.

@grimen grimen force-pushed the graphite-base/3764 branch from b8f5803 to b221e04 Compare May 20, 2026 12:25
@grimen grimen force-pushed the feat--spark-stable-receive-and-send-fixes branch from 63fd45b to 0f63f72 Compare May 20, 2026 12:25
@graphite-app graphite-app Bot changed the base branch from graphite-base/3764 to main May 20, 2026 12:26
@grimen grimen force-pushed the feat--spark-stable-receive-and-send-fixes branch from 0f63f72 to abee102 Compare May 20, 2026 12:26
@grimen grimen self-requested a review May 20, 2026 12:29
@grimen grimen merged commit c6bfcc1 into main May 20, 2026
7 of 9 checks passed
@grimen grimen deleted the feat--spark-stable-receive-and-send-fixes branch May 20, 2026 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants