add /webrtc and /certhash
#59
Conversation
src/protocol.rs
Outdated
| "webrtc" => Ok(Protocol::WebRTC), | ||
| "certhash" => { | ||
| let s = iter.next().ok_or(Error::InvalidProtocolString)?; | ||
| let decoded = multibase::Base::Base58Btc.decode(s)?; |
There was a problem hiding this comment.
| let decoded = multibase::Base::Base58Btc.decode(s)?; | |
| let decoded = multibase::decode(s)?; |
Given that a /certhash is a multibase encoded multihash, this could have any base, not just Base58Btc. Am I missing something?
For reference, see Golang implementation: https://github.com/multiformats/go-multiaddr/pull/176/files
There was a problem hiding this comment.
Is it a good idea to allow multiple different bases? It means that multiple multiaddresses with different string representations might actually be equal, and it means that decoding then re-encoding a multiaddress might produce a different result than the original, both of which are rather surprising to me.
There was a problem hiding this comment.
Also, I suppose that my concern regarding the fact that we're using a multihash has simply been ignored, as usual for my concerns.
There was a problem hiding this comment.
Is it a good idea to allow multiple different bases? It means that multiple multiaddresses with different string representations might actually be equal, and it means that decoding then re-encoding a multiaddress might produce a different result than the original, both of which are rather surprising to me.
I also don't see any immediate neither long term benefits.
There was a problem hiding this comment.
/cc @marten-seemann maybe you have some arguments in favor of certhash having various bases?
There was a problem hiding this comment.
Not sure why we're having this discussion on the Rust repo. multiformats/multiaddr#130 is the PR that added the multiaddr component, and it seems like this discussion would logically belong there.
The reason for using a multibase is the same as for any multi-thing: "We're probably wrong today, but at least we keep our way open to be right tomorrow."
If there's no need to decide for one blessed way of doing things, why would we pick one?
More specifically, different environments (e.g. browser URLs) have different encoding requirements. Multibase makes it easy to adapt to these.
There was a problem hiding this comment.
multiformats/multiaddr#130 doesn't even define the format of the payload behind /certhash. There's been no discussion about this payload.
If there's no need to decide for one blessed way of doing things, why would we pick one?
There is some need, as otherwise multiple multiaddresses with different string representation might be equal, as mentioned above.
Choosing which encoding to use is an arbitrary decision, but "leave the choice to the user" is a decision as well, and in my opinion worse in strictly all ways compared to arbitrarily picking one.
There was a problem hiding this comment.
Oh, Marten raced me to it here. Slightly adjusted reply below.
Also, I suppose that my concern regarding the fact that we're using a multihash has simply been ignored, as usual for my concerns.
Sorry in case you feel like your points are being ignored. This is definitely not deliberate.
Not sure why we're having this discussion on the Rust repo. multiformats/multiaddr#130 is the PR that added the multiaddr component, and it seems like this discussion would logically belong there.
Good point. In favor of moving the discussion there.
also switch string representation to base64url and add tests
|
Are there any plans to merge this? |
There was a problem hiding this comment.
Sorry for the delay here @melekes.
Given the ongoing discussion in multiformats/multiaddr#130 I have postponed merging here. Sorry for not communicating that clearly.
Would you mind using your fork in libp2p/rust-libp2p#2622 a bit longer until the discussion settles?
Co-authored-by: Max Inden <mail@max-inden.de>
ah, thanks for linking that discussion. |
|
With libp2p/specs@7009f94 I will merge here, thus allowing libp2p/rust-libp2p#2622 to point to the |
This is why I don't bother commenting in |
multiformats/multiaddr#131