Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a seedless-wallet lockout scenario by reconciling persisted Redux wallet state with keychain/secure storage on launch and by recovering gracefully when the wallet secret is missing after PIN verification, routing the user back to onboarding instead of stranding them on the PIN screen.
Changes:
- Added a keychain “wallet credential exists” probe (
BiometricsSDK.hasWalletData) and unit tests for it. - Updated app launch initialization to reconcile persisted wallet state vs. secure storage and trigger logout/onboarding when credentials are missing.
- Added
ensureWalletSecrethelper + tests and integrated it intoPinScreento delete/recover when wallet secret is missing post-PIN verification.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/core-mobile/app/utils/BiometricsSDK.ts | Adds hasWalletData() to detect whether any wallet-unlocking credential still exists in keychain (fail-safe on errors). |
| packages/core-mobile/app/utils/BiometricsSDK.test.ts | Adds unit tests validating hasWalletData() behavior (PIN, legacy entries, none, and error/fail-safe). |
| packages/core-mobile/app/store/app/listeners.ts | Reconciles persisted walletState against keychain on launch; logs out to onboarding when credentials are missing. |
| packages/core-mobile/app/store/app/listeners.test.ts | Adds unit tests covering launch-time wallet-state reconciliation behavior. |
| packages/core-mobile/app/new/common/utils/ensureWalletSecret.ts | Adds helper to abort login and trigger recovery when wallet secret is missing/un-decryptable after PIN/biometry verification. |
| packages/core-mobile/app/new/common/utils/ensureWalletSecret.test.ts | Adds unit tests for ensureWalletSecret success/failure paths. |
| packages/core-mobile/app/new/common/components/PinScreen.tsx | Uses ensureWalletSecret + useDeleteWallet to recover to onboarding when wallet secret is missing after successful PIN entry. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Coverage report ✅2/2 packages passed thresholds 🟢 @avalabs/core-mobile
🟢 @avalabs/k2-alpine
Artifacts and threshold sources
Source run: Mobile PR |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
packages/core-mobile/app/new/common/utils/ensureWalletSecret.ts:17
isTerminalSecretErrorcurrently treats only missing-credential and bad-decrypt message strings as terminal. Howeverdecrypt(...)can also fail deterministically withNoSaltError/InvalidVersionError(corrupt secret data), which will never succeed on retry and would leave the user stuck on the PIN screen becauseensureWalletSecretrethrows it as “transient”. Consider treating these error types (byerror.name) as terminal and triggeringonMissingthe same way.
const isTerminalSecretError = (error?: Error): boolean => {
const message = error?.message ?? ''
return (
message.includes('No credentials found') ||
message.includes('Failed to decrypt') ||
message.includes('BAD_DECRYPT') || // Android bad-decrypt
message.includes('Decrypt failed') // iOS bad-decrypt
)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
packages/core-mobile/app/new/common/components/PinScreen.tsx:166
- When
ensureWalletSecret(...)returnsfalse(terminal missing/corrupt secret), this earlyreturnleavesisProcessingset totrueand the loading animation running. If the logout/onboarding navigation is delayed for any reason, the user can be left staring at a spinner with no way to retry. Consider clearing the processing/loading UI state before returning, similar to the catch-path cleanup.
const canProceed = await ensureWalletSecret(walletId, deleteWallet)
if (!canProceed) {
// Wallet secret is gone; deleteWallet routes the user to onboarding.
return
}
bogdandobritoiu
left a comment
bogdandobritoiu
left a comment
There was a problem hiding this comment.
Nice mechanism overall — the three-state loadEncryptionKeyWithPin result, the terminal-vs-transient split in ensureWalletSecret, and the stuck-spinner recovery are all solid, and all three callers were updated consistently. One trace I'd like to resolve before this merges though (first inline comment): on a cold start the migrator appears to intercept the wiped-keychain state before the new no-credentials branch can run.
Test suggestion: a hook-level test for usePinOrBiometryLogin starting from the full corrupt state (isInitialLogin: true, with KeychainMigrator.getMigrationStatus resolving as it would with a wiped keychain) would pin down the end-to-end routing — that's the test that would catch the migration-preemption issue if it repros.
| if (!isValidPin) { | ||
| const pinResult = await BiometricsSDK.loadEncryptionKeyWithPin(pin) | ||
|
|
||
| if (pinResult === 'no-credentials') { |
There was a problem hiding this comment.
I traced the cold-start flow and I think this branch may be unreachable in the ticket's main scenario.
On a cold launch in the corrupt state, RootNavigator renders loginWithPinOrBiometry (shouldRenderOnlyPinScreen starts true), which mounts PinScreen with isInitialLogin={true}. So checkEnteredPin runs migrator.migrateIfNeeded('PIN', pin) before loadEncryptionKeyWithPin:
getMigrationStatus('PIN'):hasEncryptionKeyWithPin()→ false,hasEncryptionKeyWithBiometry()→ false → returnsRunPinMigration(orCompletePartialMigrationif the bio key survived the partial wipe — same outcome below).migrateIfNeeded→throwIfBadPin(pin)→isPinCorrect(pin, isLegacy=true)→loadLegacyWalletWithPin()readsLEGACY_SERVICE_KEY_BIO, which never existed on a modern (non-legacy) install →{ success: false }→ throwsBadPinError('Bad PIN').- The catch below treats
BadPinErroras a wrong PIN →increaseAttempt()+onWrongPin()— i.e. the endless "wrong PIN" loop from the ticket, unchanged.loadEncryptionKeyWithPinnever runs, sono-credentialsnever fires.
Since loadEncryptionKeyWithPin is only reached when getMigrationStatus returns NoMigrationNeeded (which requires the encryption key to exist), on the initial-login path this branch looks reachable only via a race between hasGenericPassword and the subsequent getGenericPassword. It is reachable from the in-session flows (PinScreenOverlay, VerifyPin) where migration is skipped — but those aren't the cold-start case.
Could you re-verify variant A on a cold start with a wallet that never had legacy keychain entries? If the test device had legacy entries (migration would silently recreate the key and unlock), or the test went through the in-session overlay, that would explain the passing dev test.
If it repros, one possible fix: have getMigrationStatus distinguish "no new keys and nothing legacy to migrate from" as its own terminal status, and route that to the same delete-and-onboard recovery instead of letting throwIfBadPin translate it into a rate-limited wrong PIN.
There was a problem hiding this comment.
Confirmed — I reproduced your trace exactly: on a cold start with a wiped keychain, getMigrationStatus returned RunPinMigration, throwIfBadPin → loadLegacyWalletWithPin found no LEGACY_SERVICE_KEY_BIO entry and translated it into BadPinError → the rate-limited wrong-PIN loop. The no-credentials branch was unreachable on the cold-start path; only the in-session flows (or a read race) could hit it.
Fixed as you suggested (abd78a1): getMigrationStatus now returns a terminal MigrationStatus.NoKeychainData when there are no new keys and nothing legacy to migrate from, and migrateIfNeeded surfaces it without attempting a migration. Both the PIN and BIO initial-login paths route it to delete-and-onboard recovery (behind an acknowledge alert — see the other thread).
The "nothing legacy" check is a new strict BiometricsSDK.hasAnyWalletData() probe that throws on transient keychain failures instead of returning false, so a flaky read can never present as a wiped wallet. Verified against react-native-keychain's native sources: iOS hasCredentialsWithSecClass resolves true even on errSecInteractionNotAllowed (present-but-locked, so a bio-protected entry never reads as absent) and rejects any unexpected OSStatus; Android reads the DataStore prefs entry rather than touching the keystore.
Also added the hook-level test you suggested: usePinOrBiometryLogin.test.ts renders the hook with isInitialLogin: true in the full corrupt state and pins the end-to-end routing — recovery alert + deleteWallet on acknowledge, no onWrongPin, no rate-limiter increment, loadEncryptionKeyWithPin never reached; plus the wrong-pin/success/BIO variants.
One related hardening that fell out of an adversarial pass over this branch: the migrations now write the wallet secret before the new encryption keys (and runPinMigration's redundant post-store reload-assert is gone), so an app death mid-migration leaves the migration re-runnable from legacy data instead of a key-without-secret state that would read as a wiped wallet after unlock. ensureWalletSecret additionally refuses destructive recovery while legacy entries still exist.
| new Error('no-credentials') | ||
| ) | ||
| onStopLoading() | ||
| deleteWallet() |
There was a problem hiding this comment.
This hook is also used by VerifyPin and VerifyWithPinOrBiometry (reveal recovery phrase, change PIN, etc.), where isInitialLogin is false — so this deleteWallet() fires in in-session verification contexts too, with no user-facing message: the user types their PIN in settings and abruptly lands on onboarding.
Consider gating this recovery to isInitialLogin, or at minimum showing an alertBadData-style alert (explain + acknowledge) before deleting, like the corrupt-data path already does.
There was a problem hiding this comment.
Addressed in abd78a1, in both directions:
- Every recovery deletion is now behind an
alertBadData-style acknowledge alert ("Wallet data not found" → Okay → delete), so an in-sessionVerifyPincan never silently drop the user into onboarding. - The in-session case you pointed at turned out to be genuinely destructive, not just jarring: a partially completed biometric migration (
runBiometricMigrationintentionally leaves the new bio key with no PIN key until the next initial PIN login — which for a biometrics user can be arbitrarily far away) reads asno-credentialsfromVerifyPin, and deleting there would wipe the legacy entriescompletePartialMigrationstill needs. Theno-credentialsbranch is now gated on the stricthasAnyWalletData()probe: recovery is only offered when the keychain is provably empty; if any other credential exists (bio key, legacy entries) it surfaces as a failed PIN check — the pre-CP-14585 behavior for that state — rather than an offer to delete.
Both cases are covered in the new usePinOrBiometryLogin.test.ts (empty keychain → alert-gated recovery; partial-bio state → no alert, no delete, wrong-PIN handling).
I kept recovery available in-session (rather than gating on isInitialLogin) because when the keychain is provably empty the wallet can never be unlocked by any flow, and the alert now makes that explicit wherever the user happens to discover it.
| setIsEnteringPin(true) | ||
| }, [disableKeypad]) | ||
|
|
||
| const handleLoginSuccess = useCallback(() => { |
There was a problem hiding this comment.
nit: handleLoginSuccess now has a wider dep list (e.g. focusPinInput changes with disableKeypad), and the useEffect(() => { if (verified) handleLoginSuccess() }) re-fires on any identity change while verified is still true — which would schedule unlock() a second time. Likelihood is low (the zustand fns are stable refs), but an early-return guard on isProcessing.value at the top of this callback would close it cheaply.
There was a problem hiding this comment.
Added — handleLoginSuccess now early-returns when isProcessing.value is already true, so a callback-identity change while verified is still true can't schedule a second unlock() (abd78a1).
| const message = error?.message ?? '' | ||
| return ( | ||
| message.includes('No credentials found') || | ||
| message.includes('Failed to decrypt') || |
There was a problem hiding this comment.
nit: this 'Failed to decrypt' match is dead code — decrypt() (EncryptionHelper) never returns falsy (it returns data or throws), so loadWalletSecret's if (!decrypted) branch that produces this message can't execute. Real decrypt failures surface as BAD_DECRYPT / Decrypt failed, which are already matched below. Safe to drop (here and ideally the dead branch in loadWalletSecret).
There was a problem hiding this comment.
Dropped, both here and the dead if (!decrypted) branch in loadWalletSecret (with a note that decrypt returns data or throws, never falsy). Left the equivalent dead branch in loadLegacyWalletWithPin untouched to keep this diff scoped — same pattern, can go in a separate cleanup (abd78a1).
A seedless session expiry can trigger a non-atomic wallet deletion: clearData sets walletState=NONEXISTENT in memory but wipes the keychain before that value is durably persisted. If the app is killed in that window, launch rehydrates a stale INACTIVE state with no keychain credential, trapping the user on a PIN screen that can never unlock (correct PIN reads as wrong, or login silently fails). There was no path back to onboarding. - BiometricsSDK.hasWalletData(): fail-safe check for any unlock credential (PIN/biometry/legacy keychain entries); returns true on error. - init(): reconcile persisted walletState against secure storage on launch; if a wallet is supposedly present but no credential exists, log out to onboarding instead of showing an unusable PIN screen. - PinScreen: on a verified PIN with a missing wallet secret, delete the wallet and route to onboarding instead of silently swallowing the failure.
Replaces the launch-time keychain reconciliation with a reactive approach that never infers wallet existence from the keychain at startup. Reviewer feedback on the launch check (hasWalletData): it could (a) read a transient keychain error as "wallet exists", (b) count a stale legacy entry as a live wallet, and (c) false-negative on an options mismatch and delete a real wallet. All three stem from inferring deletion from keychain presence/absence. Instead: - Revert init() to its original behavior (no launch keychain probe). - loadEncryptionKeyWithPin now returns 'success' | 'wrong-pin' | 'no-credentials'. no-credentials is derived from the same getGenericPassword read used for a normal unlock; transient failures throw, so a flaky read is never mistaken for a missing wallet. - checkEnteredPin routes no-credentials to deleteWallet -> onboarding, only when the user actually enters a PIN. wrong-pin still rate-limits as before. - Retain the PinScreen wallet-secret safety net (encryption key present but the per-wallet secret gone). - Remove hasWalletData and its launch-reconciliation test.
- loadEncryptionKeyWithPin: catch decrypt errors, map bad-decrypt (BAD_DECRYPT / Decrypt failed) to 'wrong-pin', rethrow all other errors so the typed contract is honored and bad-data/transient failures surface distinctly. - ensureWalletSecret: only trigger recovery (wallet delete) on terminal secret failures; rethrow transient keychain/IO errors so a flaky read can't destroy a real wallet. - Add unit tests for both paths.
… load, recover PIN screen on login failure
…eletion on strict keychain probes, make migrations crash-safe (secret before keys)
Description
Ticket: CP-14585
Fixes a seedless-wallet lockout where an expired session leaves the user stranded on the Unlock (PIN) screen instead of being routed back to onboarding.
Root cause. Wallet deletion is non-atomic. When a seedless session expires and re-authentication fails,
onLogOut → clearDatasetswalletState = NONEXISTENTin memory but the redux-persist write is async, while the keychain (encryption key + wallet secret) is wiped immediately. If the app is killed/crashes in that window ("kicked out of app"), the next launch rehydrates a staleINACTIVEstate with no keychain credential.RootNavigatorthen routes to the PIN screen — where the correct PIN reads as "wrong" (encryption key gone) or login silently fails (wallet secret gone). There was no code path back to onboarding.Approach: reactive PIN-flow recovery. Rather than probing the keychain at launch (which would risk deleting a real wallet on a transient read error, a stale-legacy false positive, or a keychain-options mismatch), recovery happens only when the user deliberately attempts to unlock:
BiometricsSDK.loadEncryptionKeyWithPin()now returns'success' | 'wrong-pin' | 'no-credentials'.no-credentialsis derived from the samegetGenericPasswordread a normal unlock already uses; a transient keychain failure throws instead of reportingno-credentials, so a flaky read is never mistaken for a missing wallet.checkEnteredPinroutesno-credentialstodeleteWallet()→ onboarding.wrong-pinstill rate-limits exactly as before. A not-yet-migrated legacy wallet migrates first, so it is never mistaken for a missing wallet.PinScreenretains the safety net for the other variant (encryption key present but the per-wallet secret gone): a verified PIN with a missing secret deletes the wallet and routes to onboarding instead of silently swallowing the error.init()is unchanged frommain— no launch-time keychain teardown.No dependencies introduced. No breaking changes.
Screenshots/Videos
N/A — no new UI; this is a recovery-path fix. Behavior is covered by unit tests; see Testing.
Testing
Dev Testing
iOS: 9422
Android: 9423
Simulating the corrupt state (wallet "exists" per redux, but the keychain credential is gone):
encryption-key-serviceentry (variant A), or thesec-storage-service-*wallet-secret entry (variant B). Or reproduce via the ticket's seedless session-expiry → failed re-auth → force-close sequence.Regression checks:
Checklist