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 /dns, /wss, /relay, /p2p protocol codes #34

Merged
merged 4 commits into from
Jan 20, 2017
Merged

Conversation

ghost
Copy link

@ghost ghost commented Jan 17, 2017

Also add a fourth column, for comments.

  • We'll be using the experimental /dns-exp protocol for now, as per /dns in multiaddrs #22. Once we're sure it's right, we'll make it into /dns.
  • Same procedure for /relay, where in addition to the semantics, we're not even sure about the name.
  • /wss on the other hand is pretty straightforward.

@ghost ghost added kind/enhancement A net-new feature or improvement to an existing feature needs review labels Jan 17, 2017
@ghost ghost requested a review from daviddias January 17, 2017 16:21
@ghost
Copy link
Author

ghost commented Jan 18, 2017

Hey, updated this -- this PR now covers:

  • /p2p
  • /wss
  • experimental /dns, /dns4, /dns6
  • experimental /relay
  • /unix which was already in go-multiaddr

@ghost ghost requested a review from jbenet January 18, 2017 03:55
275, 0, libp2p-webrtc-star

16383, V, exp-dns, experimental /dns
Copy link
Author

Choose a reason for hiding this comment

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

This is based on the highest value of a 2-byte uvarint: 2^14 - 1 = 11 1111 1111 1110 = 16383

@ghost ghost changed the title Add /dns, /wss, /relay protocol codes Add /dns, /wss, /relay, /p2p protocol codes Jan 18, 2017
275, 0, libp2p-webrtc-star
276, 0, libp2p-webrtc-direct
290, V, libp2p-relay
Copy link
Member

Choose a reason for hiding this comment

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

can it be libp2p-circuit-relay, pretty please? :)

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

yep updated! 👍

16383, V, exp-dns, experimental /dns
16382, V, exp-dns4, experimental /dns4
16381, V, exp-dns6, experimental /dns6
Copy link
Member

Choose a reason for hiding this comment

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

Do we need it to be experimental? Isn't it going to be more of an 'experimental implementation', than a 'experimental addr'?

Copy link
Author

Choose a reason for hiding this comment

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

Do we need it to be experimental? Isn't it going to be more of an 'experimental implementation', than a 'experimental addr'?

It's probably experimental on both address semantics and implementation.

The practical reason is I'm not set on whether we should do only /dns, or only /dns[4,6], or both, and would like to implement both before settling on an option.

The process reason is I'm pretty confident moving fast with protocols prefixed with /libp2p- and protocols that don't have values (like /ws), because they're likely easy to change or abandon. I feel like the more essential protocols like /ip4 and /dns need more care.

Copy link
Member

Choose a reason for hiding this comment

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

Understood :)

Copy link
Member

@daviddias daviddias left a comment

Choose a reason for hiding this comment

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

Couple of comments

Lars Gierth and others added 4 commits January 19, 2017 03:31
Also add a fourth column, for comments.

- We'll be using the experimental /dns-exp protocol for now, as per #22. Once we're sure it's right, we'll make it into /dns.
- Same procedure for /relay, where in addition to the semantics, we're not even sure about the name.
- /wss on the other hand is pretty straightforward.
- /p2p
- /wss
- experimental /dns, /dns4, /dns6
- experimental /relay
- /unix which was already in go-multiaddr
@ghost ghost force-pushed the lgierth-patch-2 branch from e25be1a to cc94f5e Compare January 19, 2017 02:32
275, 0, libp2p-webrtc-star
276, 0, libp2p-webrtc-direct
290, V, libp2p-circuit-relay
Copy link
Member

Choose a reason for hiding this comment

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

Is relay a multiaddr? Should be a 'mount' protocol, no?

Copy link
Author

Choose a reason for hiding this comment

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

The way I wanna do it (haven't gotten much feedback) is for it to be a mount/swarm protocol and transport at the same time. The former for speaking relay with other nodes, the latter to actually send stuff through it.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see. We will need to update it also here: https://github.com/libp2p/specs/blob/master/7-properties.md#757-protocol-multicodecs (that table is from April 2016)

You mention not getting much feedback on your proposal, where can I read the circuit relay spec and provide my review?

Copy link
Author

Choose a reason for hiding this comment

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

Yep will update the protocols table.

Cool to merge?

@daviddias daviddias merged commit b6fc1f6 into master Jan 20, 2017
@daviddias daviddias deleted the lgierth-patch-2 branch January 20, 2017 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement A net-new feature or improvement to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant