-
Notifications
You must be signed in to change notification settings - Fork 33
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
feat(levm): improve precompiles code clarity #1579
Conversation
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.
It's good, I just left some suggestions to improve redaction, you can either take them or maybe replace them for something else that sounds good
crates/vm/levm/src/precompiles.rs
Outdated
/// This function returns the slice defined by the limits converted to a vec. If the size to expand the | ||
/// slice are not covered by the calldata size, then the missing space needed is filled with zeros. |
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 function returns the slice defined by the limits converted to a vec. If the size to expand the | |
/// slice are not covered by the calldata size, then the missing space needed is filled with zeros. | |
/// This function returns the slice between the lower and upper limit of the calldata (as a vector), padding with zeros at the end if necessary. |
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.
Done in Improve comments
crates/vm/levm/src/precompiles.rs
Outdated
@@ -366,6 +374,7 @@ fn mod_exp(base: BigUint, exponent: BigUint, modulus: BigUint) -> BigUint { | |||
} | |||
} | |||
|
|||
/// If the result size is lower than the needed, then pads the result to right filling with zeros at left. |
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 the result size is lower than the needed, then pads the result to right filling with zeros at left. | |
/// If the result size is less than needed, pads left with zeros. |
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.
Done in Improve comments
crates/vm/levm/src/precompiles.rs
Outdated
@@ -805,7 +862,7 @@ fn blake2f_compress_f( | |||
Ok(output) | |||
} | |||
|
|||
// Reads a part of the calldata and returns what is read as u64 or an error | |||
/// Reads a part of the calldata and returns what is read as u64 or an error |
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.
/// Reads a part of the calldata and returns what is read as u64 or an error | |
/// Reads part of the calldata and returns what is read as u64 or an error |
Maybe it would be good if we clarify in which scenario it returns an error. "or an error if out of bounds" for example
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.
Done in Improve comments
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.
LGTM!
Motivation
The motivation is to improve the clarity of the code.
Description
Added some comments, made a constant and some separate functions to make the code more understandable.