feat(spark): account migration to non-custodial UI screens#3742
Conversation
b5871cf to
de381d6
Compare
3116ae5 to
510ce0b
Compare
grimen
left a comment
There was a problem hiding this comment.
Solid layering overall — MigrationExplainerLayout + StatusScreenLayout + useMigrationCheckpoint is the right decomposition, and the checkpoint test volume is reasonable. That said, because this PR is stacked on #3731 and because the checkpoint system introduces a new persistence contract that will outlive this PR, I'd like a few things tightened before merge.
Most of the feedback is refactoring that extracts pure logic out of hooks so it becomes exhaustively testable — which is also how most of the blocking items become trivial to fix.
🔴 Blocking
1. This PR inherits the blocking items from #3731.
3742 is based on 3731, which I've requested changes on (format lock-in, IV length, Drive error handling, etc.). Those need to land first — either by merging a fixed 3731 and rebasing, or by cherry-picking the fixes into this branch. Not re-raised here.
2. Checkpoint expiration doesn't match the PR description — code has a single 48h constant. — use-migration-checkpoint.ts:6
const EXPIRATION_MS = 48 * 60 * 60 * 1000The PR description says "1h for backup method selection, 24h for other steps". The code applies a flat 48h. Pick one, fix the other. Express the policy as a config object keyed by step so it's reviewable in one place:
// migration-config.ts
export const CHECKPOINT_EXPIRATION_MS = {
[MigrationCheckpoint.BackupMethod]: 60 * 60 * 1000, // 1h
[MigrationCheckpoint.CloudBackup]: 24 * 60 * 60 * 1000, // 24h
[MigrationCheckpoint.BackupAlerts]: 24 * 60 * 60 * 1000, // 24h
} as constPin with a table-driven test across { step, age, expectedExpired }.
3. loadJson failure leaves loading: true forever. — use-migration-checkpoint.ts:37
loadJson(STORAGE_KEY).then((stored) => { ... })No .catch. If storage throws (corrupt JSON, permission, any reason), the settings row spins indefinitely and the user can't enter the migration flow. Add a .catch that clears the key and resolves to loading: false, plus a test that uses mockRejectedValue — the current suite only ever uses mockResolvedValue.
4. Unmount race in the checkpoint hook. — same effect
.then calls setCheckpoint / setLoading without a cancellation guard. Fast tab switches trigger a React warning and — more importantly — a stale load can clobber a fresh one. Standard fix: isMounted ref or an abort flag.
5. Resuming a checkpoint doesn't pass route params. — useMigrationCheckpoint.getRouteForCheckpoint
return checkpointRouteMap[checkpoint] // -> just a route nameAt least one backup screen from #3731 takes params like { step: PhraseStep.First } or { challenges: [...] }. Resuming without params either crashes, silently takes defaults, or shows broken state. Two concrete fixes:
- Either restrict checkpoints to parameter-less screens only (and document which qualify), or
- Have the hook return
{ name, params }and persist params alongside the step.
Without this, the "resume from last visited step" feature is half-implemented.
6. iOS resume to sparkCloudBackupScreen is broken.
The checkpoint map includes CloudBackup → sparkCloudBackupScreen, but in #3731 handleCloudBackup early-returns on iOS. On iOS the resume target is a dead screen. getRouteForCheckpoint should validate that the resumed route is reachable on the current platform/config and fall back to DEFAULT_ROUTE otherwise. This, combined with #5, is what pushes route resolution out of being a lookup:
// checkpoint-route-resolver.ts
export type ResumeTarget = { name: MigrationRoute; params?: object }
export const resolveCheckpointRoute = (
checkpoint: MigrationCheckpointType | null,
ctx: { platform: 'ios' | 'android'; /* env, flags, ... */ }
): ResumeTarget => ...Pure, switch-statement, exhaustively unit-testable.
7. Checkpoint storage key is not namespaced by environment. — STORAGE_KEY = "migrationCheckpoint"
#3731 already namespaces Drive filenames and keychain services by galoyInstance.name (staging / mainnet / signet). This checkpoint must too — otherwise switching environments mid-flow resumes into the wrong network. Follow the same getSparkKeychainService / getSparkDriveBackupFilename pattern — and collocate all three in a single storage-ids.ts module (blocking #8) so future Spark persistence has an obvious home.
8. Extract checkpoint persistence into a pure module → migration-checkpoint-storage.ts.
useMigrationCheckpoint currently mixes three concerns: AsyncStorage I/O, validation + expiration logic, React state. Pull the first two out:
export type StoredCheckpoint = { step: MigrationCheckpointType; savedAt: number }
export const loadCheckpoint = async (key: string): Promise<StoredCheckpoint | null> => ...
export const saveCheckpoint = (key, step) => ...
export const clearCheckpoint = (key) => ...
export const isExpired = (checkpoint, now, policy) => ...
export const validateStoredCheckpoint = (raw: unknown) => ...The hook becomes ~15 lines of React wiring around these functions. This is the single biggest structural win in the PR because:
- Per-step expiration (#2) becomes a pure function you can table-test exhaustively.
- Storage failure handling (#3) is testable with a rejected mock at module level.
- Unmount race (#4) isolates cleanly in the hook layer.
- Route resolution (#6) and route map (#5) collocate with the types they operate on.
- The test suite stops coupling to
renderHookand storage mocks.
The rest of the blockers get dramatically smaller once this lands.
🟡 Non-blocking (strong suggestions)
9. Checkpoint is only cleared on success.
If the user exits mid-flow and abandons migration, there's no "cancel" / "start over" affordance — they're stuck resuming until expiration. Consider either (a) a cancel CTA on the explainer when a checkpoint exists, or (b) clearing on certain navigation events (e.g. tapping back from the explainer).
10. Hardcoded SPARK_LEARN_MORE_URL = "https://spark.info" — explainer-screen.tsx:13
Placeholder domain. Same pattern as #3731's sparkCompatibleWalletsUrl — drive it from useRemoteConfig so it can be changed without a release.
11. openExternalUrl() helper.
explainer-screen.tsx uses bare Linking.openURL. #3731's use-backup-phrase.ts uses InAppBrowser.open(...).catch(() => Linking.openURL(...)). Two different behaviors for "open an external URL" in the same feature area. Extract once and use everywhere:
export const openExternalUrl = (url: string) =>
InAppBrowser.open(url).catch(() => Linking.openURL(url))12. MoveToNonCustodialSetting mounts the full checkpoint hook just to compute a route.
The settings row uses only loading + getRouteForCheckpoint. The explainer screen and every backup screen also each mount their own useMigrationCheckpoint — N independent storage reads per session. Two improvements:
- Wrap the migration stack in a
MigrationCheckpointProviderso all screens share one instance. - For the settings row, expose a narrower
useMigrationResumeTarget()returning just{ loading, target }.
13. Rich-text with embedded link → shared <RichText> component. — explainer-screen.tsx
Second time after #3731's importantMessage that a screen needs "paragraph with an inline styled span." Explainer step 1 does it with a fragment + nested <Text onPress>, forcing translators to split a single sentence into ...Step1 + ...Step1Link keys whose order and punctuation must be correct across 28 languages — and translators can't control where the link lives in the sentence. Build once and share with #3731:
<RichText
text={LL.AccountMigration.explainerStep1({ link: "<learn>Spark</learn>" })}
handlers={{ learn: handleLearnMore }}
/>14. NumberedStepsList → standalone component.
Currently buried in MigrationExplainerLayout. Reusable primitive ("numbered list with rich-text per step") that will reappear in onboarding / tutorial flows. Extract and let MigrationExplainerLayout compose IconHero + NumberedStepsList + CTA.
15. MigrationExplainerLayout and StatusScreenLayout are near-duplicates.
Both are "icon + body + optional footer." The container / content / footer styles are duplicated. Consider unifying into a single ScreenWithIconLayout. If that's too much churn for this PR, at minimum share the style blocks.
16. StatusScreenLayout conditional icon wrapping is awkward. — status-screen-layout.tsx:24
{iconBackgroundColor ? (
<View style={styles.iconCircle}><GaloyIcon ... /></View>
) : (
<GaloyIcon ... />
)}Always wrap in <View style={styles.iconCircle}>; the background color defaults to transparent when not set. Removes the branch and keeps layout consistent.
17. The StatusScreenLayout "warning" color coupling.
_warningLight: "#FFF9E5" is passed as the transferring funds background. That's not warning semantics, it's "pending/loading." Either rename the token (_statusLight, _pendingLight) or pick a different token. Otherwise future warning UIs will adopt the same color expecting actual warning semantics.
🧪 Test coverage gaps
Volume is fine (27 tests / 5 suites), but several gaps map directly onto the blocking items above — which suggests the tests are coupled to the implementation rather than the spec:
- Per-step expiration is untested (blocking #2). Only one step is exercised against the 48h boundary. A table-driven
{ step, age, expectedExpired }test would have caught the description/code mismatch. loadJsonrejection is untested (blocking #3). Every test usesmockResolvedValue.- Unmount mid-load is untested (blocking #4).
- Route mapping is only asserted for
BackupMethod—CloudBackupandBackupAlertsare untested; a typo in either would ship. Trivial table test. - No iOS-platform behavior for
CloudBackupcheckpoint (blocking #6). - Malformed
savedAt(missing / non-number) is untested — code handles it, spec doesn't pin it. MigrationExplainerLayouthas no standalone spec despite being pitched as reusable across migration flows.StatusScreenLayoutconditional icon-circle branch — 3 tests, no assertion for theiconBackgroundColor: undefinedpath.- No integration test for the full "save → unmount → remount → resume to correct route" contract. Each piece is isolated; the seam between them isn't.
- Backup screen spec updates are mock-only (~20 lines each) — no assertion that
saveCheckpointis called with the right step when those screens mount, which is the whole reason for the changes.
The refactor in blocking #8 resolves most of these for free — isExpired(checkpoint, now, policy) and validateStoredCheckpoint(raw) can be table-tested exhaustively without renderHook or storage mocks. The tests will also stop breaking on future per-step changes.
Currently the 47h / 49h boundary test is coupled to the 48h constant — meaning fixing #2 will break the test in a misleading way (failure because 47h is no longer valid for BackupMethod, not because the new behavior is wrong). That's a sign the test is at the wrong level.
🟢 Low / nits
MigrationCheckpointenum values are strings ("backupMethod", etc.) and now part of the persisted contract. Add a// do not rename: persistedcomment so a future refactor doesn't silently break stored checkpoints.steps.map((content, index) => ... key={index})inMigrationExplainerLayout— acceptable for a static list, nit.IconHerowidth fix is a drive-by that affects #3731's screens. Reasonable, but worth eyeballing on-device before merge since existing callsites relied on the fixed width.- Settings row shows a spinner during checkpoint load — if the load is fast users see a flash. Render normally until load completes or debounce the spinner.
sparkMigrationExplainerroute name as a string literal inDEFAULT_ROUTE— would be safer as a constant imported from the navigator to catch renames at compile time.GaloyIcon name="clock"— new icon registration, make sure the icon asset is also committed.
Summary. The blockers break into three groups:
- Checkpoint correctness (#2, #3, #4, #5, #6, #7): expiration policy, storage failure, unmount race, params, platform-awareness, namespacing. Each one is small; together they define the persistence contract.
- Structural extraction (#8): pure checkpoint-storage module. Makes all six of the above trivial to fix and test, and establishes the pattern for future storage.
- Inheritance from #3731 (#1): must land first.
Collectively ~half a day of focused work, not a rewrite. The non-blocking and low items can absolutely land in follow-ups.
de381d6 to
8c43d96
Compare
554835c to
88c7729
Compare
88c7729 to
a043417
Compare
PR — Review ResponseBlockingB1: This PR inherits the blocking items from #3731Resolved. #3731 has been merged and this branch rebased on top of it. B2: Checkpoint expiration doesn't match the PR descriptionResolved. The intended behavior is a uniform 48-hour expiration for all checkpoint steps — the PR description was wrong, not the code. Updated the PR description to reflect this. The B3:
|
| Gap | Resolution |
|---|---|
| Per-step expiration untested | 6 boundary tests across all 3 steps (24h/47h/1h not expired, 49h expired) |
loadJson rejection untested |
mockRejectedValue test — returns null + clears storage |
| Unmount mid-load untested | Test with deferred promise + unmount before resolve |
| Route mapping only BackupMethod | Tests for all 3 checkpoints + null → default |
| No iOS CloudBackup behavior | Test sets Platform.OS = "ios" → returns default route |
Malformed savedAt untested |
Tests for missing savedAt and non-number savedAt |
MigrationExplainerLayout no spec |
5 tests (title, steps, step numbers, CTA button, CTA press) |
StatusScreenLayout icon branch |
Branch eliminated — always wraps |
| No integration test | save → unmount → remount → resume test in hook spec |
| Backup screens no saveCheckpoint assertion | Added assertions in backup-method and backup-alerts specs |
Coverage: 100% statements, branches, and lines on all changed files (72 tests across 10 suites).
grimen
left a comment
There was a problem hiding this comment.
All blocking and non-blocking feedback addressed. Deferred items have reasonable justifications. LGTM.
05c9e5f to
d54fdd6
Compare
76a475b to
baf3f14
Compare
## 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)
- Installed `@breeztech/breez-sdk-spark-react-native` and `react-native-fs`
- Enabled New Architecture (`newArchEnabled=true`) required by SDK turbo modules
- Added mnemonic secure storage methods to `KeyStoreWrapper` (set, get, delete) with `WHEN_UNLOCKED_THIS_DEVICE_ONLY` accessibility
- `getMnemonic` returns `string | null` — eliminates `hasMnemonic` → `getMnemonic` race, callers branch on null
- Network metadata stored alongside mnemonic (`mnemonic_network` key) and validated on SDK init — network mismatch sets Error status and skips SDK init
- Created `app/self-custodial/config.ts` — parses `BREEZ_NETWORK` env var with whitelist (`mainnet`, `regtest`), throws on unknown. `storageDir` scoped by network (`breez-sdk-spark-mainnet/` vs `breez-sdk-spark-regtest/`)
- Created `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__ && Mainnet` blocks mainnet in debug builds
- Created `app/self-custodial/mappers/transaction-mapper.ts` — maps Breez SDK `Payment` to `NormalizedTransaction` using `toWalletMoneyAmount` consistently
- Created `app/self-custodial/providers/wallet-snapshot.ts` — pure async `getSelfCustodialWalletSnapshot(sdk)` function, uses `toWalletMoneyAmount` consistently
- Created `app/self-custodial/providers/use-sdk-lifecycle.ts` — SDK lifecycle hook: init, event subscription, refresh coalescing (`refreshingRef` + `pendingRefreshRef`), AppState foreground listener, network validation, crashlytics error reporting
- Created `app/self-custodial/providers/wallet-provider.tsx` — thin context provider shell (53 lines) consuming `useSdkLifecycle`
- Wired `SelfCustodialWalletProvider` into the app component tree
- Added shared `toNumber` (BigInt conversion) and `toWalletMoneyAmount` helpers
- Updated SDK logging to `createSdkLogListener` pattern for `initLogging`
- Added `noRetryOperations` registration point in Apollo client for future self-custodial payment operations
- 10 new test suites covering bridge, mainnet guard, config (network parsing, storageDir scoping), provider, wallet snapshot, mapper, logging, helpers, and secure storage (mnemonic + network metadata)
### Account Type Selection (Story 2.2)
- Created `app/screens/account-type-selection/` with two-card grid (Custodial/Non-custodial) matching Figma design
- Routes based on `mode` param: create → T & C → wallet creation, restore custodial → login, restore self-custodial → "Coming soon" placeholder (Epic 3)
- Wired `GetStartedScreen` to route through account type selection when `nonCustodialEnabled` feature flag is active
- All strings via `LL.AccountTypeSelectionScreen.*` with translations across 28 languages
- 8 tests covering all navigation paths, selection states, and edge cases
### Wallet Creation Flow (Story 2.3)
- Created `app/screens/spark-onboarding/wallet-creation-screen.tsx` — pure UI over `{status, create}`, no business logic
- Extracted `useCreateWallet` hook — owns create → register active account → on failure, crashlytics report + error status. `CreationStatus` exported as named const
- On success: sets `activeAccountId` to `DefaultAccountId.SelfCustodial`, navigates to Home
- On failure: bridge handles mnemonic cleanup atomically, hook reports to crashlytics
- Added `DefaultAccountId` constants to `app/types/wallet.types.ts` derived from `AccountType`
- 6 tests covering creation, navigation, error handling, non-Error rejection wrapping
### Home Screen Self-Custodial Support (Story 2.4)
- `useActiveWallet()` returns derived fields: `isReady`, `isSelfCustodial`, `needsBackendAuth` — screens consume without importing `AccountType`/`ActiveWalletStatus`
- `resolveBaseState` pure function with sequential `if` guards (no nested ternaries)
- Skips GraphQL queries (`useHomeAuthedQuery`, `useRealtimePriceQuery`) when `isSelfCustodial`
- Maps wallet data from `activeWallet.wallets` for `useTotalBalance`
- Extracted `shouldShowTransferButton` named boolean (was inline compound conditional)
- Simplified `onMenuClick` with early-return pattern (was double-negation)
- Removed `as any` cast by adding `conversionDetails` to `Target` type union
- Self-custodial users navigate directly (no `AccountCreationNeededModal`)
- Fixed `initialRouteName` in `root-navigator.tsx` to check `persistentState.activeAccountId`
- Feature-flag rollback: provider initializes SDK regardless of flag state — `useSelfCustodialRollback` handles UI visibility, data preserved
- 4 tests for custodial, self-custodial, unavailable states, and rollback safety
### Infrastructure
- Added `CloudIcon` to galoy-icon component
- Added global Jest mocks for `@breeztech/breez-sdk-spark-react-native` (with `Network` enum values), `react-native-fs`, and `react-native-config` (with `BREEZ_NETWORK=regtest`)
- Feature flag defaults set to `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:
- **Crashlytics in cloud backup** (#3731): Sign-in and upload failures in `use-cloud-backup.ts` now report to crashlytics (was toast-only)
- **TransferringFundsScreen fake success** (#3742): Removed `setTimeout` + auto-navigation that unconditionally faked success after 2s. Screen now renders pending state without navigating
- **Checkpoint persistence** (#3742): `saveCheckpointToStorage` errors now caught and reported to crashlytics. `loadCheckpoint` catch block also reports to crashlytics (was double silent catch)
- **Backup confirm layer violation** (#3742): `backup-confirm-screen.tsx` uses `useActiveWallet()` instead of `useHomeAuthedQuery` — works in both custodial and self-custodial mode
- **Dead abstraction** (#3746): Removed `useHasCustodialAccount` (zero consumers, just wrapped `useIsAuthed`)
- **Inconsistent payment adapters** (#3746): `usePayments` adapters (`claimDeposit`, `listPendingDeposits`, `convert`) changed from required (with runtime `throw UnsupportedOperationError`) to optional (`undefined`) — consistent with other adapters
- **Abbreviated test names** (#3746): Renamed `scAccount` → `selfCustodialAccount`, `scReady` → `selfCustodialReady`, `scUnavailable` → `selfCustodialUnavailable`
## Pending (not in scope)
- FR1: Self-custodial visually recommended badge — no design in Figma
- FR4: App Check device verification for self-custodial — needs product decision on what to do when device verification fails
- FR44: Stale balance indicator when SDK offline — no Figma design yet
- Backup/mnemonic display — Epic 3 (#648)
- `useWalletMnemonic` currently returns mock data — wiring to real keychain mnemonic is a separate ticket
- M8 (eager SDK init) — architectural change, deferred

Summary
Implements the UI screens for the Custodial → Non-custodial migration flow as described in #636. This is a UI-only implementation — backend integration is tracked separately.
Screens implemented
New components
app/components/status-screen-layout/) — Generic centered layout with icon + text for loading/status screens. Accepts optionaliconBackgroundColorfor themed icon circles.app/screens/account-migration/migration-explainer-layout.tsx) — Reusable layout for explainer screens with icon, title, numbered steps list, and CTA button.app/components/numbered-steps-list/) — Reusable numbered list component extracted from MigrationExplainerLayout.app/screens/account-migration/hooks/) — Thin React wrapper around a pure persistence module (migration-checkpoint-storage.ts). Persists flow progress in AsyncStorage with a uniform 48-hour expiration for all checkpoint steps. Validates stored data on load, clears expired/corrupt checkpoints, and includes unmount-race protection.app/screens/account-migration/utils/) — Pure module (no React) for checkpoint persistence: load, save, clear, validate, expiration check, and route resolution. Environment-namespaced storage keys. iOS-aware route fallback for CloudBackup.app/utils/external.ts) — Shared helper: InAppBrowser with Linking fallback. Replaces duplicate patterns in explainer and backup-phrase screens.Theme additions
_warningLight: "#FFF9E5"— Static color for warning/loading icon backgroundsClockIconregistered in GaloyIconNavigation flow
Checkpoint system
i18n
AccountMigrationnamespace with 8 keys translated across all 28 languagesExisting component improvements
width: 264to respect parent paddingTest coverage