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

core: encapsulate multistream-select #3759

Closed
thomaseizinger opened this issue Apr 10, 2023 · 1 comment · Fixed by #3912
Closed

core: encapsulate multistream-select #3759

thomaseizinger opened this issue Apr 10, 2023 · 1 comment · Fixed by #3912
Milestone
v0.52.0

Comments

@thomaseizinger
Copy link
Contributor

thomaseizinger commented Apr 10, 2023 •
edited
Loading

Description

At the moment, the multiple types from multistream-select leak into the libp2p-core APIs, resulting in cascading breaking changes.

  • Error types should be mapped to a new error in libp2p-core. This would also flatten the UpgradeError type.
  • The resulting stream can be wrapped in a newtype.

Motivation

Stop the cascade of breaking changes. For example, #2831 could have been done in a backwards-compatible way if we would already have this in place.

Are you planning to do it yourself in a pull request?

Yes.
@thomaseizinger thomaseizinger added this to the v0.52.0 milestone Apr 10, 2023
@thomaseizinger thomaseizinger changed the title core: encapsulate multistream-select within libp2p-core core: encapsulate multistream-select Apr 10, 2023
@thomaseizinger thomaseizinger mentioned this issue Apr 10, 2023
Closed
4 tasks
@thomaseizinger
Copy link
Contributor Author

Here are some updated thoughts on this:

I think upgrading the transport layer should be completely encapsulated in a crate like libp2p-core (or wherever we make the upgrading live, see #3748).

For protocols, I'd also advocate that we should actually expose a newtype to them. Once we have #2863, I think they should just receive an opaque Stream type. That would make multistream-select a complete implementation detail of the transport upgrades and libp2p-swarm.

@thomaseizinger thomaseizinger mentioned this issue May 5, 2023
Merged
4 tasks
mergify bot pushed a commit that referenced this issue May 8, 2023
@thomaseizinger
feat(swarm): make stream uprade errors more ergonomic
81c424e 
The currently provided `ConnectionHandlerUpgrErr` is very hard to use. Not only does it have a long name, it also features 3 levels of nesting which results in a lot of boilerplate. Last but not least, it exposes `multistream-select` as a dependency to all protocols.

We fix all of the above by renaming the type to `StreamUpgradeError` and flattening out its interface. Unrecoverable errors during protocol selection are hidden within the `Io` variant.

Related: #3759.

Pull-Request: #3882.
@thomaseizinger thomaseizinger mentioned this issue May 11, 2023
Merged
4 tasks
@mergify mergify bot closed this as completed in #3912 May 12, 2023
mergify bot pushed a commit that referenced this issue May 12, 2023
@thomaseizinger
feat(swarm): deprecate NegotiatedSubstream in favor of Stream
9e62588 
This patch tackles two things at once that are fairly intertwined:

1. There is no such thing as a "substream" in libp2p, the spec and other implementations only talk about "streams". We fix this by deprecating `NegotiatedSubstream`.
2. Previously, `NegotiatedSubstream` was a type alias that pointed to a type from `multistream-select`, effectively leaking the version of `multistream-select` to all dependencies of `libp2p-swarm`. We fix this by introducing a `Stream` newtype.

Resolves: #3759.
Related: #3748.

Pull-Request: #3912.
@thomaseizinger thomaseizinger modified the milestones: v0.52.0, Simplify ConnectionHandler trait Sep 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.52.0
Development

Successfully merging a pull request may close this issue.

feat(swarm): deprecate NegotiatedSubstream in favor of Stream libp2p/rust-libp2p
1 participant
@thomaseizinger