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

add impl for DigestPrimitive on keccak feature #269

Closed
wants to merge 1 commit into from
Closed

add impl for DigestPrimitive on keccak feature #269

wants to merge 1 commit into from

Conversation

TheRealBluesun
Copy link

Abstract

This PR adds an impl DigestPrimitive when the feature keccak256 is used. This should resolve a build error when using the feature.

@tarcieri
Copy link
Member

tarcieri commented Dec 26, 2020

I think what you're looking for is k256::ecdsa::recoverable::Signature, which provides Ethereum-style recoverable signatures (which carry an extra byte as a "recovery ID"):

https://docs.rs/k256/0.7.1/k256/ecdsa/recoverable/index.html

Note that this signature type already has an impl equivalent to the one you're trying to add here:

https://github.com/RustCrypto/elliptic-curves/blob/80f33e8/k256/src/ecdsa/recoverable.rs#L259-L262

...also note that DigestSignature can only be impl'd once for any given signature type as it defines a "default" digest primitive, so the tests for this PR are failing due to those conflicting implementations.

If you really want to produce a standard (non-recoverable) ECDSA signature with Keccak256 (which I'm guessing isn't the case), the only way to do it is by passing a Keccak256 instance directly to DigestSigner.

@TheRealBluesun
Copy link
Author

Thank you so much for taking the time to respond!

You are correct, this was my misunderstanding. Cheers!

tarcieri added a commit that referenced this pull request Dec 26, 2020
Per #269 it seems this might've been a bit hard to discover, so this
note hopefully makes it easier.
@tarcieri
Copy link
Member

Opened #270 to hopefully clarify the documentation

tarcieri added a commit that referenced this pull request Dec 26, 2020
Per #269 it seems this might've been a bit hard to discover, so this
note hopefully makes it easier.
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.

None yet

2 participants