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: replace ProtocolName with AsRef<str> #3746

Merged
merged 34 commits into from
May 4, 2023
Merged

Conversation

thomaseizinger
Copy link
Contributor

@thomaseizinger thomaseizinger commented Apr 6, 2023

Description

Previously, a protocol could be any sequence of bytes as long as it started with /. Now, we directly parse a protocol as String which enforces it to be valid UTF8.

To notify users of this change, we delete the ProtocolName trait. The new requirement is that users need to provide a type that implements AsRef<str>.

We also add a StreamProtocol newtype in libp2p-swarm which provides an easy way for users to ensure their protocol strings are compliant. The newtype enforces that protocol strings start with /. StreamProtocol also implements AsRef<str>, meaning users can directly use it in their upgrades.

multistream-select by itself only changes marginally with this patch. The only thing we enforce in the type-system is that protocols must implement AsRef<str>.

Resolves: #2831.

Notes & open questions

This is an alternative to #3745.

I am hoping that with this change, users will already accustom to use the StreamProtocol newtype. Regarding #2863, I think it makes the most sense to remove all these traits in one go and directly have the handler return a list of StreamProtocols.

Dependencies

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

@thomaseizinger

This comment was marked as outdated.

@thomaseizinger

This comment was marked as outdated.

@thomaseizinger thomaseizinger changed the title feat: enforce ProtocolName to return &str feat: replace ProtocolName with AsRef<str> Apr 26, 2023
@thomaseizinger thomaseizinger marked this pull request as ready for review April 26, 2023 17:39
@mergify

This comment was marked as resolved.

mergify bot pushed a commit that referenced this pull request Apr 27, 2023
This type can be replaced with std-lib components.

Related: #3746.

Pull-Request: #3842.
@mergify

This comment was marked as resolved.

@mergify

This comment was marked as resolved.

@mergify

This comment was marked as resolved.

@mergify

This comment was marked as resolved.

@mergify

This comment was marked as resolved.

@mergify

This comment was marked as resolved.

@mxinden mxinden added the send-it label May 4, 2023
@mergify mergify bot merged commit c93f753 into master May 4, 2023
@mergify mergify bot deleted the feat/string-protocols-3 branch May 4, 2023 04:47
@mxinden
Copy link
Member

mxinden commented May 11, 2023

@thomaseizinger updating https://github.com/mxinden/kademlia-exporter/ right now. Was it a deliberate choice to not implement TryFrom<String> for StreamProtocol? If not, I would create a pull request.

@thomaseizinger
Copy link
Contributor Author

@thomaseizinger updating https://github.com/mxinden/kademlia-exporter/ right now. Was it a deliberate choice to not implement TryFrom<String> for StreamProtocol? If not, I would create a pull request.

The need to abstract over the creation of StreamProtocol didn't arise so I never thought of it.

What is your usecase? Why can you not use the existing, much more descriptive constructor?

@mxinden
Copy link
Member

mxinden commented May 11, 2023

A user can provide a stream protocol name via a configuration file. I am parsing the stream protocol name as a string and would like to call protocol_name.try_into.expect("something").

What is your usecase? Why can you not use the existing, much more descriptive constructor?

Works as well. Just a bit more verbose.

@thomaseizinger
Copy link
Contributor Author

I am parsing the stream protocol name as a string

Perhaps FromStr is what we need?

What is your usecase? Why can you not use the existing, much more descriptive constructor?

Works as well. Just a bit more verbose.

A matter of taste for sure, feel free to send a PR! I tend to avoid calling conversion functions explicitly because they are mostly noise.

umgefahren pushed a commit to umgefahren/rust-libp2p that referenced this pull request Mar 8, 2024
Previously, a protocol could be any sequence of bytes as long as it started with `/`. Now, we directly parse a protocol as `String` which enforces it to be valid UTF8.

To notify users of this change, we delete the `ProtocolName` trait. The new requirement is that users need to provide a type that implements `AsRef<str>`.

We also add a `StreamProtocol` newtype in `libp2p-swarm` which provides an easy way for users to ensure their protocol strings are compliant. The newtype enforces that protocol strings start with `/`. `StreamProtocol` also implements `AsRef<str>`, meaning users can directly use it in their upgrades.

`multistream-select` by itself only changes marginally with this patch. The only thing we enforce in the type-system is that protocols must implement `AsRef<str>`.

Resolves: libp2p#2831.

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

misc/multistream-select: Treat protocol as String and not [u8]
2 participants