-
Notifications
You must be signed in to change notification settings - Fork 998
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
refactor(gossipsub)!: initialize ProtocolConfig
from GossipsubConfig
#3381
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.
Other than the changelog entry, good to merge from my end.
- Initialize `ProtocolConfig` via `GossipsubConfig`. See [PR 3381]. | ||
|
||
[PR 3207]: https://github.com/libp2p/rust-libp2p/pull/3207/ | ||
[PR 3381]: https://github.com/libp2p/rust-libp2p/pull/3381/ |
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.
Why does this need a changelog entry? This is not user facing, right?
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.
It is (unfortunately) part of the public API.
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.
Happy to remove it again if you think that doesn't warrant an entry!
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.
Oh, I am surprised. No strong opinion. Let's keep as is 👍
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.
Yep, it's something I'd like to fix. See linked discussion in PR description.
Clippy stable is continuously failing with:
|
Thanks for fixing this, must have been a bad merge or something! |
Description
This simplifies the tests as we don't have to go through the
new_handler
abstraction.Notes
Extracted out of #3254.
Unfortunately, this is a breaking change because the
protocol
module is public. See #3303 (comment) for more discussion.Links to any relevant issues
Open Questions
Change checklist