-
Notifications
You must be signed in to change notification settings - Fork 1k
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
feat(multistream-select): remove Version #4731
Conversation
Removes multistream-select version selection. Instead the mechanism of `Version::V1Lazy` is always used. See also libp2p#3563 and libp2p#3749.
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.
Concept ACK
If you want to remove it in v0.53
, then I'd suggest we ship a deprecation of upgrade
as a back-port first. Perhaps we should be a bit cautious here though. So far, I think the v0.53
change does not contain anything controversial whereas I remember that V1Lazy
caused some issues in polkadot, right? Perhaps we should push it to the next breaking release and make sure polkadot upgrades to v0.53
asap after this.
@@ -82,7 +82,7 @@ fn upgrade_pipeline() { | |||
let listener_keys = identity::Keypair::generate_ed25519(); | |||
let listener_id = listener_keys.public().to_peer_id(); | |||
let mut listener_transport = MemoryTransport::default() | |||
.upgrade(upgrade::Version::V1) | |||
.upgrade() |
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.
Technically we could remove upgrade
now, right?
What do you think of deprecating upgrade
first and instead offer an authenticate
function for each transport that simply implies upgrade(V1Lazy)
?
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, while looking into the PR this also made sense to me, would simplify the API, but would also probably make it a breaking change no?
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.
At the moment it is a breaking change. Deprecating it first and offering a new API that starts with authenticate
straight from Transport
is not.
I am in favor of deprecating |
Closing here. For anyone interested in this path, feel free to cherry-pick the commit. |
Description
Removes multistream-select version selection. Instead the mechanism of
Version::V1Lazy
is always used.See also #3563 and #3749.
Notes & open questions
I suggest we deprecate
Version
before removing it. Thoughts?Change checklist