Add support for extensions of the multiaddr interface.#135
Add support for extensions of the multiaddr interface.#135willscott wants to merge 5 commits intomultiformats:masterfrom
Conversation
willscott
force-pushed
the
feat/custom-multiaddr
branch
from
2b598a8 to
469fa69
Compare
This introduces a feature flag `Custom` that can be used to allow for parsing and management of protocols that are not part of the hard-coded set of known multiaddrs. This parallels the more permissive support that is found in js and golang implementations. by default, unknown protocols will not be parsed, since the semantics of how to parse their arguments cannot be known, but they can be manually constructed and then serialized to string. Specific extension protocols can be registered in a protocol registry to extend the default set and allow for handling of multiaddrs using those additional protocols more naturally.
willscott
force-pushed
the
feat/custom-multiaddr
branch
from
469fa69 to
2904bdf
Compare
|
@willscott : Thank you for submitting the PR, Will. Appreciate it. Supporting extensibility in the multiaddr interface while keeping safe defaults is neat. Looping in @jxs and @elenaf9 from the rust-libp2p maintainers team. Would be great to get their perspective on how this fits with existing parsing and protocol handling patterns. Looking forward to seeing this evolve. |
willscott
force-pushed
the
feat/custom-multiaddr
branch
from
7487793 to
9916052
Compare
…binary<>string format round trips
willscott
force-pushed
the
feat/custom-multiaddr
branch
from
9916052 to
8aceb4d
Compare
|
Friendly ping |
@willscott : Hi Will. Thank you for the kind reminder. Appreciate it. We have a rust-libp2p maintainer's call tomorrow: please visit https://luma.com/3ub5s44e?tk=DGCnFU . I have added your PR in the agenda item to be discussed with the maintainers tomorrow. I will also ask @johannamoran to share a luma invite to @jxs and @elenaf9, today. Ccing @johannamoran . |
elenaf9
left a comment
elenaf9
left a comment
There was a problem hiding this comment.
Please see the comments below.
| multibase::Base::Base58Btc | ||
| .decode(part) | ||
| .map_err(|_| Error::InvalidProtocolString)? |
There was a problem hiding this comment.
Why Base58 bitcoin specifically?
| .map_err(|_| Error::UnknownProtocolString(unknown.to_string()))?; | ||
| let data = match iter.next() { | ||
| Some("") => vec![], | ||
| Some(s) => match multibase::Base::Base58Btc.decode(s) { |
There was a problem hiding this comment.
Again, why Base58 bitcoin?
willscott
force-pushed
the
feat/custom-multiaddr
branch
from
b1fc7bd to
3686085
Compare
|
I think I've largely addressed the requested changes. You do a raise a good question around the use of We could also plausibly error in that case, but being able to handle this is somewhat important for being able to have the display / fmt of printing an inbound multiaddr not lead to a panic or failure - you want to have some representation of these unknown things you can display even if it involves this sort of arbitrary choice. |
I am not sure I understand what you mean. Can you elaborate a bit more on your usecase? |
no - happy to switch to Base64
you parse from the wire a multiaddr of a peer. it uses a protocol you don't know. the first thing you're likely to do is to log it. that will currently cause a panic, because the display / to-string won't have that protocol, and so will not know how to proceed. - in this |
elenaf9
left a comment
elenaf9
left a comment
There was a problem hiding this comment.
you parse from the wire a multiaddr of a peer. it uses a protocol you don't know. the first thing you're likely to do is to log it. that will currently cause a panic, because the display / to-string won't have that protocol, and so will not know how to proceed. - in this
unknownsupported feature, we allow that printing to happen by encoding the parameter associated with the unknown protocol code.
Why can't we just log the Error::UnknownProtocolString, which includes the data as well?
If I understand it correctly, the purpose of Protocol::Unknown is to allow Protocol::from_bytes to succeed, so you can then later manually parse the Multiaddr again by using the registry.
Wdyt of just adding a second method Protocol::from_bytes_with_registry? I think that would save us the need for this extra step via Protocol::Unknown, which IMO is a bit confusing.
| #[cfg(feature = "custom")] | ||
| unknown if unknown.starts_with("unknown-") => { | ||
| let id_str = &unknown["unknown-".len()..]; | ||
| let id: u32 = id_str | ||
| .parse() | ||
| .map_err(|_| Error::UnknownProtocolString(unknown.to_string()))?; | ||
| let data = match iter.next() { | ||
| Some("") => vec![], | ||
| Some(s) => match multibase::Base::Base64Url.decode(s) { | ||
| Ok(d) => d, | ||
| Err(_) => return Err(Error::InvalidProtocolString), | ||
| }, | ||
| None => vec![], | ||
| }; | ||
| Ok(Protocol::Unknown(id, std::borrow::Cow::Owned(data))) | ||
| } |
There was a problem hiding this comment.
I don't really see a use-case for this. Why would a user want to parse a multiaddr String with an unknown Protocol back into a Multiaddr?
4 participants
This introduces a feature flag
Customthat can be used to allow for parsing and management of protocols that are not part of the hard-coded set of known multiaddrs.This parallels the more permissive support that is found in js and golang implementations.
by default, unknown protocols will not be parsed, since the semantics of how to parse their arguments cannot be known, but they can be manually constructed and then serialized to string. Specific extension protocols can be registered in a protocol registry to extend the default set and allow for handling of multiaddrs using those additional protocols more naturally.