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.

Sign up for GitHub

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

Jump to bottom

core: remove upgrade::transfer module #4011

Closed
4 tasks done
Tracked by #3271
thomaseizinger opened this issue May 30, 2023 · 4 comments · Fixed by #4788
Closed
4 tasks done
Tracked by #3271

core: remove upgrade::transfer module #4011

thomaseizinger opened this issue May 30, 2023 · 4 comments · Fixed by #4788
Labels
getting-started Issues that can be tackled if you don't know the internals of libp2p very well help wanted priority:nicetohave

Comments

@thomaseizinger
Copy link
Contributor

thomaseizinger commented May 30, 2023 •
edited

Description

This module contains functions for writing and reading varint-prefixed messages. These kind of utility functions do not belong in libp2p-core and unnecessarily increase the public API. All of its usages are actually as part of a libp2p_request_response::Codec.

Tasks
Beta Give feedback
  1. autonat: migrate to quick-protobuf-codec #4489
    difficulty:moderate help wanted
  4. read_varint should not return 0 upon EOF #4565

This issue should be tackled in 4 separate PRs, one for each of the above crates and a fourth one to deprecate all functions in the upgrade::transfer module. We can remove them in the next breaking release then.

Motivation

Shrink public API of libp2p-core and thus avoid unnecessary version bumps in other crates.

Are you planning to do it yourself in a pull request?

No.
@thomaseizinger thomaseizinger added priority:nicetohave help wanted getting-started Issues that can be tackled if you don't know the internals of libp2p very well labels May 30, 2023
@thomaseizinger thomaseizinger mentioned this issue May 30, 2023
Open
@tcoratger
Copy link
Contributor

@thomaseizinger Concerning file-sharing example, if I understand well, up to now, there is an implementation of the trait request_response::Codec for the structure FileExchangeCodec.

The objective here is simply to copy paste the Codec implementation details that are applied to the structure Codec in cbor.rs for example? As codec in cbor.rs is a private module, we cannot access directly from the exterior.

@thomaseizinger
Copy link
Contributor Author

thomaseizinger commented May 31, 2023 •
edited

@thomaseizinger Concerning file-sharing example, if I understand well, up to now, there is an implementation of the trait request_response::Codec for the structure FileExchangeCodec.

The objective here is simply to copy paste the Codec implementation details that are applied to the structure Codec in cbor.rs for example? As codec in cbor.rs is a private module, we cannot access directly from the exterior.

The cbor module is designed to be consumed via its Behaviour type alias. Check the documentation in there.

The objective is to use the cbor::Behaviour in the file sharing example instead of writing our own codec.

@tcoratger tcoratger mentioned this issue Jun 6, 2023
Merged
4 tasks
mergify bot pushed a commit that referenced this issue Jun 6, 2023
@tcoratger
feat(examples): use cbor codec in file-sharing example
90d6a77 
Remove the use of the core `upgrade::transfer` module in `file-sharing` example in favor of `cbor` codec.

Related #4011.

Pull-Request: #4036.
@tcoratger tcoratger mentioned this issue Jun 7, 2023
Merged
4 tasks
mergify bot pushed a commit that referenced this issue Jun 7, 2023
@tcoratger
feat(request-response): use cbor codec in ping example
d2dab94 
Remove the use of the core `upgrade::transfer` module in `ping` example (`request-response` protocol) in favor of `cbor` codec.

Related #4011.

Pull-Request: #4046.
@tcoratger
Copy link
Contributor

Concerning AutoNAT modification, if we choose to remove the read_length_prefixed and write_length_prefixed functions to use quick_protobuf_codec::Codec in combination with asynchronous_codec::Framed, this will mean to me that we will have to modify the Codec trait according to me (maybe I'm wrong).

For me, if we want to use asynchronous_codec::Framed::new via the io, then the io type must implement both AsyncRead and AsyncWrite and no longer one or the other, as it is currently depending on whether it is a read or a write.

@thomaseizinger
Copy link
Contributor Author

See #4051 for an example of how we use Framed with the codec trait

@thomaseizinger thomaseizinger mentioned this issue Sep 28, 2023
Closed
@thomaseizinger thomaseizinger mentioned this issue Nov 2, 2023
Merged
4 tasks
@mergify mergify bot closed this as completed in #4788 Nov 2, 2023
mergify bot pushed a commit that referenced this issue Nov 2, 2023
@thomaseizinger
feat(core): remove upgrade::transfer module
fab9205 
As described in #4011, these utility functions don't belong in `libp2p-core`. Users can use `quick-protobuf-codec` if they need to write varint-prefixed protobuf messages. For writing varint-prefixed bytes, the `unsigned-varint` crate offers a various codec implementations.

`libp2p-core` is the base dependency of all other crates. Thus, we should only expose items there that are actually needed by all other crates. For implementation details like how bytes are written, downstream crates (including users) should reach for other crates.

Depends-On: #4787.
Resolves: #4011.

Pull-Request: #4788.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
getting-started Issues that can be tackled if you don't know the internals of libp2p very well help wanted priority:nicetohave
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat(core): remove upgrade::transfer module libp2p/rust-libp2p
2 participants
@thomaseizinger @tcoratger