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: leverage type-safe /p2p in multiaddr #4037

Merged
merged 34 commits into from
Jun 8, 2023
Merged

Conversation

thomaseizinger
Copy link
Contributor

@thomaseizinger thomaseizinger commented Jun 6, 2023

Description

This updates multiaddr to version 0.19.

Depends-On: #3656.
Depends-On: multiformats/rust-multiaddr#83.
Resolves: #4039.

Notes & open questions

This currently does not compile because PeerId exported from multiaddr is not the same as the one we use across the workspace here. But, the resolution of all other errors shows that this will work.

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 thomaseizinger added this to the v0.52.0 milestone Jun 6, 2023
Base automatically changed from libp2p-identity-no-multiaddr to master June 6, 2023 17:27
@mergify

This comment was marked as resolved.

@thomaseizinger thomaseizinger marked this pull request as ready for review June 6, 2023 17:57
Comment on lines +106 to +113
[patch.crates-io]

# Patch away `libp2p-idnentity` in our dependency tree with the workspace version.
# `libp2p-identity` is a leaf dependency and used within `rust-multiaddr` which is **not** part of the workspace.
# As a result, we cannot just reference the workspace version in our crates because the types would mismatch with what
# we import via `rust-multiaddr`.
# This is expected to stay here until we move `libp2p-identity` to a separate repository which makes the dependency relationship more obvious.
libp2p-identity = { path = "identity" }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mxinden I did not expect that we need this but it actually makes sense. We can't just depend on libp2p-idenity via path in here because it will mismatch with what is coming from multiaddr.

Copy link
Member

Choose a reason for hiding this comment

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

This makes sense. I think cargo publish will ignore all patch sections, thus this does not affect our published crates.

@mergify

This comment was marked as resolved.

core/Cargo.toml Outdated Show resolved Hide resolved
Comment on lines -3404 to -3416
[[package]]
name = "multihash-derive"
version = "0.8.1"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "1d6d4752e6230d8ef7adf7bd5d8c4b1f6561c1014c5ba9a37445ccefe18aa1db"
dependencies = [
"proc-macro-crate",
"proc-macro-error",
"proc-macro2",
"quote",
"syn 1.0.109",
"synstructure",
]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🎉

Comment on lines -3917 to -3949
[[package]]
name = "proc-macro-crate"
version = "1.1.3"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "e17d47ce914bf4de440332250b0edd23ce48c005f59fab39d3335866b114f11a"
dependencies = [
"thiserror",
"toml",
]

[[package]]
name = "proc-macro-error"
version = "1.0.4"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "da25490ff9892aab3fcf7c36f08cfb902dd3e71ca0f9f9517bea02a73a5ce38c"
dependencies = [
"proc-macro-error-attr",
"proc-macro2",
"quote",
"syn 1.0.109",
"version_check",
]

[[package]]
name = "proc-macro-error-attr"
version = "1.0.4"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "a1be40180e52ecc98ad80b184934baf3d0d29f979574e439af5a55274b35f869"
dependencies = [
"proc-macro2",
"quote",
"version_check",
]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🎉

@@ -11,4 +11,4 @@ async-trait = "0.1"
env_logger = "0.10"
futures = "0.3.28"
libp2p = { path = "../../libp2p", features = ["async-std", "dns", "kad", "mdns", "noise", "macros", "tcp", "websocket", "yamux"] }
multiaddr = { version = "0.17.1" }
multiaddr = { workspace = true }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, this one we should probably remove in favor of accessing it from libp2p.

@@ -13,5 +13,5 @@ either = "1.8"
env_logger = "0.10"
futures = "0.3.28"
libp2p = { path = "../../libp2p", features = ["async-std", "dns", "kad", "noise", "macros", "request-response", "tcp", "websocket", "yamux"] }
multiaddr = { version = "0.17.1" }
multiaddr = { workspace = true }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

And here too!

multiaddr = { workspace = true }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

And here.

multiaddr = { workspace = true }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

And here.

Comment on lines -1522 to -1523
/// The provided peer identity is invalid.
InvalidPeerId(Multihash),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is probably worth a changelog entry.

Copy link
Member

Choose a reason for hiding this comment

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

👍 6872e90

@mergify

This comment was marked as resolved.

Comment on lines +106 to +113
[patch.crates-io]

# Patch away `libp2p-idnentity` in our dependency tree with the workspace version.
# `libp2p-identity` is a leaf dependency and used within `rust-multiaddr` which is **not** part of the workspace.
# As a result, we cannot just reference the workspace version in our crates because the types would mismatch with what
# we import via `rust-multiaddr`.
# This is expected to stay here until we move `libp2p-identity` to a separate repository which makes the dependency relationship more obvious.
libp2p-identity = { path = "identity" }
Copy link
Member

Choose a reason for hiding this comment

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

This makes sense. I think cargo publish will ignore all patch sections, thus this does not affect our published crates.

Comment on lines -1522 to -1523
/// The provided peer identity is invalid.
InvalidPeerId(Multihash),
Copy link
Member

Choose a reason for hiding this comment

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

👍 6872e90

@mxinden mxinden added the send-it label Jun 8, 2023
@mergify mergify bot merged commit ed14630 into master Jun 8, 2023
@mergify mergify bot deleted the feat/typesafe-multiaddr-p2p branch June 8, 2023 01:38
umgefahren pushed a commit to umgefahren/rust-libp2p that referenced this pull request Mar 8, 2024
This updates `multiaddr` to version `0.19`.

Depends-On: libp2p#3656.
Depends-On: multiformats/rust-multiaddr#83.
Resolves: libp2p#4039.

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

Re-enable version check of libp2p-identity
2 participants