Skip to content
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

Merged
merged 10 commits into from
Mar 7, 2024

Conversation

fgimenez
Copy link
Member

@fgimenez fgimenez commented Mar 6, 2024

Closes: #4

Copy link
Member

@gakonst gakonst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great start

Comment on lines +8 to +19
/// 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],
])
}
Copy link
Member

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

Copy link
Member Author

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will make it pub

Precompile::Standard(p256_verify as StandardPrecompileFn),
);

fn p256_verify(i: &Bytes, target_gas: u64) -> PrecompileResult {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

excellent

@fgimenez fgimenez changed the title WIP: feat: integrate secp256r1 precompile feat: integrate secp256r1 precompile Mar 7, 2024
@fgimenez fgimenez marked this pull request as ready for review March 7, 2024 10:21
@fgimenez fgimenez requested review from Rjected and mattsse March 7, 2024 10:21
@fgimenez fgimenez force-pushed the fgimenez/add-secp256r1-precompile branch from 8e4e4fd to 203084d Compare March 7, 2024 10:32
crates/precompile/src/secp256r1.rs Outdated Show resolved Hide resolved
crates/precompile/src/secp256r1.rs Outdated Show resolved Hide resolved
crates/precompile/src/secp256r1.rs Outdated Show resolved Hide resolved
crates/precompile/src/secp256r1.rs Outdated Show resolved Hide resolved
@DaniPopes
Copy link
Member

Note that I don't know if this merits its own crate, it could just be a module in the main crate. LGTM otherwise.

@fgimenez
Copy link
Member Author

fgimenez commented Mar 7, 2024

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 precompile submodule and made pub(crate)

Copy link
Contributor

@mattsse mattsse left a 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

@fgimenez fgimenez force-pushed the fgimenez/add-secp256r1-precompile branch from 61e5aed to b99172f Compare March 7, 2024 16:03
@fgimenez
Copy link
Member Author

fgimenez commented Mar 7, 2024

I think we want to move all of those to a separate crate so they can be reused, not only by us

makes sense, reverted

Copy link
Member

@Rjected Rjected left a 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)]
Copy link
Member

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

Copy link
Member Author

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]

crates/precompile/src/secp256r1.rs Outdated Show resolved Hide resolved
crates/precompile/src/secp256r1.rs Outdated Show resolved Hide resolved
@fgimenez fgimenez merged commit cb52bc7 into main Mar 7, 2024
11 checks passed
@fgimenez fgimenez deleted the fgimenez/add-secp256r1-precompile branch March 7, 2024 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

secp256r1 precompile integration in EVM config
6 participants