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

protocols/gossipsub: Allow publishing to anything that implements Into<TopicHash> #2862

Merged
merged 6 commits into from
Sep 5, 2022

Conversation

GamePad64
Copy link
Contributor

@GamePad64 GamePad64 commented Aug 31, 2022

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

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

@GamePad64 GamePad64 marked this pull request as ready for review August 31, 2022 21:21
Copy link
Contributor

@thomaseizinger thomaseizinger left a 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.)

@@ -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>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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.

Copy link
Contributor

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?

Copy link
Contributor Author

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.

@thomaseizinger thomaseizinger changed the title Allow publishing to gossipsub using topichash protocols/gossipsub: Allow publishing to anything that implements Into<TopicHash> Sep 2, 2022
Copy link
Contributor

@thomaseizinger thomaseizinger left a 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 :)

Copy link
Contributor

@AgeManning AgeManning left a 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

Copy link
Member

@mxinden mxinden left a 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.

@mxinden mxinden merged commit b8c3b28 into libp2p:master Sep 5, 2022
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.

4 participants