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: add with_p2p method on multiaddr #102

Merged

Conversation

stormshield-frb
Copy link
Contributor

@stormshield-frb stormshield-frb commented Oct 12, 2023

PR context

In order to have a uniformized usage of multiaddr with a /p2p suffix in the rust-libp2p repository, we are finding ourselves in need of utility method to easily ensure that a multiaddr is terminated by the p2p protocol for a given PeerId.

Our first thought was to only implement this method in our code base but since it is needed in several crates of ours and it could be useful to other people, we thought it could be a good idea to implement it directly on Multiaddr.

This is related to the following PR on the rust-libp2p repository: libp2p/rust-libp2p#4596

Implementation decisions

After some discussions, we (@thomaseizinger and myself) decided to implement a simple with_p2p function on multiaddr.

We studied a possibility to improve the push and with functions to be smart enough to not append the same protocol twice since it does not have any meaning and would result in an unusable multiaddr (except for some protocols like certhash). Although it would probably be a great idea and a great improvement, it would result in an important breaking API change since we would then need to return a Result (because a call to with or push could fail when trying to append the same protocol twice with a different content). Since this seems quite overkill for the current need we decided in the end that we leave the push and with function as it is for now.

We also studied the addition of a from_str_and_peer function, but seeing the resulting implementation, we decided it was not worth it to expand the public API surface for a one-line function.

pub fn from_str_and_peer(addr: &str, peer: PeerId) -> Result<Self> {
        addr.parse::<Self>()?.with_p2p(peer).map_err(|_| Error::InvalidMultiaddr)
}

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
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.

Thank you for tackling this inconsistency across the codebase @stormshield-frb! I am in favor of a helper function in multiaddr.

src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Thanks for working in the feedback! Two points.

tests/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Yeah that works, thank you!

@thomaseizinger
Copy link
Contributor

Can you update the PR description to reflect our findings?

@thomaseizinger thomaseizinger requested review from mxinden and vmx October 18, 2023 10:26
Copy link
Member

@vmx vmx left a comment

Choose a reason for hiding this comment

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

I can't say much whether this addition is useful or not, but from a Rust perspective it looks good to me.

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.

Thank you! Well done.

I suggest I cut a new release once we have some pull request using this new method, e.g. libp2p/rust-libp2p#4596. Sounds good?

@mxinden mxinden changed the title feat!: add to_p2p_terminated method on multiaddr feat!: add with_p2p_terminated method on multiaddr Oct 18, 2023
@mxinden mxinden changed the title feat!: add with_p2p_terminated method on multiaddr feat!: add with_p2p method on multiaddr Oct 18, 2023
@mxinden mxinden changed the title feat!: add with_p2p method on multiaddr feat: add with_p2p method on multiaddr Oct 18, 2023
@mxinden mxinden merged commit d0e9ec2 into multiformats:master Oct 18, 2023
5 checks passed
@stormshield-frb stormshield-frb deleted the feat/add-to-p2p-terminated-method branch October 23, 2023 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants