chore: provide newtypes for verification key and signature#1111
Merged
chore: provide newtypes for verification key and signature#1111
Conversation
github-actions
bot
added
conductor
Fraser999
commented
May 24, 2024
| fn get_address_pretty(signing_key: &SigningKey) -> String { | ||
| let address = Address::from_verification_key(signing_key.verification_key()); | ||
| let address = *signing_key.verification_key().address(); | ||
| hex::encode(address.to_vec()) |
Contributor
Author
There was a problem hiding this comment.
We're hex-encoding here and in a couple of functions above - maybe that should be changed to base64?
Contributor
There was a problem hiding this comment.
we need to change addresses to bech32 :/ addresses are annoying cause sometimes cometbft encodes them as hex (like when logging) and other times base64 (like in the json-rpc responses)
noot
reviewed
May 24, 2024
| /// An Ed25519 verification key. | ||
| #[derive(Clone)] | ||
| pub struct VerificationKey { | ||
| key: Ed25519VerificationKey, |
Contributor
There was a problem hiding this comment.
maybe impl From<Ed25519VerificationKey> as well?
Contributor
There was a problem hiding this comment.
That would make it part of the public API of this crate, but the goal is to avoid that.
noot
approved these changes
May 24, 2024
github-merge-queue bot
pushed a commit
that referenced
this pull request
Sep 4, 2024
## Summary Added a lazily-initialized field `address_bytes` to `VerificationKey`. ## Background Testing showed that `address_bytes` was called multiple times on a given `VerificationKey` (up to 11 times in some cases). Each time the key's bytes were being hashed, so this change ensures that hashing only happens once for a given verification key instance. Note that this was implemented previously in #1111 and was then reverted in #1124. However, when reverted, the manual impls of `PartialEq`, `Eq`, `PartialOrd`, `Ord` and `Hash` were left as-is, as were the unit tests for these. Hence this PR doesn't need to make any changes to these trait impls or tests. ## Changes - Added `address_bytes: OnceLock<[u8; ADDRESS_LEN]>` to `VerificiationKey`. ## Testing No new tests required. ## Related Issues Closes #1351.
jbowen93
pushed a commit
that referenced
this pull request
Sep 6, 2024
## Summary Added a lazily-initialized field `address_bytes` to `VerificationKey`. ## Background Testing showed that `address_bytes` was called multiple times on a given `VerificationKey` (up to 11 times in some cases). Each time the key's bytes were being hashed, so this change ensures that hashing only happens once for a given verification key instance. Note that this was implemented previously in #1111 and was then reverted in #1124. However, when reverted, the manual impls of `PartialEq`, `Eq`, `PartialOrd`, `Ord` and `Hash` were left as-is, as were the unit tests for these. Hence this PR doesn't need to make any changes to these trait impls or tests. ## Changes - Added `address_bytes: OnceLock<[u8; ADDRESS_LEN]>` to `VerificiationKey`. ## Testing No new tests required. ## Related Issues Closes #1351.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The
ed25519-consensustypes not already wrapped in newtypes are wrapped in this PR.Background
This is partly for consistency (we recently provided a newtype for the signing key), partly to avoid having third party types in our core crate's public API, and partly to support memoizing the address of the verification key.
Changes
cryptomodule ofastria-coreforVerificationKey,SignatureandError.ed25519-consensustypes with the newtypes.Address::from_verification_keywithVerificationKey::address.Testing
Provided unit tests for the
VerificationKeyto ensure thePartialEq,Eq,PartialOrd,OrdandHashimpls are correct given that they're hand-rolled in order to exclude the memoized address field.Related Issues
Closes #1079.