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

Provide newtype wrappers for ed25519-consensus types #1079

Closed
Fraser999 opened this issue May 17, 2024 · 0 comments · Fixed by #1111
Closed

Provide newtype wrappers for ed25519-consensus types #1079

Fraser999 opened this issue May 17, 2024 · 0 comments · Fixed by #1111

Comments

@Fraser999
Copy link
Contributor

Fraser999 commented May 17, 2024

As part of #1074 (specifically, 3515ac0), we put the ed25519_consensus::SigningKey behind a newtype wrapper, mainly to avoid having its Debug impl exposed.

We want to wrap all the types from ed25519-consensus which we consume in a similar way, partly for consistency, but mainly since it's good practice to not include dependencies' types in a crate's public API.

github-merge-queue bot pushed a commit that referenced this issue May 27, 2024
## Summary
The `ed25519-consensus` types 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
- Provided newtypes in the `crypto` module of `astria-core` for
`VerificationKey`, `Signature` and `Error`.
- Replaced all usages of the naked `ed25519-consensus` types with the
newtypes.
- Replaced `Address::from_verification_key` with
`VerificationKey::address`.

## Testing
Provided unit tests for the `VerificationKey` to ensure the `PartialEq`,
`Eq`, `PartialOrd`, `Ord` and `Hash` impls are correct given that
they're hand-rolled in order to exclude the memoized address field.

## Related Issues
Closes #1079.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant