-
Notifications
You must be signed in to change notification settings - Fork 545
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
Return Error if CKZG or BLS features are disabled, but are called #1448
Conversation
let a_aff = &extract_g1_input(&input[..G1_INPUT_ITEM_LENGTH], false)?; | ||
let b_aff = &extract_g1_input(&input[G1_INPUT_ITEM_LENGTH..], false)?; | ||
let a_aff = &extract_g1_input(&input[..G1_INPUT_ITEM_LENGTH], false).unwrap(); | ||
let b_aff = &extract_g1_input(&input[G1_INPUT_ITEM_LENGTH..], false).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why unwrap
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if I use expect
, then what panic msg should I give? Or should I use something else altogether?
@@ -53,7 +53,7 @@ pub(super) fn g1_msm(input: &Bytes, gas_limit: u64) -> PrecompileResult { | |||
// NB: Scalar multiplications, MSMs and pairings MUST perform a subgroup check. | |||
// | |||
// So we set the subgroup_check flag to `true` | |||
let p0_aff = &extract_g1_input(slice, true)?; | |||
let p0_aff = &extract_g1_input(slice, true).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why unwrap?
@@ -64,14 +64,15 @@ pub(super) fn g1_msm(input: &Bytes, gas_limit: u64) -> PrecompileResult { | |||
&extract_scalar_input( | |||
&input[i * g1_mul::INPUT_LENGTH + G1_INPUT_ITEM_LENGTH | |||
..i * g1_mul::INPUT_LENGTH + G1_INPUT_ITEM_LENGTH + SCALAR_LENGTH], | |||
)? | |||
) | |||
.unwrap() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unwrap?
Ok { gas_used: u64, output: Bytes }, | ||
Error { error_type: PrecompileError }, | ||
FatalError { msg: String }, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should structure this differently and still use the Result
at the end it enables ?
.
So this would mean having; Result<PrecompileOutput, PrecompileErrors>
Where:
enum PrecompileErrors {
Error(PrecompileError),
Fatal {
msg: String
},
}
And maybe where we are at it add PrecompileOutput
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay working on it!
facing some issues with return types, would appreciate some pointers |
mainly in |
|
||
/// A precompile operation result. | ||
/// | ||
/// Returns either `Ok((gas_used, return_bytes))` or `Err(error)`. | ||
pub type PrecompileResult = Result<(u64, Bytes), PrecompileError>; | ||
pub enum PrecompileErrors { | ||
Error(PrecompileError), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could implement impl From<PrecompileError> for PrecompileErrors {
And transform from previous return Err(Error::OutOfGas);
to return Err(Error::OutOfGas.into());
What kind of issues? |
return type errors to be exact. I am not being able to figure out a solution
at PrecompileResult::ok(gas_used, out.into()) |
crates/primitives/Cargo.toml
Outdated
@@ -29,6 +29,7 @@ hashbrown = "0.14" | |||
auto_impl = "1.2" | |||
bitvec = { version = "1", default-features = false, features = ["alloc"] } | |||
bitflags = { version = "2.5.0", default-features = false } | |||
revm-precompile = "7.0.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be imported, it is cyclic dependency
I will take this over |
Closing this in favor of: #1499 |
Closes #1435
As of now I have only changed the
PrecompileResult
to enum and changed some code according to it. Please feel free to correct me if I have done anything wrong.