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

Feature/add nip 58 support in eventbuilder #118

Merged

Conversation

0xtrr
Copy link
Contributor

@0xtrr 0xtrr commented Jun 24, 2023

Description

This PR adds convenience functions to the EventBuilder for creating a new badge and awarding a badge to a set of pubkeys.

Notes to the reviewers

I'd love to get a second look at the functions to verify that it makes sense as is.

I didn't add a function for setting profile badges (the last operation in NIP-58) because I simply couldn't figure out a good way to structure the function inputs so that we ensure a consecutive order in the a and e tags.

Changelog notice

# Added
- EventBuilder for defining a badge
- EventBuilder for awarding a badge

Checklists

All Submissions:

@0xtrr

This comment was marked as outdated.

@yukibtc
Copy link
Member

yukibtc commented Jun 25, 2023

Thanks!

Have you checked the nip58 module?

Probably it can be possible to move some things, like BadgeDefinitionBuilder, in EventBuilder (remove BadgeDefinitionBuilder and use your define_badge method).

If you have not time, let me know that I'll try to merge these two implementations of nip58.

@0xtrr
Copy link
Contributor Author

0xtrr commented Jun 25, 2023

Thanks!

Have you checked the nip58 module?

Probably it can be possible to move some things, like BadgeDefinitionBuilder, in EventBuilder (remove BadgeDefinitionBuilder and use your define_badge method).

If you have not time, let me know that I'll try to merge these two implementations of nip58.

Ohh lol, forgot to theck the nip modules. I'll close this PR then as the nip58 module contains what I need and seems like a much better overall implementation!

@0xtrr 0xtrr closed this Jun 25, 2023
@0xtrr
Copy link
Contributor Author

0xtrr commented Jun 25, 2023

Probably it can be possible to move some things, like BadgeDefinitionBuilder, in EventBuilder (remove BadgeDefinitionBuilder and use your define_badge method).

Well.. maybe that's not a bad idea. The structs specifically related to the NIP will belong inside the module, then we can expose a builder of such event in the event builder. I think that makes more sense, but I'm not sure what you intend the NIP modules to be. Do you want each NIP-specific module to take care of everything related to that specific NIP (i.e. data structures as well as building the events) or should they contain related datastructures and operations only and then building the event can be exposed through the EventBuilder interface?

@yukibtc
Copy link
Member

yukibtc commented Jun 25, 2023

Until now the event composition was done with EventBuilder. Everything else related to the NIP is inside the dedicated nipXX module.

The only one struct that build an event outside the event::builder module, seems to be the BadgeDefinitionBuilder.
I prefer to keep the code organized in the same way for all modules, so in this case we can:
A) move the BadgeDefinitionBuilder code and inside a method in EventBuilder
B) keep the BadgeDefinitionBuilder but do the same for other NIPs (the EventBuiler will be like a "proxy")

If we opt for Option B, I think that BadgeDefinitionBuilder should return the EventBuilder instead of the Event.

@0xtrr
Copy link
Contributor Author

0xtrr commented Jun 25, 2023

I'd vote for option A as that makes the most sense for me. I can refactor this PR to fit option A, then you can see if you like it or not.

@0xtrr 0xtrr reopened this Jun 25, 2023
@yukibtc
Copy link
Member

yukibtc commented Jul 4, 2023

Let me know when the PR is ready to be reviewed

@0xtrr
Copy link
Contributor Author

0xtrr commented Jul 5, 2023

Let me know when the PR is ready to be reviewed

I will! I'm implementing this in nostr-tool to verify that it works as expected but I'm currently having some problems with the get_events_of() function where I always get 0 events back when querying for the badge definition event I created. Need some more time to figure out why that happens.

@0xtrr
Copy link
Contributor Author

0xtrr commented Jul 10, 2023

This PR is ready for review @yukibtc. I want to say that I'm not an Rust expert(still learning) and these changes are fairly technical for me so please take a good look over the code to ensure that I'm not introducing any newbie mistakes here. The code in the event builder is mostly from the old nip58 functions.

Example implementation can be found in this PR: 0xtrr/nostr-tool#54

@yukibtc
Copy link
Member

yukibtc commented Jul 11, 2023

Before merge, can you squash the commits together?
So will remain a single commit, like: "nostr: add NIP58 support in EventBuilder"

@0xtrr 0xtrr force-pushed the feature/add-nip-58-support-in-eventbuilder branch from 4dfe1dd to cf531e7 Compare July 11, 2023 13:35
@0xtrr
Copy link
Contributor Author

0xtrr commented Jul 11, 2023

Before merge, can you squash the commits together? So will remain a single commit, like: "nostr: add NIP58 support in EventBuilder"

Done

Copy link
Member

@yukibtc yukibtc left a comment

Choose a reason for hiding this comment

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

utACK cf531e7

@yukibtc yukibtc merged commit f00a289 into rust-nostr:master Jul 12, 2023
@0xtrr 0xtrr deleted the feature/add-nip-58-support-in-eventbuilder branch July 12, 2023 09:01
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