feat: Add dynamic crypto backend for ecrecover#2634
Conversation
kevaundray
changed the title
kevaundray
changed the title
kevaundray
changed the title
kevaundray
force-pushed
the
kw/add-dyn-crypto-rs
branch
from
71a385a to
59fbb2f
Compare
kevaundray
force-pushed
the
kw/add-dyn-crypto-rs
branch
from
59fbb2f to
e5114be
Compare
kevaundray
requested review from
DaniPopes,
klkvr,
mattsse and
yash-atreya
as code owners
| #[cfg(feature = "crypto-backend")] | ||
| if let Some(provider) = super::backend::try_get_provider() { | ||
| return provider.recover_signer_unchecked(&sig, &hash.0); | ||
| } |
There was a problem hiding this comment.
If crypto-baackend feature-flag is set, then this will call whatever backend is set here.
To install your own backend, you can call install_default_provider in your own codebase. install_default_provider can only be called once, calling it more than once will return an error
mattsse
left a comment
mattsse
left a comment
There was a problem hiding this comment.
only have nits, otherwise this lgtm
but we can add some additional docs for the backend type
| // Try dynamic backend first when crypto-backend feature is enabled | ||
| #[cfg(feature = "crypto-backend")] | ||
| if let Some(provider) = super::backend::try_get_provider() { | ||
| return provider.recover_signer_unchecked(&sig, &hash.0); | ||
| } |
There was a problem hiding this comment.
A different strategy would be to make it such that the crypto-backend was exactly like the other two backends. It would need to have a recover_signer_unchecked function instead of a method.
The downside of this approach is that the fallback would need to be defined in the crypto-backend module or we could just return an error, if no backend has been installed
There was a problem hiding this comment.
if no backend has been installed
this I'd like to avoid because this will be very unpleasant for users that just want to do some simple rpc stuff
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
mattsse
left a comment
mattsse
left a comment
There was a problem hiding this comment.
this lgtm
mind taking a look at this as well @klkvr @DaniPopes
3 participants
Motivation
One motivation for this is to allow downstream users to plug in their own implementation of certain functions. The downstream user we have in mind are zkVM guest program implementers where they could add precompiles using this strategy.
Solution
PR Checklist