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

Add support for tls protocol #48

Merged
merged 10 commits into from
Jul 14, 2021
Merged

Conversation

whereistejas
Copy link
Contributor

@whereistejas whereistejas commented Jul 4, 2021

Hi @mxinden,
Creating this PR to close the issue #47.
This PR refers the Go code from PR 153.

i have a couple of questions regarding the tests, it seems like we are missing a few tests:

  1. construct_success
  2. TestTextMarshaler

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Thanks @whereistejas for the help!

Could you extend the <Protocol as Arbitrary> implementation with the new Protocol Tls?

impl Arbitrary for Proto {
fn arbitrary<G: Gen>(g: &mut G) -> Self {
use Protocol::*;
match u8::arbitrary(g) % 25 { // TODO: Add Protocol::Quic

construct_success

Feel free to add additional test cases.

TestTextMarshaler

Would this not be covered by to_from_str_identityt?

src/protocol.rs Outdated
Comment on lines 17 to 18
// All these values are obtained by converting hexadecimal protocol codes to u32.
// The protocol codes are present in multiformats/multicodec repository.
Copy link
Member

Choose a reason for hiding this comment

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

👍 for documenting this.

I am not exactly sure what the source of truth is, though I think we should link to multiformats/multiaddr instead.

Suggested change
// All these values are obtained by converting hexadecimal protocol codes to u32.
// The protocol codes are present in multiformats/multicodec repository.
// Protocols as well as their corresponding codes are defined in
// https://github.com/multiformats/multiaddr/blob/master/protocols.csv .

@whereistejas
Copy link
Contributor Author

Hi @mxinden , I have added the extra tests and added the Tls proto, too. Please, let me know if any other changes are pending/required. :)

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

As far as I can tell you would need to adjust the Arbitrary implementation of Proto to potentially generate the value 25. The modulo operation is a bit error prone, though I don't know of a better solution.

-        match u8::arbitrary(g) % 25  { // TODO: Add Protocol::Quic
+        match u8::arbitrary(g) % 26  { // TODO: Add Protocol::Quic

@whereistejas
Copy link
Contributor Author

@mxinden I have made the change you suggested.

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Wow, thank you for the quick turn-around time @whereistejas!

Would you mind including a changelog entry? Other than that, this looks good to me. Thanks.

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 65e438e..42a6fb7 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,3 +1,9 @@
+# 0.14.0 [unreleased]
+
+- Add support for `tls` protocol. See [PR 48].
+
+[PR 48]: https://github.com/multiformats/rust-multiaddr/pull/48
+
 # 0.13.0 [2021-07-08]
 
 - Update to multihash v0.14.0 (see [PR 44]).
diff --git a/Cargo.toml b/Cargo.toml
index 33cce4c..8162f62 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -7,7 +7,7 @@ keywords = ["multiaddr", "ipfs"]
 license = "MIT"
 name = "multiaddr"
 readme = "README.md"
-version = "0.13.0"
+version = "0.14.0"

@whereistejas
Copy link
Contributor Author

Hi @mxinden, done!

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.

2 participants