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

feat(swarm): deprecate NegotiatedSubstream in favor of Stream #3912

Merged
merged 10 commits into from
May 12, 2023

Conversation

thomaseizinger
Copy link
Contributor

@thomaseizinger thomaseizinger commented May 11, 2023

Description

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.

Notes & open questions

Despite being an invasive and technically breaking change, I think the upgrade should be pretty painless.

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

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.

One small nit pick. Otherwise looks good to me. Good to get rid of Substream*!

@@ -470,24 +472,23 @@ impl<'a> IncomingInfo<'a> {
}
}

struct SubstreamUpgrade<UserData, Upgrade> {
struct SubstreamUpgrade<UserData, TOk, TErr> {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
struct SubstreamUpgrade<UserData, TOk, TErr> {
struct StreamUpgrade<UserData, TOk, TErr> {

Given that this is an internal type, why not already rename it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It didn't even occur to me, yes makes a lot of sense :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are a fair few more usages of "substream" but I think those can easily go in separate PRs.

Copy link
Member

Choose a reason for hiding this comment

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

There are a fair few more usages of "substream" but I think those can easily go in separate PRs.

👍 works for me.

swarm/CHANGELOG.md Outdated Show resolved Hide resolved
swarm/CHANGELOG.md Outdated Show resolved Hide resolved
@mergify mergify bot merged commit 9e62588 into master May 12, 2023
@mergify mergify bot deleted the feat/swarm/decouple-mss branch May 12, 2023 06:19
@drHuangMHT
Copy link
Contributor

What about SubstreamProtocol? That's what you need to negotiate for an outbound (sub)stream, am I right?

@thomaseizinger
Copy link
Contributor Author

What about SubstreamProtocol? That's what you need to negotiate for an outbound (sub)stream, am I right?

We should get rid of all the "sub"-stream terminology yes. I've not had time to follow up on this PR. Most of the symbols are in individual crates so it is not urgent.

Perhaps the only one we should rename as part of this breaking release is Muxer::Substream.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

core: encapsulate multistream-select
3 participants