feat(spark): shared wallet abstractions and provider foundation#3746
feat(spark): shared wallet abstractions and provider foundation#3746esaugomez31 wants to merge 34 commits intofeat--account-migration-to-non-custodial-uifrom
Conversation
grimen
left a comment
There was a problem hiding this comment.
PR Review: Shared wallet abstractions and provider foundation
Solid foundation PR — clean layering, strong test coverage, low deployment risk. Requesting changes on a few items below; medium/low are suggestions.
🔴 High priority (address before merge)
1. use-active-wallet.ts:17 — rollback effect is fragile and untested
The hasRolledBack one-shot ref won't re-fire if nonCustodialEnabled toggles off→on→off mid-session. Drop the ref (the effect is idempotent once setActiveAccountId lands) or key it off the flag value so it re-arms on transitions. Add tests covering:
- Rollback fires when the flag is disabled with a custodial fallback present.
- Rollback does not mutate self-custodial persistent data (spec NFR15).
- Note the no-fallback branch (spec
architecture.md:300-302— maintenance screen) as a known follow-up, not blocking this PR.
2. payment-adapter.ts:73,86 — empty-string sentinels on error
return { invoice: "", errors: [toPaymentError("Failed to create invoice")] }
return { address: "", errors: [toPaymentError("Failed to get address")] }Callers that don't check errors first will render a QR code for "". Make invoice/address optional and omit on failure, or return a discriminated union.
3. Migration chain 3→4→5→6→7 is untested end-to-end
Only the direct 6→7 step is tested. Add one test starting from schema 3 that walks the full chain.
🟡 Medium priority (worth doing in this PR)
4. Split CustodialWalletProvider — extract pure mapper
wallet-provider.tsx:38-72 mixes status derivation, wallet filtering, and transaction partitioning in one useMemo. Extract mapHomeAuthedToActiveWalletState(data, {loading, error, isAuthed}) into app/custodial/mappers/. Provider becomes ~15 lines of glue; the mapper is trivially testable without renderHook or mocks.
5. Extract the rollback effect into its own hook
use-active-wallet.ts:19-29 — distinct concern from "return the active wallet state." Pull into useSelfCustodialRollback(...). Pairs naturally with fix #1 and makes the rollback independently testable.
6. transaction-mapper.ts — fee handling and test gaps
fee: toMoneyAmount(tx.settlementFee, ...)always sets a fee even whensettlementFee === 0. Field is typedfee?, so omit on zero.- Add coverage for
settlementFee === 0and BTC-only / USD-only wallet configurations in the provider tests.
7. Move account ID constants out of the hook
CUSTODIAL_DEFAULT_ID / SELF_CUSTODIAL_DEFAULT_ID in use-account-registry.ts:15-16 are system-level identifiers, not hook internals. Move to wallet.types.ts or a dedicated account-ids.ts so tests and other modules don't duplicate string literals.
🟢 Low priority (follow-up OK)
8. Extract createCustodialDescriptor / createSelfCustodialDescriptor factories from useAccountRegistry.
9. Extract markSelected(accounts, activeId) as a pure helper.
10. Move transaction partitioning (wallet-provider.tsx:52-60) into transaction-mapper.ts.
11. Add failed(message) helper in payment-adapter.ts to consolidate repeated failed-result construction.
12. Replace logSdkEvent if-ladder with a dispatch map (self-custodial/logging.ts:22-40).
13. logging.ts:20 — unknown log levels default to Error, sending unknown debug lines to Crashlytics. Default to Info or Debug.
14. Feature flag cascade test — lock the stableBalanceEnabled ⇒ nonCustodialEnabled invariant.
Spec alignment note
Verified suggestions against blink-specs/self-custodial/architecture.md:
create*naming convention is preserved — the spec mandatescreateSelfCustodial*symmetry across all seven payment adapters, so the custodial side keeps itscreate*prefix even on non-factory exports.- Rollback behavior in #1 respects NFR15 (no mutation of self-custodial local data).
- Shared adapter interfaces in
payment.types.tsmatch the spec's contract definitions 1:1.
Verdict
Approve after #1, #2, #3 are addressed. Medium items are cheap to fold into this PR since the affected files are all new. Low items are fine as follow-ups.
860b015 to
5b2cb7e
Compare
554835c to
88c7729
Compare
8368d8e to
a759b91
Compare
88c7729 to
a043417
Compare
a759b91 to
3fe7201
Compare
grimen
left a comment
There was a problem hiding this comment.
All review feedback has been addressed — nice work.
Minor note for follow-up: The createCustodial* prefix on payment adapters (createCustodialSendPayment, createCustodialClaimDeposit, etc.) leaks the concrete type into what should be an implementation-agnostic adapter interface. Once the routing layer (via usePayments / useActiveWallet) fully hides adapter selection from consumers, consider dropping the prefix to just createSendPayment, createClaimDeposit, etc. — each scoped by their module path. Not blocking, but worth aligning before the pattern solidifies across more adapters.
| status: AccountStatus.Available, | ||
| } | ||
| const scAccount = { | ||
| id: "sc-default", |
There was a problem hiding this comment.
Nit: scAccount isn't very readable — even in tests, prefer spelling out selfCustodialAccount. Not blocking this PR, but something to be aware of going forward.
3fe7201 to
c6482ce
Compare
f54d8d9 to
76a475b
Compare

Summary
Implements the foundation and shared wallet abstractions for self-custodial integration. This establishes the provider-agnostic layer that allows custodial and self-custodial accounts to coexist behind shared interfaces.
No UI changes. No Breez SDK dependency. All existing custodial behavior is unchanged.
What this PR does
WalletState,ActiveWalletState,NormalizedTransaction, 7 payment adapter interfaces,AccountDescriptor,ContactAdapterinapp/types/nonCustodialEnabledandstableBalanceEnabledin Firebase Remote Config with cascade ruleactiveAccountIdfor account selectionapp/custodial/HomeAuthedquery toActiveWalletStateuseActiveWallet(),usePayments(),useAccountRegistry(),useHasCustodialAccount(),useMonetaryPreferences()logSdkEvent()andconnectToSdkLogger()inapp/self-custodial/ready for Breez SDK integrationAccountTypeSelectionScreen,BackupScreen,RestoreScreen,BackupNudge,StableBalance,BackendFeatureGateArchitecture
New files (17)
app/types/wallet.types.ts,transaction.types.ts,payment.types.ts,contact.types.tsapp/custodial/adapters/wallet-adapter.ts,adapters/payment-adapter.ts,mappers/transaction-mapper.ts,providers/wallet-provider.tsxapp/hooks/use-account-registry.ts,use-active-wallet.ts,use-payments.ts,use-has-custodial-account.ts,use-monetary-preferences.tsapp/self-custodial/logging.tsModified files (5)
app/config/feature-flags-context.tsx— 2 new flagsapp/store/persistent-state/state-migrations.ts— schema 7app/hooks/index.ts— barrel exportsapp/app.tsx—CustodialWalletProviderin component tree.storybook/views/story-screen.tsx— schema version bump