Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NIP07 - add nip44 calls #940

Closed
wants to merge 6 commits into from
Closed

NIP07 - add nip44 calls #940

Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3163174
add nip44 calls
monlovesmango Dec 20, 2023
860bd97
Update nip44 calls
monlovesmango Jan 4, 2024
e9a8077
remove versioning
monlovesmango Jan 7, 2024
75582ce
remove conversationKey methods
monlovesmango Jan 9, 2024
575ddba
add getConversationKey method
monlovesmango Feb 20, 2024
76b5257
update getConversationKey to take array
monlovesmango Feb 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions 07.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ Aside from these two basic above, the following functions can also be implemente
async window.nostr.getRelays(): { [url: string]: {read: boolean, write: boolean} } // returns a basic map of relay urls to relay policies
async window.nostr.nip04.encrypt(pubkey, plaintext): string // returns ciphertext and iv as specified in nip-04 (deprecated)
async window.nostr.nip04.decrypt(pubkey, ciphertext): string // takes ciphertext and iv as specified in nip-04 (deprecated)
async window.nostr.nip44.encrypt([[pubkey1, plaintext1], [pubkey2, plaintext2], ...]): string[] // takes array of [pubkey, plaintext] tuples, returns array of ciphertexts as specified in nip-44
async window.nostr.nip44.decrypt([[pubkey1, ciphertext1], [pubkey2, ciphertext2], ...]): string[] // takes array of [pubkey, ciphertext] tuples, returns array of plaintexts as specified in nip-44
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will also just add real quick, that letting you encrypt/decrypt multiple at once leads to the possibility that the signer will return an array with a different length than the one you passed in. It's not really a big deal since clients need to validate the response anyway.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if theres an error, I would think that returned array should retain the same number of values and just put a blank value for the one that err'ed. would probably be good to explicitly say that here.

do you think thats a valid way of handling it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If any individual message has an error, the whole function call should throw.

Copy link
Member

@alexgleason alexgleason Feb 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The more I think about it, the more I think it's actually a problem. The client has no way to validate messages before sending them to the signer, right? So any invalid text can cause the signer to throw.

First I would question if we really need to support decrypting multiple messages in a single call. Is it just because Alby prompts every time?

If we really need to do it, it should have a return type of Promise<string>[] instead of Promise<string[]> so individual calls can be either resolved or rejected.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, it is because for the NIP44 calls in NIP46 it would be extremely inefficient to fetch one decryption at a time. so it is better for NIP46 to have these calls be arrays, and we want the NIP07 calls to be consistent with the NIP46 calls. hence why the NIP44 calls have array parameters.

can you return a promise from a browser extension? returning a promise definitely wouldn't work for NIP46 either, which we are trying to stay consistent with.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found the Alby MR and commented here: getAlby/lightning-browser-extension#2653 (review)

Copy link
Member

@staab staab Feb 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does Alby do about failed decrypt calls at the moment

That's a super good point, it throws.

Alby doesn't have to do batching unless it's converting calls to NIP-46

Alby may want to do batching for UX rather than performance reasons. So instead of 300 pop ups, they should really show "Coracle wants to decrypt 300 messages. ok?"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should just do what it's currently doing for nip04. Users will check "remember this choice", which is not the best UX, but it can be fixed on the Alby side later. We don't need to fix that problem in NIP-07.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 I might agree with you. I know @fiatjaf currently wants the interface the same. Maybe he would be convinced by your argument.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, Amber, the Android Signer, also does batching to avoid creating a new dialog for each approval. The experience is pretty cool and makes a lot of sense since, depending on the user's data, Amethyst might blast the signer with 500 decrypt calls in a second.

```

### Implementation
Expand Down