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

core: Add Serialize and Deserialize to PeerId and MessageId #2405

Closed
wants to merge 3 commits into from

Conversation

laptou
Copy link
Contributor

@laptou laptou commented Dec 28, 2021

  • added a new feature serde to the libp2p crate
  • when this feature is enabled, Serialize and Deserialize are implemented for PeerId and gossipsub::MessageId
  • fixes an issue with GossipsubConfigBuilder::build() where it requires &self to be borrowed for 'static lifetime due to lifetime elision

Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Thanks!

One comment on the feature name in libp2p-core!

core/Cargo.toml Show resolved Hide resolved
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Some more comments :)

core/Cargo.toml Show resolved Hide resolved
core/Cargo.toml Show resolved Hide resolved
core/Cargo.toml Show resolved Hide resolved
Comment on lines +27 to +37
#[cfg(feature = "serde")]
use serde::{Serialize, Deserialize};

/// Public keys with byte-lengths smaller than `MAX_INLINE_KEY_LENGTH` will be
/// automatically used as the peer id using an identity multihash.
const MAX_INLINE_KEY_LENGTH: usize = 42;

/// Identifier of a peer of the network.
///
/// The data is a multihash of the public key of the peer.
#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#[cfg(feature = "serde")]
use serde::{Serialize, Deserialize};
/// Public keys with byte-lengths smaller than `MAX_INLINE_KEY_LENGTH` will be
/// automatically used as the peer id using an identity multihash.
const MAX_INLINE_KEY_LENGTH: usize = 42;
/// Identifier of a peer of the network.
///
/// The data is a multihash of the public key of the peer.
#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
/// Public keys with byte-lengths smaller than `MAX_INLINE_KEY_LENGTH` will be
/// automatically used as the peer id using an identity multihash.
const MAX_INLINE_KEY_LENGTH: usize = 42;
/// Identifier of a peer of the network.
///
/// The data is a multihash of the public key of the peer.
#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]

This would contain it to a single line.

protocols/gossipsub/Cargo.toml Show resolved Hide resolved
@thomaseizinger thomaseizinger changed the title add Serialize and Deserialize to PeerId and MessageId core: Add Serialize and Deserialize to PeerId and MessageId Dec 29, 2021
@mxinden
Copy link
Member

mxinden commented Dec 29, 2021

//CC @elenaf9 as you proposed this in #2341.

@mxinden
Copy link
Member

mxinden commented Dec 29, 2021

fixes an issue with GossipsubConfigBuilder::build() where it requires &self to be borrowed for 'static lifetime due to lifetime elision

@laptou would you mind proposing this change in a separate pull request?

@laptou
Copy link
Contributor Author

laptou commented Dec 29, 2021

I've created a new branch on my fork that has just the changes relevant to adding Serde, but I can't change the branch associated with this PR, so I will close it and open another one.

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.

3 participants