feat(spark): self-custodial onboarding and wallet creation#3747
Conversation
esaugomez31
changed the title
grimen
changed the title
grimen
left a comment
grimen
left a comment
There was a problem hiding this comment.
Solid foundational PR with good structure, test scaffolding, and feature-flag gating. The code quality is largely there — what concerns me is shipping safety. This PR introduces real-fund custody on mainnet, flips New Architecture on, and leaves backup to a future epic. That combination needs guardrails before this can be enabled anywhere, even internally.
My comments below are organized by severity. High = I'd block on these. Medium = should be addressed before the feature flag is turned on, but don't block merge. Low = follow-up cleanups.
🔴 High — block on these
H1. No backup path = guaranteed fund loss if the flag is ever enabled
Epic 3 (backup/mnemonic display) is explicitly out of scope, but this PR creates wallets that can receive Bitcoin. A QA tester or internal user flipping nonCustodialEnabled on a device with real sats has no recovery path if they uninstall or lose the device.
Minimum bar before merge:
- Code-gate this. Either (a) refuse to render Receive until
hasBackup === true, or (b) hard-blocknonCustodialEnabledfrom taking effect in mainnet builds until Epic 3 lands. - Document the constraint in the PR description as an explicit merge condition, not a soft norm.
This is the single highest-risk item in the PR.
H2. Network.Mainnet is hardcoded
const config = defaultConfig(Network.Mainnet)Combined with H1, this means QA cannot test this feature end-to-end without risking real funds. Parameterize via react-native-config:
- Add
BREEZ_NETWORKto env, parse with a whitelist (throw on unknown — don't default to mainnet). - Scope
storageDirby network (breez-mainnet/,breez-regtest/) — Breez persists local state to disk, and sharing directories across networks corrupts state. - Store network alongside the mnemonic (
{mnemonic, network, createdAt}) and refuse to load a wallet whose stored network doesn't match the current config. The mnemonic is BIP39-entropy and network-agnostic, but the wallet's interpretation of it is not. Silent network mismatches eat funds. - Add
if (__DEV__ && network === Mainnet) throwas a belt-and-suspenders guard against debug builds accidentally creating mainnet wallets.
This PR is the cheapest possible moment to add network metadata to stored mnemonics — there are no existing users to migrate.
H3. Mnemonic keychain accessibility is wrong
RNSecureKeyStore.set(MNEMONIC, mnemonic, { accessible: ACCESSIBLE.ALWAYS_THIS_DEVICE_ONLY })ALWAYS_THIS_DEVICE_ONLY is the weakest iOS protection class and is deprecated by Apple — the seed is readable when the device is locked. For a root seed this should be WHEN_UNLOCKED_THIS_DEVICE_ONLY (or AFTER_FIRST_UNLOCK_THIS_DEVICE_ONLY if background SDK sync is genuinely required). Add a test that asserts the exact constant so this can't silently regress.
H4. newArchEnabled=true is a whole-app platform change buried in a feature PR
Flipping New Architecture on Android affects every native module, not just Breez SDK. The PR description frames it as "SDK Integration" but the blast radius is the entire app. Before merge:
- Confirm
react-native-quick-crypto+react-native-nitro-modulesare on compatible versions (the architecture doc warns they're version-locked). - Full regression pass on a real Android device, not just CI unit tests — turbo module breakage is invisible to Jest.
- Clarify iOS New Arch status (already on, or does this silently diverge platforms?).
- Document the rollback plan: feature flag alone can't undo a native architecture change if a crash surfaces post-release.
Call this out as a platform change at the top of the PR description.
H5. The new home-screen self-custodial test doesn't actually test anything
it("skips GraphQL queries when self-custodial is active", async () => {
jest.doMock("@app/hooks/use-active-wallet", () => ({...}))
render(<HomeScreen />)
await act(async () => {})
})No expect, and jest.doMock after the top-level import doesn't re-mock the already-loaded module — the SUT is still using the real hook. This test passes regardless of behavior, which is worse than having no test. Replace with a top-level jest.mock and assert both the skip flags and the wallet data source. Add a negative test for the custodial path so the || isSelfCustodial addition can't flip the wrong way.
H6. Persistent state migration for activeAccountId
activeAccountId is added to persistent state with no migration for existing installs where the field is undefined. The root navigator's initialRouteName depends on this. Add:
- An explicit test with an old-shape persistent state fixture (no
activeAccountId) → assert it routes to the correct default, notundefined-crashes. - If your persistent-state system uses versioning, bump the version and add the migration.
This is a one-line regression waiting to happen for every existing user on first launch post-update.
🟡 Medium — address before the flag is enabled
M1. Wallet-creation orchestration is in the view layer
wallet-creation-screen.tsx directly calls selfCustodialCreateWallet(), writes activeAccountId via updateState, and calls KeyStoreWrapper.deleteMnemonic() in the catch block. The architecture doc is explicit that these are AccountLifecycleService responsibilities:
Token removal and mnemonic deletion are separate operations. Ordinary account switching must never call destructive lifecycle methods.
Extract useCreateSelfCustodialAccount() (or a service method) that owns: create → register active account → on failure, cleanup. Screen becomes a dumb view over {status, create}. Also fixes: swallowed error (currently catch {}), makes the crashlytics addition a one-line change, and keeps Send/Receive screens from copying the same pattern.
M2. home-screen.tsx branches on custody type
const isSelfCustodial = activeWallet.accountType === SelfCustodial && ...
// ...skip: !isAuthed || isSelfCustodial
// ...wallets = isSelfCustodial ? mapped : apollo
// ...if (isSelfCustodial || isAuthed)The spec is explicit: "Screens consume useActiveWallet()... without knowing the provider." Every Send/Receive/Convert screen will copy this pattern if it isn't fixed now. Add derived fields to useActiveWallet() (isReady, needsBackendAuth, etc.), move the wallet-shape mapping into the hook, and remove AccountType/ActiveWalletStatus imports from the screen entirely.
M3. SelfCustodialWalletProvider lifecycle — multiple latent bugs
- No debounce on
refreshWallets. Synced + PaymentSucceeded + ClaimedDeposits fire in rapid succession during initial sync → concurrentgetInfo/listPaymentsand racingsetWallets. Coalesce with a pending-flag ref or short debounce. - StrictMode double-invoke can start a second SDK init while the first
disconnectSdkis in flight. Add an abort token and explicit test under<StrictMode>. - Teardown doesn't await
disconnectSdk(can't — cleanups are sync). Combined with the above, thesdkRef.currentcan be the wrong instance. Guard against overlappinginitializeruns. - Event allowlist is hardcoded in the provider. Lock it in a test (
PaymentFailed/Optimization/LightningAddressChangedmust NOT trigger refresh) so future changes are conscious. - No
AppStateforeground handling. User backgrounds for 10 minutes, foregrounds, sees stale balance forever with no auto-refresh. Day-one bug. Architecture doc §6 explicitly requires reconnect on foreground (NFR13: backoff 1s/3s/9s). Minimum: foreground listener that callsrefreshWallets. Full backoff can wait.
M4. bridge.ts atomicity and splitting
- Atomicity:
setMnemonicsucceeds,connectfails → mnemonic is orphaned in keychain. The screen catches this today, but bridge-level atomicity is safer (deletes in its owncatch) and doesn't rely on every caller remembering. - Splitting:
bridge.tscurrently owns config building, SDK init/teardown, logging singleton, wallet-creation orchestration, and stable-balance activation. Split intosdk/config.ts,sdk/client.ts,wallet/create.ts,wallet/restore.ts. Matches the spec'sself-custodial/adapters/structure and prevents the file from becoming a 500-line dumping ground as Send/Receive land.
M5. getMnemonic silently returns "" on failure
public static async getMnemonic(): Promise<string> {
try { return await RNSecureKeyStore.get(MNEMONIC) }
catch { return "" }
}This conflates "wallet doesn't exist" with "keychain temporarily failed" — a transient error looks identical to "no wallet," which can silently drop a user to Unavailable (or worse). Change to Promise<string | null> and let callers distinguish. The provider's hasMnemonic → getMnemonic sequence is also a small race; call getMnemonic once and branch on null.
M6. Crashlytics on SDK init and wallet-creation paths
The bridge and provider only log to logSdkEvent (appears to be local-only). For a fund-custody feature:
- SDK init failures must hit crashlytics with context (flag state, mnemonic present, network).
wallet-creation-screen'scatch {}currently swallows the error entirely — at minimum log it.refreshWalletsfailures are logged but not reported; users will see stale balances with zero telemetry.
Without this, the first production incident will be untriageable.
M7. Feature-flag rollback behavior is unspecified
if (!hasMnemonic || !nonCustodialEnabled) { setStatus(Unavailable); return }If the flag is turned off remotely after wallets exist, users lose access to funds (SDK never initializes, incoming payments never surface). NFR15 says rollback must preserve self-custodial data — but preservation ≠ accessibility. Pick one and document/test:
- Rollback hides creation but existing wallets keep working, OR
- Rollback fully hides, data preserved until flag returns
Silent invisibility is the worst option for a fund-holding wallet.
M8. Breez SDK init is "eager" in name only
The architecture doc mandates eager init at launch within a 5s NFR1 budget. This PR inits inside SelfCustodialWalletProvider's useEffect — after React tree construction, after Apollo. Measure cold-start time-to-wallet-ready. If it's already near 5s with an empty wallet, it only gets worse with real tx history.
M9. noRetryOperations plumbing
The architecture doc: "Breez SDK payment operations must be added to noRetryOperations." Add the registration point now even if empty, so the first Send PR doesn't have to touch global Apollo config and get re-reviewed by a different team.
M10. Provider refresh pipeline is hard to test
refreshWallets is ~50 lines doing getInfo → token lookup → build wallets → listPayments → map → filter by currency → setState. Extract getSelfCustodialWalletSnapshot(sdk): Promise<WalletState[]> as a pure async function. Unit-testable without React, and the provider becomes just lifecycle.
M11. Critical provider test coverage missing
Beyond H5, the provider has thin tests for its highest-churn code. Add:
- Unmount during
initSdkin flight →disconnectSdkcalled, no state update, no warnings - StrictMode double-invoke → exactly one live SDK at the end
- Event handler fired after unmount → no-op
nonCustodialEnabled=falsewith mnemonic →initSdknever called- Retry → old SDK disconnected, new init runs
- Init failure → status
Error,sdkRef.current === null - Rapid event burst → coalesced correctly (whatever you decide in M3)
M12. Wallet-creation failure-mode coverage
Add: setMnemonic succeeds but updateUserSettings fails → deleteMnemonic called; deleteMnemonic itself throws → screen still reaches error state; unmount mid-creation → no updateState on an abandoned user; call-order assertion (updateState before navigate).
M13. secureStorage mnemonic test hardening
- Assert exact
accessibleconstant (locks H3) hasMnemonicreturns false on keychain missgetMnemonicdistinguishes missing vs error (will fail until M5 is fixed — that's the point)deleteMnemonicon already-absent key doesn't throw
🟢 Low — follow-up cleanups
L1. DefaultAccountId belongs in account-registry, not wallet.types.ts
Types should be dumb; IDs and factories belong in app/account-registry/ per the spec. TODO + follow-up is fine.
L2. Thin wrapper over KeyStoreWrapper for mnemonic access
A 10-line self-custodial/storage/mnemonic.ts interface decouples self-custodial code from a custodial-era utility name and makes test mocking a one-liner. Each current test mocks @app/utils/storage/secureStorage separately — that drift will grow.
L3. Extract resolveAccountTypeRoute({mode, custody}) as a pure function
Account-type selection screen encodes a routing table that will be referenced by deep-links and re-entry flows in Epic 3. Pure function, 4-row table test, reusable. Not this PR if time-constrained.
L4. Minor code hygiene
toUsdMoneyAmount(Number(usdBalance))—usdBalanceis alreadyNumber(...). Dead call.CreationStatusconst + type alias with same name — works, slightly confusing.initializeLoggingIIFE module singleton — a plain module-levellet initialized = falseis clearer and test-resettable.- Verify
toNumber/toWalletMoneyAmountare actually shared with custodial code. If not, move toself-custodial/utils/.
L5. Transaction mapper edge cases
Unknown PaymentDetails_Tags handling, zero-amount payments, missing tokenMetadata, Send vs Receive for each detail tag. Table-driven tests.
L6. Spec-compliance audit in PR description
Map which Story 2.x items explicitly defer which architectural components (AccountRegistry, AccountLifecycleService, useHasCustodialAccount, BackendFeatureGate, usePayments, useContacts, noRetryOperations). Helps the next reviewer not have to re-derive the gap.
L7. Account-type selection test (table-driven)
Four rows: {mode, custody} → expected route. Cheap, prevents an entire class of regressions.
L8. root-navigator initialRouteName test
Fixture with self-custodial id → Home; custodial token → Home; neither → GetStarted. One-line regression guard.
L9. Minor test quality
- Add a
renderWithProvidershelper — current tests re-mock@rn-vui/themed,i18n-react,testProps, navigation etc. in every file. Will drift. - Pick one SDK mock source. Currently both
__mocks__/@breeztech/...auto-mock AND per-testjest.mock(...)with different shapes exist — drift risk.
Summary
What I'd actually block merge on: H1–H6.
What I'd block the feature flag being enabled on: all Highs + all Mediums, especially M3 (provider lifecycle), M6 (crashlytics), M7 (rollback semantics).
The code-quality bones are good. The refactoring notes (M1, M2, M4, M10) matter mostly because every future screen (Send/Receive/Convert) will copy whatever patterns this PR establishes — so fixing them now is much cheaper than later.
But the shipping-safety items are what concern me most: real funds + no backup + mainnet hardcoded + new architecture flipped on is a lot of risk surface area for a single PR, even behind a feature flag. I'd push hard for H1 and H2 to be resolved before this lands, or for the PR description to explicitly commit that nonCustodialEnabled will not be turned on in any mainnet build until Epic 3 ships — enforced in code, not policy.
Nice work overall. The architecture alignment is clearly intentional and the test scaffolding sets a good precedent. Let's get the safety rails in place before wiring up the fund-moving screens on top.
esaugomez31
force-pushed
the
feat--spark-onboarding-wallet-creation
branch
from
7fdc7ee to
dac973f
Compare
esaugomez31
force-pushed
the
feat--spark-foundation-shared-abstractions
branch
from
860b015 to
5b2cb7e
Compare
esaugomez31
force-pushed
the
feat--spark-onboarding-wallet-creation
branch
from
dac973f to
292c460
Compare
esaugomez31
force-pushed
the
feat--spark-foundation-shared-abstractions
branch
from
5b2cb7e to
8368d8e
Compare
esaugomez31
force-pushed
the
feat--spark-onboarding-wallet-creation
branch
from
292c460 to
9d063f6
Compare
esaugomez31
force-pushed
the
feat--spark-foundation-shared-abstractions
branch
from
8368d8e to
a759b91
Compare
esaugomez31
changed the base branch from
feat--spark-foundation-shared-abstractions
to
graphite-base/3747
esaugomez31
force-pushed
the
feat--spark-onboarding-wallet-creation
branch
from
9d063f6 to
8660e92
Compare
esaugomez31
force-pushed
the
graphite-base/3747
branch
from
a759b91 to
3fe7201
Compare
esaugomez31
changed the base branch from
graphite-base/3747
to
feat--spark-foundation-shared-abstractions
PR — Review ResponseHigh — Block on theseH1. No backup path = guaranteed fund lossStatus: Done
// app/self-custodial/bridge.ts:56-60
if (!__DEV__) {
throw new Error("Wallet creation is disabled in production builds until backup flow is available")
}Test: H2.
|
Code review — cross-stack audit (#3731 → #3742 → #3746 → #3747)Reviewed all 4 stacked PRs for readability issues, code smells, and correctness concerns. Grouped by severity. None of these block merge, but they should be tracked and addressed before the feature flag is enabled. 🔴 CriticalC1. Swallowed error in C2. C3. Mnemonic returned through the call stack unnecessarily — 🟡 Major — readabilityR1. R2. Double-negation in if (isSelfCustodial || isAuthed) {
if (!isSelfCustodial && target === "receiveBitcoin" && ...)Checks R3. Unreadable compound conditional — if (
isSelfCustodial ||
!isIos ||
dataUnauthed?.globals?.network !== "mainnet" ||
levelAccount === AccountLevel.Two ||
...Mixes custody type, platform, network, and account level in one guard. Extract to a named boolean like R4. Nested ternary in R5.
The custodial counterparts are spelled out ( 🟡 Major — code smellsS1. S2. Cloud backup toast-only error handling — S3. Fire-and-forget checkpoint persistence — S4. Double silent catch in checkpoint storage — S5. Backup confirm screen coupled to GraphQL — S6. S7. S8. Summary
The most impactful pattern is custody-type awareness leaking into view code (R1-R3). Every Send/Receive/Convert screen will copy whatever patterns |
|
@grimen Please check Cross-stack audit responseCriticalC1. Swallowed error in
|
grimen
left a comment
grimen
left a comment
There was a problem hiding this comment.
All code review issues have been resolved. Approving.
…pshot consistency
grimen
force-pushed
the
graphite-base/3747
branch
from
c6482ce to
8afc3c3
Compare
grimen
force-pushed
the
feat--spark-onboarding-wallet-creation
branch
from
2d9af35 to
092375b
Compare
grimen
force-pushed
the
feat--spark-onboarding-wallet-creation
branch
from
092375b to
d6e7ff4
Compare
2 participants
Summary
Implements the self-custodial onboarding and wallet creation flow for Blink mobile, enabling new users to create a non-custodial wallet powered by Breez SDK (Spark).
What's done
SDK Integration (Story 2.1)
@breeztech/breez-sdk-spark-react-nativeandreact-native-fsnewArchEnabled=true) required by SDK turbo modulesKeyStoreWrapper(set, get, delete) withWHEN_UNLOCKED_THIS_DEVICE_ONLYaccessibilitygetMnemonicreturnsstring | null— eliminateshasMnemonic→getMnemonicrace, callers branch on nullmnemonic_networkkey) and validated on SDK init — network mismatch sets Error status and skips SDK initapp/self-custodial/config.ts— parsesBREEZ_NETWORKenv var with whitelist (mainnet,regtest), throws on unknown.storageDirscoped by network (breez-sdk-spark-mainnet/vsbreez-sdk-spark-regtest/)app/self-custodial/bridge.ts— SDK init, wallet creation (Promise<void>, mnemonic not exposed through promise chain), wallet restore. Bridge-level atomicity: on failure, deletes mnemonic + reports to crashlytics. Guards:!__DEV__blocks production,__DEV__ && Mainnetblocks mainnet in debug buildsapp/self-custodial/mappers/transaction-mapper.ts— maps Breez SDKPaymenttoNormalizedTransactionusingtoWalletMoneyAmountconsistentlyapp/self-custodial/providers/wallet-snapshot.ts— pure asyncgetSelfCustodialWalletSnapshot(sdk)function, usestoWalletMoneyAmountconsistentlyapp/self-custodial/providers/use-sdk-lifecycle.ts— SDK lifecycle hook: init, event subscription, refresh coalescing (refreshingRef+pendingRefreshRef), AppState foreground listener, network validation, crashlytics error reportingapp/self-custodial/providers/wallet-provider.tsx— thin context provider shell (53 lines) consuminguseSdkLifecycleSelfCustodialWalletProviderinto the app component treetoNumber(BigInt conversion) andtoWalletMoneyAmounthelperscreateSdkLogListenerpattern forinitLoggingnoRetryOperationsregistration point in Apollo client for future self-custodial payment operationsAccount Type Selection (Story 2.2)
app/screens/account-type-selection/with two-card grid (Custodial/Non-custodial) matching Figma designmodeparam: create → T & C → wallet creation, restore custodial → login, restore self-custodial → "Coming soon" placeholder (Epic 3)GetStartedScreento route through account type selection whennonCustodialEnabledfeature flag is activeLL.AccountTypeSelectionScreen.*with translations across 28 languagesWallet Creation Flow (Story 2.3)
app/screens/spark-onboarding/wallet-creation-screen.tsx— pure UI over{status, create}, no business logicuseCreateWallethook — owns create → register active account → on failure, crashlytics report + error status.CreationStatusexported as named constactiveAccountIdtoDefaultAccountId.SelfCustodial, navigates to HomeDefaultAccountIdconstants toapp/types/wallet.types.tsderived fromAccountTypeHome Screen Self-Custodial Support (Story 2.4)
useActiveWallet()returns derived fields:isReady,isSelfCustodial,needsBackendAuth— screens consume without importingAccountType/ActiveWalletStatusresolveBaseStatepure function with sequentialifguards (no nested ternaries)useHomeAuthedQuery,useRealtimePriceQuery) whenisSelfCustodialactiveWallet.walletsforuseTotalBalanceshouldShowTransferButtonnamed boolean (was inline compound conditional)onMenuClickwith early-return pattern (was double-negation)as anycast by addingconversionDetailstoTargettype unionAccountCreationNeededModal)initialRouteNameinroot-navigator.tsxto checkpersistentState.activeAccountIduseSelfCustodialRollbackhandles UI visibility, data preservedInfrastructure
CloudIconto galoy-icon component@breeztech/breez-sdk-spark-react-native(withNetworkenum values),react-native-fs, andreact-native-config(withBREEZ_NETWORK=regtest)false(controlled via Firebase Remote Config)Cross-stack audit fixes (from #3731, #3742, #3746)
Changes addressing code smells identified during cross-stack review that were consolidated under this PR:
use-cloud-backup.tsnow report to crashlytics (was toast-only)setTimeout+ auto-navigation that unconditionally faked success after 2s. Screen now renders pending state without navigatingsaveCheckpointToStorageerrors now caught and reported to crashlytics.loadCheckpointcatch block also reports to crashlytics (was double silent catch)backup-confirm-screen.tsxusesuseActiveWallet()instead ofuseHomeAuthedQuery— works in both custodial and self-custodial modeuseHasCustodialAccount(zero consumers, just wrappeduseIsAuthed)usePaymentsadapters (claimDeposit,listPendingDeposits,convert) changed from required (with runtimethrow UnsupportedOperationError) to optional (undefined) — consistent with other adaptersscAccount→selfCustodialAccount,scReady→selfCustodialReady,scUnavailable→selfCustodialUnavailablePending (not in scope)
useWalletMnemoniccurrently returns mock data — wiring to real keychain mnemonic is a separate ticket