Fix wallet restore: prioritize window.nostr decrypt over NDK signer #54
Conversation
- Reorder decrypt to try window.nostr first (original working method) - Fall back to NDK signer for NIP-46 remote signers - Add fallback between NIP-44 and NIP-04 encryption methods - Fixes cross-signer restore (Chrome/Alby backup → iOS Safari restore)
There was a problem hiding this comment.
Pull request overview
This PR fixes cross-signer wallet restore compatibility by reordering decryption method priority and adding automatic fallback between encryption methods. Backups created with one NIP-07 signer (e.g., Chrome/Alby) were failing to decrypt when restored with a different signer (e.g., iOS Safari with Nostr Build Shack).
- Prioritizes
window.nostrdecrypt over NDK signer methods for better cross-signer compatibility - Adds automatic fallback between NIP-44 and NIP-04 encryption methods
- Implements iterative retry logic with error handling for multiple decryption approaches
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const result = await signer.nip44Decrypt(sender, ciphertext); | ||
| if (result) return result; |
There was a problem hiding this comment.
The early return when result is truthy could silently skip errors. If the decrypt function returns an empty string (which is falsy in JavaScript), it will fall through to try other methods even though the decryption technically succeeded. This could lead to unexpected behavior where empty encrypted values are not properly handled. Consider checking for undefined/null explicitly instead of relying on truthiness.
| } catch (e) { | ||
| console.warn(`[Encryption] ${tryMethod} decryption failed, trying next method...`, e); | ||
| lastError = e instanceof Error ? e : new Error(String(e)); | ||
| // Continue to next method |
There was a problem hiding this comment.
The continue statement is unnecessary as it's the last statement in the catch block. The loop will naturally proceed to the next iteration. Removing it would improve code clarity.
| const ndkInstance = get(ndk); | ||
| const signer = ndkInstance.signer; | ||
| if (signer) { | ||
| const sender = new NDKUser({ pubkey: senderPubkey }); |
There was a problem hiding this comment.
The NDKUser and get(ndk) are instantiated on every iteration of the methods loop, even though they're the same for all attempts. This is inefficient and could be moved outside the loop to avoid unnecessary object creation. The ndkInstance and sender should be created once before the loop begins.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| for (const tryMethod of methods) { | ||
| try { | ||
| // First try window.nostr (NIP-07) - this is the original working method | ||
| const nostr = (window as any).nostr; |
There was a problem hiding this comment.
The window.nostr reference is retrieved on every iteration of the methods loop using (window as any).nostr. This should be retrieved once before the loop to improve performance and code clarity.
| for (const tryMethod of methods) { | |
| try { | |
| // First try window.nostr (NIP-07) - this is the original working method | |
| const nostr = (window as any).nostr; | |
| const nostr = (window as any).nostr; | |
| for (const tryMethod of methods) { | |
| try { | |
| // First try window.nostr (NIP-07) - this is the original working method |
| console.log('[Encryption] Trying window.nostr.nip44.decrypt...'); | ||
| const result = await nostr.nip44.decrypt(senderPubkey, ciphertext); | ||
| if (result) return result; | ||
| } | ||
| if (tryMethod === 'nip04' && nostr?.nip04?.decrypt) { | ||
| console.log('[Encryption] Trying window.nostr.nip04.decrypt...'); | ||
| const result = await nostr.nip04.decrypt(senderPubkey, ciphertext); | ||
| if (result) return result; | ||
| } | ||
|
|
||
| // Fallback to NDK signer (for NIP-46 remote signers, etc.) | ||
| const ndkInstance = get(ndk); | ||
| const signer = ndkInstance.signer; | ||
| if (signer) { | ||
| const sender = new NDKUser({ pubkey: senderPubkey }); | ||
|
|
||
| if (tryMethod === 'nip44' && typeof signer.nip44Decrypt === 'function') { | ||
| console.log('[Encryption] Trying NDK signer.nip44Decrypt...'); | ||
| const result = await signer.nip44Decrypt(sender, ciphertext); | ||
| if (result) return result; | ||
| } else if (tryMethod === 'nip04' && typeof signer.nip04Decrypt === 'function') { | ||
| console.log('[Encryption] Trying NDK signer.nip04Decrypt...'); | ||
| const result = await signer.nip04Decrypt(sender, ciphertext); | ||
| if (result) return result; | ||
| } | ||
| } | ||
| } catch (e) { | ||
| console.warn(`[Encryption] ${tryMethod} decryption failed, trying next method...`, e); |
There was a problem hiding this comment.
Console logs should be removed or replaced with a proper logging mechanism before production. Debug logs like these should typically be behind a debug flag or removed entirely, as they can expose sensitive information about encryption attempts and clutter production logs.
| const result = await nostr.nip44.decrypt(senderPubkey, ciphertext); | ||
| if (result) return result; |
There was a problem hiding this comment.
The early return when result is truthy could silently skip errors. If the decrypt function returns an empty string (which is falsy in JavaScript), it will fall through to try other methods even though the decryption technically succeeded. This could lead to unexpected behavior where empty encrypted values are not properly handled. Consider checking for undefined/null explicitly instead of relying on truthiness.
| const result = await nostr.nip04.decrypt(senderPubkey, ciphertext); | ||
| if (result) return result; |
There was a problem hiding this comment.
The early return when result is truthy could silently skip errors. If the decrypt function returns an empty string (which is falsy in JavaScript), it will fall through to try other methods even though the decryption technically succeeded. This could lead to unexpected behavior where empty encrypted values are not properly handled. Consider checking for undefined/null explicitly instead of relying on truthiness.
2 participants
Summary
Fixes cross-signer wallet restore compatibility. Backups created with one signer (e.g., Chrome/Alby) were failing to decrypt when restored with a different signer (e.g., iOS Safari with Nostr Build Shack).
Problem
The unified encryption service was trying NDK signer decrypt methods before
window.nostr, but some signers behave differently through their NDK interface vs directwindow.nostrcalls.Solution
window.nostrfirst (the original working method)Test plan