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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Aug 19, 2022
Merged

add /webrtc and /certhash #59

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ecb05e6
add `/webrtc` and `/certhash`
melekes Jul 13, 2022
9692716
update changelog
melekes Jul 13, 2022
c09d140
fix error
melekes Jul 13, 2022
d9e68fa
make certhash multibase
melekes Jul 14, 2022
dd16c4d
fix test
melekes Jul 14, 2022
c8dcc39
Update CHANGELOG.md
mxinden Jul 17, 2022
7cb2408
Update src/protocol.rs
melekes Jul 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,15 @@
# 0.15.0 [unreleased]

- Add `WebRTC` instance for `Multiaddr`. See [PR 59]
- Add `Certhash` instance for `Multiaddr`. See [PR 59
mxinden marked this conversation as resolved.
Show resolved Hide resolved

- Add support for Noise protocol. See [PR 53].

- Use `multibase` instead of `bs58` for base58 encoding. See [PR 56].

[PR 53]: https://github.com/multiformats/rust-multiaddr/pull/53
[PR 56]: https://github.com/multiformats/rust-multiaddr/pull/56
[PR 59]: https://github.com/multiformats/rust-multiaddr/pull/59

# 0.14.0 [2022-02-02]

Expand Down
31 changes: 31 additions & 0 deletions src/protocol.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ const IP4: u32 = 4;
const IP6: u32 = 41;
const P2P_WEBRTC_DIRECT: u32 = 276;
const P2P_WEBRTC_STAR: u32 = 275;
const WEBRTC: u32 = 280;
const CERTHASH: u32 = 281;
melekes marked this conversation as resolved.
Show resolved Hide resolved
const P2P_WEBSOCKET_STAR: u32 = 479;
const MEMORY: u32 = 777;
const ONION: u32 = 444;
Expand Down Expand Up @@ -80,6 +82,8 @@ pub enum Protocol<'a> {
Ip6(Ipv6Addr),
P2pWebRtcDirect,
P2pWebRtcStar,
WebRTC,
Certhash(Multihash),
P2pWebSocketStar,
/// Contains the "port" to contact. Similar to TCP or UDP, 0 means "assign me a port".
Memory(u64),
Expand Down Expand Up @@ -192,6 +196,12 @@ impl<'a> Protocol<'a> {
}
"p2p-websocket-star" => Ok(Protocol::P2pWebSocketStar),
"p2p-webrtc-star" => Ok(Protocol::P2pWebRtcStar),
"webrtc" => Ok(Protocol::WebRTC),
"certhash" => {
let s = iter.next().ok_or(Error::InvalidProtocolString)?;
let decoded = multibase::Base::Base58Btc.decode(s)?;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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

Copy link
Member

@tomaka tomaka Jul 14, 2022 •
edited
Loading

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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?

Copy link

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.

Copy link
Member

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.

Copy link
Member

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.

multiformats/multiaddr#130 (comment)

Ok(Protocol::Certhash(Multihash::from_bytes(&decoded)?))
}
"p2p-webrtc-direct" => Ok(Protocol::P2pWebRtcDirect),
"p2p-circuit" => Ok(Protocol::P2pCircuit),
"memory" => {
Expand Down Expand Up @@ -268,6 +278,12 @@ impl<'a> Protocol<'a> {
}
P2P_WEBRTC_DIRECT => Ok((Protocol::P2pWebRtcDirect, input)),
P2P_WEBRTC_STAR => Ok((Protocol::P2pWebRtcStar, input)),
WEBRTC => Ok((Protocol::WebRTC, input)),
CERTHASH => {
let (n, input) = decode::usize(input)?;
let (data, rest) = split_at(n, input)?;
Ok((Protocol::Certhash(Multihash::from_bytes(data)?), rest))
}
P2P_WEBSOCKET_STAR => Ok((Protocol::P2pWebSocketStar, input)),
MEMORY => {
let (data, rest) = split_at(8, input)?;
Expand Down Expand Up @@ -441,6 +457,13 @@ impl<'a> Protocol<'a> {
}
Protocol::P2pWebSocketStar => w.write_all(encode::u32(P2P_WEBSOCKET_STAR, &mut buf))?,
Protocol::P2pWebRtcStar => w.write_all(encode::u32(P2P_WEBRTC_STAR, &mut buf))?,
Protocol::WebRTC => w.write_all(encode::u32(WEBRTC, &mut buf))?,
Protocol::Certhash(hash) => {
w.write_all(encode::u32(CERTHASH, &mut buf))?;
let bytes = hash.to_bytes();
w.write_all(encode::usize(bytes.len(), &mut encode::usize_buffer()))?;
w.write_all(&bytes)?
}
Protocol::P2pWebRtcDirect => w.write_all(encode::u32(P2P_WEBRTC_DIRECT, &mut buf))?,
Protocol::P2pCircuit => w.write_all(encode::u32(P2P_CIRCUIT, &mut buf))?,
Protocol::Memory(port) => {
Expand All @@ -466,6 +489,8 @@ impl<'a> Protocol<'a> {
Ip6(a) => Ip6(a),
P2pWebRtcDirect => P2pWebRtcDirect,
P2pWebRtcStar => P2pWebRtcStar,
WebRTC => WebRTC,
Certhash(hash) => Certhash(hash),
P2pWebSocketStar => P2pWebSocketStar,
Memory(a) => Memory(a),
Onion(addr, port) => Onion(Cow::Owned(addr.into_owned()), port),
Expand Down Expand Up @@ -502,6 +527,12 @@ impl<'a> fmt::Display for Protocol<'a> {
Ip6(addr) => write!(f, "/ip6/{}", addr),
P2pWebRtcDirect => f.write_str("/p2p-webrtc-direct"),
P2pWebRtcStar => f.write_str("/p2p-webrtc-star"),
WebRTC => f.write_str("/webrtc"),
Certhash(hash) => write!(
f,
"/certhash/{}",
multibase::Base::Base58Btc.encode(hash.to_bytes())
melekes marked this conversation as resolved.
Show resolved Hide resolved
),
P2pWebSocketStar => f.write_str("/p2p-websocket-star"),
Memory(port) => write!(f, "/memory/{}", port),
Onion(addr, port) => {
Expand Down