Nip44 Encryption & Nip59 Gift Wrapping

[manimejia]

• updates encryption processing for NDKEvent to optionally handle Nip44
• adds Nip44 as an optional encryption method for all NDKSigner classes
• implements Nip44 encryption for NDKPrivateKeySigner and NDKNip07Signer
• adds NIP59 gift wrap processing to NDKEvent using NIP44 or NIP04.

This code is being tested.

[manimejia]

Im having a hard time testing this. Can't run test scripts locally and can't run this fork (via git) as dependency of my existing project.
On both accounts npm install fails with Unsupported URL Type "workspace:" workspace:*. I tried installing pnpm locally but could not due to node version conflicts or something. (It got difficult)

Maybe I'm a dunce, but I have no way of "testing" this code at the moment. Please help? @erskingardner @pablof7z

[sabhas]

I started working on the same Nip44 encryption for Signers yesterday. Before starting I checked there was no PR. Now I just saw your PR. I completed the implementation earlier in the morning but I have been testing this since then. Initially, I had no idea how to test this but I remembered that we can create an installable file by running pnpm pack.

First run pnpm run build from the root of the project
Then go to ndk folder and run pnpm pack
After this you should have a file like nostr-dev-kit-ndk-5.0.0.tgz

copy the whole file path and go to some other project where you use this ndk and run npm i /path/to/your-package-name-version.tgz

[manimejia]

I'm just now adding NIP59 (gift wrap) support to encryption.ts. This will make a "feature complete" implementation of best practices for (private message) event encryption, using NDKEvent.

[sabhas]

During some testing, I observed that this statement always returns false. I provided a hex string and it returned false, which is logical as it's not an instance of the String class.

Should I create a separate issue for this, or can it be fixed in this pull request?

[manimejia]

@sabhas issues with instantiation of NDKNip46Signer would not be covered by this PR ... until I start messing around with implementing NIP44 encryption for NDKNip46Signer ... which I have not yet.

[sabhas]

@sabhas issues with instantiation of NDKNip46Signer would not be covered by this PR ... until I start messing around with implementing NIP44 encryption for NDKNip46Signer ... which I have not yet.

So, we'll have to wait for another PR that will support nip44 encryption for NDKNip46Signer.

[manimejia]

@sabhas probably. Only because I have a release deadline for my own project, that requires "gift wrapped" direct messages, but does not implement Nip46 (remote signing).
In addition ... it doesn't seem that the issue you highlighted has anything to do with NIP44 encryption in general, or the code in this PR specifically.

[sabhas]

@manimejia I agree that the issue I highlighted is not relevant to nip44 encryption.
But you have already implemented nip44 encryption for 2 signer classes (nip07 and private-key signer). I thought it would make sense to implement this for nip46 too.

[manimejia]

@sabhas yes it would make sense. But TBH, I have not wrapped my head around Nip46, and NDK implementation yet. So I table it.

I'm under a deadline (working unpaid hours) to get a release pushed so that I can maybe raise some funding to continue work. So time is critical, and THIS pr will be limited. Thanks.

[erskingardner]

I like the approach of trying to keep the event.encrypt() and event.decrypt() methods in the same place. But adding an non-optional param to those methods still breaks user space (which @pablof7z and I are very against). Making the nip param optional and default to nip-04 would maintain the current functionality.

Long term I think we probably want to refactor all the encryption code into their own modules/etc. so that we can continue to add different types of encryption but for now this is probably ok.

@pablof7z thoughts?

[manimejia]

I am following @sabhas suggestion for testing.

First run pnpm run build from the root of the project
Then go to ndk folder and run pnpm pack. After this you should have a file like nostr-dev-kit-ndk-5.0.0.tgz

I am testing now for NIP44 and NIP59 functionality. Some bugs to iron out.

[manimejia]

I just added some tests, but I'm not (yet) sure how to get jest running on my local. I DID test the gift wrapping functionality in my running Svelte app, using the SAME code as the gift wrap test in this latest commit. It failed when calling giftUnwrap(), with the following error:

Error: invalid MAC
    decrypt22 chunk-F5SKXYI2.js:4938
    decrypt @nostr-dev-kit_ndk.js:7064
    giftUnwrap @nostr-dev-kit_ndk.js:7161
    instance testencryption.svelte:25

I've tried to get to the bottom of this... but I don't have a clue how encryption can succeed but decryption fails. This is where I'm stuck for now.

[manimejia]

Bump. Still stuck. Eager to get this PR merged. Any feedback? @erskingardner @pablof7z

[erskingardner]

Hey @manimejia sorry for the delay. I've been deep down an MLS rabbit hole. I'll have a look through this PR properly tomorrow morning.

[sigit-io]

We can drop a $210 bounty on this if you can add NIP46 / nsecbunker signing support.. ?

[erskingardner]

@manimejia Overall this looks good to me. From what I could see, this defaults to using NIP-04 encryption so we're not breaking anything for library users.

I do think that we need to cover remote signers before we can merge this PR though.

[manimejia]

@erskingardner before I move on to Nip46 support, I need to make sure what I've built already works. See above for my troubles in testing. Do you have suggestions or help for me to get this working "as it is"?

[manimejia]

We can drop a $210 bounty on this if you can add NIP46 / nsecbunker signing support.. ?

@sigit-io i will tackle this as soon as I can test my existing contribution. See above comment. Thanks for the generous offer.