Skip to content

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

Merged
romac merged 8 commits intocometbft:mainfrom
cratelyn:kate/add-ed25519-consensus-conversions
Mar 19, 2024
Merged

tendermint: PrivateKey and PublicKey have From impls for ed25519-consensus types#1401
romac merged 8 commits intocometbft:mainfrom
cratelyn:kate/add-ed25519-consensus-conversions

Conversation

@cratelyn
Copy link
Copy Markdown
Contributor

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
Copy Markdown

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
Copy Markdown
Contributor

@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 cometbft:main Mar 19, 2024
@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
cometbft/tendermint-rs#1401, which upstreamed a direct conversion
from `ed25519_consensus`'s to `tendermint`'s representation of a public
consensus key.

* cometbft/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
cometbft/tendermint-rs#1401, which upstreamed a direct conversion
from `ed25519_consensus`'s to `tendermint`'s representation of a public
consensus key.

* cometbft/tendermint-rs#1401
melekes added a commit to cometbft/cometbft-rs that referenced this pull request Apr 24, 2025
melekes added a commit to cometbft/cometbft-rs that referenced this pull request May 3, 2025
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