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.

Sign up for GitHub

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

Jump to bottom

core/identity: Implement Hash and Ord for PublicKey #2915

Merged
merged 12 commits into from
Sep 22, 2022
Merged

core/identity: Implement Hash and Ord for PublicKey #2915

merged 12 commits into from
Sep 22, 2022

Conversation

futpib
Copy link
Contributor

@futpib futpib commented Sep 19, 2022 •
edited
Loading

Description

Implemented Ord and Hash instances for PublicKey to enable putting them in a HashSet/BTreeSet.

Links to any relevant issues

Fixes #2910

Open Questions

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

@futpib futpib marked this pull request as ready for review September 19, 2022 11:04
thomaseizinger
thomaseizinger reviewed Sep 19, 2022
View reviewed changes
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.

Thank you for working on this!

I've left a few comments :)

core/CHANGELOG.md Outdated Show resolved Hide resolved
core/src/identity.rs Outdated Show resolved Hide resolved
core/src/identity/ecdsa.rs Show resolved Hide resolved
core/src/identity/ed25519.rs Outdated Show resolved Hide resolved
core/src/identity/secp256k1.rs Show resolved Hide resolved
@futpib @thomaseizinger
Update core/CHANGELOG.md
0e58af1 
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
thomaseizinger
thomaseizinger requested changes Sep 20, 2022
View reviewed changes
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 for following up with this! Please see inline comment.

core/src/identity/ecdsa.rs Outdated Show resolved Hide resolved
thomaseizinger
thomaseizinger approved these changes Sep 21, 2022
View reviewed changes
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.

Thank you! :)

I've triggered CI. One last comment, otherwise this should be good to go in!

core/src/identity/rsa.rs Outdated Show resolved Hide resolved
futpib and others added 2 commits September 21, 2022 10:41
@futpib @thomaseizinger
Update core/src/identity/rsa.rs
bcedf12 
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
@futpib
cargo fmt
8c00997
mxinden
mxinden approved these changes Sep 21, 2022
View reviewed changes
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 @futpib for the patch and @thomaseizinger for the guidance.

@mxinden
Copy link
Member

mxinden commented Sep 21, 2022

Interoperability test failures due to libp2p/test-plans#41. Merging here.

mxinden
mxinden reviewed Sep 21, 2022
View reviewed changes
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.

Oh, missed one thing.

core/CHANGELOG.md Show resolved Hide resolved
@futpib futpib requested a review from mxinden September 21, 2022 19:31
@thomaseizinger thomaseizinger changed the title Implement Hash + Ord for PublicKey core/identity: Implement Hash and Ord for PublicKey Sep 22, 2022
@thomaseizinger
Copy link
Contributor

Fixed the merge conflicts :)
Let's get this in once CI passes!

@mxinden
Copy link
Member

mxinden commented Sep 22, 2022

Interoperability tests failing due to libp2p/test-plans#41. Merging here. Thanks @futpib!

@mxinden mxinden merged commit bcff814 into libp2p:master Sep 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomaseizinger thomaseizinger thomaseizinger approved these changes

@mxinden mxinden Awaiting requested review from mxinden

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ord, Eq, Hash instances for PublicKey for use with collections (sets, maps)
3 participants
@futpib @mxinden @thomaseizinger