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

Jump to bottom

nostr: add NIP-62 support #777

Merged
merged 1 commit into from
Feb 20, 2025
Merged

nostr: add NIP-62 support #777

merged 1 commit into from
Feb 20, 2025

Conversation

TheAwiteb
Copy link
Contributor

@TheAwiteb TheAwiteb commented Feb 19, 2025 •
edited
Loading

Description

Close #775 by support NIP-62

Notes to the reviewers

Checklist

  • I followed the contribution guidelines
  • I ran just precommit or just check before committing (It works without PM test, features issue)
  • I updated the CHANGELOG (if applicable)

@TheAwiteb
Copy link
Contributor Author

There is no nip56 feature in nostr-database, should I remove PM test? it work though

https://github.com/rust-nostr/nostr/actions/runs/13415177654/job/37474156560?pr=777#step:3:1825

@TheAwiteb
Copy link
Contributor Author

Also thank you for this clean code, it's actually easy to understand 🙌

@TheAwiteb
Copy link
Contributor Author

What error should I return if empty relays list passed to the request_vanish_with_reason function? or where should I create it?

@yukibtc I didn't handled the ["relay", "ALL_RELAYS"] yet, what do you think if the relays list is empty then this means all relays?

yukibtc
yukibtc requested changes Feb 20, 2025
View reviewed changes
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.

Thanks! I've marked some changes to do.

I would also add the TagStandard::AllRelays variant, to represent the ["relay", "ALL_RELAYS"] tag. What do you think?

@yukibtc
Copy link
Member

yukibtc commented Feb 20, 2025

@TheAwiteb, an edit on my review: I think for now is probably better to not handle the vanish request in the databases.

@TheAwiteb
Copy link
Contributor Author

better to not handle the vanish request in the databases.

Alright, I'll remove all changes from nostr-database and nostr-lmdb

@TheAwiteb
Copy link
Contributor Author

TheAwiteb commented Feb 20, 2025 •
edited
Loading

I think for now is probably better to not handle the vanish request in the databases.

I resolved all databases request changes (not planned)

@TheAwiteb TheAwiteb requested a review from yukibtc February 20, 2025 12:28
@TheAwiteb
Copy link
Contributor Author

There still changes on crates/nostr-lmdb/src/store/ingester.rs. Should I remove it?

I'll rebase the branch on master right now

@yukibtc
Copy link
Member

yukibtc commented Feb 20, 2025 •
edited
Loading

I've tried to do some changes at commit c367ef1, instead of bothering you with change requests :)
Let me know what do you think

@TheAwiteb
Copy link
Contributor Author

I think it's ready to be merged 🚀🚀

@yukibtc yukibtc changed the title WIP: nostr: add NIP-62 support nostr: add NIP-62 support Feb 20, 2025
yukibtc added a commit to TheAwiteb/nostr that referenced this pull request Feb 20, 2025
@TheAwiteb @yukibtc
nostr: add NIP-62 support
6b1323c 
Closes rust-nostr#775
Pull-Request: rust-nostr#777

Reviewed-by: Yuki Kishimoto <yukikishimoto@protonmail.com>
Co-authored-by: Yuki Kishimoto <yukikishimoto@protonmail.com>
Signed-off-by: Yuki Kishimoto <yukikishimoto@protonmail.com>
Signed-off-by: Awiteb <a@4rs.nl>
Signed-off-by: Yuki Kishimoto <yukikishimoto@protonmail.com>
@TheAwiteb @yukibtc
nostr: add NIP-62 support
75d56ee 
Closes rust-nostr#775
Pull-Request: rust-nostr#777

Reviewed-by: Yuki Kishimoto <yukikishimoto@protonmail.com>
Co-authored-by: Yuki Kishimoto <yukikishimoto@protonmail.com>
Signed-off-by: Yuki Kishimoto <yukikishimoto@protonmail.com>
Signed-off-by: Awiteb <a@4rs.nl>
@yukibtc yukibtc merged commit 75d56ee into rust-nostr:master Feb 20, 2025
7 checks passed
@yukibtc
Copy link
Member

yukibtc commented Feb 20, 2025

Squashed and merged

@yukibtc
Copy link
Member

yukibtc commented Feb 20, 2025

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yukibtc yukibtc yukibtc requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support NIP-62 (Request to Vanish)
2 participants
@TheAwiteb @yukibtc