-
Notifications
You must be signed in to change notification settings - Fork 46
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 /webrtc
and /certhash
#59
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you Anton for working on this.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/cc @marten-seemann maybe you have some arguments in favor of certhash
having various bases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me 🙏
Will leave this open for a bit longer to allow others to get involved. Will merge next week.
Are there any plans to merge this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <[email protected]>
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