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(request-response): add modules for json and cbor messages #3952

Merged
merged 45 commits into from
May 24, 2023

Conversation

dgarus
Copy link
Contributor

@dgarus dgarus commented May 16, 2023

Description

This patch adds two modules to libp2p::request_response:

  • cbor
  • json

Both define a Behaviour type-alias that comes with a Codec implementation which uses the respective serde crate to serialize and deserialize the messages.

Fixes #3905.

Notes & open questions

  • added a common implementation of Behavior struct to use with the SerdeCodec<Request, Response>
  • added modules to create Behavior with the json or cbor codec
  • added integration tests for serde_codecs

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

@dgarus dgarus changed the title 3905 Serde support feat(request-response): serde (json and/or cbor) support May 16, 2023
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.

Thanks for tackling this! I've left some comments with ideas :)

protocols/request-response/Cargo.toml Outdated Show resolved Hide resolved
protocols/request-response/src/serde_codec.rs Outdated Show resolved Hide resolved
protocols/request-response/src/serde_codec.rs Outdated Show resolved Hide resolved
protocols/request-response/src/serde_codec.rs Outdated Show resolved Hide resolved
protocols/request-response/src/cbor.rs Outdated Show resolved Hide resolved
protocols/request-response/src/serde_codec.rs Outdated Show resolved Hide resolved
protocols/request-response/src/cbor.rs Outdated Show resolved Hide resolved
protocols/request-response/src/cbor.rs Outdated Show resolved Hide resolved
protocols/request-response/src/cbor.rs Outdated Show resolved Hide resolved
protocols/request-response/src/cbor.rs Outdated Show resolved Hide resolved
protocols/request-response/tests/cbor.rs Outdated 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.

Good progress! :)

protocols/request-response/src/cbor.rs Outdated Show resolved Hide resolved
protocols/request-response/src/cbor.rs Outdated Show resolved Hide resolved
@dgarus
Copy link
Contributor Author

dgarus commented May 19, 2023

Good progress! :)

Thanks!

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.

Thanks! I've left some more comments but the direction here looks good! :)

protocols/request-response/src/cbor.rs Outdated Show resolved Hide resolved
protocols/request-response/src/cbor.rs Outdated Show resolved Hide resolved
protocols/request-response/src/lib.rs Outdated Show resolved Hide resolved
protocols/request-response/CHANGELOG.md Outdated Show resolved Hide resolved
protocols/request-response/CHANGELOG.md Outdated Show resolved Hide resolved
protocols/request-response/src/cbor.rs Outdated Show resolved Hide resolved
protocols/request-response/src/cbor.rs Outdated Show resolved Hide resolved
protocols/request-response/src/cbor.rs Outdated Show resolved Hide resolved
protocols/request-response/src/cbor.rs Outdated Show resolved Hide resolved
protocols/request-response/src/cbor.rs Outdated Show resolved Hide resolved
dgarus and others added 8 commits May 20, 2023 14:49
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
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.

Great! :)

Almost there. I'd like to see whether we can remove the length-prefixing.

protocols/request-response/Cargo.toml Outdated Show resolved Hide resolved
protocols/request-response/Cargo.toml Outdated Show resolved Hide resolved
protocols/request-response/src/cbor.rs Outdated Show resolved Hide resolved
protocols/request-response/src/cbor.rs Outdated Show resolved Hide resolved
@dgarus
Copy link
Contributor Author

dgarus commented May 23, 2023

@thomaseizinger

I knew should be a better solution to wrap behavior :)

core/src/upgrade/transfer.rs Outdated Show resolved Hide resolved
protocols/request-response/src/cbor.rs Outdated 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.

Awesome! Some final requests and then we can merge this!

protocols/request-response/src/codec.rs Outdated Show resolved Hide resolved
protocols/request-response/src/codec.rs Outdated Show resolved Hide resolved
protocols/request-response/src/json.rs Outdated Show resolved Hide resolved
protocols/request-response/src/json.rs Outdated Show resolved Hide resolved
protocols/request-response/src/json.rs Outdated Show resolved Hide resolved
@thomaseizinger thomaseizinger changed the title feat(request-response): serde (json and/or cbor) support feat(request-response): add convenience modules for json and cbor messages May 23, 2023
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.

Awesome! Thanks for bearing with us on the iterations. I think this is a really nice addition that will make using rust-libp2p easier :)

@thomaseizinger thomaseizinger changed the title feat(request-response): add convenience modules for json and cbor messages feat(request-response): add modules for json and cbor messages May 24, 2023
@mergify mergify bot merged commit a5cd0d0 into libp2p:master May 24, 2023
@dgarus
Copy link
Contributor Author

dgarus commented May 24, 2023

Awesome! Thanks for bearing with us on the iterations. I think this is a really nice addition that will make using rust-libp2p easier :)

Thanks for the review, it is the best way to deep dive into p2p and Rust.

@dgarus dgarus deleted the 3905-serde_support branch May 24, 2023 11:31
Comment on lines +26 to +41
/// # use libp2p_request_response::{cbor, ProtocolSupport, self as request_response};
/// # use libp2p_swarm::{StreamProtocol, SwarmBuilder};
/// #[derive(Debug, serde::Serialize, serde::Deserialize)]
/// struct GreetRequest {
/// name: String,
/// }
///
/// #[derive(Debug, serde::Serialize, serde::Deserialize)]
/// struct GreetResponse {
/// message: String,
/// }
///
/// let behaviour = cbor::Behaviour::<GreetRequest, GreetResponse>::new(
/// [(StreamProtocol::new("/my-cbor-protocol"), ProtocolSupport::Full)],
/// request_response::Config::default()
/// );
Copy link
Member

Choose a reason for hiding this comment

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

Wonderful how simple this turned out to be. Thanks @dgarus and @thomaseizinger!

umgefahren pushed a commit to umgefahren/rust-libp2p that referenced this pull request Mar 8, 2024
This patch adds two modules to `libp2p::request_response`:

- `cbor`
- `json`

Both define a `Behaviour` type-alias that comes with a `Codec` implementation which uses the respective `serde` crate to serialize and deserialize the messages.

Fixes libp2p#3905.

Pull-Request: libp2p#3952.
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.

feat(request-response): serde (json and/or cbor) support
3 participants