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

webrtc/: Add libp2p WebRTC browser-to-server spec #412

Merged
merged 135 commits into from
Nov 9, 2022

Conversation

mxinden
Copy link
Member

@mxinden mxinden commented May 11, 2022

This pull requests summarizes the status quo of the ongoing WebRTC in libp2p work. In other words, it represents the summary of all past discussions like #220.

While describing both the browser-to-server and browser-to-browser use-case, priority is on the former. Once the browser-to-server use-case is sufficiently specified, I will remove the browser-to-browser use-case, merge this pull request, and start iteration two, namely to sufficiently specify the browser-to-browser use-case.

Remaining work items for browser-to-server:

webrtc/README.md Outdated Show resolved Hide resolved
webrtc/README.md Outdated Show resolved Hide resolved
webrtc/README.md Outdated Show resolved Hide resolved
webrtc/README.md Outdated Show resolved Hide resolved
webrtc/README.md Outdated Show resolved Hide resolved
webrtc/README.md Outdated Show resolved Hide resolved
webrtc/README.md Outdated Show resolved Hide resolved
webrtc/README.md Outdated Show resolved Hide resolved
webrtc/README.md Outdated Show resolved Hide resolved
webrtc/README.md Outdated Show resolved Hide resolved
webrtc/README.md Outdated Show resolved Hide resolved
webrtc/README.md Outdated Show resolved Hide resolved
webrtc/README.md Outdated Show resolved Hide resolved
webrtc/README.md Outdated Show resolved Hide resolved
melekes added a commit to melekes/rust-libp2p that referenced this pull request Oct 28, 2022
@mxinden mxinden changed the title webrtc/: Add libp2p WebRTC specification webrtc/: Add libp2p WebRTC browser-to-server spec Oct 29, 2022
@ckousik
Copy link
Contributor

ckousik commented Oct 31, 2022

Perhaps we should also say that a=max-message-size:16384 SDP attribute should be set to the maximum frame size (16kB). By default, it's 64kB (see https://datatracker.ietf.org/doc/rfc8841/), but it might be a good idea to align these two variables. Wdyt? The alternative is to not include attribute in SDP and allow messages up to 64kB.

That sounds good to me @melekes. @ckousik @John-LittleBearLabs any objections?

No objections from my end.

@mxinden
Copy link
Member Author

mxinden commented Nov 2, 2022

@marten-seemann thank you for your review. I addressed all your comments.

I followed both of your major suggestions.

1. Inferring hash algorithm from multiaddr for easier role out of new hash algorithm. [webrtc/: Add libp2p WebRTC browser-to-server spec #412 (comment)](https://github.com/libp2p/specs/pull/412#discussion_r999745879)

2. Prefixing ufrag to ease general future upgrade mechanism via ufrag. [webrtc/: Add libp2p WebRTC browser-to-server spec #412 (comment)](https://github.com/libp2p/specs/pull/412#discussion_r999768836)

Would you mind taking another look?

Friendly ping @marten-seemann. Does this look good to you?

@tomaka
Copy link
Member

tomaka commented Nov 7, 2022

I understand that you're being over cautious with clicking the merge button, but can we now consider that this version of the spec is "final", in the sense that the logic isn't going to change before merging, and the only things that could change are possible clarifications of the language?
I'd like to start releasing versions that "officially" support this spec.

@mxinden
Copy link
Member Author

mxinden commented Nov 9, 2022

Quite a long journey. 475 (+1) comments and 135 commits, 14 reviewers, ...

I am merging here as a Working Draft. We can promote it once we have a released implementation.

A couple of shoutouts:

  • @ckousik and the Little Bear Labs folks providing all your WebRTC knowledge.
  • @melekes for all your input from the Rust side. Thanks for bearing with us throughout the process.
  • @Menduist and @s1fr0 for your input on the Noise handshake, especially proposing the Noise prologue mechanism!
  • @thomaseizinger filling the many gaps, thinking deep into the stream semantics introduced here.
  • @marten-seemann for the high-quality comments across the specification.
  • ...

Up next: browser-to-browser use-case.

@mxinden mxinden merged commit e74a82b into libp2p:master Nov 9, 2022
@tomaka
Copy link
Member

tomaka commented Nov 9, 2022

Shoutout to me for having started the whole thing before it was aggressively taken over and not receiving a shoutout

mergify bot pushed a commit to paritytech/smoldot that referenced this pull request Nov 11, 2022
Let's wait a bit to see if there's an answer [to this
comment](libp2p/specs#412 (comment))
before merging this.
mergify bot pushed a commit to libp2p/rust-libp2p that referenced this pull request Nov 17, 2022
Hey 👋 This is a WebRTC transport implemented in accordance w/ the [spec](libp2p/specs#412). It's based on the [webrtc-rs](https://github.com/webrtc-rs/webrtc) library.

Resolves: #1066.
@GlenDC GlenDC mentioned this pull request Jan 17, 2023
17 tasks
@marcus-pousette
Copy link

I am working a project that heavily relies on the libp2p-js impl. I am currently working towards enabling a huge amount of dapps that benefit greatly from WebRTC. What I am missing (?) currently is a way for having js WebRTC listeners. Now it seems to only be supported/to-be-supported for the the Go and Rust impl. Correct me if I have understood the spec. incorrectly, but is this something that is planned for? Or not?

@MarcoPolo
Copy link
Contributor

Follow #497

mxinden added a commit to mxinden/specs that referenced this pull request May 31, 2023
With libp2p#412 and
libp2p#497 merged, this roadmap item can be moved
to "done".
mxinden added a commit that referenced this pull request Jun 1, 2023
With #412 and
#497 merged, this roadmap item can be moved
to "done".

Co-authored-by: Prithvi Shahi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.