Add GiftWrap and NIP-17 DMs #279

rodant · 2024-11-03T12:09:06Z

This PR is a continuation of the work by @manimejia in #233. Besides fixing test errors support for NIP-46 signers must still be added.

Thanks to @manimejia for the great work so far!

UNTESTED with a test written

also some buigfiixes for encryptionEnabled() UNTESTED : this commit has not been tested yet.

coppied Nip44 test for encrypt and decrypt new Nip59 test for gift wrap and unwrap

…gners.

…or-signers.

rodant · 2024-12-09T12:06:04Z

I'm glad for bringing this PR on the table now. Besides adding support for Nip-46 signers and convenience methods for Nip-17 encryption, some bugs were fixed and several unit tests were added.

I'd still format all the PR files according to the projects configuration, but haven't done it yet to keep the review simpler. I'm looking forward to your thoughts.

ekzyis · 2024-12-10T06:41:05Z

Hello, thanks for the PR! Will this also support NIP-44 for NWC?

I think not but wanted to make sure if I understand correctly that NIP-44 for NWC will require another PR.

rodant · 2024-12-10T09:09:42Z

Hello, thanks for the PR! Will this also support NIP-44 for NWC?

I think not but wanted to make sure if I understand correctly that NIP-44 for NWC will require another PR.

Hi, I'm not sure about NWC, but Nip-44 encryption is generally supported, even before this PR. It comes to the signer used, if it support it, you can use Nip-44 encryption with NDK.

rodant · 2024-12-17T11:00:59Z

Hi all of you, just bumping this PR. Any thoughts from the contributors, @pablof7z , @erskingardner ?

erskingardner · 2024-12-18T11:16:33Z

@rodant can you fix the merge conflicts? I'll try and review then.

rodant · 2024-12-18T11:25:51Z

@rodant can you fix the merge conflicts? I'll try and review then.

Hi, sure. I'll take care of this in the next 2 hours. Thanks for reacting!

ndk/src/events/gift-wrapping.ts

ndk/src/events/index.ts

ndk/src/events/encryption.ts

erskingardner · 2024-12-18T11:37:51Z

@rodant had a quick spin through now and left a few comments - mostly very small.

…gners

erskingardner · 2024-12-18T16:51:42Z

@rodant FYI, i've not been working on NDK for a few months so I've pinged @pablof7z to have a look before merging just to make sure it's not clobbering anything else he's working on

rodant · 2024-12-19T08:32:41Z

@rodant FYI, i've not been working on NDK for a few months so I've pinged @pablof7z to have a look before merging just to make sure it's not clobbering anything else he's working on

All right, thanks for your quick and helpful review. It was a pleasure to interact with you.

pablof7z

some changes requested, most importantly there is no signature validation of the inner event!

pablof7z · 2024-12-09T13:10:24Z

ndk/src/events/encryption.ts

+    this: NDKEvent,
+    recipient?: NDKUser,
+    signer?: NDKSigner,
+    nip : EncryptionNip | undefined = defaultEncryption


Nip04 is/should be getting deprecated, so this should just default to nip44, if a developer wants to stick with nip04 encryption they should always pass in that scheme; this approach makes the choice between nip04 and nip44 appear much more subjective.

It sounds good, I'll change it.

pablof7z · 2024-12-09T13:12:41Z

ndk/src/events/kinds/index.ts

@@ -19,6 +19,12 @@ export enum NDKKind {
    // NIP-22
    GenericRespose = 22,

+    // Nip 59 : Gift Wrap
+    GiftWrap = 1059,
+    GiftWrapSeal = 13,


I try to keep the definitions ordered by number, so 13 and 14 should be way further up.

pablof7z · 2024-12-19T12:40:15Z

ndk/src/events/encryption.ts

+export type EncryptionMethod = 'encrypt' | 'decrypt'
+
+// some clients may wish to set a default for message encryption...
+let defaultEncryption : EncryptionNip | undefined = undefined;


this should be anchored to the singleton ndk -- some applications might want to have multiple instances of NDK with different default encryptions

Ok, it makes sense. I'll move this as well as the EncryptionNip type definition to the NDK class. This way there are less circular dependencies.

pablof7z · 2024-12-19T12:49:02Z

ndk/src/signers/nip46/index.ts

    ): Promise<string> {
        const promise = new Promise<string>((resolve, reject) => {
            if (!this.bunkerPubkey) throw new Error("Bunker pubkey not set");

            this.rpc.sendRequest(
                this.bunkerPubkey,
-                method + "_encrypt",
+                `${nip}_${method}`,
                [recipient.pubkey, value],


this should now read peer instead of sender or recipient.

pablof7z · 2024-12-19T13:06:48Z

ndk/src/events/gift-wrapping.ts

+    const signer = NDKPrivateKeySigner.generate();
+    const content = await signer.encrypt(recipient, JSON.stringify(sealed), params?.encryptionNip || 'nip44')
+    const pubkey = (await signer.user()).pubkey
+    let wrap : any = {


this should just be something like:

const wrap = new NDKEvent(); wrap.kind = NDKKind.GiftWrap, wrap.created_at = approximateNow(5) if (params?.wrapTags) wrap.tags = params.wrapTags; wrap.tag(recipient); wrap.content = JSON.stringify(sealed.rawEvent()) await wrap.encrypt(recipient, signer); await wrap.sign(signer); return wrap;

Yes, your version is kind of more elegant. I'll take over your proposal.

pablof7z · 2024-12-19T13:08:54Z

ndk/src/events/gift-wrapping.ts

+      if (!rumor) throw new Error("Failed to decrypt seal")
+
+      if (seal.pubkey === rumor.pubkey) {
+        return new NDKEvent(this.ndk, rumor as NostrEvent)


Important: need to manually perform a signature validation of the rumor otherwise anyone could fake these

I guess you mean the seal event, right? The rumor must not be signed according to the nip. That's really important, good point.

manimejia and others added 21 commits June 4, 2024 10:37

rename nip04.ts to encryption.ts

1691c4f

implement nip44 as optional encryption nip for signers

0647703

UNTESTED implement nip44 as optional encryption for NDKPrivateSigner

4bc3457

UNTESTED with a test written

BUGFIX wrong import and wrong privateKey params

8bbf1cd

ADDED Nip44 support for NDKNip07Signer

e23795a

also some buigfiixes for encryptionEnabled() UNTESTED : this commit has not been tested yet.

UNTESTED adds Nip59 giftWrap and giftUnwrap for NDKEvents

1e7c901

ADDED tests to encryption.test.ts

0a776a2

coppied Nip44 test for encrypt and decrypt new Nip59 test for gift wrap and unwrap

UPDATE add encryoption kinds to NDKKind enum

a02e181

Fix compile errors matching current master of upstream ndk repo.

c0c8c58

Merge remote-tracking branch 'ndk-repo/master' into encryption-for-si…

6581a71

…gners.

Fix compile errors.

9b2ea4d

Revert dev dependency changes.

d85d540

Fallback to original pnpm-lock file.

e65ec14

Extract gift-wrapping from encryption.ts

d7f52bb

Fix MAC error in encryption test.

3ebf52c

Add tests for Nip07 signer and refactorings.

c4edc71

Small refactoring.

8efba6b

Support for Nip46 signers and unit tests for that.

3036b52

Test for forced Nip04 encryption and bug fixing.

4e83ac7

Nip17 encryption methods and more tests.

20d2daf

Merge remote-tracking branch 'nostr-dev-kit/master' into encryption-f…

b20e6bd

…or-signers.

rodant marked this pull request as ready for review December 9, 2024 12:06

rodant mentioned this pull request Dec 9, 2024

Nip44 Encryption & Nip59 Gift Wrapping #233

Open

ekzyis mentioned this pull request Dec 10, 2024

[NIP-47] Add versioning and migrate to NIP-44. nostr-protocol/nips#1531

Open

ekzyis mentioned this pull request Dec 10, 2024

Support NIP-44 encryption for NWC stackernews/stacker.news#1699

Open

erskingardner requested changes Dec 18, 2024

View reviewed changes

rodant added 2 commits December 18, 2024 13:09

Merge remote-tracking branch 'ndk-repo/master' into encryption-for-si…

6776f05

…gners

Fix some of the review comments.

40c4b55

rodant requested a review from erskingardner December 18, 2024 16:02

erskingardner approved these changes Dec 18, 2024

View reviewed changes

pablof7z requested changes Dec 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GiftWrap and NIP-17 DMs #279

Add GiftWrap and NIP-17 DMs #279

rodant commented Nov 3, 2024 •

edited

Loading

rodant commented Dec 9, 2024

ekzyis commented Dec 10, 2024 •

edited

Loading

rodant commented Dec 10, 2024

rodant commented Dec 17, 2024

erskingardner commented Dec 18, 2024

rodant commented Dec 18, 2024

erskingardner commented Dec 18, 2024

erskingardner commented Dec 18, 2024

rodant commented Dec 19, 2024

pablof7z left a comment

pablof7z Dec 9, 2024

rodant Dec 19, 2024

pablof7z Dec 9, 2024

pablof7z Dec 19, 2024

rodant Dec 19, 2024

pablof7z Dec 19, 2024

pablof7z Dec 19, 2024

rodant Dec 19, 2024

pablof7z Dec 19, 2024

rodant Dec 19, 2024

Add GiftWrap and NIP-17 DMs #279

Are you sure you want to change the base?

Add GiftWrap and NIP-17 DMs #279

Conversation

rodant commented Nov 3, 2024 • edited Loading

rodant commented Dec 9, 2024

ekzyis commented Dec 10, 2024 • edited Loading

rodant commented Dec 10, 2024

rodant commented Dec 17, 2024

erskingardner commented Dec 18, 2024

rodant commented Dec 18, 2024

erskingardner commented Dec 18, 2024

erskingardner commented Dec 18, 2024

rodant commented Dec 19, 2024

pablof7z left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodant commented Nov 3, 2024 •

edited

Loading

ekzyis commented Dec 10, 2024 •

edited

Loading