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: make upgrade::apply an implementation detail of transport upgrades #3748

Closed
Tracked by #3271
thomaseizinger opened this issue Apr 6, 2023 · 5 comments · Fixed by #3915
Closed
Tracked by #3271

core: make upgrade::apply an implementation detail of transport upgrades #3748

thomaseizinger opened this issue Apr 6, 2023 · 5 comments · Fixed by #3915
Assignees
@thomaseizinger

Comments

@thomaseizinger
Copy link
Contributor

thomaseizinger commented Apr 6, 2023 •
edited
Loading

Description

Currently, we expose the generic upgrade::apply function. I'd suggest we make it an implementation detail and transition all usages to something else. The usages are only in tests and in from_fn (which I am suggesting to remove: #3747).

Motivation

Smaller, more focused API surface.

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

Yes.
@thomaseizinger thomaseizinger added the decision-pending Marks issues where a decision is pending before we can move forward. label Apr 6, 2023
@thomaseizinger
Copy link
Contributor Author

@mxinden please voice your opinion on this.

This was referenced Apr 11, 2023
Merged
Merged
@thomaseizinger
Copy link
Contributor Author

thomaseizinger commented Apr 11, 2023 •
edited
Loading

The order of things to achieve this is:

  1. Remove the dependency of upgrade::apply from all current usages:
  1. Make the current function crate private and expose a deprecated, public wrapper. Telling people that it is not meant to be used.
  2. With the next series of breaking changes, remove the function

This was referenced Apr 11, 2023
Merged
Merged
@thomaseizinger thomaseizinger self-assigned this Apr 11, 2023
This was referenced Apr 11, 2023
Open
Closed
@thomaseizinger thomaseizinger mentioned this issue Apr 19, 2023
Merged
4 tasks
mergify bot pushed a commit that referenced this issue Apr 26, 2023
@thomaseizinger
refactor(deflate): implement tests with futures_ringbuf
7fa8bef 
Previously, we used a TCP transport to test the deflate compression. Really, all we need is an in-memory ringbuffer that we can plug in at both ends. This PR uses `futures_ringbuf` for that.

Related #3748.

Pull-Request: #3771.
mergify bot pushed a commit that referenced this issue Apr 26, 2023
@thomaseizinger
refactor(muxer): replace MemoryTransport with futures_ringbuf
35132c4 
Instead of relying on the `MemoryTransport` to provide us with a duplex stream, we use the `futures_ringbuf` crate. This saves us several lines of code and removes the dependency on `libp2p_core::upgrade::apply`.

Related #3748.

Pull-Request: #3772.
mergify bot pushed a commit that referenced this issue Apr 26, 2023
@thomaseizinger
refactor(plaintext): implement tests with futures_ringbuf
4dd2e0e 
Instead of doing an entire connection establishment dance, we simply use a futures-aware ringbuffer to implement the test. This removes the dependency of `libp2p-plaintext` on the `libp2p_core::upgrade::apply` function.

Related #3748.

Pull-Request: #3770.
@mxinden
Copy link
Member

mxinden commented Apr 28, 2023

Sorry for the late reply. Fine by me. Is the change worth the effort though? I would not give it a high priority.

@thomaseizinger
Copy link
Contributor Author

Sorry for the late reply. Fine by me. Is the change worth the effort though? I would not give it a high priority.

I think it is worth the effort but I agree that it is not high priority. I am working on it as a small side project. Plus, I think the linked changes are a net-positive by themselves.

@thomaseizinger thomaseizinger removed the decision-pending Marks issues where a decision is pending before we can move forward. label Apr 28, 2023
@thomaseizinger
Copy link
Contributor Author

Also, really what I am doing is resolving your TODO from a year ago: 96dbfcd#diff-f176cef6da6d32fd9e595a428f86a279f3d84f0a06e41ab3b98491737048a91eR30 🙃

@thomaseizinger thomaseizinger mentioned this issue Apr 28, 2023
Merged
4 tasks
mergify bot pushed a commit that referenced this issue May 2, 2023
@thomaseizinger
refactor(noise): implement tests using futures_ringbuf
4ca8885 
Instead of creating a connection via a TCP transport, we can use the `futures_ringbuf` crate and run the noise encryption on an in-memory transport. This removes the dependency on the `libp2p_core::upgrade::apply` function and takes less code to implement.

Related #3748.

Pull-Request: #3773.
mergify bot pushed a commit that referenced this issue May 8, 2023
@thomaseizinger
refactor(identify): bring tests up to workspace standard
fa4adc8 
This patch refactors the identify tests to use `libp2p-swarm-test`. This allows us to delete quite a bit of code and makes several dev-dependencies obsolete.

The `correct_transfer` test is made obsolete by more precise assertions in the `periodic_identify` test. This allows us to remove the dependency on the `upgrade::{apply_inbound,apply_outbound}` functions.

Finally, we also fix a bug where the reported listen addresses to the other node could contain duplicates.

Related: #3748.

Pull-Request: #3851.
This was referenced May 11, 2023
Merged
Merged
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.
@mergify mergify bot closed this as completed in #3915 May 15, 2023
mergify bot pushed a commit that referenced this issue May 15, 2023
@thomaseizinger
feat(core): make the upgrade module an implementation detail
08f0b5e 
Resolves: #3748.

Pull-Request: #3915.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@thomaseizinger thomaseizinger

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat(core): make the upgrade module an implementation detail libp2p/rust-libp2p
2 participants
@thomaseizinger @mxinden