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

Encapsulate relay::client::Connection into an Newtype type that only exposes AsyncRead and AsyncWrite #3255

Closed
jxs opened this issue Dec 16, 2022 · 4 comments · Fixed by #3829
Closed

Encapsulate relay::client::Connection into an Newtype type that only exposes AsyncRead and AsyncWrite #3255

jxs opened this issue Dec 16, 2022 · 4 comments · Fixed by #3829
Labels
difficulty:easy getting-started Issues that can be tackled if you don't know the internals of libp2p very well

Comments

@jxs
Copy link
Member

jxs commented Dec 16, 2022

Motivation

As per #3238/files#r1050261660 Connection is a public enum variant with struct variants, any change to it becomes a subsequent breaking change.

Desired changes

Encapsulate that one into an Newtype that only exposes AsyncRead and AsyncWrite as its interface:

  • Rename the type to ConnectionState & make it private
  • Create a struct Connection as Newtype that encapsulates ConnectionState.
  • Move the AsyncRead and AsyncWrite implementations from the enum to the struct

this depends on #3238 as it renames RelayedConnection to Connection and is itself a breaking change. Probably should also wait on the discussion started on #3221 (comment).

CC @thomaseizinger

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

Yes if required.
@jxs jxs added difficulty:easy getting-started Issues that can be tackled if you don't know the internals of libp2p very well labels Dec 16, 2022
@jxs jxs mentioned this issue Dec 16, 2022
Merged
4 tasks
@mxinden
Copy link
Member

mxinden commented Dec 19, 2022

Sounds good to me. Thanks for documenting it.

@Danijel-Enoch
Copy link

Hello guys I am new to Open Source Contribution . Can I try this?

@thomaseizinger
Copy link
Contributor

Hello guys I am new to Open Source Contribution . Can I try this?

Sure thing! Let me know if you need more mentoring or any other help :)
Best to have a go yourself and open a draft PR if you get stuck, then we can discuss things there!

@Danijel-Enoch
Copy link

Thanks
Would get started

@tcoratger tcoratger mentioned this issue Apr 25, 2023
Merged
3 tasks
@mergify mergify bot closed this as completed in #3829 May 2, 2023
mergify bot pushed a commit that referenced this issue May 2, 2023
@tcoratger
feat(relay): hide internals of Connection
c728824 
Relayed connections to other peers are created from streams to the relay itself. Internally, such a connection has different states. These however are not relevant to the user and should be encapsulated to allow for more backwards-compatible changes. The only interface exposed is `AsyncRead` and `AsyncWrite`.

Resolves: #3255.

Pull-Request: #3829.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
difficulty:easy getting-started Issues that can be tackled if you don't know the internals of libp2p very well
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat(relay): hide internals of Connection tcoratger/rust-libp2p
4 participants
@jxs @thomaseizinger @mxinden @Danijel-Enoch