-
Notifications
You must be signed in to change notification settings - Fork 32
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