-
Notifications
You must be signed in to change notification settings - Fork 85
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
feat: add webrtc-w3c protocol #150
Conversation
Adds support for the webrtc-w3c protocol described by the following spec: libp2p/specs#497
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.
Looks good to me. Here as well, will give others time to comment before merging.
multiformats/multicodec#316 is merged. I re-triggered CI which is green now. I remember having a discussion around keeping pull requests open for a certain amount of time. In the back of my head, that time frame was 48h. Thus I am merging here. Please comment in case we settled on a different timeline. |
Sorry for engaging here late. Please request a review from me in the future for faster responses. I'm strongly opposed to call this webrtc-w3c. It confuses the protocol with one API that makes use of that protocol. The browser API is specified by the W3C. Two reasons not to call this webrtc-w3c:
|
Arguments speaking for the name:
While non-browser nodes might not use the W3C API, they would still be limited by the above mentioned constraints and would have to adhere to the protocol flow dictated by the W3C specification.
In the browser-to-server specification, the browser does not correctly follow the W3C specification. E.g. it does SDP munging, it misuses the ufrag, ... I am still in favor of the name @marten-seemann with the above arguments, do you still feel strongly about not naming it |
Re-offering |
Is it possible to rename Otherwise, I would suggest
|
That applies to browser-to-server as well. Necessarily. The W3C API is the only one that's available in the browser.
All of this is specified by the IETF in a large number of RFCs.
That's a quite unintuitive naming. Of course the WebRTC contains holepunching and NAT traversal! That's an essential part of the WebRTC protocol. It's confusing to point it out explicitly. And it seems even more confusing to name something "webrtc" that doesn't contain these features.
I'd be favor of this approach. It's the right thing to do, and the only reason we didn't end up with this naming is that we specified browser-to-server first (arguably, we should have done it the other way around). In general, I'd argue that we should avoid breaking changes when it comes to multiaddr components as much as possible, but I think it's justified in this case as:
I'd also like to point out that we're only talking about the string representation. On the wire, multiaddresses are exchanged in their byte representation, which would be unaffected by the suggested renaming. |
I would not want to rename the existing code point. Folks are already using it. I don't want to push a breaking change. I personally don't care which exact name we choose here. I think having this as a multiaddr is technically wrong because it's not a direction on how to reach a node, it's a capability the node supports. It works over any existing connection (see my arguments here). Having this as part of the multiaddr is a stop-gap temporary hack until we have a better way of encoding a Peer's multiaddr's and their capabilities. I do care that we resolve this quickly so things can continue to progress and we don't spend time having to update names all over the place. To that end I'll suggest |
So far it seems the name will be changed. The current proposals are
For the last one, should this group outright decide against the breaking change or can we ask interested parties from Parity? If they dissent, that's a stronger case against the last option. Again not sure whether there are other power users but if we do the breaking change, might as well do it now before it is truly widely deployed |
While I like
I could understand if they'll push back against a renaming, and I truly feel sorry for suggesting to create extra work for them with this renaming. This is not definitely not ideal. |
I'd like to make another case for
This isn't a confusion at all but actually by design. The specification mentions the Yes we can maybe find a more general name but why bother? The name isn't wrong per se, is it? Also, the one API you are mentioning happens to be implemented by all browsers which is the main platform we want to target with this. If tomorrow a new platform is released that also offers WebRTC capabilities but uses a different API, we'd have to write a new spec anyway because the current one only mentions the |
Summary of a synchronous call with @marten-seemann and @p-shahi. Individual preferenceFor the record, @marten-seemann is still in favor of the rename of I am against the rename as I don't think it is worth the breaking change. Otherwise I don't have a strong opinion on the protocol name. @p-shahi, as mentioned above, gave great input from a user perspective where Consensus in the meetingAfter some discussion we reached consensus on the below within the participants of the meeting:
Steps forwardI am under the impression that neither of the others involved in this discussion (@ckousik @MarcoPolo @Menduist @thomaseizinger) feel very strongly about the naming. In case you do and want to block the above suggestions, please speak up. Unless there are any objections within the next 24h, we will go ahead with the above renames. Details for the avid reader
|
Not a blocking opinion but I think it is not a very descriptive name. Do we have a definition somewhere of what a "private" node is? I've not come across this term other than in the context of pnet. |
In the connectivity site's hole punching section we write
It might be a good idea to lift this definition into a glossary in the specs and docs site |
@tomaka was in favor of |
We have decided to rename `webrtc-w3c` to `webrtc-private-to-private`. See rational in multiformats/multiaddr#150. No software has been released using `webrtc-w3c`, thus renaming is not a breaking change.
We have decided to rename `webrtc-w3c` to `webrtc-private-to-private`. See rational in multiformats#150. No software has been released using `webrtc-w3c`, thus renaming is not a breaking change.
We have decided to rename `webrtc-w3c` to `webrtc-private-to-private`. See rational in multiformats/multiaddr#150. No software has been released using `webrtc-w3c`, thus renaming is not a breaking change. Ignoring the automated `multiaddr` `v11.5.0` release. See also multiformats#309 that introduced `webrtc-w3c`.
No blocking objections in the past 24h. Interpreting this as consensus. Pull requests updating the multiaddr name to
Approvals from @marten-seemann @MarcoPolo and @achingbrain very much appreciated 🙏 |
See decision making in multiformats/multiaddr#150 (comment).
Considered non-breaking change as `/webrtc` will still be parsed to `Protocol::WebRTC` and no known production deployment of `libp2p-webrtc` known. See multiformats/multiaddr#150 (comment) for context.
…t` (#84) Considered non-breaking change as `/webrtc` will still be parsed to `Protocol::WebRTC` and no known production deployment of `libp2p-webrtc` known. See multiformats/multiaddr#150 (comment) for context.
…t` (multiformats#84) Considered non-breaking change as `/webrtc` will still be parsed to `Protocol::WebRTC` and no known production deployment of `libp2p-webrtc` known. See multiformats/multiaddr#150 (comment) for context.
…t` (#84) (#87) Considered non-breaking change as `/webrtc` will still be parsed to `Protocol::WebRTC` and no known production deployment of `libp2p-webrtc` known. See multiformats/multiaddr#150 (comment) for context.
This commit disables the WebRTC tests for rust-libp2p v0.50.0 and v0.51.0 and chromium-js v0.41.0 and v0.42.0. Neither of these support the new `/webrtc-direct` (see multiformats/multiaddr#150 for context on rename). The missing support is blocking libp2p/rust-libp2p#3688 namely to upgrade to using `/webrtc-direct`. (Note that this is only blocking CI. Users can already use `/webrtc-direct` with the latest released rust-libp2p.) We will backport `/webrtc-direct` support to the rust-libp2p `v0.51.0` interop binary and then re-enable it here. I assume we can do the same for js-libp2p v0.42.0. Instead of disabling the versions with missing support for `/webrtc-direct` we could as well patch our interoperability test logic. For the sake of simplicity, I am proposing simply disabling the versions for now.
This commit disables the WebRTC tests for rust-libp2p v0.50.0 and v0.51.0 and chromium-js v0.41.0 and v0.42.0. Neither of these support the new `/webrtc-direct` (see multiformats/multiaddr#150 for context on rename). The missing support is blocking libp2p/rust-libp2p#3688 namely to upgrade to using `/webrtc-direct`. (Note that this is only blocking CI. Users can already use `/webrtc-direct` with the latest released rust-libp2p.) We will backport `/webrtc-direct` support to the rust-libp2p `v0.51.0` interop binary and then re-enable it here. Instead of disabling the versions with missing support for `/webrtc-direct` we could as well patch our interoperability test logic. For the sake of simplicity, I am proposing simply disabling the versions for now.
See multiformats/multiaddr#150 (comment) for context on the rename.
See multiformats/multiaddr#150 (comment) for context on discussion. - Renames the `webrtc` transport identifier to `webrtc-direct`. - Re-enables `webrtc-direct` support for rust-libp2p v0.51. Previously disabled in libp2p#160. See libp2p/rust-libp2p#3781 for corresponding change on the rust-libp2p side. - Leaves JS v0.41 and v0.42 untouched. To be done in a follow-up alongside JS dependency updates.
See multiformats/multiaddr#150 (comment) for context on the rename. Pull-Request: #3781.
This commit disables the WebRTC tests for rust-libp2p v0.50.0 and v0.51.0 and chromium-js v0.41.0 and v0.42.0. Neither of these support the new `/webrtc-direct` (see multiformats/multiaddr#150 for context on rename). The missing support is blocking libp2p/rust-libp2p#3688 namely to upgrade to using `/webrtc-direct`. (Note that this is only blocking CI. Users can already use `/webrtc-direct` with the latest released rust-libp2p.) We will backport `/webrtc-direct` support to the rust-libp2p `v0.51.0` interop binary and then re-enable it here. Instead of disabling the versions with missing support for `/webrtc-direct` we could as well patch our interoperability test logic. For the sake of simplicity, I am proposing simply disabling the versions for now.
See multiformats/multiaddr#150 (comment) for context on discussion. - Renames the `webrtc` transport identifier to `webrtc-direct`. - Re-enables `webrtc-direct` support for rust-libp2p v0.51. Previously disabled in #160. See libp2p/rust-libp2p#3781 for corresponding change on the rust-libp2p side. - Leaves JS v0.41 and v0.42 untouched. To be done in a follow-up alongside JS dependency updates.
See decision making in multiformats/multiaddr#150 (comment).
See decision making in multiformats/multiaddr#150 (comment).
Summary
Adds support for the webrtc-w3c protocol described by the following spec: libp2p/specs#497
multiformats/multicodec#316