add backups to Nostr #209
add backups to Nostr #209
Conversation
WalkthroughAdds Nostr-based encrypted backup/restore (NostrStorage + handlers), threads a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Backup Screen
participant BackupLib as handleNostrBackup
participant NostrStorage
participant Relays as Nostr Relays
participant Config as Config Context
User->>UI: Trigger backup (via toggle or side-effect)
UI->>BackupLib: handleNostrBackup(config)
BackupLib->>NostrStorage: save(nostrAppName, payload)
Note over NostrStorage: derive ephemeral key, nip44 encrypt payload,\ncreate kind-4 event, publish with timeout
NostrStorage->>Relays: Publish event
Relays-->>NostrStorage: Ack / error
alt success
BackupLib->>Config: updateConfig({... , lastBackup: ts}, true/false)
Config-->>UI: updated config
else failure
NostrStorage-->>BackupLib: throw/log error
BackupLib-->>UI: report error
end
sequenceDiagram
participant User
participant UI as Restore Screen
participant RestoreLib as handleNostrRestore
participant NostrStorage
participant Relays as Nostr Relays
participant IndexedDB
participant Config as Config Context
User->>UI: Submit nsec (seckey) to restore
UI->>RestoreLib: handleNostrRestore(seckey, updateConfig)
RestoreLib->>NostrStorage: load(nostrAppName)
Note over NostrStorage: subscribe, collect kind-4 events, pick newest,\ndecrypt via nip44 using seckey and sender pubkey
NostrStorage-->>RestoreLib: decrypted payload
RestoreLib->>Config: updateConfig(restoredConfig, false)
RestoreLib->>IndexedDB: save reverseSwaps & submarineSwaps
RestoreLib-->>UI: resolve (await completed)
UI->>User: Navigate to Success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
|
I would automatically backup always? Why we need user to click the button? Also the Nostr Relay should be changeable? Can we whitelist only the nip44 kind right? |
src/lib/nostr.ts
Outdated
| import { finalizeEvent, generateSecretKey, getPublicKey, nip44, SimplePool, UnsignedEvent, Event } from 'nostr-tools' | ||
| import { EncryptedDirectMessage } from 'nostr-tools/kinds' | ||
|
|
||
| const defaultRelays = ['wss://relay.damus.io', 'wss://relay.primal.net'] |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/lib/types.ts (1)
17-17: LGTM! Consider adding a documentation comment.The addition of
lastBackupto track backup timestamps integrates well with the backup/restore feature. The field is correctly typed asnumberand initialized to0in the default config, making it backward compatible.Consider adding a JSDoc comment to clarify the field's purpose and format:
fiat: Fiats + /** Unix timestamp (milliseconds) of the last successful backup to Nostr; 0 if never backed up */ lastBackup: number nostr: booleanpublic/wallet-service-worker.mjs (1)
1-11139: Built artifact — please exclude and share source insteadpublic/wallet-service-worker.mjs appears to be a Vite build output. Per project conventions, we don’t review compiled artifacts. Please:
- Commit the corresponding source changes (e.g., src/**) that generate this bundle so we can review the real deltas.
- Exclude build outputs from PRs (e.g., via .gitignore or marking as linguist-generated) to keep diffs focused.
Confirm which source files produce this asset and I’ll review those. Based on learnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
public/wallet-service-worker.mjs(49 hunks)src/lib/nostr.ts(1 hunks)src/lib/types.ts(1 hunks)src/providers/config.tsx(1 hunks)src/screens/Init/Connect.tsx(1 hunks)src/screens/Init/Restore.tsx(2 hunks)src/screens/Init/Success.tsx(1 hunks)src/screens/Settings/Backup.tsx(3 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-08-06T09:53:28.428Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 134
File: src/screens/Settings/Password.tsx:34-39
Timestamp: 2025-08-06T09:53:28.428Z
Learning: In src/screens/Settings/Password.tsx, the saveNewPassword function doesn't need error handling for getPrivateKey because it's only called after authentication is verified. The useEffect validates the oldPassword using isValidPassword() which internally calls getPrivateKey(), and only sets authenticated=true if successful. The saveNewPassword function has a guard clause that returns early if not authenticated, so getPrivateKey(oldPassword) is guaranteed to work when called.
Applied to files:
src/screens/Init/Connect.tsxsrc/screens/Settings/Backup.tsx
📚 Learning: 2025-07-21T18:25:36.391Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 121
File: src/screens/Init/Restore.tsx:58-58
Timestamp: 2025-07-21T18:25:36.391Z
Learning: In src/screens/Init/Restore.tsx, the Input component for private key restoration intentionally omits the `value` prop to make it uncontrolled. This is a security feature to prevent the private key from being displayed if users navigate away and return to the screen, avoiding potential exposure of sensitive data in the UI.
Applied to files:
src/screens/Init/Connect.tsxsrc/screens/Settings/Backup.tsxsrc/screens/Init/Restore.tsxsrc/screens/Init/Success.tsx
📚 Learning: 2025-06-30T18:33:29.839Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
Applied to files:
src/screens/Init/Connect.tsxpublic/wallet-service-worker.mjs
📚 Learning: 2025-06-30T18:23:34.163Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: public/wallet-service-worker.mjs:6350-6353
Timestamp: 2025-06-30T18:23:34.163Z
Learning: Do not review `public/wallet-service-worker.mjs` as it is a built file generated by Vite.
Applied to files:
public/wallet-service-worker.mjs
📚 Learning: 2025-07-07T14:41:35.991Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 106
File: src/lib/bolt11.ts:153-155
Timestamp: 2025-07-07T14:41:35.991Z
Learning: In src/lib/bolt11.ts, the code is copied from the original bitcoinjs/bolt11 library (https://github.com/bitcoinjs/bolt11/blob/master/payreq.js) and should be kept unchanged to maintain compatibility with the upstream source, even if there are potential improvements or syntax issues.
Applied to files:
public/wallet-service-worker.mjs
🧬 Code graph analysis (3)
src/screens/Settings/Backup.tsx (5)
src/providers/config.tsx (1)
ConfigContext(31-40)src/providers/wallet.tsx (1)
WalletContext(39-53)src/lib/nostr.ts (1)
NostrStorage(6-114)src/lib/privateKey.ts (1)
nsecToPrivateKey(24-28)src/lib/logs.ts (1)
consoleError(40-44)
src/screens/Init/Restore.tsx (4)
src/providers/config.tsx (1)
ConfigContext(31-40)src/lib/nostr.ts (1)
NostrStorage(6-114)src/lib/types.ts (1)
Config(8-24)src/lib/logs.ts (1)
consoleError(40-44)
src/screens/Init/Success.tsx (1)
src/providers/config.tsx (1)
ConfigContext(31-40)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (5)
src/screens/Init/Success.tsx (3)
9-9: LGTM!Correctly imports
ConfigContextto access the newlastBackupfield for conditional rendering.
12-12: LGTM!Appropriately uses
useContextto access configuration data.
18-22: LGTM! Clear conditional logic for restoration messaging.The nested conditional correctly distinguishes between three scenarios:
- Restoring with Nostr backup (lastBackup truthy): indicates wallet, settings, and swaps restored
- Restoring without Nostr backup (lastBackup falsy/0): indicates only wallet restored
- Creating new wallet: indicates wallet created
The logic properly leverages
lastBackupas a numeric timestamp where 0 (never backed up) is falsy and any valid timestamp is truthy.src/screens/Init/Connect.tsx (1)
25-25: LGTM! Good defensive programming.The spread operator
...initInfoensures that any additional fields ininitInfoare preserved when clearing credentials, rather than replacing the entire object. This is particularly important given the broader PR introduces new fields (like restoration state) that may be present ininitInfoduring the init-to-wallet transition.src/providers/config.tsx (1)
11-11: LGTM! Proper initialization and backward compatibility.Initializing
lastBackupto0correctly represents "never backed up" and maintains backward compatibility. The config upgradability logic at lines 80-82 (config = { ...defaultConfig, ...config }) ensures that existing stored configs without this field will automatically receive the default value of0.
Kukks
left a comment
Kukks
left a comment
There was a problem hiding this comment.
- Make sure this system is expandable, so that:
- it's easily possible to add new items for backup
- we can do a drop in replacement for a non-nostr backup backend
- Restore should be automatic on wallet import
- We should be able to SYNC. Meaning 2 wallets using the same keys should be able to share state without overwriting each other's backups.
That kills the purpose: when restoring, the only thing the wallet know is a hardcoded list of relay to query for backups. If the user changes the relay list, that information will not be available to the wallet when restoring.
Yes, but bare in mind nip44 doesn't define any specific kind. |
Deploying wallet-signet with
|
| Latest commit: |
b8b2bf0
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://df032ac8.wallet-23u.pages.dev |
| Branch Preview URL: | https://nostr.wallet-23u.pages.dev |
Deploying wallet-mutinynet with
|
| Latest commit: |
b8b2bf0
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://db64d0bb.arkade-wallet.pages.dev |
| Branch Preview URL: | https://nostr.arkade-wallet.pages.dev |
Deploying wallet-bitcoin with
|
| Latest commit: |
b8b2bf0
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ac1f2731.wallet-bitcoin.pages.dev |
| Branch Preview URL: | https://nostr.wallet-bitcoin.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/screens/Settings/Server.tsx (1)
54-66: Await backup operation before page reload.The issue identified in the review comment is valid.
updateConfig(line 52 in src/providers/config.tsx) is not async and does not awaithandleNostrBackup. When called without await at Server.tsx line 59, the page reloads immediately at line 60 before the asynchronous backup operation completes, risking loss of the new server configuration.Fix: Make
updateConfigasync, awaithandleNostrBackupinside it, and await theupdateConfigcall in Server.tsx.
♻️ Duplicate comments (1)
src/screens/Init/Restore.tsx (1)
51-55: Await restore result before navigatingWe still jump to
InitSuccesseven whenhandleNostrRestorereturnsnull(no backup) or throws, so the UI claims success while nothing was restored and the user never sees an error. Please await the restore, gate navigation on a truthy payload, and surface failures to the user. OncehandleNostrRestorereturns the loaded data (see backup.ts comment), a fix like this keeps the flow honest:- const handleProceed = () => { - setInitInfo({ privateKey, password: defaultPassword, restoring: true }) - handleNostrRestore(privateKey!, updateConfig) - .then(() => navigate(Pages.InitSuccess)) - .catch((err) => consoleError(err, 'Error restoring from nostr')) - } + const handleProceed = async () => { + if (!privateKey) return + setLabel('Restoring backup…') + setError('') + setInitInfo({ privateKey, password: defaultPassword, restoring: true }) + try { + const payload = await handleNostrRestore(privateKey, updateConfig) + setLabel(buttonLabel) + if (!payload) { + setError('No Nostr backup was found for this key.') + return + } + navigate(Pages.InitSuccess) + } catch (err) { + consoleError(err, 'Error restoring from nostr') + setLabel(buttonLabel) + setError('We could not restore your backup. Please retry.') + } + }
🧹 Nitpick comments (5)
src/screens/Apps/Index.tsx (1)
94-117: Inconsistent punctuation across app descriptions.The updated descriptions for Boltz, LendaSat, and LendaSwap no longer have trailing periods, but the Fuji Money description on line 117 still does. Consider removing the period from line 117 to maintain consistency.
Apply this diff to make the descriptions consistent:
- <App name='Fuji Money' icon={<FujiMoneyIcon />} desc='Synthetic Assets on the Bitcoin network' /> + <App name='Fuji Money' icon={<FujiMoneyIcon />} desc='Synthetic Assets on the Bitcoin network' />Note: If the trailing period is intentional for Fuji Money, consider adding periods back to the other descriptions.
src/providers/config.tsx (1)
53-57: Consider more robust URL validation.The protocol normalization logic works for common cases but could be improved:
- The in-place modification of
config.aspUrlmight be unexpected behavior- Consider using URL parsing to validate the entire URL structure
Consider this more defensive approach:
- // add protocol to aspUrl if missing - if (!config.aspUrl.startsWith('http://') && !config.aspUrl.startsWith('https://')) { - const protocol = config.aspUrl.startsWith('localhost') ? 'http://' : 'https://' - config.aspUrl = protocol + config.aspUrl - } + // normalize aspUrl protocol + if (!config.aspUrl.startsWith('http://') && !config.aspUrl.startsWith('https://')) { + const protocol = config.aspUrl.startsWith('localhost') ? 'http://' : 'https://' + config = { ...config, aspUrl: protocol + config.aspUrl } + } + // validate URL format + try { + new URL(config.aspUrl) + } catch (e) { + console.warn('Invalid aspUrl format:', config.aspUrl) + }src/lib/nostr.ts (3)
21-38: Consider documenting the pubkey format handling.The constructor strips an optional
0xprefix from pubkeys, which is more commonly associated with Ethereum than Nostr. While the logic is correct, this behavior should be documented to clarify the expected input format.Add a comment explaining the format handling:
} else if (options.pubkey) { this.pubkey = options.pubkey + // Handle optional 0x prefix (non-standard for Nostr but may appear in some contexts) if (this.pubkey.length === 66) { this.pubkey = options.pubkey.slice(2) } if (this.pubkey.length !== 64) { throw new Error('Invalid pubkey length') }
46-47: Consider the implications of ephemeral keys.Each backup is signed with a new ephemeral keypair, which means:
- Backups cannot be attributed to a consistent sender
- It's harder to verify authenticity when restoring
- No way to filter or identify legitimate backups by sender
While this approach works for privacy, consider if you want to use a persistent keypair derived from the user's secret key for accountability.
If you want consistent sender attribution, consider deriving a persistent backup keypair:
async save(app: string, payload: NostrStorageData): Promise<void> { const pool = new SimplePool() - const sk = generateSecretKey() - const pk = getPublicKey(sk) + if (!this.seckey) throw new Error('Secret key is required for saving data') + // Use the user's actual keypair instead of ephemeral + const pk = this.pubkeyThen update the encryption to use the recipient's pubkey directly for a self-encrypted message, or sign with a deterministic derived key.
63-68: Consider making the timeout configurable.The 10-second publish timeout is hardcoded. Making it configurable would improve flexibility for different network conditions.
+ private readonly publishTimeout: number + - constructor(options: { seckey?: Uint8Array; pubkey?: string; relays?: string[] }) { + constructor(options: { seckey?: Uint8Array; pubkey?: string; relays?: string[]; publishTimeout?: number }) { this.relays = options.relays || defaultRelays + this.publishTimeout = options.publishTimeout || 10000 // ... } async save(app: string, payload: NostrStorageData): Promise<void> { // ... try { await Promise.race([ pool.publish(this.relays, signedEvent), new Promise((_, reject) => { - setTimeout(() => reject(new Error('Publish timeout')), 10000) + setTimeout(() => reject(new Error('Publish timeout')), this.publishTimeout) }), ])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
src/components/Balance.tsx(1 hunks)src/lib/backup.ts(1 hunks)src/lib/lightning.ts(3 hunks)src/lib/nostr.ts(1 hunks)src/lib/types.ts(1 hunks)src/providers/config.tsx(5 hunks)src/providers/lightning.tsx(3 hunks)src/providers/wallet.tsx(3 hunks)src/screens/Apps/Index.tsx(3 hunks)src/screens/Init/Restore.tsx(2 hunks)src/screens/Settings/Backup.tsx(3 hunks)src/screens/Settings/Display.tsx(1 hunks)src/screens/Settings/Fiat.tsx(1 hunks)src/screens/Settings/Index.tsx(0 hunks)src/screens/Settings/Nostr.tsx(0 hunks)src/screens/Settings/Notifications.tsx(1 hunks)src/screens/Settings/Server.tsx(1 hunks)src/screens/Settings/Theme.tsx(1 hunks)
💤 Files with no reviewable changes (2)
- src/screens/Settings/Nostr.tsx
- src/screens/Settings/Index.tsx
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-06T09:53:28.428Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 134
File: src/screens/Settings/Password.tsx:34-39
Timestamp: 2025-08-06T09:53:28.428Z
Learning: In src/screens/Settings/Password.tsx, the saveNewPassword function doesn't need error handling for getPrivateKey because it's only called after authentication is verified. The useEffect validates the oldPassword using isValidPassword() which internally calls getPrivateKey(), and only sets authenticated=true if successful. The saveNewPassword function has a guard clause that returns early if not authenticated, so getPrivateKey(oldPassword) is guaranteed to work when called.
Applied to files:
src/screens/Settings/Backup.tsxsrc/screens/Init/Restore.tsx
📚 Learning: 2025-07-21T18:25:36.391Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 121
File: src/screens/Init/Restore.tsx:58-58
Timestamp: 2025-07-21T18:25:36.391Z
Learning: In src/screens/Init/Restore.tsx, the Input component for private key restoration intentionally omits the `value` prop to make it uncontrolled. This is a security feature to prevent the private key from being displayed if users navigate away and return to the screen, avoiding potential exposure of sensitive data in the UI.
Applied to files:
src/screens/Settings/Backup.tsxsrc/screens/Init/Restore.tsx
📚 Learning: 2025-06-30T18:33:29.839Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
Applied to files:
src/providers/wallet.tsx
🧬 Code graph analysis (9)
src/screens/Settings/Backup.tsx (2)
src/providers/config.tsx (1)
ConfigContext(31-40)src/components/Toggle.tsx (1)
Toggle(13-27)
src/screens/Settings/Notifications.tsx (1)
src/lib/notifications.ts (1)
sendTestNotification(29-33)
src/providers/lightning.tsx (1)
src/lib/lightning.ts (1)
LightningSwapProvider(17-162)
src/screens/Init/Restore.tsx (3)
src/providers/config.tsx (1)
ConfigContext(31-40)src/lib/backup.ts (1)
handleNostrRestore(22-40)src/lib/logs.ts (1)
consoleError(40-44)
src/lib/backup.ts (2)
src/lib/types.ts (1)
Config(8-23)src/lib/nostr.ts (3)
NostrStorageData(10-14)NostrStorage(16-141)nostrAppName(6-6)
src/lib/nostr.ts (1)
src/lib/types.ts (1)
Config(8-23)
src/lib/lightning.ts (3)
src/lib/types.ts (1)
Config(8-23)src/providers/asp.tsx (1)
AspInfo(6-6)src/lib/backup.ts (1)
handleNostrBackup(10-20)
src/providers/wallet.tsx (1)
src/providers/config.tsx (1)
ConfigContext(31-40)
src/providers/config.tsx (3)
src/lib/types.ts (1)
Config(8-23)src/lib/storage.ts (1)
saveConfigToStorage(23-25)src/lib/backup.ts (1)
handleNostrBackup(10-20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (13)
src/screens/Settings/Theme.tsx (1)
18-18: LGTM! Backup flag correctly set for user preference.The addition of the
truebackup flag for theme changes is appropriate, as theme is a persistent user preference that should be included in Nostr backups.src/screens/Settings/Fiat.tsx (1)
18-18: LGTM! Backup flag correctly set for user preference.The addition of the
truebackup flag for fiat currency changes is appropriate, as currency preference should be included in Nostr backups.src/screens/Settings/Notifications.tsx (1)
14-24: LGTM! Backup flag correctly set for notification preferences.The addition of the
truebackup flag for both enabling and disabling notifications is appropriate, as notification preferences are persistent user settings that should be included in Nostr backups.src/components/Balance.tsx (1)
30-30: Excellent choice! Transient UI state correctly excludes backup.Passing
falsefor the balance visibility toggle is the right decision. Since this is a transient UI preference that users may toggle frequently, avoiding backup on each toggle prevents unnecessary backup operations and improves UX.src/lib/types.ts (1)
17-19: LGTM! Type renaming improves clarity.The renaming of fields improves code clarity:
nostr→nostrBackupmakes the purpose explicit (backup functionality via Nostr)npub→pubkeyis more accurate, as this stores the hex-encoded public key rather than the bech32 npub formatThese changes are consistent with the broader Nostr backup integration.
src/screens/Settings/Display.tsx (1)
18-20: LGTM! Backup flag correctly set for user preference.The addition of the
truebackup flag for currency display changes is appropriate, as display preferences are persistent user settings that should be included in Nostr backups.src/providers/wallet.tsx (1)
16-16: No issues found—pubkey duplication is intentional and correctly implemented.Both
config.pubkeyandwallet.pubkeyare derived and set together during wallet initialization (lines 220–227 insrc/providers/wallet.tsx). They serve distinct purposes:config.pubkeypersists for backup/Nostr functionality, whilewallet.pubkeysupports runtime state management and routing decisions. No independent updates or drift between them were detected—this is sound design.src/providers/config.tsx (4)
5-5: LGTM: Import added for Nostr backup functionality.The import is correctly added to support the new backup integration.
12-14: LGTM: Default config updated with Nostr fields.The new fields
nostrBackupandpubkeyare appropriately initialized with sensible defaults (disabled and empty).
73-73: LGTM: Backup flag correctly set to false for initialization.Both
resetConfigand the initial config load correctly passfalsefor the backup parameter, avoiding unnecessary backup operations during initialization.Also applies to: 85-85
27-27: All callers updated correctly.Verification complete: All 11 call sites of
updateConfighave been properly updated to pass thebackupparameter. The breaking change has been consistently applied across the codebase, with appropriate boolean values passed based on context (true for user-initiated settings changes, false for programmatic updates).src/lib/nostr.ts (2)
10-20: LGTM: Type definition and class properties.The
NostrStorageDatatype correctly defines the backup structure, and the class properties are appropriately declared.
125-140: LGTM: Encryption and decryption implementation.The
encryptDataanddecryptDatamethods correctly use NIP-44 conversation keys for encryption. The error handling for missing secret keys is appropriate.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/lib/nostr.ts (3)
29-31: Document the "0x" prefix handling.The constructor silently strips a "0x" prefix from 66-character pubkeys, but this behavior is undocumented. Consider adding a JSDoc comment explaining that hex-encoded pubkeys with "0x" prefix are automatically handled.
Optionally, you could also validate that the pubkey contains only valid hexadecimal characters after length validation:
if (this.pubkey.length !== 64) { throw new Error('Invalid pubkey length') } + if (!/^[0-9a-fA-F]{64}$/.test(this.pubkey)) { + throw new Error('Invalid pubkey format: must be 64 hex characters') + } this.seckey = null
70-70: Use consistent logging utility.The
load()method uses theconsoleErrorutility (line 117), but this success log usesconsole.logdirectly. For consistency and to ensure logs are captured uniformly (e.g., by the logging system thatconsoleErrorintegrates with), consider using a logging utility here as well.If a success logging utility exists (e.g.,
consoleLogor similar), use it:- console.log('Message published to Nostr successfully') + consoleLog('Message published to Nostr successfully')If no such utility exists, the current approach is acceptable but consider adding one for consistency.
22-22: Consider adding JSDoc for the constructor.The constructor has non-trivial logic (pubkey validation, "0x" prefix handling, seckey/pubkey mutual exclusivity) that would benefit from documentation explaining the parameters and behavior.
Example:
+ /** + * Initialize NostrStorage with either a secret key or public key + * @param options.seckey - Optional secret key (Uint8Array). If provided, pubkey is derived. + * @param options.pubkey - Optional public key (hex string, with or without '0x' prefix). Required if seckey not provided. + * @param options.relays - Optional array of relay URLs. Defaults to hardcoded relay list. + * @throws Error if neither seckey nor pubkey is provided, or if pubkey format is invalid + */ constructor(options: { seckey?: Uint8Array; pubkey?: string; relays?: string[] }) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/lib/lightning.ts(3 hunks)src/lib/nostr.ts(1 hunks)src/providers/config.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/lightning.ts
- src/providers/config.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/lib/nostr.ts (2)
src/lib/types.ts (1)
Config(8-23)src/lib/logs.ts (1)
consoleError(40-44)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (2)
src/lib/nostr.ts (2)
132-135: LGTM!The NIP-44 encryption implementation correctly derives a conversation key using the ephemeral secret key and the recipient's public key, then encrypts the data.
143-147: LGTM!The NIP-44 decryption implementation correctly derives the conversation key using the stored secret key and the sender's public key. The guard against missing
seckeyis good defensive programming.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib/nostr.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/lib/nostr.ts (2)
src/lib/types.ts (1)
Config(8-23)src/lib/logs.ts (1)
consoleError(40-44)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (6)
src/lib/nostr.ts (6)
1-9: LGTM!The imports are appropriate, and the relay URLs now correctly use the WebSocket protocol (
wss://). ThenostrAppNameconstant provides a clear identifier for filtering backup events.
11-15: LGTM!The type definition clearly captures the backup data structure, incorporating config and both swap types.
148-151: LGTM!The NIP-44 encryption correctly derives the conversation key from the ephemeral sender key and recipient's public key.
159-163: LGTM!The NIP-44 decryption correctly derives the conversation key from the recipient's secret key and sender's public key, with appropriate guard for missing secret key.
29-46: Review comment is not substantiated by code analysis.The optional
relaysparameter in the constructor is defensive design that safely defaults to hardcoded relays. Both backup and restore instantiations in the codebase pass no custom relays and rely entirely on the hardcoded defaults, so restore functionality is not broken. The optional parameter remains unused infrastructure and poses no risk.
52-84: Clarify the event kind choice for NIP-44 encrypted app backups.NIP-44 defines an encrypted payload format, not a specific event kind — it depends on the use-case. While kind 4 is historically used for private messages (NIP-04), your use case stores app backup data, making kind 4 semantically imprecise. Consider:
- Using an application-specific kind (30000-39999 range) to better signal that these are encrypted backups, not traditional messages
- Verifying that clients processing these events won't incorrectly assume NIP-04 encryption
The current implementation will function with kind 4, but an app-specific kind makes intent clearer and avoids potential client confusion.
Saves (and restore) settings and swaps list to Nostr encrypted with nip44.
@Kukks please review
Preview
Screen.Recording.2025-11-06.at.11.15.58.mov
Summary by CodeRabbit
New Features
Changes