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

feat: Add WebTransport protocol based on BiagioFesta/wtransport #5701

Open
wants to merge 49 commits into
base: master
Choose a base branch
from

Conversation

dgarus
Copy link
Contributor

@dgarus dgarus commented Nov 28, 2024

@dgarus dgarus changed the title Wtransport usage feat: Add WebTransport protocol based on BiagioFesta/wtransport Dec 25, 2024
@dgarus dgarus marked this pull request as ready for review January 22, 2025 09:20
transports/tls/CHANGELOG.md Outdated Show resolved Hide resolved
transports/tls/src/certificate.rs Outdated Show resolved Hide resolved
transports/webtransport/src/certificate.rs Outdated Show resolved Hide resolved
transports/webtransport/src/transport.rs Show resolved Hide resolved
@dgarus
Copy link
Contributor Author

dgarus commented Jan 22, 2025

@jxs @thomaseizinger @dariusc93
Hi! Please review.

Thanks!

@dariusc93 dariusc93 requested review from jxs and elenaf9 January 22, 2025 12:51
@elenaf9
Copy link
Contributor

elenaf9 commented Jan 26, 2025

Hi @dgarus, thank you for your PR, it's much appreciated!

I'll try to give this a review as soon as possible, but it might take some days until I get to it.

@elenaf9 elenaf9 linked an issue Feb 16, 2025 that may be closed by this pull request
Copy link
Contributor

@elenaf9 elenaf9 left a comment

Choose a reason for hiding this comment

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

Thank your for this great effort and sorry for the delay. I started reviewing it, but didn't look into the certification logic yet.

This PR differs in some aspects from the previous webtransport PR #5564. In particular it:

  • now builds on the wtransport crate, instead of h3-webtransport
  • doesn't extend libp2p_quic anymore but instead is a separate crate

Could you briefly elaborate what motivated these changes? In particular for latter, because now the webtransport::GenTransport duplicates a lot of quic::GenTransport.

transports/tls/src/certificate.rs Outdated Show resolved Hide resolved
transports/webtransport/src/config.rs Outdated Show resolved Hide resolved
transports/webtransport/src/connection/stream.rs Outdated Show resolved Hide resolved
transports/webtransport/src/transport.rs Outdated Show resolved Hide resolved
transports/webtransport/src/transport.rs Outdated Show resolved Hide resolved
transports/webtransport/src/transport.rs Outdated Show resolved Hide resolved
transports/webtransport/src/transport.rs Show resolved Hide resolved
transports/webtransport/src/transport.rs Outdated Show resolved Hide resolved
@dgarus
Copy link
Contributor Author

dgarus commented Feb 17, 2025

Thank your for this great effort and sorry for the delay. I started reviewing it, but didn't look into the certification logic yet.

This PR differs in some aspects from the previous webtransport PR #5564. In particular it:

  • now builds on the wtransport crate, instead of h3-webtransport
  • doesn't extend libp2p_quic anymore but instead is a separate crate

Could you briefly elaborate what motivated these changes? In particular for latter, because now the webtransport::GenTransport duplicates a lot of quic::GenTransport.

Hi!
We discussed these changes with @jxs on the Slack chat:
image

And as the result, it is here 🙂

With webtrasport and quic together in the single transport implementation, it's difficult (at least for me) to keep the whole picture in mind. So, the main motivation is to make things simpler and more transparent.
The second one is wtransport has a client part, which is used in a smoke test.

Additionally, if a user wants to use pure webtransport, but it is a part of quic transport, it can perform some unnecessary extra things related to a dialer feature.

@dgarus
Copy link
Contributor Author

dgarus commented Feb 17, 2025

@elenaf9 thank you for review!
@thomaseizinger and I discussed certificates here, this link could be useful for you.

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.

transport/webtransport: Add WebTransport protocol
3 participants