-
Notifications
You must be signed in to change notification settings - Fork 709
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
Replace Multiaddr
& related types with substrate-specific types
#4198
Conversation
Multiaddr
& Multihash
typeslibp2p-identity
types with substrate-specific types
The CI pipeline was cancelled due to failure one of the required jobs. |
libp2p-identity
types with substrate-specific typeslibp2p-identity
related types with substrate-specific types
libp2p-identity
related types with substrate-specific typesMultiaddr
& related types with substrate-specific types
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't look that closely, but these are mainly wrappers any way?
|
||
/// Convert public key to `PeerId`. | ||
pub fn to_peer_id(&self) -> PeerId { | ||
litep2p::PeerId::from(litep2p::crypto::PublicKey::Ed25519(self.clone().into())).into() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we not directly construct the PeerId?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would require hashing PublicKey
type serialized as protobuf, and I tried to avoid including the protobuf schema into the already bloated wrapper sources.
} | ||
} | ||
|
||
// TODO: uncomment this after upgrading `multihash` crate to v0.19.1. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there anything blocking us from updating to multihash 0.19.1? Maybe we could do that in a followup and raise an issue to not forget about it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it should be done at the same time as upgrading libp2p
. I will mention it in the libp2p PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Nice job here!
There are a few CI steps that are failing, but other than that we are good to go 👍
…ritytech#4198) This PR introduces custom types / substrate wrappers for `Multiaddr`, `multiaddr::Protocol`, `Multihash`, `ed25519::*` and supplementary types like errors and iterators. This is needed to unblock `libp2p` upgrade PR paritytech#1631 after paritytech#2944 was merged. `libp2p` and `litep2p` currently depend on different versions of `multiaddr` crate, and introduction of this "common ground" types is needed to support independent version upgrades of `multiaddr` and dependent crates in `libp2p` & `litep2p`. While being just convenient to not tie versions of `libp2p` & `litep2p` dependencies together, it's currently not even possible to keep `libp2p` & `litep2p` dependencies updated to the same versions as `multiaddr` in `libp2p` depends on `libp2p-identity` that we can't include as a dependency of `litep2p`, which has it's own `PeerId` type. In the future, to keep things updated on `litep2p` side, we will likely need to fork `multiaddr` and make it use `litep2p` `PeerId` as a payload of `/p2p/...` protocol. With these changes, common code in substrate uses these custom types, and `litep2p` & `libp2p` backends use corresponding libraries types.
/// or litep2p's `Multiaddr` when passed to the corresponding network backend. | ||
|
||
#[derive(PartialEq, Eq, PartialOrd, Ord, Clone, Hash)] | ||
pub struct Multiaddr { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This version is missing impl From<IpAddr> for Multiaddr
from libp2p's implementation, can you add it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in #4855.
Add `From` implementation used by downstream project. Ref. #4198 (comment) CC @nazar-pc
…ritytech#4198) This PR introduces custom types / substrate wrappers for `Multiaddr`, `multiaddr::Protocol`, `Multihash`, `ed25519::*` and supplementary types like errors and iterators. This is needed to unblock `libp2p` upgrade PR paritytech#1631 after paritytech#2944 was merged. `libp2p` and `litep2p` currently depend on different versions of `multiaddr` crate, and introduction of this "common ground" types is needed to support independent version upgrades of `multiaddr` and dependent crates in `libp2p` & `litep2p`. While being just convenient to not tie versions of `libp2p` & `litep2p` dependencies together, it's currently not even possible to keep `libp2p` & `litep2p` dependencies updated to the same versions as `multiaddr` in `libp2p` depends on `libp2p-identity` that we can't include as a dependency of `litep2p`, which has it's own `PeerId` type. In the future, to keep things updated on `litep2p` side, we will likely need to fork `multiaddr` and make it use `litep2p` `PeerId` as a payload of `/p2p/...` protocol. With these changes, common code in substrate uses these custom types, and `litep2p` & `libp2p` backends use corresponding libraries types.
…4855) Add `From` implementation used by downstream project. Ref. paritytech#4198 (comment) CC @nazar-pc
This PR introduces custom types / substrate wrappers for
Multiaddr
,multiaddr::Protocol
,Multihash
,ed25519::*
and supplementary types like errors and iterators.This is needed to unblock
libp2p
upgrade PR #1631 after #2944 was merged.libp2p
andlitep2p
currently depend on different versions ofmultiaddr
crate, and introduction of this "common ground" types is needed to support independent version upgrades ofmultiaddr
and dependent crates inlibp2p
&litep2p
.While being just convenient to not tie versions of
libp2p
&litep2p
dependencies together, it's currently not even possible to keeplibp2p
&litep2p
dependencies updated to the same versions asmultiaddr
inlibp2p
depends onlibp2p-identity
that we can't include as a dependency oflitep2p
, which has it's ownPeerId
type. In the future, to keep things updated onlitep2p
side, we will likely need to forkmultiaddr
and make it uselitep2p
PeerId
as a payload of/p2p/...
protocol.With these changes, common code in substrate uses these custom types, and
litep2p
&libp2p
backends use corresponding libraries types.