Repository navigation
feat(wallet): mnemonic identity for new wallets - #624
Merged
Merged
Conversation
Add lib/mnemonic.ts with encrypted mnemonic storage (PBKDF2 + AES-GCM), retrieval, and BIP86 Taproot key derivation for Nostr backup operations.
New wallet creation now generates a 12-word mnemonic and creates a MnemonicIdentity (BIP86 Taproot derivation) instead of a raw private key. Legacy wallets using SingleKey continue to work for unlock and restore.
…ckup Restore screen now auto-detects mnemonic (12 words) vs nsec/hex input. Backup screen shows recovery phrase for mnemonic wallets, nsec for legacy.
Backup e2e tests now expect "Recovery phrase" labels and 12-word mnemonic output. The shared getSecret utility handles both mnemonic and legacy wallet types.
- Add validateMnemonic guard to deriveNostrKeyFromMnemonic to fail fast
on invalid input instead of silently deriving a wrong key
- Normalize copy casing: use sentence case ("private key") consistently
instead of mixed Title Case ("Private Key")
- Add test for invalid mnemonic rejection
- DOMException from crypto.subtle.decrypt extends Error, so wrong password threw raw crypto errors instead of "Invalid password" - Add defensive null check on derived.privateKey in BIP32 derivation
…s toHex) ServiceWorkerWallet.setup requires identity.toHex() to extract the private key for the worker. MnemonicIdentity doesn't implement toHex, causing wallet boot to fail silently. Derive the BIP86 key from the mnemonic and wrap it in SingleKey instead.
Instead of deriving a BIP86 key from the mnemonic and wrapping it in SingleKey (which loses the mnemonic identity), send the raw mnemonic and isMainnet flag to the service worker. The worker's custom buildServices creates MnemonicIdentity internally for signing. - Add custom buildServices to MessageBus in wallet-service-worker.ts handling mnemonic, privateKey, and publicKey wallet configs - Add mnemonic path in initSvcWorkerWallet that manually sets up the service worker, posts INITIALIZE_MESSAGE_BUS with mnemonic data, constructs ServiceWorkerWallet, and sends INIT_WALLET - Store mnemonic in ref for restartWallet, clear on lock - Remove unused deriveNostrKeyFromMnemonic import
isValidPassword/noUserDefinedPassword checked encrypted_private_key which doesn't exist for mnemonic wallets, causing the password change screen to never authenticate. Now checks encrypted_mnemonic first. Password.tsx saveNewPassword also re-encrypts the mnemonic (not just the private key) when changing passwords on mnemonic wallets.
The conflict-fix commit accidentally replaced 'static' with 'hd', re-enabling address rotation. All wallets (mnemonic and legacy) must use a static address.
Contributor
WalkthroughThis PR adds BIP39 mnemonic-based wallet support, enabling users to create and restore wallets using 12-word recovery phrases alongside the existing private-key flow. The core mnemonic module encrypts phrases using AES-GCM with PBKDF2-derived keys, derives Taproot keys for Nostr signing, and persists encrypted mnemonics to localStorage. The wallet provider branches initialization and unlock flows based on whether a mnemonic is configured. Init/Restore/Settings screens are updated to handle both mnemonic and private-key identities, with dynamic UI labels. LNURL credential derivation is refactored to work asynchronously with the new Identity type. ChangesMnemonic-based wallet support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying wallet-bitcoin with
|
| Latest commit: |
008b13b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5c17f368.wallet-bitcoin.pages.dev |
| Branch Preview URL: | https://feat-mnemonic-identity-final.wallet-bitcoin.pages.dev |
Deploying wallet-staging with
|
| Latest commit: |
9aac7fb
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://7c8708b9.wallet-staging-95g.pages.dev |
| Branch Preview URL: | https://feat-mnemonic-identity-final.wallet-staging-95g.pages.dev |
Deploying wallet-mutinynet with
|
| Latest commit: |
008b13b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://11072719.arkade-wallet.pages.dev |
| Branch Preview URL: | https://feat-mnemonic-identity-final.arkade-wallet.pages.dev |
Deploying tmp-boltz-upstream-mainnet-arkade-wallet with
|
| Latest commit: |
008b13b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5cf27c60.tmp-boltz-upstream-mainnet-arkade-wallet.pages.dev |
| Branch Preview URL: | https://feat-mnemonic-identity-final.tmp-boltz-upstream-mainnet-arkade-wallet.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
src/test/e2e/backup.test.ts (2)
46-48: ⚡ Quick winImprove assertion clarity with explicit non-null check.
If the mnemonic element returns
null, the test will fail with "expected undefined to be 12", which is not very descriptive. Consider asserting that the mnemonic is truthy before checking the word count for clearer test diagnostics.💡 Suggested improvement
- // Verify 12-word mnemonic is shown const mnemonic = await page.getByTestId('private-key').textContent() + expect(mnemonic).toBeTruthy() + // Verify 12-word mnemonic is shown expect(mnemonic?.trim().split(/\s+/).length).toBe(12)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/e2e/backup.test.ts` around lines 46 - 48, Add an explicit non-null/truthy assertion before splitting the mnemonic: after obtaining mnemonic via page.getByTestId('private-key').textContent() assert that mnemonic is not null/undefined (e.g., expect(mnemonic).toBeTruthy() or expect(mnemonic).not.toBeNull()) and only then call mnemonic.trim().split(/\s+/).length to assert it equals 12; reference the mnemonic local variable in src/test/e2e/backup.test.ts to locate where to add the check.
21-23: ⚡ Quick winImprove assertion clarity with explicit non-null check.
If the mnemonic element returns
null, the test will fail with "expected undefined to be 12", which is not very descriptive. Consider asserting that the mnemonic is truthy before checking the word count for clearer test diagnostics.💡 Suggested improvement
- // Verify 12-word mnemonic is shown const mnemonic = await page.getByTestId('private-key').textContent() + expect(mnemonic).toBeTruthy() + // Verify 12-word mnemonic is shown expect(mnemonic?.trim().split(/\s+/).length).toBe(12)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/e2e/backup.test.ts` around lines 21 - 23, The test currently assumes page.getByTestId('private-key') returns a string and then checks word count, which leads to unclear errors if it returns null; update the test around the mnemonic variable to first assert the value is non-null/truthy (e.g., using expect(mnemonic).toBeTruthy() or expect(mnemonic).not.toBeNull()) before performing the split and length check, referencing the mnemonic variable and the page.getByTestId('private-key') call so the subsequent expect(...split(...).length).toBe(12) runs only when mnemonic is present.src/test/e2e/utils.ts (1)
239-245: ⚡ Quick winRename parameter to reflect dual-credential support.
The parameter name
nsecis now misleading since the function accepts either a legacy nsec/hex private key or a 12-word mnemonic recovery phrase. Consider renaming it tocredentialorsecretto better reflect its dual purpose.♻️ Suggested refactor
-async function restoreWallet(page: Page, nsec: string): Promise<void> { +async function restoreWallet(page: Page, credential: string): Promise<void> { await page.getByText('Other login options').click() await page.getByText('Restore wallet').click() - await page.locator('input[name="private-key"]').fill(nsec) + await page.locator('input[name="private-key"]').fill(credential) await page.getByText('Continue').click() await waitForWalletPage(page) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/e2e/utils.ts` around lines 239 - 245, Rename the misleading parameter nsec in the restoreWallet function to a neutral name like credential or secret to reflect that it can be either a legacy nsec/hex private key or a 12-word mnemonic; update the function signature async function restoreWallet(page: Page, credential: string): Promise<void> (and any internal references to nsec) as well as all call sites that pass an nsec value, and adjust any JSDoc/type annotations or tests that mention nsec to use the new name so the intent is clear across restoreWallet and its usages.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@package.json`:
- Around line 58-59: The test scripts "test:e2e" and "test:e2e:ui" use
POSIX-style env assignment which fails on Windows; update both scripts to prefix
environment variables with the existing cross-env tool (e.g., use cross-env
VITE_NOSTR_RELAY_URL=... VITE_LNURL_SERVER_URL=... playwright test and cross-env
... playwright test --ui) so the variables are set cross-platform; edit the
package.json scripts entries named "test:e2e" and "test:e2e:ui" to use cross-env
accordingly.
In `@src/lib/mnemonic.ts`:
- Around line 7-19: Wrap all direct localStorage accesses in hasMnemonic,
setMnemonic, and getMnemonic in try/catch blocks to handle DOMExceptions
(SecurityError/quota). For hasMnemonic, return false if storage access throws;
for setMnemonic (which calls encryptMnemonic) and getMnemonic (which calls
decryptMnemonic), catch storage errors and rethrow a controlled Error (e.g.,
"StorageUnavailable" or the existing app-specific error type/message) that the
rest of the auth/unlock flow expects. Reference STORAGE_KEY when accessing
storage and preserve current behavior for non-storage failures (still propagate
encryptMnemonic/decryptMnemonic errors).
In `@src/providers/wallet.tsx`:
- Around line 242-251: detectPasswordState incorrectly assumes presence of an
encrypted mnemonic means the mnemonic is the active credential; instead detect
the actual active credential and prefer it when both are present. Update
detectPasswordState (and the similar logic at the other location) to: check both
hasMnemonic and hasLegacyKey (or equivalent helpers), then determine the active
credential by either consulting the authoritative flag/state (e.g., an
activeCredential or walletType indicator if available) or by attempting to
unlock/decrypt each credential in a deterministic order (call getMnemonic and
getPrivateKey with defaultPassword and treat the one that successfully decrypts
as active); only fall back to noUserDefinedPassword() when no credentials exist.
Use the functions detectPasswordState, hasMnemonic, getMnemonic, hasLegacyKey,
getPrivateKey, and noUserDefinedPassword to locate and implement this behavior.
In `@src/screens/Init/Connect.tsx`:
- Around line 28-40: The effect currently returns when password is missing or
when neither mnemonic nor privateKey are present, leaving the UI stuck; update
the useEffect in Connect.tsx to explicitly handle those cases by checking if
(!password) or if (!mnemonic && !privateKey) and immediately fail fast: call
abortConnectionWithError with a clear Error message (e.g. "Missing password" or
"No credentials provided") or, if there's a state toggler like setConnectDone
available in this component, call it to flip the connection flow back to
onboarding; keep the existing promises for setMnemonic/setPrivateKey ->
initWallet -> setInitialized as-is, but ensure the missing-credentials branch
triggers abortConnectionWithError (or setConnectDone) so the loading screen
doesn't hang.
In `@src/screens/Init/Restore.tsx`:
- Around line 140-142: Update the restore helper text in the Restore component
so it lists both supported key formats; locate the TextSecondary element inside
Restore (the JSX block rendering the instruction paragraph) and change the copy
to mention that both 12-word recovery phrases and private keys in either 'nsec'
or raw hex formats are accepted (e.g., "Enter your 12-word recovery phrase, or a
private key starting with 'nsec' or a raw hex key. Do not share it with
anyone."). Ensure you only modify the displayed string in the TextSecondary JSX
(no logic changes).
- Around line 44-78: The early return in the useEffect when someKey is empty
leaves previous parsed wallet state set; update the beginning of the useEffect
(the block referencing someKey, trimmed, setMnemonic, setPrivateKey, setLabel,
setError) so that when someKey is falsy or trimmed is empty you explicitly clear
parsed state by calling setMnemonic(undefined), setPrivateKey(undefined),
setLabel(buttonLabel) (or the initial label value) and setError('') before
returning, ensuring the Continue button and state reflect an empty input.
In `@src/screens/Settings/Backup.tsx`:
- Around line 76-87: The current showPrivateKey function sets showSecret(true)
and closes the dialog regardless of verifyPassword() outcome; change the flow so
that after obtaining password (via authenticateUser(...) or
enteredPassword.current) you call verifyPassword(password), and if it fails
(result is falsy/empty) setError('Invalid password') and return without calling
setSecret, setShowSecret, or setDialog; only when verifyPassword returns a
truthy secret should you call setSecret(result), setShowSecret(true) and
setDialog(false). Ensure you still clear errors on success (setError('')) and
keep existing references to verifyPassword, authenticateUser,
enteredPassword.current, wallet.lockedByBiometrics, wallet.passkeyId, setSecret,
setShowSecret, setDialog and setError.
---
Nitpick comments:
In `@src/test/e2e/backup.test.ts`:
- Around line 46-48: Add an explicit non-null/truthy assertion before splitting
the mnemonic: after obtaining mnemonic via
page.getByTestId('private-key').textContent() assert that mnemonic is not
null/undefined (e.g., expect(mnemonic).toBeTruthy() or
expect(mnemonic).not.toBeNull()) and only then call
mnemonic.trim().split(/\s+/).length to assert it equals 12; reference the
mnemonic local variable in src/test/e2e/backup.test.ts to locate where to add
the check.
- Around line 21-23: The test currently assumes page.getByTestId('private-key')
returns a string and then checks word count, which leads to unclear errors if it
returns null; update the test around the mnemonic variable to first assert the
value is non-null/truthy (e.g., using expect(mnemonic).toBeTruthy() or
expect(mnemonic).not.toBeNull()) before performing the split and length check,
referencing the mnemonic variable and the page.getByTestId('private-key') call
so the subsequent expect(...split(...).length).toBe(12) runs only when mnemonic
is present.
In `@src/test/e2e/utils.ts`:
- Around line 239-245: Rename the misleading parameter nsec in the restoreWallet
function to a neutral name like credential or secret to reflect that it can be
either a legacy nsec/hex private key or a 12-word mnemonic; update the function
signature async function restoreWallet(page: Page, credential: string):
Promise<void> (and any internal references to nsec) as well as all call sites
that pass an nsec value, and adjust any JSDoc/type annotations or tests that
mention nsec to use the new name so the intent is clear across restoreWallet and
its usages.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a7c9023c-7f66-43d9-8fe4-5e38473beb2d
📒 Files selected for processing (14)
package.jsonsrc/lib/mnemonic.tssrc/lib/privateKey.tssrc/providers/flow.tsxsrc/providers/wallet.tsxsrc/screens/Init/Connect.tsxsrc/screens/Init/Init.tsxsrc/screens/Init/Restore.tsxsrc/screens/Settings/Backup.tsxsrc/screens/Settings/Password.tsxsrc/test/e2e/backup.test.tssrc/test/e2e/utils.tssrc/test/lib/mnemonic.test.tssrc/test/screens/settings/backup.test.tsx
ghost
approved these changes
May 19, 2026
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Arkana Code Review — wallet#624: mnemonic identity for new wallets
Verdict: APPROVE — clean, well-structured change. No protocol-critical bugs found.
What I checked
- Full diff (all 11 files, 397+/106-)
- Full context of
wallet.tsx,privateKey.ts,mnemonic.ts,storage.ts,constants.ts,wallet.ts - SDK types:
Identity,MnemonicIdentity,SingleKey,ServiceWorkerWallet.setupoptions clearStorage()useslocalStorage.clear()— correctly wipesencrypted_mnemonicon reset ✅- Old
deriveKeyFromSeedusedm/44/1237/0'(custom path), newMnemonicIdentityuses BIP-86m/86'/coinType'/0'. No migration issue since old wallets never surfaced the mnemonic to users — they backed up via nsec, which still works ✅ walletMode: 'static'is correctly hardcoded inServiceWorkerWallet.setup✅- AES-GCM + PBKDF2 (100k iter, SHA-256) for mnemonic encryption mirrors existing key storage ✅
- Cross-repo:
MnemonicIdentityandIdentitycome from@arkade-os/sdk(ts-sdk). No downstream consumer breakage —walletis the only consumer ofServiceWorkerWallet.setup.
Issues to address (non-blocking)
1. Hardcoded storage key in privateKey.ts — fragile coupling
src/lib/privateKey.ts:57: localStorage.getItem('encrypted_mnemonic') duplicates the STORAGE_KEY constant from mnemonic.ts. If the key name ever changes in mnemonic.ts, this silent divergence would break password validation for mnemonic wallets. Consider exporting the key name or a hasMnemonic check from mnemonic.ts and using a static import (no circular dep risk since privateKey.ts doesn't import from mnemonic.ts today — the dynamic import is unnecessary here).
2. Duplicated isMainnet logic
src/providers/wallet.tsx:601 defines isMainnet() as a local function. src/screens/Init/Restore.tsx:88-92 inlines the exact same check. Extract to a shared utility (e.g., src/lib/network.ts) to avoid drift.
3. privateKey field still in InitSvcWorkerWalletParams
src/providers/wallet.tsx:56: The privateKey?: string field in InitSvcWorkerWalletParams is no longer passed by any caller — initWallet and restartWallet both pass identity directly now. The fallback in initSvcWorkerWallet (line 386: params.identity ?? (params.privateKey ? SingleKey.fromHex(params.privateKey) : undefined)) is dead code. Clean it up to avoid confusion.
4. deriveNostrKeyFromMnemonic derives the same key as the wallet signing key
src/lib/mnemonic.ts:47-57: Uses BIP-86 m/86'/coinType'/0'/0/0 — the same derivation path as MnemonicIdentity. This means the Nostr backup encryption key IS the wallet signing key. This is consistent with legacy behavior (raw privkey was used for both), but worth a code comment making this explicit, since it's a deliberate security tradeoff.
Observations (no action needed)
- Error handling in
unlockWalletcorrectly catchesDOMExceptionfrom AES-GCM decryption failure and maps it to "Invalid password" detectPasswordStateinwallet.tsxproperly short-circuits for mnemonic wallets before falling through tonoUserDefinedPassword()for legacy wallets- E2E tests updated for mnemonic flow; unit tests cover encrypt/decrypt round-trips and key derivation
initInfocorrectly cleared (includingmnemonic: undefined) after connect flow completes
LGTM. Ship it after addressing the non-blocking items (or file follow-up issues).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/providers/lnurl.tsx`:
- Around line 225-233: Effect currently leaves previous credentials and allows a
stale xOnlyPublicKey promise to overwrite new ones; update the useEffect that
depends on svcWallet to clear credentials immediately when svcWallet is falsy
and to cancel/ignore results from any prior async derivation when svcWallet
changes: inside the effect, generate a local cancel token (boolean flag or
AbortController) before calling svcWallet.identity.xOnlyPublicKey(), check the
token before calling setCredentials, and also call setCredentials(undefined)
when svcWallet is removed; reference svcWallet, identity.xOnlyPublicKey,
deriveLnurlCredentials, and setCredentials to implement this
cancellation/clearing logic so old promises cannot keep sessions alive or
overwrite new credentials.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 44d9e141-327b-4bdf-8bf7-e98bc2b10931
📒 Files selected for processing (2)
src/providers/lnurl.tsxsrc/screens/Init/Restore.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/screens/Init/Restore.tsx
ghost
approved these changes
May 19, 2026
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Arkana Follow-up Review — wallet#624 (new commits since 3d2bd8be)
Verdict: APPROVE — the new LNURL credential plumbing is sound. No protocol-critical issues.
What changed since my last review
5 files touched across 6 commits (3d2bd8be→78db45f0):
flow.tsx: AddedlnurlInfo: Uint8Array | undefined+ setter to FlowContextlnurl.tsx: ReplacedsvcWallet.identity.toHex()→useMemopattern with a fire-and-forgetuseEffectconsuminglnurlInfofrom FlowContextwallet.tsx:initWalletnow callssetLnurlInfo(secret)with derived Nostr key (mnemonic) or raw private key (legacy)Restore.tsx: Minor — removed stalevalue={someKey}proppackage.json: RemovedVITE_LNURL_SERVER_URLfrom e2e test scripts
What I verified
- Secret key lifecycle:
lnurlInfois set ininitWallet, consumed inLnurlProvideruseEffect, immediately cleared viasetLnurlInfo(undefined)✅ - LNURL session teardown on wallet lock:
credentialsstate persists but session stops becauseconnectedgoes false in the session lifecycle effect — correct ✅ - No key material leaks: the
Uint8Arrayis hex-encoded, passed toderiveLnurlCredentials, then the source is cleared from context ✅ deriveNostrKeyFromMnemonicuses BIP-86 path matchingMnemonicIdentity— consistent with my previous review ✅- Removed
VITE_LNURL_SERVER_URLfrom e2e — this is safe, the LNURL server URL comes from app config, not a test env var that would break e2e ✅
Non-blocking observations
1. credentials state not cleared on wallet lock
src/providers/lnurl.tsx: When the wallet locks, credentials remains in LnurlProvider state until new credentials overwrite it on next unlock. No security exposure since the LNURL session is already torn down (via connected dependency), but explicitly clearing credentials on lock would be cleaner defense-in-depth. Consider adding a cleanup effect or resetting on connected → false.
2. My previous non-blocking items still apply
The 4 items from my original review (hardcoded storage key in privateKey.ts, duplicated isMainnet, dead privateKey field in InitSvcWorkerWalletParams, and the shared derivation path comment) are still valid. File follow-up issues if not addressing in this PR.
LGTM. The architectural shift from direct identity access to FlowContext-mediated key passing is a reasonable decoupling. Ship it.
ghost
reviewed
May 19, 2026
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Arkana Re-check — wallet#624 (post-merge from master, 572809fc)
Only change since my last review (78db45f0) is a merge from master bringing in two unrelated files:
src/components/InputAddress.tsx— addslightning:prefix handling in address validationsrc/screens/Apps/Boltz/Settings.tsx— import cleanup + text color token update
No merge conflicts with the mnemonic feature files. All PR-specific code (mnemonic.ts, wallet.tsx, Init/*, Settings/*, tests) is unchanged from my last approved review.
Verdict: Previous approval stands. Still waiting on human sign-off for the protocol-adjacent credential storage changes.
ghost
approved these changes
May 20, 2026
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Arkana Incremental Review — wallet#624 (commits since 572809fc)
Reviewed 7 new commits: 307c613e→008b13b0.
Changes reviewed
| Commit | Verdict |
|---|---|
307c613e cross-env for e2e scripts |
✅ cross-env already in devDeps, consistent with start:mainnet |
59cea617 fail fast in Connect on missing credentials |
✅ Good defensive guard — prevents undefined mnemonic/privateKey from silently doing nothing |
a06f82b7 clear parsed state in Restore on empty input |
✅ Fixes stale state when user clears the input field |
01791072 mention raw hex in Restore help text |
✅ Copy-only |
e231b238 keep backup dialog open on invalid password |
✅ Good security fix — previously setSecret('') then setShowSecret(true) would close the dialog and show ******* with an empty secret, silently failing. Now returns early, keeping the dialog open so the user sees the error |
779c99a6 document nostr backup key = wallet signing key |
✅ Important documentation of a deliberate tradeoff |
008b13b0 remove unused privateKey field from InitSvcWorkerWalletParams |
✅ Clean type tightening — identity is now required, dead code removed |
Notes
- Connect.tsx:30 —
abortConnectionWithErroris called insideuseEffectbefore itsconstdeclaration at line 65. This works because the arrow function is only invoked asynchronously (React effect runs after render, by which time the closure has captured the function). Not a bug, but worth noting. - Backup.tsx:83-86 — The early return fix is correct. One minor observation: if
authenticateUserrejects andsetErroris called with the error object,passwordwill beundefinedand the function returns early at line 82. This is fine behavior. - wallet.tsx:55 — Making
identityrequired (non-optional) is the right call. All callers already construct anIdentitybefore callinginitSvcWorkerWallet.
No protocol-critical changes in this batch. All changes are UI polish, type cleanup, and defensive guards. LGTM.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/providers/lnurl.tsx (1)
237-243: 💤 Low valueAdd
setLnurlInfoto the dependency array.
setLnurlInfois called inside the effect but not listed in the dependency array. While React guaranteessetStatefunctions are stable, ESLint'sexhaustive-depsrule will flag this. Adding it keeps the effect self-documenting and lint-clean.Suggested fix
useEffect(() => { if (!lnurlInfo) return const seckey = hex.encode(lnurlInfo) const credentials = deriveLnurlCredentials(seckey) setCredentials(credentials) setLnurlInfo(undefined) - }, [lnurlInfo]) + }, [lnurlInfo, setLnurlInfo])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/providers/lnurl.tsx` around lines 237 - 243, The useEffect that reads lnurlInfo and calls setLnurlInfo should include setLnurlInfo in its dependency array to satisfy exhaustive-deps; update the effect whose body references lnurlInfo, hex.encode, deriveLnurlCredentials, setCredentials and setLnurlInfo so the dependency array is [lnurlInfo, setLnurlInfo] (keeping other referenced stable helpers as-is).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/providers/lnurl.tsx`:
- Around line 237-243: The useEffect that reads lnurlInfo and calls setLnurlInfo
should include setLnurlInfo in its dependency array to satisfy exhaustive-deps;
update the effect whose body references lnurlInfo, hex.encode,
deriveLnurlCredentials, setCredentials and setLnurlInfo so the dependency array
is [lnurlInfo, setLnurlInfo] (keeping other referenced stable helpers as-is).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0469772f-6a23-4358-bcff-86044ba30598
📒 Files selected for processing (8)
package.jsonsrc/lib/mnemonic.tssrc/providers/flow.tsxsrc/providers/lnurl.tsxsrc/providers/wallet.tsxsrc/screens/Init/Connect.tsxsrc/screens/Init/Restore.tsxsrc/screens/Settings/Backup.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
MnemonicIdentity(from the ARK SDK) instead of a rawSingleKey. Legacy wallets (nsec/hex private key) continue to work unchanged.walletMode: 'static', ensuring a single persistent address with no rotation.localStorageusing AES-GCM (PBKDF2-derived key, 100k iterations), mirroring how private keys were already stored.hasMnemonic()to handle both credential types.What changed and why
src/lib/mnemonic.tshasMnemonic,setMnemonic,getMnemonic,deriveNostrKeyFromMnemonic(BIP-86 path for Nostr backup)src/providers/wallet.tsxinitWalletnow accepts{ mnemonic?, privateKey? }.unlockWallet,restartWallet, andreinitSvcWalletupdated to pass anIdentityobject throughout.walletMode: 'static'enforced onServiceWorkerWallet.setup.src/screens/Init/Init.tsxhandleNewWalletgenerates a mnemonic and passes it throughinitInfoinstead of deriving a raw seed.src/screens/Init/Restore.tsx@scure/bip39, or a legacy nsec/hex key.src/screens/Init/Connect.tsxinitWallet.src/screens/Settings/Backup.tsxsrc/screens/Settings/Password.tsxsrc/test/lib/mnemonic.test.tsderiveNostrKeyFromMnemonic.src/test/e2e/backup.test.tsStatic address — no rotation
walletMode: 'static'is passed toServiceWorkerWallet.setupfor every wallet type. This was explicitly re-verified: a conflict-resolution commit had accidentally set this to'hd'(which would enable rotation), and that was corrected before this PR was opened.Test plan
pnpm test:unitpasses (mnemonic unit tests)pnpm test:e2ebackup and restore tests passSummary by CodeRabbit
New Features
Refactor
Tests