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

tendermint: PrivateKey and PublicKey have From impls for ed25519-consensus types #1401

Merged

Conversation

cratelyn
Copy link
Collaborator

currently, when working with tendermint::{PrivateKey, PublicKey}, users of the ed25519-consensus crate must convert a ed25519_consensus::VerificationKey or ed25519_consensus::SigningKey via TryFrom<&'_ [u8]>. this has the unfortunate effect of requiring a bounds check, on a value that is already known to be a valid key.

tendermint::PublicKey::from_raw_ed25519(&vk.to_bytes()).expect("consensus key is valid")

this branch introduces some additional From<T> implementations, to facilitate free conversions from ed25519-consensus types.

  • 649dd37 tendermint: PrivateKey is From<ed25519_consensus::SigningKey>
  • 4f1a95a tendermint: SigningKey is From<ed25519_consensus::SigningKey>
  • ad24ea5 tendermint: PublicKey is From<ed25519_consensus::VerificationKey>
  • 8769244 tendermint: VerificationKey is From<ed25519_consensus::VerificationKey>

an internal SigningKey::new() constructor is also added, for the sake of consistency with VerificationKey::new(). this is optional, i'd be happy to back out of that change if it seems prudent.

some additional from_ed25519_consensus methods are provided, for cases where type inference might not suffice. these felt complimentary to existing methods like from_raw_ed25519, but are another optional change i'd be happy to back out of.

@codecov-commenter
Copy link

codecov-commenter commented Mar 19, 2024

Codecov Report

Attention: Patch coverage is 11.53846% with 23 lines in your changes are missing coverage. Please review.

Project coverage is 59.3%. Comparing base (99ed0b9) to head (a1bce2c).

❗ Current head a1bce2c differs from pull request most recent head 34e0c13. Consider uploading reports for the commit 34e0c13 to get more accurate results

Files Patch % Lines
tendermint/src/private_key.rs 0.0% 10 Missing ⚠️
tendermint/src/crypto/ed25519/signing_key.rs 0.0% 6 Missing ⚠️
tendermint/src/public_key.rs 42.8% 4 Missing ⚠️
tendermint/src/crypto/ed25519/verification_key.rs 0.0% 3 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main   #1401     +/-   ##
=======================================
- Coverage   60.1%   59.3%   -0.8%     
=======================================
  Files        271     271             
  Lines      26221   26549    +328     
=======================================
- Hits       15768   15762      -6     
- Misses     10453   10787    +334     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@romac romac left a comment

Choose a reason for hiding this comment

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

Very nice, thanks! 💐

@romac romac merged commit f11a1be into informalsystems:main Mar 19, 2024
22 checks passed
@romac romac changed the title 🔁 PrivateKey and PublicKey have From impls for ed25519-consensus types tendermint: PrivateKey and PublicKey have From impls for ed25519-consensus types Mar 19, 2024
cratelyn added a commit to penumbra-zone/penumbra that referenced this pull request Mar 19, 2024
this updates the todo comment to point to
informalsystems/tendermint-rs#1401, which upstreamed a direct conversion
from `ed25519_consensus`'s to `tendermint`'s representation of a public
consensus key.

* informalsystems/tendermint-rs#1401
cratelyn added a commit to penumbra-zone/penumbra that referenced this pull request Mar 19, 2024
this updates the todo comment to point to
informalsystems/tendermint-rs#1401, which upstreamed a direct conversion
from `ed25519_consensus`'s to `tendermint`'s representation of a public
consensus key.

* informalsystems/tendermint-rs#1401
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