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

Add conversions between sockets and multiaddrs #99

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jmcph4
Copy link

@jmcph4 jmcph4 commented Sep 5, 2023

This PR allows two type conversions involving both Multiaddr and SocketAddr:

  • An infallible conversion from SocketAddr to Multiaddr
  • A fallible conversion from Multiaddr to SocketAddr (as the space of all possible values of the former is obviously much larger than that for the latter)

These are certainly fairly minor changes; however, they enable some niceties such as:

match libp2p_event {
    FromSwarm::NewListenAddr { listener_id: _, addr: Multiaddr } => if let Ok(sock) = addr.try_into::<SocketAddr>() {
        do_something_with_socket(sock);
    } else {
        panic!("Something went wrong!");
    }
}

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.

We should consider Udp as well and not just Tcp.

I think these would be better served as dedicated functions so we can include the tcp bit in the name to make this clear.

What is also not clear to me is what should happen with the remaining components of the multiaddr? Currently we just drop them. Should we return them instead?

@jmcph4
Copy link
Author

jmcph4 commented Sep 6, 2023

We should consider Udp as well and not just Tcp.

I think these would be better served as dedicated functions so we can include the tcp bit in the name to make this clear.

I agree, I'll add the equivalent UDP methods and make the existing TCP ones dedicated methods rather than From and TryFrom implementations.

What is also not clear to me is what should happen with the remaining components of the multiaddr? Currently we just drop them. Should we return them instead?

Are you referring to the case where we need to produce a SocketAddr from a multiaddr like /ip4/192.0.2.0/tcp/4321/p2p/QmcEPrat8ShnCph8WjkREzt5CPXF2RwhYxYBALDcLC1iV6? Given we're abandoning the From and TryFrom approach, we could perhaps take an approach like:

pub fn into_tcp_socket(&self) -> Option<(SocketAddr, Multiaddr)> {
    /* snip */
    Some((sock, remaining))
}

Which would yield (sock: 192.0.2.0:4321, remaining: /p2p/QmcEPrat8ShnCph8WjkREzt5CPXF2RwhYxYBALDcLC1iV6).

The issue with this is that it implicitly assumes a left-to-right parsing approach for what it means to convert a multiaddr into a SocketAddr. I think this is the most sane approach, but it's still something to consider.

@thomaseizinger
Copy link
Contributor

thomaseizinger commented Sep 6, 2023

The issue with this is that it implicitly assumes a left-to-right parsing approach for what it means to convert a multiaddr into a SocketAddr. I think this is the most sane approach, but it's still something to consider.

Left-to-right is how you are meant to parse multiaddr, see https://github.com/multiformats/multiaddr?tab=readme-ov-file#interpreting-multiaddrs.

Perhaps, the functions should be called something like:

impl Multiaddr {
	pub fn pop_front_tcp_socket(&mut self) -> Option<SocketAddr>;
}

This modifies the Multiaddr instead of returning the remaining bits. Getting None would mean the multiaddr is unmodified.

@thomaseizinger
Copy link
Contributor

Multiaddr is a very fundamental crate to the Rust libp2p ecosystem. I am a bit hesitant to include a lot of convenience functions in here if it can be implemented externally as well.

Perhaps what we should add is a pop_front function that modifies the Multiaddr.

You should then be able to implement an efficient extension trait that uses iter() to peek at the first components and if they represent the correct components, you can use pop_front to remove them.

Curious to hear what other maintainers think.

match socket {
SocketAddr::V4(sock) => Self::empty()
.with(Protocol::Ip4(*sock.ip()))
.with(Protocol::Tcp(sock.port())),
Copy link
Member

Choose a reason for hiding this comment

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

SocketAddr might as well specify a UDP address, thus this From implementation is not correct.

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.

None yet

3 participants