-
Notifications
You must be signed in to change notification settings - Fork 965
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
protocols/gossipsub: Allow publishing to anything that implements Into<TopicHash>
#2862
protocols/gossipsub: Allow publishing to anything that implements Into<TopicHash>
#2862
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for contributing!
I think this is okay but I'd like to also have @mxinden 's opinion on this.
Can you also make a changelog entry please? (Depending on which API we settle on of course.)
protocols/gossipsub/src/behaviour.rs
Outdated
@@ -586,20 +586,29 @@ where | |||
Ok(true) | |||
} | |||
|
|||
/// Publishes a message with multiple topics to the network. | |||
/// Publishes a message to a topic in the network. | |||
pub fn publish<H: Hasher>( | |||
&mut self, | |||
topic: Topic<H>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
topic: Topic<H>, | |
topic: impl Into<TopicHash>, |
Would it make sense to generalise the API to this instead of duplicating the function?
This would allow you to pass either a topic or a topic hash.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @AgeManning What do you think of this API change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this approach, as it extends the existing API without overcomplicating it. Changed this PR to match this comment and added a changelog item.
Modify publish() signature to use impl Into<TopicHash>.
Into<TopicHash>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good from my end!
Going to wait a little longer to see if we can get a 2nd review in :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this looks good to me also. Thanks for the contribution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thanks for the patch.
Description
I store TopicHashes for gossipsub topics in my app for performance reasons. And rehashing Topics every time I want to publish seems redundant to me.
Change checklist