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 QUIC transport to the chat example #3635

Merged
merged 8 commits into from
Mar 24, 2023

Conversation

yellowred
Copy link
Contributor

@yellowred yellowred commented Mar 20, 2023

Description

See: #3501 (comment)

Notes & open questions

Removed development_transport and used a custom transport, since we don't want Quic in dev transport.
However, I anticipate others will copy this code to their new projects when they need Quic+Tcp, so there is a question of the purpose of the development_transport function.

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

@yellowred yellowred changed the title Add Quic transport to the Chat example feat: Add Quic transport to the Chat example Mar 20, 2023
@yellowred yellowred marked this pull request as ready for review March 20, 2023 00:48
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Thank you! A few questions.

examples/chat-example/src/main.rs Outdated Show resolved Hide resolved
examples/chat-example/src/main.rs Show resolved Hide resolved
examples/chat-example/src/main.rs Outdated Show resolved Hide resolved
examples/chat-example/src/main.rs Outdated Show resolved Hide resolved
examples/chat-example/src/main.rs Show resolved Hide resolved
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

One request, other LGTM

examples/chat-example/Cargo.toml Outdated Show resolved Hide resolved
examples/chat-example/src/main.rs Show resolved Hide resolved
@thomaseizinger
Copy link
Contributor

Removed development_transport and used a custom transport, since we don't want Quic in dev transport.\nHowever, I anticipate others will copy this code to their new projects when they need Quic+Tcp, so there is a question of the purpose of the development_transport function.

It is a legitimate question. Personally, I think users should compose their transport for a production application themselves anyway.

But, it is useful to get started quickly.

Maybe we can incorporate this into SwarmBuilder somehow?

An idea sketch would be:

SwarmBuilder::new(local_peer_id)
   .tokio_executor()
   .default_transport()
   .behaviour(my_behaviour)
   .build()

The builder would be type-state guided such that you must call either default_transport or custom_transport(my_transport).

@yellowred
Copy link
Contributor Author

But, it is useful to get started quickly.

Maybe we can incorporate this into SwarmBuilder somehow?

I like this idea. Everyone will be able to start quickly with a default and customize later on when they see a need for that.

@thomaseizinger thomaseizinger changed the title feat: Add Quic transport to the Chat example feat: add QUIC transport to the chat example Mar 22, 2023
@thomaseizinger
Copy link
Contributor

The clippy build is failing due to a recent PR that merged into master (#3580), can you address that please?

@mxinden
Copy link
Member

mxinden commented Mar 22, 2023

Idea above for a default_transport method on Swarm or an extension trait to Swarm sounds good to me. I don't have an opinion on the exact implementation of the mechanism. I do believe providing some default is valuable. Anecdotally just yesterday in the community call someone asked for the best defaults on a transport to get started.

@thomaseizinger
Copy link
Contributor

Idea above for a default_transport method on Swarm or an extension trait to Swarm sounds good to me. I don't have an opinion on the exact implementation of the mechanism. I do believe providing some default is valuable. Anecdotally just yesterday in the community call someone asked for the best defaults on a transport to get started.

As you are pointing out, it would have to be an extension trait because libp2p-swarm does not pull in any specific transports. I'll open an issue.

@yellowred
Copy link
Contributor Author

@thomaseizinger Added Quic as a separate crate.

@mergify mergify bot merged commit 7ffa63b into libp2p:master Mar 24, 2023
@yellowred yellowred deleted the quic_example branch March 27, 2023 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants