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.

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

fix encrypt bug, add Event.quick #26

Merged
merged 2 commits into from
Apr 20, 2023
Merged

Conversation

ntheden
Copy link
Contributor

@ntheden ntheden commented Apr 19, 2023

Fix a bug in the decipher, add Event.quick because it's useful, and add send and receive unit tests to start with.

ntheden referenced this pull request Apr 19, 2023
@@ -45,8 +45,24 @@ class EncryptedDirectMessage extends Event {
return ciphertext;
}

String getPlaintext(String receiverPrivkey, String senderPubkey) {
return Nip4.decipher(receiverPrivkey, senderPubkey, content);
String getPlaintext(String receiverPrivkey, [String senderPubkey=""]) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Decrypting an event should always use the sender pub key provided in the event (not in the tags). But I left senderPubkey as an optional parameter.

Other thing is, if the decryption fails, there is no way to indicate that to the caller. I think getPlaintext() should throw an exception when decryption fails. @ethicnology do you have an opinion on that?

Copy link
Owner

Choose a reason for hiding this comment

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

I did this refactoring based on the signature of your function decrypt but you are right, this should be as simple as possible so we should probably remove this param

Copy link
Owner

@ethicnology ethicnology left a comment

Choose a reason for hiding this comment

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

@no-prob Thanks for this patch

I'm glad to work with you on it.

I can let you change what we talked about or i can do it myself (tell me)

385f211?notification_referrer_id=NT_kwDOAYjwnbM2MTg5MzcxMzMyOjI1NzUxNzA5#commitcomment-109726063

@@ -45,8 +45,24 @@ class EncryptedDirectMessage extends Event {
return ciphertext;
}

String getPlaintext(String receiverPrivkey, String senderPubkey) {
return Nip4.decipher(receiverPrivkey, senderPubkey, content);
String getPlaintext(String receiverPrivkey, [String senderPubkey=""]) {
Copy link
Owner

Choose a reason for hiding this comment

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

I did this refactoring based on the signature of your function decrypt but you are right, this should be as simple as possible so we should probably remove this param

@@ -271,6 +271,20 @@ class Event {
);
}

factory Event.quick(
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think I will keep Event.quick to keep as less code as possible in the library.

The quicky way to create a basic event is to use Event.from which only require the kind, tags, content and privkey supposing that everyone want to create a kind=1 event is a huge assumption while in the context of nip4 we reduced the possibilities.

I can initialize tags with a empty array to reduce even more the input params to kind content and privkey

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

@ethicnology ethicnology merged commit 8519248 into ethicnology:develop Apr 20, 2023
ethicnology pushed a commit that referenced this pull request Apr 20, 2023
* fix encrypt bug, add Event.quick

* a send and a receive unit test
ethicnology added a commit that referenced this pull request Apr 20, 2023
ethicnology added a commit that referenced this pull request Apr 20, 2023
ethicnology added a commit that referenced this pull request Apr 20, 2023
ethicnology added a commit that referenced this pull request Apr 20, 2023
ethicnology added a commit that referenced this pull request Apr 20, 2023
ethicnology added a commit that referenced this pull request Apr 20, 2023
@ntheden
Copy link
Contributor Author

ntheden commented Apr 20, 2023

I can let you change what we talked about or i can do it myself (tell me)

I will write the rest of the mentioned test cases now, the ones mentioned in
#15 (comment)

But I wasn't sure what you meant with this question:

Could generates random nostr keys to generate unit tests for Encrypted Direct Message with the hardcoded keys ?

ethicnology pushed a commit that referenced this pull request May 25, 2023
* fix encrypt bug, add Event.quick

* a send and a receive unit test
ethicnology added a commit that referenced this pull request May 25, 2023
ethicnology added a commit that referenced this pull request May 25, 2023
ethicnology added a commit that referenced this pull request May 25, 2023
* feat: Add NIP04 support (#25)

* add encrypted direct messages (NIP04) support, with
code from https://github.com/vishalxl/nostr_console.git

* formatting + ok

* clean up comments

* add couple testcases

* remove couple dependencies

kepler: removed by adding Kepler class locally since the
github repo is archived
crypto: replace sha 256 hash with pointycastle equivalent

* re-org

* more re-org

* remove `encrypt` dependency

* message changes belong in separate PR

* comment fix

* leave comment on where kepler came from

* fix: #25 warnings

* refactor: PR #25

* fix: decipher bug, add Event.quick (#26)

* fix encrypt bug, add Event.quick

* a send and a receive unit test

* refactor: PR #26

* chore: rename/reorganize files from #25 & #26

* feat: NIPS 1, 5, 10, 19, 20, 28, 51

* refactor: nip20

* fix: useless import

* chore: format

* refactor: PR #29

* runtime data errors are exceptions not asserts

* refactor: PR #28

* chore: update documentation

* refactor: mark NIP04 as deprecated to warn users about controversial discussions about its harmfullness

* refactor: remove utility functions setMetadata, textNote and recommendServer

---------

Co-authored-by: no-prob <113266379+no-prob@users.noreply.github.com>
Co-authored-by: water <130329555+water783@users.noreply.github.com>
Co-authored-by: hazeycode <22148308+hazeycode@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants