multiformats / rust-multiaddr

multiaddr implementation in rust
https://crates.io/crates/multiaddr
Other
86 stars 45 forks source link

feat: add with_p2p method on multiaddr #102

Closed stormshield-frb closed 10 months ago

stormshield-frb commented 10 months ago

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: https://github.com/libp2p/rust-libp2p/pull/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)
}
thomaseizinger commented 10 months ago

Can you update the PR description to reflect our findings?