-
Notifications
You must be signed in to change notification settings - Fork 30
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: integrate secp256r1 precompile #6
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.
great start
/// Const function for making an address by concatenating the bytes from two given numbers. | ||
/// | ||
/// Note that 32 + 128 = 160 = 20 bytes (the length of an address). This function is used | ||
/// as a convenience for specifying the addresses of the various precompiles. | ||
use revm_primitives::Address; | ||
#[inline] | ||
const fn u64_to_address(x: u64) -> Address { | ||
let x = x.to_be_bytes(); | ||
Address::new([ | ||
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, x[0], x[1], x[2], x[3], x[4], x[5], x[6], x[7], | ||
]) | ||
} |
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.
think alloy should support this? @DaniPopes
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.
found it in revm, but it's not public there https://github.com/bluealloy/revm/blob/main/crates/precompile/src/lib.rs#L263 would be indeed very helpful to be able to import it from somewhere
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.
Will make it pub
crates/precompile/src/secp256r1.rs
Outdated
Precompile::Standard(p256_verify as StandardPrecompileFn), | ||
); | ||
|
||
fn p256_verify(i: &Bytes, target_gas: u64) -> PrecompileResult { |
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.
excellent
8e4e4fd
to
203084d
Compare
Co-authored-by: DaniPopes <57450786+DaniPopes@users.noreply.github.com>
Note that I don't know if this merits its own crate, it could just be a module in the main crate. LGTM otherwise. |
agree, was thinking about this, moved to node's |
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.
nice!
Note that I don't know if this merits its own crate
I think we want to move all of those to a separate crate so they can be reused, not only by us
61e5aed
to
b99172f
Compare
makes sense, reverted |
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.
nice! this is great, my comments can be addressed in followups
|
||
#[rstest] | ||
// test vectors from https://github.com/daimo-eth/p256-verifier/tree/master/test-vectors | ||
#[case::ok_1("4cee90eb86eaa050036147a12d49004b6b9c72bd725d39d4785011fe190f0b4da73bd4903f0ce3b639bbbf6e8e80d16931ff4bcf5993d58468e8fb19086e8cac36dbcd03009df8c59286b162af3bd7fcc0450c9aa81be5d10d312af6c66b1d604aebd3099c618202fcfe16ae7770b0c49ab5eadf74b754204a3bb6060e44eff37618b065f9832de4ca6ca971a7a1adc826d0f7c00181a5fb2ddf79ae00b4e10e", true)] |
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.
interesting that this generates multiple tests, I usually just do this with an array / loop
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.
yep, there are other niceties from rstest
like fixtures with #[once]
Closes: #4