feat(wallet): mnemonic-based identity for wallet creation#545
Conversation
Deploying wallet-bitcoin with
|
| Latest commit: |
f02a21f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c560e3ff.wallet-bitcoin.pages.dev |
| Branch Preview URL: | https://feat-mnemonic-identity.wallet-bitcoin.pages.dev |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds client-side encrypted BIP39 mnemonic storage (localStorage AES‑GCM with PBKDF2-derived key), BIP86 key derivation, and integrates mnemonic-aware flows across wallet init/unlock/restart, restore/backup UIs, service-worker wiring, and tests. Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as Init/Restore Screen
participant Mnemonic as Mnemonic Module
participant WalletProv as Wallet Provider
participant SW as Service Worker
User->>UI: Generate or enter mnemonic + password
UI->>Mnemonic: setMnemonic(mnemonic, password)
Mnemonic->>Mnemonic: PBKDF2-HMAC-SHA-256 (100k) -> key
Mnemonic->>Mnemonic: AES-256-GCM encrypt (iv)
Mnemonic-->>Mnemonic: store salt||iv||ciphertext in localStorage
UI->>WalletProv: initWallet({ mnemonic })
WalletProv->>Mnemonic: deriveNostrKeyFromMnemonic(mnemonic, isMainnet)
Mnemonic->>Mnemonic: BIP39 validate -> seed -> BIP86 derive (m/86'/{coinType}'/0'/0/0)
Mnemonic-->>WalletProv: privateKey (Uint8Array 32)
WalletProv->>SW: provision identity / init svc worker with identity
SW-->>WalletProv: wallet identity ready
WalletProv-->>UI: Wallet initialized
UI-->>User: Ready
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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-mutinynet with
|
| Latest commit: |
f02a21f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://45057514.arkade-wallet.pages.dev |
| Branch Preview URL: | https://feat-mnemonic-identity.arkade-wallet.pages.dev |
Deploying tmp-boltz-upstream-mainnet-arkade-wallet with
|
| Latest commit: |
f02a21f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c1db6547.tmp-boltz-upstream-mainnet-arkade-wallet.pages.dev |
| Branch Preview URL: | https://feat-mnemonic-identity.tmp-boltz-upstream-mainnet-arkade-wallet.pages.dev |
![arkanaai[bot]](https://avatars.githubusercontent.com/u/170820085?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review — feat(wallet): mnemonic-based identity for wallet creation
Verdict: Approve ✅ — well-structured migration from raw private key to mnemonic-based identity. Clean backward compatibility for legacy SingleKey wallets. Key derivation is correctly delegated to the SDK's MnemonicIdentity.
Not protocol-critical (no VTXO handling, signing logic, forfeit paths, or exit path changes). Identity creation is the SDK's responsibility; wallet just passes the mnemonic through.
Findings
1. (Medium) isMainnet logic duplicated — extract to shared utility
src/providers/wallet.tsx:537-538:
const isMainnet = (network: NetworkName | string): boolean =>
network !== 'testnet' && network !== 'mutinynet' && network !== 'signet' && network !== 'regtest'src/screens/Init/Restore.tsx:80-84 — same logic inlined:
const isNet =
aspInfo.network !== 'testnet' &&
aspInfo.network !== 'mutinynet' &&
aspInfo.network !== 'signet' &&
aspInfo.network !== 'regtest'This should live in src/lib/constants.ts or a network utility. Two copies = two places to forget when a new network name is added.
2. (Low) Non-null assertion on derived.privateKey! — src/lib/mnemonic.ts:30
const derived = masterNode.derive(`m/86'/${coinType}'/0'`).deriveChild(0).deriveChild(0)
return derived.privateKey!The SDK's SeedIdentity constructor (seedIdentity.ts:139-141) explicitly checks for null and throws "Failed to derive private key". This wallet code should do the same:
if (!derived.privateKey) throw new Error('Failed to derive private key from mnemonic')
return derived.privateKeyThe ! assertion silently turns a derivation failure into an opaque downstream crash.
3. (Info) Nostr backup key = wallet signing key — intentional?
deriveNostrKeyFromMnemonic uses path m/86'/coinType'/0'/0/0, which is the same path as MnemonicIdentity's BIP86 derivation. The Nostr backup ECDH key is the wallet's signing key. This matches legacy behavior (where the single private key was used for both), but worth documenting explicitly in the function's JSDoc since it's non-obvious with HD derivation.
4. (Low) Missing test coverage
- No test for
Restore.tsxmnemonic auto-detection (space-based heuristic, valid/invalid phrase handling) - No test for
unlockWalletmnemonic path inwallet.tsx - Restore mnemonic → Nostr backup key derivation untested end-to-end
The existing tests cover storage and backup screen well. These gaps are minor but would add confidence.
5. (Info) restartWallet cleanup — nice improvement
Old code had a hacky HasToHex type assertion to extract the private key hex. New code reuses svcWallet.identity directly. Cleaner and eliminates a class of errors if the identity type changes.
6. (Info) Reset path is clean
clearStorage() calls localStorage.clear() → encrypted_mnemonic gets wiped. No orphaned secrets after wallet reset. ✅
Summary
| Category | Count |
|---|---|
| Bugs | 0 |
| Security issues | 0 |
| Should fix (medium) | 1 — extract isMainnet |
| Should fix (low) | 1 — null check on derived key |
| Informational | 3 |
Clean PR. The two actionable items are minor and can be done in a follow-up.
🤖 Reviewed by Arkana
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/providers/wallet.tsx (1)
2-17:⚠️ Potential issue | 🟡 MinorMerge duplicate
@arkade-os/sdkimport (ESLint failure).Static analysis (
no-duplicate-imports) flags the separateimport type { Identity }statement. Combine it with the value import above to clear CI.♻️ Proposed diff
import { ArkNote, ServiceWorkerWallet, NetworkName, SingleKey, MnemonicIdentity, AssetDetails, WalletBalance, IVtxoManager, migrateWalletRepository, getMigrationStatus, rollbackMigration, IndexedDBWalletRepository, IndexedDBContractRepository, + type Identity, } from '@arkade-os/sdk' -import type { Identity } from '@arkade-os/sdk'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/providers/wallet.tsx` around lines 2 - 17, There's a duplicate import from '@arkade-os/sdk' — merge the separate type import into the main import to satisfy no-duplicate-imports; update the existing import that currently lists ArkNote, ServiceWorkerWallet, NetworkName, SingleKey, MnemonicIdentity, AssetDetails, WalletBalance, IVtxoManager, migrateWalletRepository, getMigrationStatus, rollbackMigration, IndexedDBWalletRepository, IndexedDBContractRepository to also include the type-only symbol Identity (use the `type Identity` form if needed), and remove the standalone `import type { Identity }` line.
🧹 Nitpick comments (5)
src/screens/Settings/Backup.tsx (1)
176-176: StaletestIdfor mnemonic wallets.
testId='private-key'is now shared with recovery-phrase content. Consider a more generic id (e.g.,'backup-secret') so tests don't imply the wrong secret type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/screens/Settings/Backup.tsx` at line 176, The testId on the InputFake component is stale and specific to private keys; change it to a generic id to cover both private-key and recovery-phrase cases: update the InputFake instance (the JSX using InputFake with props testId='private-key', text={showSecret ? secret : '*******'}) to use a neutral testId such as 'backup-secret' (or similar) so tests no longer imply a specific secret type while keeping the existing showSecret and secret usage intact.src/test/lib/mnemonic.test.ts (1)
7-59: LGTM — good coverage of the mnemonic module.Covers storage roundtrip, wrong-password rejection, missing-mnemonic rejection, and deterministic/derivation behavior.
Optional: add a test that asserts
hasMnemonic()returnsfalseafter a clear (mirroring wallet reset), and one validating that the encrypted payload inlocalStorageis not equal to the plaintext mnemonic — both would guard against regressions in the encryption path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/lib/mnemonic.test.ts` around lines 7 - 59, Add two tests: (1) verify hasMnemonic() returns false after calling localStorage.clear() (mirroring a wallet reset) by ensuring hasMnemonic() is false both before and after setMnemonic(testMnemonic, testPassword) followed by localStorage.clear(); and (2) after calling setMnemonic(testMnemonic, testPassword) assert that the stored encrypted payload retrieved from localStorage (read the key used by setMnemonic/getMnemonic) is not equal to the plaintext testMnemonic to ensure encryption is applied; reference hasMnemonic, setMnemonic, getMnemonic, testMnemonic, testPassword and localStorage when adding these assertions.src/screens/Init/Restore.tsx (2)
82-104: Duplicate "is-mainnet" logic and misleading variable name.The same
network !== 'testnet' && ...check exists insrc/providers/wallet.tsx(lines 538–539) where anisMainnet(network)helper is defined locally. Export that helper (e.g., fromsrc/lib/network.tsor similar) and reuse it here to avoid drift when supported networks change. Additionally,isNetreads as "is on a network"; the intent is mainnet.♻️ Proposed diff
- let seckey: Uint8Array - if (mnemonic) { - setInitInfo({ mnemonic, password: defaultPassword, restoring: true }) - const isNet = - aspInfo.network !== 'testnet' && - aspInfo.network !== 'mutinynet' && - aspInfo.network !== 'signet' && - aspInfo.network !== 'regtest' - seckey = deriveNostrKeyFromMnemonic(mnemonic, isNet) - } else { + let seckey: Uint8Array + if (mnemonic) { + setInitInfo({ mnemonic, password: defaultPassword, restoring: true }) + seckey = deriveNostrKeyFromMnemonic(mnemonic, isMainnet(aspInfo.network)) + } else {(Then import the shared
isMainnethelper.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/screens/Init/Restore.tsx` around lines 82 - 104, The duplicate mainnet detection logic in handleProceed (used to compute seckey via deriveNostrKeyFromMnemonic and reading aspInfo.network) should be replaced by the shared isMainnet helper used elsewhere (previously in wallet provider); import and call isMainnet(aspInfo.network) instead of the inline network !== 'testnet' && ... expression, and rename the local variable isNet to isMainnet to reflect intent; update the call to deriveNostrKeyFromMnemonic(mnemonic, isMainnet) so behavior is unchanged but uses the centralized isMainnet implementation.
44-78: Mnemonic detection viaincludes(' ')is a bit brittle.Inputs pasted with newlines or non-breaking spaces between words would fall into the private-key branch and fail validation with a misleading error. Consider normalizing whitespace (
.replace(/\s+/g, ' ').trim()) and then detecting via the presence of any whitespace, before handing tovalidateMnemonic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/screens/Init/Restore.tsx` around lines 44 - 78, Normalize the incoming someKey first (e.g., const normalized = someKey.replace(/\s+/g, ' ').trim()) and use that for mnemonic detection and validation: detect mnemonics by checking for any whitespace (/\s/ or normalized.includes(' ')), pass the normalized string to validateMnemonic and setMnemonic/setPrivateKey/setLabel/setError accordingly; for the private-key branch strip all whitespace before parsing (e.g., const pkInput = normalized.replace(/\s+/g, '')) and use pkInput with nsecToPrivateKey / hex.decode / invalidPrivateKey so inputs with newlines or non-breaking spaces are handled correctly while preserving existing error handling (functions: useEffect, someKey, validateMnemonic, setMnemonic, setPrivateKey, setLabel, setError, nsecToPrivateKey, hex.decode, invalidPrivateKey, extractError).src/providers/wallet.tsx (1)
538-539: Consider exportingisMainnetfor reuse.The same network check is reimplemented inline in
src/screens/Init/Restore.tsx(handleProceed). Promote this helper to a shared module (e.g.,src/lib/network.ts) and import it in both places to keep the list of non-mainnet networks single-sourced.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/providers/wallet.tsx` around lines 538 - 539, isMainnet is duplicated; extract and export a single helper to be reused. Create a shared module named network.ts that exports the function isMainnet(network: NetworkName | string): boolean (same logic as current const isMainnet), export any needed NetworkName type or import it inside the module, then replace the inline implementation in wallet.tsx (the current const isMainnet) and the check in Restore.tsx (handleProceed) with an import from the new module so both files use the single-source helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/mnemonic.ts`:
- Around line 35-38: Run the project formatter (e.g., pnpm format) to apply
Prettier to the new crypto helper changes: reformat the file containing the
crypto.subtle.importKey calls so the importKey argument lists and surrounding
statements match the project's Prettier style; specifically reformat the const
keyMaterial = await crypto.subtle.importKey('raw', encoder.encode(password),
'PBKDF2', false, ['deriveBits','deriveKey']) occurrence and the second
crypto.subtle.importKey call around the deriveKey/unwrap areas (the same block
around lines with importKey at ~73-76) so CI's pnpm format:check passes. Ensure
no other logic changes—only run formatting.
- Around line 25-30: deriveNostrKeyFromMnemonic should validate the input
mnemonic and fail fast instead of silently producing a wrong key; update
deriveNostrKeyFromMnemonic to call validateMnemonic(mnemonic) at the start
(import it if missing) and throw a clear Error (e.g., "Invalid mnemonic") when
validation fails, then proceed to mnemonicToSeedSync/HDKey derivation only for
valid mnemonics so callers get an explicit failure on bad input.
In `@src/screens/Settings/Backup.tsx`:
- Around line 118-122: The copy mixes casing between mnemonic and legacy modes;
change the "Private Key" wording to sentence case to match "recovery phrase"
(e.g., use "private key") where it's rendered in TextSecondary (check the
isMnemonicWallet branch) and update the analogous warning box copy referenced
around the other block (warning component/text at lines ~188–196) so both modes
use consistent sentence-case terms ("recovery phrase" and "private key").
---
Outside diff comments:
In `@src/providers/wallet.tsx`:
- Around line 2-17: There's a duplicate import from '@arkade-os/sdk' — merge the
separate type import into the main import to satisfy no-duplicate-imports;
update the existing import that currently lists ArkNote, ServiceWorkerWallet,
NetworkName, SingleKey, MnemonicIdentity, AssetDetails, WalletBalance,
IVtxoManager, migrateWalletRepository, getMigrationStatus, rollbackMigration,
IndexedDBWalletRepository, IndexedDBContractRepository to also include the
type-only symbol Identity (use the `type Identity` form if needed), and remove
the standalone `import type { Identity }` line.
---
Nitpick comments:
In `@src/providers/wallet.tsx`:
- Around line 538-539: isMainnet is duplicated; extract and export a single
helper to be reused. Create a shared module named network.ts that exports the
function isMainnet(network: NetworkName | string): boolean (same logic as
current const isMainnet), export any needed NetworkName type or import it inside
the module, then replace the inline implementation in wallet.tsx (the current
const isMainnet) and the check in Restore.tsx (handleProceed) with an import
from the new module so both files use the single-source helper.
In `@src/screens/Init/Restore.tsx`:
- Around line 82-104: The duplicate mainnet detection logic in handleProceed
(used to compute seckey via deriveNostrKeyFromMnemonic and reading
aspInfo.network) should be replaced by the shared isMainnet helper used
elsewhere (previously in wallet provider); import and call
isMainnet(aspInfo.network) instead of the inline network !== 'testnet' && ...
expression, and rename the local variable isNet to isMainnet to reflect intent;
update the call to deriveNostrKeyFromMnemonic(mnemonic, isMainnet) so behavior
is unchanged but uses the centralized isMainnet implementation.
- Around line 44-78: Normalize the incoming someKey first (e.g., const
normalized = someKey.replace(/\s+/g, ' ').trim()) and use that for mnemonic
detection and validation: detect mnemonics by checking for any whitespace (/\s/
or normalized.includes(' ')), pass the normalized string to validateMnemonic and
setMnemonic/setPrivateKey/setLabel/setError accordingly; for the private-key
branch strip all whitespace before parsing (e.g., const pkInput =
normalized.replace(/\s+/g, '')) and use pkInput with nsecToPrivateKey /
hex.decode / invalidPrivateKey so inputs with newlines or non-breaking spaces
are handled correctly while preserving existing error handling (functions:
useEffect, someKey, validateMnemonic, setMnemonic, setPrivateKey, setLabel,
setError, nsecToPrivateKey, hex.decode, invalidPrivateKey, extractError).
In `@src/screens/Settings/Backup.tsx`:
- Line 176: The testId on the InputFake component is stale and specific to
private keys; change it to a generic id to cover both private-key and
recovery-phrase cases: update the InputFake instance (the JSX using InputFake
with props testId='private-key', text={showSecret ? secret : '*******'}) to use
a neutral testId such as 'backup-secret' (or similar) so tests no longer imply a
specific secret type while keeping the existing showSecret and secret usage
intact.
In `@src/test/lib/mnemonic.test.ts`:
- Around line 7-59: Add two tests: (1) verify hasMnemonic() returns false after
calling localStorage.clear() (mirroring a wallet reset) by ensuring
hasMnemonic() is false both before and after setMnemonic(testMnemonic,
testPassword) followed by localStorage.clear(); and (2) after calling
setMnemonic(testMnemonic, testPassword) assert that the stored encrypted payload
retrieved from localStorage (read the key used by setMnemonic/getMnemonic) is
not equal to the plaintext testMnemonic to ensure encryption is applied;
reference hasMnemonic, setMnemonic, getMnemonic, testMnemonic, testPassword and
localStorage when adding these assertions.
🪄 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: 5dadb24c-ad74-4954-a3b8-5d0bc670fed0
📒 Files selected for processing (9)
src/lib/mnemonic.tssrc/providers/flow.tsxsrc/providers/wallet.tsxsrc/screens/Init/Connect.tsxsrc/screens/Init/Init.tsxsrc/screens/Init/Restore.tsxsrc/screens/Settings/Backup.tsxsrc/test/lib/mnemonic.test.tssrc/test/screens/settings/backup.test.tsx
There was a problem hiding this comment.
Arkana Code Review — wallet#545 (mnemonic-based identity)
Overall this is a clean, well-structured migration from raw private keys to BIP39 mnemonics with MnemonicIdentity. Architecture is sound — derivation paths align between deriveNostrKeyFromMnemonic (wallet) and MnemonicIdentity (SDK), both resolving to m/86'/coinType'/0'/0/0. Legacy SingleKey wallets correctly preserved. Tests cover the new module. Good work.
🔴 Must fix
1. unlockWallet swallows "Invalid password" for mnemonic wallets — src/providers/wallet.tsx (diff hunk around unlockWallet)
The old code had two separate try/catch blocks: one for password validation (getPrivateKey), one for initWallet. The new code merges them:
try {
if (hasMnemonic()) {
const mnemonic = await getMnemonic(password) // throws DOMException on wrong password
setAuthState('authenticated')
await initWallet({ mnemonic })
} else { ... }
} catch (err) {
setAuthState('locked')
throw err instanceof Error ? err : new Error('Invalid password')
}When getMnemonic(password) fails due to a wrong password, AES-GCM throws a DOMException ("The operation failed for an operation-specific reason"). Since DOMException extends Error, err instanceof Error is true, so the cryptic crypto error propagates to the user instead of "Invalid password".
Fix: Separate password validation from wallet init, mirroring the old pattern:
if (hasMnemonic()) {
let mnemonic: string
try {
mnemonic = await getMnemonic(password)
} catch {
setAuthState('locked')
throw new Error('Invalid password')
}
setAuthState('authenticated')
try {
await initWallet({ mnemonic })
} catch (err) {
setAuthState('locked')
throw err
}
} else { ... same pattern for getPrivateKey ... }🟡 Should fix
2. Duplicate isMainnet logic — src/providers/wallet.tsx + src/screens/Init/Restore.tsx
The same network detection logic is copy-pasted:
wallet.tsx:const isMainnet = (network) => network !== 'testnet' && network !== 'mutinynet' && ...Restore.tsx:const isNet = aspInfo.network !== 'testnet' && aspInfo.network !== 'mutinynet' && ...
Extract to a shared utility (e.g., in lib/wallet.ts or lib/mnemonic.ts) to avoid drift.
3. deriveNostrKeyFromMnemonic — unsafe non-null assertion — src/lib/mnemonic.ts:31
return derived.privateKey!BIP32 derivation from a valid seed should always yield a private key, but a defensive check prevents silent undefined propagation if the invariant is ever broken:
const pk = derived.privateKey
if (!pk) throw new Error('BIP32 derivation failed: no private key')
return pkℹ️ Observations (no action required)
clearStoragecovers mnemonic wipe:localStorage.clear()inclearStorage()removesencrypted_mnemonicon wallet reset. ✅- Derivation path alignment verified:
deriveNostrKeyFromMnemonicandMnemonicIdentityboth use BIP86m/86'/coinType'/0'/0/0. Nostr backup/restore keys will match. ✅ restartWalletidentity reuse: ReusingsvcWallet.identitydirectly instead of extracting hex is cleaner and works becauseMnemonicIdentityis stateless. ✅- Restore input detection: Using
trimmed.includes(' ')to distinguish mnemonic from nsec/hex is reasonable —validateMnemonichandles word count and checksum validation.
There was a problem hiding this comment.
Arkana Follow-up Review — wallet#545 (post c3f0ce29)
New commit c3f0ce29 merges duplicate @arkade-os/sdk imports — looks correct, no issues.
All three findings from my previous review remain unaddressed:
🔴 Still open — unlockWallet wrong-password UX (src/providers/wallet.tsx:572-587)
getMnemonic(password) throws a DOMException on wrong password. Since DOMException extends Error, the err instanceof Error check on line 585 is true, so the raw crypto error ("The operation failed for an operation-specific reason") propagates to the user instead of "Invalid password". Separate the password-validation try/catch from the wallet-init try/catch as suggested in my prior review.
🟡 Still open — Duplicate isMainnet logic (wallet.tsx:537 vs Restore.tsx:79-83)
Same network-detection logic copy-pasted in two places. Extract to a shared utility.
🟡 Still open — Unsafe privateKey! assertion (src/lib/mnemonic.ts:31)
derived.privateKey! — add a defensive null check with a descriptive error.
There was a problem hiding this comment.
Arkana Follow-up Review — wallet#545 (post c3f0ce29)
New commit c3f0ce29 merges duplicate @arkade-os/sdk imports — looks correct, no issues.
All three findings from my previous review remain unaddressed:
🔴 Still open — unlockWallet wrong-password UX (src/providers/wallet.tsx:572-587)
getMnemonic(password) throws a DOMException on wrong password. Since DOMException extends Error, the err instanceof Error check on line 585 is true, so the raw crypto error ("The operation failed for an operation-specific reason") propagates to the user instead of "Invalid password". Separate the password-validation try/catch from the wallet-init try/catch as suggested in my prior review.
🟡 Still open — Duplicate isMainnet logic (wallet.tsx:537 vs Restore.tsx:79-83)
Same network-detection logic copy-pasted in two places. Extract to a shared utility.
🟡 Still open — Unsafe privateKey! assertion (src/lib/mnemonic.ts:31)
derived.privateKey! — add a defensive null check with a descriptive error.
There was a problem hiding this comment.
Arkana Follow-up Review — wallet#545 (post ef73fd94)
New commits reviewed
d598ffd5— e2e test updates for mnemonic walletsef73fd94— CodeRabbit feedback (validateMnemonic guard, sentence-case copy, invalid mnemonic test)
New changes look good. validateMnemonic guard in deriveNostrKeyFromMnemonic is the right fix — fails fast on bad input instead of silently deriving a wrong key. E2e tests properly handle both mnemonic and legacy wallet types via the getSecret utility. Sentence-case normalization is clean. ✅
Prior findings — still open
🔴 unlockWallet wrong-password UX — src/providers/wallet.tsx:585
Still the same issue. When getMnemonic(password) fails due to wrong password, crypto.subtle.decrypt throws a DOMException (which extends Error). The catch block:
throw err instanceof Error ? err : new Error('Invalid password')...passes the DOMException straight through because DOMException instanceof Error === true. User sees a raw crypto error instead of "Invalid password". Fix:
catch (err) {
setAuthState('locked')
if (err instanceof DOMException) throw new Error('Invalid password')
throw err instanceof Error ? err : new Error('Invalid password')
}🟡 Unsafe privateKey! assertion — src/lib/mnemonic.ts:36
The new validateMnemonic guard reduces risk here, but derived.privateKey can still be null if BIP32 derivation hits an edge case (probability ~2^-128, but defensive code costs one line):
if (!derived.privateKey) throw new Error('BIP32 derivation yielded no private key')
return derived.privateKey🤖 Reviewed by Arkana
There was a problem hiding this comment.
Arkana Follow-up Review — wallet#545 (post ef73fd94)
New commits reviewed
d598ffd5— e2e test updates for mnemonic walletsef73fd94— CodeRabbit feedback (validateMnemonic guard, sentence-case copy, invalid mnemonic test)
New changes look good. validateMnemonic guard in deriveNostrKeyFromMnemonic is the right fix — fails fast on bad input instead of silently deriving a wrong key. E2e tests properly handle both mnemonic and legacy wallet types via the getSecret utility. Sentence-case normalization is clean. ✅
Prior findings — still open
🔴 unlockWallet wrong-password UX — src/providers/wallet.tsx:585
Still the same issue. When getMnemonic(password) fails due to wrong password, crypto.subtle.decrypt throws a DOMException (which extends Error). The catch block:
throw err instanceof Error ? err : new Error('Invalid password')...passes the DOMException straight through because DOMException instanceof Error === true. User sees a raw crypto error instead of "Invalid password". Fix:
catch (err) {
setAuthState('locked')
if (err instanceof DOMException) throw new Error('Invalid password')
throw err instanceof Error ? err : new Error('Invalid password')
}🟡 Unsafe privateKey! assertion — src/lib/mnemonic.ts:36
The new validateMnemonic guard reduces risk here, but derived.privateKey can still be null if BIP32 derivation hits an edge case (probability ~2^-128, but defensive code costs one line):
if (!derived.privateKey) throw new Error('BIP32 derivation yielded no private key')
return derived.privateKey🤖 Reviewed by Arkana
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/test/e2e/utils.ts (1)
232-238: Nit:restoreWallet'snsecparameter is now misleading.Since the helper now receives either a 12-word mnemonic or an
nsec/hex key (see the matching rename ofgetNsec→getSecretjust above), consider renaming the param for clarity.✏️ Proposed tweak
-async function restoreWallet(page: Page, nsec: string): Promise<void> { +async function restoreWallet(page: Page, secret: string): Promise<void> { await page.getByText('Other login options').click() await page.getByText('Restore wallet').click() - await page.locator('ion-input[name="private-key"] input').fill(nsec) + await page.locator('ion-input[name="private-key"] input').fill(secret) await page.getByText('Continue').click() await waitForWalletPage(page) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/e2e/utils.ts` around lines 232 - 238, Rename the misleading parameter nsec in the restoreWallet function to a clearer name like secret (or privateKeyOrMnemonic) to reflect it may be a 12-word mnemonic or an nsec/hex key; update the function signature async function restoreWallet(page: Page, secret: string): Promise<void> and replace all internal references to use secret, and update any call sites that pass getSecret (formerly getNsec) so they pass the new parameter name; keep the locator 'ion-input[name="private-key"]' unchanged but ensure tests compile after renaming.src/screens/Init/Restore.tsx (1)
82-104: RenameisNetand dedupe mainnet detection withwallet.tsx.Two things here:
isNetreads like a boolean meaning "has a network" — it actually meansisMainnet. Rename for clarity.- This mainnet-by-exclusion check is duplicated verbatim by the
isMainnethelper insrc/providers/wallet.tsx(lines 538–539). If a new testnet variant is added toNetworkName, forgetting one site will silently derive mainnet keys from the recovery phrase on that network — a non-trivial hazard. Extract a single helper (e.g.,src/lib/network.ts) and import it in both files.♻️ Proposed refactor
New shared helper (illustrative):
// src/lib/network.ts import { NetworkName } from '@arkade-os/sdk' export const isMainnetNetwork = (network: NetworkName | string): boolean => network !== 'testnet' && network !== 'mutinynet' && network !== 'signet' && network !== 'regtest'Then in
Restore.tsx:- let seckey: Uint8Array - if (mnemonic) { - setInitInfo({ mnemonic, password: defaultPassword, restoring: true }) - const isNet = - aspInfo.network !== 'testnet' && - aspInfo.network !== 'mutinynet' && - aspInfo.network !== 'signet' && - aspInfo.network !== 'regtest' - seckey = deriveNostrKeyFromMnemonic(mnemonic, isNet) - } else { + let seckey: Uint8Array + if (mnemonic) { + setInitInfo({ mnemonic, password: defaultPassword, restoring: true }) + seckey = deriveNostrKeyFromMnemonic(mnemonic, isMainnetNetwork(aspInfo.network)) + } else { setInitInfo({ privateKey, password: defaultPassword, restoring: true }) seckey = privateKey! }And in
wallet.tsx, replace the localisMainnethelper with the same import.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/screens/Init/Restore.tsx` around lines 82 - 104, Rename the local boolean isNet in handleProceed to a clear name like isMainnet and replace its duplicated mainnet-by-exclusion logic by creating a shared helper (e.g., export const isMainnetNetwork(network: NetworkName | string): boolean) in a new module (suggested src/lib/network.ts); import and use that helper inside Restore.tsx when calling deriveNostrKeyFromMnemonic(mnemonic, isMainnet) and also replace the duplicated isMainnet helper in src/providers/wallet.tsx with the same shared import so both deriveNostrKeyFromMnemonic and wallet code rely on one canonical mainnet check.src/providers/wallet.tsx (1)
538-570:initWalletsilently prefers mnemonic when both are passed; duplicateisMainnetwith Restore.tsx.Two observations:
- If a caller accidentally passes both
mnemonicandprivateKey,mnemonicwins andprivateKeyis ignored without warning. Given both paths derive different identities and pubkeys, a stricter guard would catch upstream mistakes earlier.isMainnet(538–539) duplicates the exclusion-list logic insrc/screens/Init/Restore.tsx. See the consolidated suggestion on that file — extracting a single helper tosrc/lib/network.tsavoids the two getting out of sync when a new test network is added.🛡️ Optional stricter guard
- if (credentials.mnemonic) { + if (credentials.mnemonic && credentials.privateKey) { + throw new Error('Provide either mnemonic or privateKey, not both') + } + if (credentials.mnemonic) { const mnemonicIdentity = MnemonicIdentity.fromMnemonic(credentials.mnemonic, { isMainnet: isMainnet(network) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/providers/wallet.tsx` around lines 538 - 570, initWallet silently prefers mnemonic when both mnemonic and privateKey are provided and also duplicates the isMainnet logic; fix by (1) adding an explicit guard at the start of initWallet that throws or returns an error if both credentials.mnemonic and credentials.privateKey are present so callers are not silently misled (reference: initWallet, credentials.mnemonic, credentials.privateKey), and (2) remove the inline isMainnet definition and import a single shared helper (e.g., isMainnet or isTestNetwork helper) from your consolidated network utility (extract the current logic from this file and Restore.tsx into one function in your network utility and reference that from initWallet) so the exclusion-list logic is centralized and not duplicated.src/screens/Settings/Backup.tsx (1)
76-88: Optional: renameshowPrivateKeyfor consistency with the new secret abstraction.The handler now reveals either a mnemonic or an nsec, so
showPrivateKeyis misleading againstsecret/showSecret/secretLabel. Also,result ?? ''on line 84 is dead (verifyPassword always resolves to a string) and can be simplified.♻️ Proposed rename + simplification
- const showPrivateKey = async () => { + const revealSecret = async () => { if (!secret) { const password = wallet.lockedByBiometrics ? await authenticateUser(wallet.passkeyId).catch(setError) : enteredPassword.current if (!password) return const result = await verifyPassword(password) setError(result ? '' : 'Invalid password') - setSecret(result ?? '') + setSecret(result) } setShowSecret(true) setDialog(false) }And update the button binding at line 154:
- <Button onClick={showPrivateKey} label='Confirm' /> + <Button onClick={revealSecret} label='Confirm' />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/screens/Settings/Backup.tsx` around lines 76 - 88, Rename the handler showPrivateKey to a neutral name like revealSecret or handleRevealSecret to reflect it can expose either a mnemonic or nsec (ties to secret/secretLabel/showSecret), update any UI/button that binds the old showPrivateKey reference to the new name, and simplify the verifyPassword result handling by removing the dead `result ?? ''` expression and directly using the returned string (setError(result ? '' : 'Invalid password'); setSecret(result);). Ensure you update all usages of showPrivateKey to the new handler name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/providers/wallet.tsx`:
- Around line 572-587: unlockWallet currently rethrows raw errors from
getMnemonic/getPrivateKey (which propagate DOMException from
crypto.subtle.decrypt) causing callers to miss the expected "Invalid password"
string; update unlockWallet to catch decryption-related errors from
getMnemonic/getPrivateKey and normalize them to a new Error('Invalid password')
while still preserving and rethrowing non-decryption errors; specifically,
inside unlockWallet (which calls getMnemonic, getPrivateKey, setAuthState and
initWallet) detect DOMException or errors whose message/constructor indicates a
decryption failure and throw a consistent Error('Invalid password') after
setting setAuthState('locked'), otherwise rethrow the original error.
---
Nitpick comments:
In `@src/providers/wallet.tsx`:
- Around line 538-570: initWallet silently prefers mnemonic when both mnemonic
and privateKey are provided and also duplicates the isMainnet logic; fix by (1)
adding an explicit guard at the start of initWallet that throws or returns an
error if both credentials.mnemonic and credentials.privateKey are present so
callers are not silently misled (reference: initWallet, credentials.mnemonic,
credentials.privateKey), and (2) remove the inline isMainnet definition and
import a single shared helper (e.g., isMainnet or isTestNetwork helper) from
your consolidated network utility (extract the current logic from this file and
Restore.tsx into one function in your network utility and reference that from
initWallet) so the exclusion-list logic is centralized and not duplicated.
In `@src/screens/Init/Restore.tsx`:
- Around line 82-104: Rename the local boolean isNet in handleProceed to a clear
name like isMainnet and replace its duplicated mainnet-by-exclusion logic by
creating a shared helper (e.g., export const isMainnetNetwork(network:
NetworkName | string): boolean) in a new module (suggested src/lib/network.ts);
import and use that helper inside Restore.tsx when calling
deriveNostrKeyFromMnemonic(mnemonic, isMainnet) and also replace the duplicated
isMainnet helper in src/providers/wallet.tsx with the same shared import so both
deriveNostrKeyFromMnemonic and wallet code rely on one canonical mainnet check.
In `@src/screens/Settings/Backup.tsx`:
- Around line 76-88: Rename the handler showPrivateKey to a neutral name like
revealSecret or handleRevealSecret to reflect it can expose either a mnemonic or
nsec (ties to secret/secretLabel/showSecret), update any UI/button that binds
the old showPrivateKey reference to the new name, and simplify the
verifyPassword result handling by removing the dead `result ?? ''` expression
and directly using the returned string (setError(result ? '' : 'Invalid
password'); setSecret(result);). Ensure you update all usages of showPrivateKey
to the new handler name.
In `@src/test/e2e/utils.ts`:
- Around line 232-238: Rename the misleading parameter nsec in the restoreWallet
function to a clearer name like secret (or privateKeyOrMnemonic) to reflect it
may be a 12-word mnemonic or an nsec/hex key; update the function signature
async function restoreWallet(page: Page, secret: string): Promise<void> and
replace all internal references to use secret, and update any call sites that
pass getSecret (formerly getNsec) so they pass the new parameter name; keep the
locator 'ion-input[name="private-key"]' unchanged but ensure tests compile after
renaming.
🪄 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: a55e7fb7-2446-4f3c-a405-c1a1548ede42
📒 Files selected for processing (7)
src/lib/mnemonic.tssrc/providers/wallet.tsxsrc/screens/Init/Restore.tsxsrc/screens/Settings/Backup.tsxsrc/test/e2e/backup.test.tssrc/test/e2e/utils.tssrc/test/lib/mnemonic.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/test/lib/mnemonic.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/mnemonic.ts
There was a problem hiding this comment.
Arkana Follow-up Review — wallet#545 (post ee33a0e8)
Commit ee33a0e8 addresses both outstanding findings from my previous review:
-
✅ DOMException → "Invalid password" —
src/providers/wallet.tsx:585:DOMExceptionfromcrypto.subtle.decryptis now caught and re-thrown as a cleanError('Invalid password'). Users no longer see raw crypto errors on wrong password. -
✅ Null-guard on
derived.privateKey—src/lib/mnemonic.ts:34-35: Non-null assertion replaced with explicit check + descriptive error. Defensive and correct.
Minor note: The DOMException catch is inside a try block that also wraps initWallet(). If initWallet ever threw a DOMException, it would be masked as "Invalid password". In practice this is fine — initWallet does SDK/network ops, not WebCrypto — but worth knowing.
No outstanding issues from my side. All prior feedback addressed.
🤖 Reviewed by Arkana
There was a problem hiding this comment.
Arkana Follow-up Review — wallet#545 (post 0d70ff35)
Single new commit: 0d70ff35 — switches service worker identity from MnemonicIdentity to SingleKey.fromPrivateKey(deriveNostrKeyFromMnemonic(...)).
Why: MnemonicIdentity lacks toHex(), which the service worker serialization requires. SingleKey implements it.
Verified:
- ✅
deriveNostrKeyFromMnemonicusesm/86'/coinType'/0'/0/0— identical toMnemonicIdentity.fromMnemonic()in ts-sdk (seedIdentity.ts:63-72). Same key, different wrapper. No identity mismatch. - ✅
MnemonicIdentityimport fully removed; no stale references. - ✅
pubkeynow derived viaidentity.compressedPublicKey()(consistent withSingleKeyAPI, same underlying secp256k1 math).
One naming nit (non-blocking): deriveNostrKeyFromMnemonic is now the primary wallet identity derivation, not just a Nostr helper. The JSDoc still says "Used for Nostr backup operations." Consider renaming to deriveKeyFromMnemonic and updating the doc — this avoids future confusion about whether Nostr and wallet identity use different keys.
No bugs. No protocol-correctness issues.
🤖 Reviewed by Arkana
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/providers/wallet.tsx (1)
571-587: Consider movingsetAuthState('authenticated')to afterinitWalletresolves.Today, if
getMnemonic/getPrivateKeysucceeds butinitSvcWorkerWalletlater fails (SW activation timeout, message bus timeout, migration error — all of whichinitSvcWorkerWalletrethrows), the flow briefly transitionslocked → authenticated → locked. Because downstream data gating usesinitialized, this doesn't corrupt data, but any effect or render keyed offauthState === 'authenticated'(navigation, analytics, UI) will fire for a failed unlock.Moving the state transition to after
await initWallet(...)keepsauthStatemonotonic for a given unlock attempt:Proposed change
const unlockWallet = async (password: string) => { try { if (hasMnemonic()) { const mnemonic = await getMnemonic(password) - setAuthState('authenticated') await initWallet({ mnemonic }) } else { const privateKey = await getPrivateKey(password) - setAuthState('authenticated') await initWallet({ privateKey }) } + setAuthState('authenticated') } catch (err) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/providers/wallet.tsx` around lines 571 - 587, unlockWallet currently sets setAuthState('authenticated') before awaiting initWallet, causing a transient authenticated state if initWallet fails; move the setAuthState('authenticated') call to after the awaited initWallet({ mnemonic }) and initWallet({ privateKey }) calls so authentication is only set on successful initialization, keeping the existing try/catch and DOMException handling intact and ensuring both branches (getMnemonic/getPrivateKey → initWallet) update authState only after initWallet resolves.src/lib/mnemonic.ts (2)
67-71:atob+split('')+charCodeAtis fine here but brittle.For small payloads (salt 16 + iv 12 + AES-GCM ciphertext of a 12-word mnemonic ≈ 150 bytes) this is safe. If the helper is ever reused to encrypt larger secrets, prefer a byte-accurate decoder (e.g.,
Uint8Array.from(atob(s), c => c.charCodeAt(0))) to avoid subtle bugs with surrogate pairs and to drop an unnecessary intermediate string/array.Minor simplification
- const combined = new Uint8Array( - atob(encryptedMnemonic) - .split('') - .map((c) => c.charCodeAt(0)), - ) + const combined = Uint8Array.from(atob(encryptedMnemonic), (c) => c.charCodeAt(0))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mnemonic.ts` around lines 67 - 71, The current construction of combined using atob(encryptedMnemonic).split('').map(c => c.charCodeAt(0)) is brittle and creates an extra intermediate array and can mishandle surrogate pairs; replace it with a byte-accurate single-step conversion such as using Uint8Array.from(atob(encryptedMnemonic), c => c.charCodeAt(0)) so the code in mnemonic.ts that produces combined (the combined variable) decodes base64 to bytes reliably and without the unnecessary split/map intermediate.
38-92: Extract shared PBKDF2 + AES-GCM crypto helper to eliminate duplication.
encryptMnemonic/decryptMnemonicduplicate the crypto logic fromencryptPrivateKey/decryptPrivateKeyinprivateKey.ts. Both implement identical PBKDF2 + AES-GCM encryption with the same parameters (100000 iterations, SHA-256, 256-bit AES-GCM key). The hardcoded salt/IV/offset values (16, 12, 28) appear in both files.Future changes to crypto parameters risk inconsistency across the codebase. Consider extracting a shared pair of helpers (e.g.,
encryptWithPassword/decryptWithPasswordin a new or existingcrypto.tsutility module) and adding named constants for salt length, IV length, and combined data offsets to reduce duplication and ensure consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mnemonic.ts` around lines 38 - 92, encryptMnemonic and decryptMnemonic duplicate PBKDF2+AES‑GCM logic present in encryptPrivateKey/decryptPrivateKey; extract that shared logic into a new utility (e.g., encryptWithPassword/decryptWithPassword in crypto.ts) and replace the implementations in mnemonic.ts and privateKey.ts to call those helpers, and define named constants (SALT_LEN = 16, IV_LEN = 12, HEADER_LEN = SALT_LEN + IV_LEN or COMBINED_OFFSET = 28) to replace hardcoded values so all crypto parameters and offsets are centralized and consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/providers/wallet.tsx`:
- Around line 198-210: detectPasswordState currently calls
getMnemonic(defaultPassword) which triggers an expensive PBKDF2 on every
wallet.pubkey change; instead, when you save a mnemonic in setMnemonic, persist
a boolean flag (e.g., isDefaultPassword) alongside the encrypted blob indicating
whether the defaultPassword was used, and change detectPasswordState to
synchronously read that flag for the hasMnemonic branch (or mirror the existing
noUserDefinedPassword flag) rather than calling getMnemonic; update setMnemonic
to set the flag and update detectPasswordState (and any callers expecting a
promise) so it returns/uses the synchronous flag value and avoids the PBKDF2
decrypt probe.
---
Nitpick comments:
In `@src/lib/mnemonic.ts`:
- Around line 67-71: The current construction of combined using
atob(encryptedMnemonic).split('').map(c => c.charCodeAt(0)) is brittle and
creates an extra intermediate array and can mishandle surrogate pairs; replace
it with a byte-accurate single-step conversion such as using
Uint8Array.from(atob(encryptedMnemonic), c => c.charCodeAt(0)) so the code in
mnemonic.ts that produces combined (the combined variable) decodes base64 to
bytes reliably and without the unnecessary split/map intermediate.
- Around line 38-92: encryptMnemonic and decryptMnemonic duplicate
PBKDF2+AES‑GCM logic present in encryptPrivateKey/decryptPrivateKey; extract
that shared logic into a new utility (e.g.,
encryptWithPassword/decryptWithPassword in crypto.ts) and replace the
implementations in mnemonic.ts and privateKey.ts to call those helpers, and
define named constants (SALT_LEN = 16, IV_LEN = 12, HEADER_LEN = SALT_LEN +
IV_LEN or COMBINED_OFFSET = 28) to replace hardcoded values so all crypto
parameters and offsets are centralized and consistent.
In `@src/providers/wallet.tsx`:
- Around line 571-587: unlockWallet currently sets setAuthState('authenticated')
before awaiting initWallet, causing a transient authenticated state if
initWallet fails; move the setAuthState('authenticated') call to after the
awaited initWallet({ mnemonic }) and initWallet({ privateKey }) calls so
authentication is only set on successful initialization, keeping the existing
try/catch and DOMException handling intact and ensuring both branches
(getMnemonic/getPrivateKey → initWallet) update authState only after initWallet
resolves.
🪄 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: f8512808-b401-4d84-a02c-1888dee6deac
📒 Files selected for processing (2)
src/lib/mnemonic.tssrc/providers/wallet.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/providers/wallet.tsx (1)
724-732:⚠️ Potential issue | 🟠 MajorClear the in-memory mnemonic on reset.
lockWalletclearsmnemonicRef, butresetWalletleaves the plaintext mnemonic in memory after storage and repositories are cleared. Reset should remove that secret too.Proposed fix
const resetWallet = async () => { if (!svcWallet) throw new Error('Service worker not initialized') + mnemonicRef.current = undefined await clearStorage() await svcWallet.clear() await svcWallet.walletRepository.clear()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/providers/wallet.tsx` around lines 724 - 732, The resetWallet function currently clears storage and repositories but leaves the in-memory plaintext mnemonic; update resetWallet to also clear the in-memory secret the same way lockWallet does by setting mnemonicRef.current to null (or empty string) and any related in-memory mnemonic fields to undefined/null so the plaintext is removed from memory; locate resetWallet and mnemonicRef (and lockWallet for reference) and apply the same clearing logic there.
♻️ Duplicate comments (1)
src/providers/wallet.tsx (1)
201-210:⚠️ Potential issue | 🟡 MinorAvoid decrypting the mnemonic just to classify password state.
This still runs PBKDF2/AES-GCM on every
wallet.pubkeyauth-state check for custom-password mnemonic wallets. Persist a passwordless/default-password flag alongside the encrypted mnemonic and read that instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/providers/wallet.tsx` around lines 201 - 210, detectPasswordState is currently decrypting the mnemonic via getMnemonic (triggering PBKDF2/AES-GCM) to determine password state; instead persist a boolean flag (e.g., isPasswordless or usesDefaultPassword) alongside the encrypted mnemonic when creating/updating the wallet and read that flag in detectPasswordState (use hasMnemonic() and the stored flag) to return true/false without calling getMnemonic; update the mnemonic storage routines to set this flag and remove the try/catch decrypt path from detectPasswordState so wallet.pubkey auth-state checks no longer perform expensive decryption.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/providers/wallet.tsx`:
- Around line 477-480: The timeout in the INIT_WALLET flow currently rejects
with new Error('INIT_WALLET timed out'), which bypasses the existing retry
predicate that only recognizes activation failures or "MessageBus timed out";
change the rejection to use the same error text the retry logic expects (e.g.
reject(new Error('MessageBus timed out'))) so the timer in this block (timer,
cleanup, reject, MESSAGE_BUS_INIT_TIMEOUT_MS) will trigger the existing
retry/unregister recovery instead of being treated as a non-retryable failure.
- Around line 439-451: The INITIALIZE_MESSAGE_BUS config sent in
serviceWorker.postMessage (tag: 'INITIALIZE_MESSAGE_BUS') is missing
arkServer.publicKey and indexerUrl causing undefined values downstream; update
the config object in the mnemonic initialization branch to include
arkServer.publicKey (alongside arkServer.url) and indexerUrl (alongside
esploraUrl) so the worker's buildServices handler and
Wallet.create/ReadonlyWallet.create receive both values.
---
Outside diff comments:
In `@src/providers/wallet.tsx`:
- Around line 724-732: The resetWallet function currently clears storage and
repositories but leaves the in-memory plaintext mnemonic; update resetWallet to
also clear the in-memory secret the same way lockWallet does by setting
mnemonicRef.current to null (or empty string) and any related in-memory mnemonic
fields to undefined/null so the plaintext is removed from memory; locate
resetWallet and mnemonicRef (and lockWallet for reference) and apply the same
clearing logic there.
---
Duplicate comments:
In `@src/providers/wallet.tsx`:
- Around line 201-210: detectPasswordState is currently decrypting the mnemonic
via getMnemonic (triggering PBKDF2/AES-GCM) to determine password state; instead
persist a boolean flag (e.g., isPasswordless or usesDefaultPassword) alongside
the encrypted mnemonic when creating/updating the wallet and read that flag in
detectPasswordState (use hasMnemonic() and the stored flag) to return true/false
without calling getMnemonic; update the mnemonic storage routines to set this
flag and remove the try/catch decrypt path from detectPasswordState so
wallet.pubkey auth-state checks no longer perform expensive decryption.
🪄 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: a5b52d8c-4dd8-49e0-b1d7-23dd700082b6
📒 Files selected for processing (5)
package.jsonsrc/lib/privateKey.tssrc/providers/wallet.tsxsrc/screens/Settings/Password.tsxsrc/wallet-service-worker.ts
✅ Files skipped from review due to trivial changes (1)
- package.json
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
- Run prettier --write on all src files to fix pre-existing formatting - Fix eslint glob in lint scripts: use escaped double quotes instead of single quotes for cross-platform compatibility (Windows+Unix)
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.
Kukks
force-pushed
the
feat/mnemonic-identity
branch
from
d5bbac2 to
dafa539
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
src/providers/wallet.tsx (2)
439-451:⚠️ Potential issue | 🟠 MajorSend the full worker config required by
buildServices.The worker reads
config.arkServer.publicKeyandconfig.indexerUrl, but this mnemonic init payload only sendsarkServer.urlandesploraUrl. That can passundefinedinto the worker’sWallet.create()/ReadonlyWallet.create()path.Run this read-only check to confirm the producer/consumer mismatch:
#!/bin/bash rg -n -C3 'INITIALIZE_MESSAGE_BUS|arkServer\.publicKey|indexerUrl|Wallet\.create|ReadonlyWallet\.create' --glob '*.ts' --glob '*.tsx'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/providers/wallet.tsx` around lines 439 - 451, The INITIALIZE_MESSAGE_BUS payload sent via serviceWorker.postMessage is missing fields the worker expects (config.arkServer.publicKey and config.indexerUrl), causing undefined to flow into buildServices -> Wallet.create / ReadonlyWallet.create; update the payload constructed in the serviceWorker.postMessage call to include the full worker config: add arkServer.publicKey (using the same source as arkServerUrl) and indexerUrl (using the app's indexer URL variable), and ensure settlementConfig and other existing keys are unchanged so buildServices receives a complete config object.
477-480:⚠️ Potential issue | 🟠 MajorKeep
INIT_WALLETtimeouts retryable.The retry predicate below only matches activation failures or
"MessageBus timed out", so this timeout bypasses the existing retry/unregister recovery.🐛 Proposed fix
const timer = setTimeout(() => { cleanup() - reject(new Error('INIT_WALLET timed out')) + reject(new Error('MessageBus timed out during INIT_WALLET')) }, MESSAGE_BUS_INIT_TIMEOUT_MS)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/providers/wallet.tsx` around lines 477 - 480, The timeout handler for INIT_WALLET currently rejects with 'INIT_WALLET timed out' which bypasses the existing retry/unregister logic; update the timeout rejection in the timer inside the INIT_WALLET flow so it either rejects with the same "MessageBus timed out" message or throws an error type that the existing retry predicate recognizes (reference the timer constant MESSAGE_BUS_INIT_TIMEOUT_MS, the cleanup() call, and the reject() invocation) so the retry/unregister recovery path is triggered instead of being bypassed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/providers/wallet.tsx`:
- Around line 653-664: When initializing a wallet from a private key and when
performing a wallet reset, explicitly clear any lingering mnemonic reference to
avoid sending stale recovery material: set mnemonicRef.current = undefined (or
null) before calling initSvcWorkerWallet in the private-key branch (the code
that uses SingleKey.fromPrivateKey, hex.encode, updateConfig and
initSvcWorkerWallet) and likewise clear mnemonicRef inside the
reset/restartWallet code path so restartWallet cannot reuse the old mnemonic
after switching credentials.
In `@src/screens/Init/Connect.tsx`:
- Around line 28-40: The effect currently returns early when password is falsy
and otherwise only handles branches for mnemonic or privateKey, which can leave
the loader hanging if credentials are missing; update the useEffect to "fail
fast": if password is missing call abortConnectionWithError with a clear Error
(e.g., "Missing password") and return, and after the mnemonic/privateKey
branches add an else that calls abortConnectionWithError with a clear Error
(e.g., "No mnemonic or privateKey provided") so
setMnemonic/setPrivateKey/initWallet aren't awaited indefinitely and the
component can exit loading state (refer to the useEffect block and functions
setMnemonic, setPrivateKey, initWallet, abortConnectionWithError,
setInitialized).
In `@src/screens/Init/Restore.tsx`:
- Around line 132-137: The Input for the secret should be made uncontrolled to
avoid persisting sensitive text in React state: remove the value={someKey} prop
from the <Input name='private-key' ... /> usage in Restore.tsx and keep the
onChange handler (setSomeKey) if you still need to capture the input
transiently; ensure no other code reads someKey to re-render the secret and
confirm the Input only receives name='private-key', label='Recovery phrase or
private key', and onChange={setSomeKey} (no value or controlled props).
- Around line 44-62: When the input is cleared or contains only whitespace,
clear stored credentials and UI state: ensure the useEffect handles the
early-return case for !someKey or trimmed === '' by calling
setMnemonic(undefined), setPrivateKey(undefined), setLabel(buttonLabel) (or an
appropriate default), and setError('') so the previous secret cannot be reused;
also normalize and detect mnemonics by treating any whitespace (/\s/) as word
separators — e.g. trim then collapse internal whitespace to single spaces before
passing to validateMnemonic(trimmedNormalized, wordlist) and only route to
private-key parsing when there are truly no whitespace characters.
In `@src/screens/Settings/Backup.tsx`:
- Around line 76-87: The showPrivateKey handler currently sets
setShowSecret(true) and closes the dialog regardless of verifyPassword outcome;
change it so that after calling verifyPassword(password) you only call
setSecret(result), setShowSecret(true) and setDialog(false) when result is
truthy (verification succeeded), and on failure only call setError('Invalid
password') (and do not change setShowSecret or setDialog). Update logic in
showPrivateKey to gate revealing/closing behind the successful verifyPassword
result and use the existing symbols: showPrivateKey, verifyPassword, setSecret,
setShowSecret, setDialog, setError, enteredPassword and wallet.
---
Duplicate comments:
In `@src/providers/wallet.tsx`:
- Around line 439-451: The INITIALIZE_MESSAGE_BUS payload sent via
serviceWorker.postMessage is missing fields the worker expects
(config.arkServer.publicKey and config.indexerUrl), causing undefined to flow
into buildServices -> Wallet.create / ReadonlyWallet.create; update the payload
constructed in the serviceWorker.postMessage call to include the full worker
config: add arkServer.publicKey (using the same source as arkServerUrl) and
indexerUrl (using the app's indexer URL variable), and ensure settlementConfig
and other existing keys are unchanged so buildServices receives a complete
config object.
- Around line 477-480: The timeout handler for INIT_WALLET currently rejects
with 'INIT_WALLET timed out' which bypasses the existing retry/unregister logic;
update the timeout rejection in the timer inside the INIT_WALLET flow so it
either rejects with the same "MessageBus timed out" message or throws an error
type that the existing retry predicate recognizes (reference the timer constant
MESSAGE_BUS_INIT_TIMEOUT_MS, the cleanup() call, and the reject() invocation) so
the retry/unregister recovery path is triggered instead of being bypassed.
🪄 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: 035cc315-3bf9-4db8-b890-8fb4f9cb632a
📒 Files selected for processing (15)
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.tsxsrc/wallet-service-worker.ts
✅ Files skipped from review due to trivial changes (4)
- src/providers/flow.tsx
- src/test/lib/mnemonic.test.ts
- package.json
- src/lib/mnemonic.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/wallet-service-worker.ts
- src/test/e2e/backup.test.ts
- src/test/screens/settings/backup.test.tsx
| useEffect(() => { | ||
| if (!password || !privateKey) return | ||
| setPrivateKey(privateKey, password) | ||
| .then(() => initWallet(privateKey)) | ||
| .then(() => setInitialized(true)) | ||
| .catch(abortConnectionWithError) | ||
| if (!password) return | ||
| if (mnemonic) { | ||
| setMnemonic(mnemonic, password) | ||
| .then(() => initWallet({ mnemonic })) | ||
| .then(() => setInitialized(true)) | ||
| .catch(abortConnectionWithError) | ||
| } else if (privateKey) { | ||
| setPrivateKey(privateKey, password) | ||
| .then(() => initWallet({ privateKey })) | ||
| .then(() => setInitialized(true)) | ||
| .catch(abortConnectionWithError) | ||
| } |
There was a problem hiding this comment.
Fail fast when init credentials are missing.
If InitConnect mounts with a missing password or with neither mnemonic nor privateKey, no branch completes and the loading screen can hang forever.
🐛 Proposed fix
useEffect(() => {
- if (!password) return
+ if (!password || (!mnemonic && !privateKey)) {
+ abortConnectionWithError(new Error('Missing wallet credentials'))
+ return
+ }
if (mnemonic) {
setMnemonic(mnemonic, password)
.then(() => initWallet({ mnemonic }))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/screens/Init/Connect.tsx` around lines 28 - 40, The effect currently
returns early when password is falsy and otherwise only handles branches for
mnemonic or privateKey, which can leave the loader hanging if credentials are
missing; update the useEffect to "fail fast": if password is missing call
abortConnectionWithError with a clear Error (e.g., "Missing password") and
return, and after the mnemonic/privateKey branches add an else that calls
abortConnectionWithError with a clear Error (e.g., "No mnemonic or privateKey
provided") so setMnemonic/setPrivateKey/initWallet aren't awaited indefinitely
and the component can exit loading state (refer to the useEffect block and
functions setMnemonic, setPrivateKey, initWallet, abortConnectionWithError,
setInitialized).
| useEffect(() => { | ||
| if (!someKey) return | ||
| let privateKey = undefined | ||
| const trimmed = someKey.trim() | ||
|
|
||
| // Detect mnemonic (input contains spaces) | ||
| if (trimmed.includes(' ')) { | ||
| if (validateMnemonic(trimmed, wordlist)) { | ||
| setMnemonic(trimmed) | ||
| setPrivateKey(undefined) | ||
| setLabel(buttonLabel) | ||
| setError('') | ||
| } else { | ||
| setMnemonic(undefined) | ||
| setPrivateKey(undefined) | ||
| setLabel('Invalid recovery phrase') | ||
| setError('Invalid recovery phrase') | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
Clear stale credentials and normalize pasted recovery phrases.
If the user clears the field after entering a valid secret, the effect returns before clearing mnemonic/privateKey, so Continue can still restore the previous secret. Also, newline/tab-separated recovery phrases are currently routed to private-key parsing.
🐛 Proposed fix
useEffect(() => {
- if (!someKey) return
- const trimmed = someKey.trim()
+ const trimmed = someKey?.trim() ?? ''
+ if (!trimmed) {
+ setMnemonic(undefined)
+ setPrivateKey(undefined)
+ setLabel(buttonLabel)
+ setError('')
+ return
+ }
+ const normalizedMnemonic = trimmed.replace(/\s+/g, ' ')
// Detect mnemonic (input contains spaces)
- if (trimmed.includes(' ')) {
- if (validateMnemonic(trimmed, wordlist)) {
- setMnemonic(trimmed)
+ if (/\s/.test(trimmed)) {
+ if (validateMnemonic(normalizedMnemonic, wordlist)) {
+ setMnemonic(normalizedMnemonic)
setPrivateKey(undefined)
setLabel(buttonLabel)
setError('')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/screens/Init/Restore.tsx` around lines 44 - 62, When the input is cleared
or contains only whitespace, clear stored credentials and UI state: ensure the
useEffect handles the early-return case for !someKey or trimmed === '' by
calling setMnemonic(undefined), setPrivateKey(undefined), setLabel(buttonLabel)
(or an appropriate default), and setError('') so the previous secret cannot be
reused; also normalize and detect mnemonics by treating any whitespace (/\s/) as
word separators — e.g. trim then collapse internal whitespace to single spaces
before passing to validateMnemonic(trimmedNormalized, wordlist) and only route
to private-key parsing when there are truly no whitespace characters.
| <Input | ||
| name='private-key' | ||
| label='Recovery phrase or private key' | ||
| onChange={setSomeKey} | ||
| value={someKey} | ||
| /> |
There was a problem hiding this comment.
Keep the restore secret input uncontrolled.
Passing value={someKey} can re-display the recovery phrase/private key from React state across re-renders/navigation. Remove the value prop and keep this input uncontrolled.
Based on learnings, the Input component for private key restoration intentionally omits the value prop to prevent sensitive data from being displayed if users navigate away and return.
🛡️ Proposed fix
<Input
name='private-key'
label='Recovery phrase or private key'
onChange={setSomeKey}
- value={someKey}
/>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Input | |
| name='private-key' | |
| label='Recovery phrase or private key' | |
| onChange={setSomeKey} | |
| value={someKey} | |
| /> | |
| <Input | |
| name='private-key' | |
| label='Recovery phrase or private key' | |
| onChange={setSomeKey} | |
| /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/screens/Init/Restore.tsx` around lines 132 - 137, The Input for the
secret should be made uncontrolled to avoid persisting sensitive text in React
state: remove the value={someKey} prop from the <Input name='private-key' ... />
usage in Restore.tsx and keep the onChange handler (setSomeKey) if you still
need to capture the input transiently; ensure no other code reads someKey to
re-render the secret and confirm the Input only receives name='private-key',
label='Recovery phrase or private key', and onChange={setSomeKey} (no value or
controlled props).
|
Ran the "Test plan" steps: they all pass. Will review it against the conversation and add a review myself. |
Triple-tapping the loading logo toggles a global dev mode persisted in localStorage (DevModeProvider). The tap logic is lifted out of LoadingLogo into the new context so any loading logo in the app shares the same state. A new "Contracts" entry appears in the Advanced settings list only when dev mode is active. It renders all contracts from ContractManager sorted active-first, each card showing type, state, address, and script — the latter two shortened and copyable. Pull-to-refresh is disabled on this screen since it is a static view on ContractManager data.
2 participants
Summary
MnemonicIdentityfrom the SDK for BIP86 Taproot key derivationINITIALIZE_MESSAGE_BUS, where a custombuildServicescreatesMnemonicIdentityinternally for signing — no key extraction needed on the main threadSingleKeywallets continue to work for unlock and restoreDOMExceptionfromcrypto.subtle.decryptderived.privateKeyTest plan
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Tests