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

[Feature] Add ecrecover #544

Closed
kayabaNerve opened this issue Apr 15, 2024 · 3 comments
Closed

[Feature] Add ecrecover #544

kayabaNerve opened this issue Apr 15, 2024 · 3 comments
Labels
enhancement New feature or request

Comments

@kayabaNerve
Copy link
Contributor

Component

consensus, eips, genesis

Describe the feature you would like

alloy itself exposes recover_signer yet not a 1:1 implementation of ecrecover.

ethers-rs did expose a recover function, internally calling the ecdsa crate.

ecdsa doesn't explicitly implement ecrecover, as defined in Ethereum. Please note their policy on low/high s values: RustCrypto/elliptic-curves#991

This did break an EVM dependent on it (which bubbled up into foundry IIRC).

While I could implement ecrecover myself based on ecdsa, I do not want to have to review the ecdsa crate for their current policies/impacted functions. I'd like a function which just works and is guaranteed to work in this context. While I do not believe the ecdsa crate's policies are expected to change, I'll note they require additional code around their recovery in order for their recovery to be used as ecrecover (justifying the definition of a dedicated ecrecover function regardless).

Additional context

No response

@kayabaNerve kayabaNerve added the enhancement New feature or request label Apr 15, 2024
@DaniPopes
Copy link
Member

This is already implemented in alloy_primitives::Signature::recover*, see also this test for recovering a non-normalized signature.

@kayabaNerve
Copy link
Contributor Author

Apologies. I looked through the docs and couldn't find it. I don't believe they're generating properly for Signature.

@DaniPopes
Copy link
Member

DaniPopes commented Apr 17, 2024

Yes you're right, could you open an issue in alloy-rs/core about that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants