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(identity): make Keypair and Publickey opaque #3866

Merged
merged 35 commits into from
May 5, 2023

Conversation

tcoratger
Copy link
Contributor

@tcoratger tcoratger commented May 2, 2023

Description

Keypair and Publickey are rendered opaque:

  • Keypair is replaced by a private KeyPairInner enum that is encapsulated inside the Keypair pub struct
  • Publickey is replaced by a private PublickeyInner enum that is encapsulated inside the Publickey pub struct

Resolves #3860.

Notes & open questions

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • A changelog entry has been made in the appropriate crates

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.

Direction looks good to me, a few minor comments!

Thanks for all your contributions :)

identity/src/keypair.rs Outdated Show resolved Hide resolved
identity/src/keypair.rs Outdated Show resolved Hide resolved
identity/src/keypair.rs Outdated Show resolved Hide resolved
transports/noise/src/protocol/x25519.rs Outdated Show resolved Hide resolved
Copy link
Member

@dariusc93 dariusc93 left a comment

Choose a reason for hiding this comment

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

Didnt notice the PR. Its a great start here.

Since I am not able to make a comment on the line itself, In x25519.rs on line 129 make sure to change logic in X25519::linked so it is using PublicKey::try_into_ed25519

identity/src/lib.rs Outdated Show resolved Hide resolved
identity/src/keypair.rs Outdated Show resolved Hide resolved
identity/src/keypair.rs Outdated Show resolved Hide resolved
identity/src/keypair.rs Outdated Show resolved Hide resolved
identity/src/keypair.rs Outdated Show resolved Hide resolved
identity/src/keypair.rs Outdated Show resolved Hide resolved
identity/src/keypair.rs Outdated Show resolved Hide resolved
identity/src/keypair.rs Outdated Show resolved Hide resolved
identity/src/keypair.rs Outdated Show resolved Hide resolved
identity/src/keypair.rs Outdated Show resolved Hide resolved
@thomaseizinger
Copy link
Contributor

Thank you for the additional help in reviewing @dariusc93, much appreciated! :)

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.

Really good work, looking forward to merging this, thank you!

A few minor comments but direction looks good.

identity/src/keypair.rs Outdated Show resolved Hide resolved
identity/src/keypair.rs Outdated Show resolved Hide resolved
identity/src/keypair.rs Show resolved Hide resolved
identity/src/keypair.rs Outdated Show resolved Hide resolved
identity/src/keypair.rs Outdated Show resolved Hide resolved
identity/src/lib.rs Outdated Show resolved Hide resolved
@tcoratger tcoratger marked this pull request as ready for review May 4, 2023 23:20
@mergify

This comment was marked as resolved.

identity/src/keypair.rs Outdated Show resolved Hide resolved
identity/src/keypair.rs Outdated Show resolved Hide resolved
identity/src/keypair.rs Outdated Show resolved Hide resolved
identity/src/keypair.rs Outdated Show resolved Hide resolved
identity/src/keypair.rs Outdated Show resolved Hide resolved
Copy link
Member

@dariusc93 dariusc93 left a comment

Choose a reason for hiding this comment

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

Cant comment on the line itself but on line 281 should be changed to KeypairInner::Ecdsa(key.into())

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.

Awesome, I am excited to land this!

Only a few more minor things :)

@mergify
Copy link
Contributor

mergify bot commented May 5, 2023

This pull request has merge conflicts. Could you please resolve them @tcoratger? 🙏

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!

This is a very exciting PR to land, it should make everything around Keypair and PublicKey much easier to use.

@mergify mergify bot merged commit 1493804 into libp2p:master May 5, 2023
@tcoratger
Copy link
Contributor Author

Thanks!

This is a very exciting PR to land, it should make everything around Keypair and PublicKey much easier to use.

@thomaseizinger Thanks for your help, nice library to collaborate :)

@tcoratger tcoratger deleted the identity-keypair-publickey-opaque branch May 15, 2023 17:36
@folex
Copy link
Contributor

folex commented Jun 22, 2023

Hi! I've some API concerns about that change. It makes some of our code pretty hairy.

#3860 (comment)

Hope to discuss that problem and look for possible solutions! Thank you.

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.

identity: make Keypair and Publickey opaque
4 participants