[bordalix]

Summary by CodeRabbit

• Refactor
  • Consolidated encrypted credential storage management by centralizing storage key definitions, improving consistency and maintainability of how mnemonics and private keys are persisted and accessed throughout the application.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 1b0d7b60-40b0-4b1c-8ce2-69f5ec88fc7e

📥 Commits

Reviewing files that changed from the base of the PR and between 8da7062 and 255f1bb.

📒 Files selected for processing (3)

• src/lib/mnemonic.ts
• src/lib/privateKey.ts
• src/lib/storageKeys.ts

---

Walkthrough

Two exported string constants (MNEMONIC_STORAGE_KEY and NSEC_STORAGE_KEY) are added to a new src/lib/storageKeys.ts file. mnemonic.ts and privateKey.ts replace their previous local or hardcoded localStorage key strings with these shared constants, with no changes to encryption logic or exported function signatures.

Changes

Storage Key Centralization

Layer / File(s)	Summary
storageKeys constants definition src/lib/storageKeys.ts	Adds and exports MNEMONIC_STORAGE_KEY ('encrypted_mnemonic') and NSEC_STORAGE_KEY ('encrypted_private_key') as the canonical localStorage key strings.
Mnemonic and private key modules updated to use shared constants src/lib/mnemonic.ts, src/lib/privateKey.ts	Replaces all local/hardcoded storage key usages with the imported constants. setMnemonic now removes NSEC_STORAGE_KEY on write; setPrivateKey removes MNEMONIC_STORAGE_KEY on write, ensuring consistent cross-module cleanup.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'prevent double keys in local storage' directly summarizes the main objective of consolidating storage key constants and removing duplicate mnemonic entries, which is the core change across all modified files.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix_local_storage

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[cloudflare-workers-and-pages]

Deploying wallet-bitcoin with    Cloudflare Pages

Latest commit:	fc31333
Status:	✅  Deploy successful!
Preview URL:	https://d9aeccbc.wallet-bitcoin.pages.dev
Branch Preview URL:	https://fix-local-storage.wallet-bitcoin.pages.dev

View logs

[cloudflare-workers-and-pages]

Deploying wallet-mutinynet with    Cloudflare Pages

Latest commit:	fc31333
Status:	✅  Deploy successful!
Preview URL:	https://2323354e.arkade-wallet.pages.dev
Branch Preview URL:	https://fix-local-storage.arkade-wallet.pages.dev

View logs

[cloudflare-workers-and-pages]

Deploying tmp-boltz-upstream-mainnet-arkade-wallet with    Cloudflare Pages

Latest commit:	fc31333
Status:	✅  Deploy successful!
Preview URL:	https://eb0373d9.tmp-boltz-upstream-mainnet-arkade-wallet.pages.dev
Branch Preview URL:	https://fix-local-storage.tmp-boltz-upstream-mainnet-arkade-wallet.pages.dev

View logs

[ghost]

Code Review — PR #677: prevent double keys in local storage

Summary

The PR does two things:

1. Refactors hardcoded 'encrypted_mnemonic' / 'encrypted_private_key' strings into exported constants (MNEMONIC_STORAGE_KEY, NSEC_STORAGE_KEY)
2. Adds mutual exclusion: setMnemonic() removes any stored private key, and setPrivateKey() removes any stored mnemonic

The mutual-exclusion logic is correct in intent — a wallet should have one identity source, not both. But there's one real issue and one gap.

---

🔴 Issue: Circular dependency introduced

mnemonic.ts:4 — import { NSEC_STORAGE_KEY } from './privateKey'
privateKey.ts:3 — import { MNEMONIC_STORAGE_KEY } from './mnemonic'

This creates a static circular import. On master, privateKey.ts was deliberately careful to use a dynamic import for mnemonic (await import('./mnemonic') at privateKey.ts:59) to avoid this exact cycle.

While ES module semantics mean the const string literals should resolve before any function runs, circular dependencies are fragile and a known source of subtle bugs — especially across bundler versions. If module evaluation order ever changes (e.g. a new import is added to either file), you could get undefined constants silently.

Fix: Extract both constants into a shared file (e.g. src/lib/storageKeys.ts):

// src/lib/storageKeys.ts
export const MNEMONIC_STORAGE_KEY = 'encrypted_mnemonic'
export const NSEC_STORAGE_KEY = 'encrypted_private_key'

Then both mnemonic.ts and privateKey.ts import from storageKeys.ts. No cycle.

---

🟡 Missing test coverage

The new mutual-exclusion behavior (the actual point of this PR) has no tests. The existing mnemonic.test.ts and privatekey.test.ts don't cover:

• Setting a mnemonic clears any stored private key
• Setting a private key clears any stored mnemonic
• isValidPassword works correctly after a key-type switch

These are easy to add to the existing test files and would catch regressions in the exclusion logic.

---

✅ What's good

• The constant refactoring is a clear improvement — eliminates the raw string 'encrypted_mnemonic' in isValidPassword (privateKey.ts:60 on master)
• The mutual-exclusion semantics are correct: only one identity source at a time
• No protocol-critical code affected (no VTXO/signing/forfeit/round changes)

---

Requesting changes for the circular dependency. The fix is straightforward — extract constants to a shared module. Tests are a nice-to-have but the circular dep should be resolved before merge.

[ghost]

Code Review — PR #677: prevent double keys in local storage

Summary

The PR does two things:

1. Refactors hardcoded 'encrypted_mnemonic' / 'encrypted_private_key' strings into exported constants (MNEMONIC_STORAGE_KEY, NSEC_STORAGE_KEY)
2. Adds mutual exclusion: setMnemonic() removes any stored private key, and setPrivateKey() removes any stored mnemonic

The mutual-exclusion logic is correct in intent — a wallet should have one identity source, not both. But there's one real issue and one gap.

---

🔴 Issue: Circular dependency introduced

mnemonic.ts:4 — import { NSEC_STORAGE_KEY } from './privateKey'
privateKey.ts:3 — import { MNEMONIC_STORAGE_KEY } from './mnemonic'

This creates a static circular import. On master, privateKey.ts was deliberately careful to use a dynamic import for mnemonic (await import('./mnemonic') at line 59) to avoid this exact cycle.

While ES module semantics mean the const string literals should resolve before any function runs, circular dependencies are fragile and a known source of subtle bugs — especially across bundler versions. If module evaluation order ever changes (e.g. a new import is added to either file), you could get undefined constants silently.

Fix: Extract both constants into a shared file (e.g. src/lib/storageKeys.ts):

// src/lib/storageKeys.ts
export const MNEMONIC_STORAGE_KEY = 'encrypted_mnemonic'
export const NSEC_STORAGE_KEY = 'encrypted_private_key'

Then both mnemonic.ts and privateKey.ts import from storageKeys.ts. No cycle.

---

🟡 Missing test coverage

The new mutual-exclusion behavior (the actual point of this PR) has no tests. The existing mnemonic.test.ts and privatekey.test.ts don't cover:

• Setting a mnemonic clears any stored private key
• Setting a private key clears any stored mnemonic
• isValidPassword works correctly after a key-type switch

These are easy to add to the existing test files and would catch regressions in the exclusion logic.

---

✅ What's good

• The constant refactoring is a clear improvement — eliminates the raw string 'encrypted_mnemonic' in isValidPassword (master privateKey.ts:58)
• The mutual-exclusion semantics are correct: only one identity source at a time
• No protocol-critical code affected (no VTXO/signing/forfeit/round changes)

---

Requesting changes for the circular dependency. The fix is straightforward — extract constants to a shared module. Tests are a nice-to-have but the circular dep should be resolved before merge.

[ghost]

Follow-up Review — PR #677 (commit 255f1bb)

The circular dependency issue from my previous review is resolved. ✅

What changed: New src/lib/storageKeys.ts extracts both constants (MNEMONIC_STORAGE_KEY, NSEC_STORAGE_KEY). Both mnemonic.ts and privateKey.ts now import from storageKeys.ts — no cycle. The dynamic await import('./mnemonic') in isValidPassword is correctly preserved.

Verified:

• All localStorage calls use the correct constant
• Mutual exclusion logic intact: setMnemonic removes nsec, setPrivateKey removes mnemonic
• No protocol-critical code touched

LGTM. The test coverage gap (🟡) from my earlier review still stands as a nice-to-have, but not blocking.

[ghost]

Follow-up Review — PR #677 (commit 255f1bb)

The circular dependency issue from my previous review is resolved. ✅

What changed: New src/lib/storageKeys.ts extracts both constants (MNEMONIC_STORAGE_KEY, NSEC_STORAGE_KEY). Both mnemonic.ts and privateKey.ts now import from storageKeys.ts — no cycle. The dynamic await import('./mnemonic') in isValidPassword is correctly preserved.

Verified:

• All localStorage calls use the correct constant
• Mutual exclusion logic intact: setMnemonic removes nsec, setPrivateKey removes mnemonic
• No protocol-critical code touched

LGTM. The test coverage gap (🟡) from my earlier review still stands as a nice-to-have, but not blocking.

[pietro909]

mnemonic.test.ts and privatekey.test.ts already exist: worth adding a couple of dedicated tests asserting the new behavior?

[ghost]

Follow-up Review — commit fc31333 (tests added)

New commit adds exactly what pietro909 requested. No issues.

New tests in mnemonic.test.ts:

• Verifies setMnemonic removes NSEC_STORAGE_KEY from localStorage ✅

New tests in privatekey.test.ts:

• setPrivateKey / getPrivateKey roundtrip ✅
• Wrong password throws ✅
• Missing key throws ✅
• setPrivateKey removes MNEMONIC_STORAGE_KEY from localStorage ✅

All four new test cases directly assert the mutual-exclusion invariant that is the whole point of this PR. Tests use beforeEach(() => localStorage.clear()) for proper isolation. LGTM.

No protocol-critical concerns — this is client-side credential storage refactoring, not VTXO/signing/forfeit logic.