fix: feature flag compiler errors #256
Conversation
Wodann
force-pushed
the
fix/secp256k1
branch
from
be56636
to
08dfa6c
Compare
| secp256k1 = ["revm_precompiles/secp256k1"] | ||
| k256 = ["revm_precompiles/k256_ecrecover"] | ||
| web3db = [] |
There was a problem hiding this comment.
Removed deprecated feature flag web3db, as it will cause cargo t --all-features to fail
Wodann
force-pushed
the
fix/secp256k1
branch
from
08dfa6c
to
70d1322
Compare
| @@ -1,9 +1,7 @@ | |||
| #[cfg(any(feature = "k256_ecrecover", feature = "secp256k1"))] | |||
There was a problem hiding this comment.
Missing feature cfg, as this only works when either the k256_ecrecover or secp256k1 feature flags are on. Ensures that cargo t --no-default-features succeeds.
|
Rebased on latest. @rakita if there's anything you'd like me to change, please let me know. |
crates/revm_precompiles/src/lib.rs
Outdated
| @@ -112,6 +112,7 @@ impl Precompiles { | |||
| static INSTANCE: OnceCell<Precompiles> = OnceCell::new(); | |||
| INSTANCE.get_or_init(|| { | |||
| let fun = vec![ | |||
| #[cfg(any(feature = "k256_ecrecover", feature = "secp256k1"))] | |||
There was a problem hiding this comment.
I dont want to disable ECRECOVER if default is not present, it seems like a footgun. As I said here I am fine with build failure: #252 (comment)
Having this would make this more user-friendly: https://doc.rust-lang.org/std/macro.compile_error.html
Something like
#[cfg(all(not(feature = "k256_ecrecover"), not(feature = "secp256k1")))]
compile_error!(
"To support ecrecover precompile please enable one of these two features, `k256_ecrecover` or `secp256k1`
);
There was a problem hiding this comment.
Added the above mentioned compile_error to the secp256k1 module
|
to unblock this I decided to have Reasoning behind this can be found here: @Wodann please tell me if this works for you |
|
The problem by reverting those changes (even with a default feature set) is that when people compile with |
Totally agreeing with this, I had bad choice of the word |
|
@Wodann does this work for you? |
|
Yup, LGTM! Thanks for clarifying 😊 |
No description provided.