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

[runtime-transaction] add secp256r1 to precompile signature details #3878

Merged

Conversation

samkim-crypto
Copy link

@samkim-crypto samkim-crypto commented Dec 3, 2024

Problem

The secp256r1 precompile (#3152) was added, but the logic in some of the crates in the monorepo such as runtime-transaction does not yet account for the new precompile.

Summary of Changes

Added a secp256r1 signatures field to the main types in the runtime-transaction.

The actual costs for the new precompile has to be added to the cost-model crate, but I will do it on a follow-up.

Fixes #

@samkim-crypto samkim-crypto force-pushed the runtime-transaction/secp256r1 branch from 431401b to 1ea4e37 Compare December 3, 2024 02:33
@samkim-crypto samkim-crypto force-pushed the runtime-transaction/secp256r1 branch from 1ea4e37 to 3f503c6 Compare December 3, 2024 02:53
@samkim-crypto samkim-crypto marked this pull request as ready for review December 3, 2024 09:02
@samkim-crypto samkim-crypto requested a review from a team as a code owner December 3, 2024 09:02
LucasSte
LucasSte previously approved these changes Dec 3, 2024
Copy link

@buffalojoec buffalojoec left a comment

Choose a reason for hiding this comment

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

Looks good, just curious if we can avoid the dependency or not.

@@ -140,26 +150,31 @@ mod tests {
Pubkey::new_unique(),
solana_sdk::secp256k1_program::ID,
solana_sdk::ed25519_program::ID,
solana_secp256r1_program::ID,

Choose a reason for hiding this comment

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

Ahh bummer, is this ID not included in the SDK anywhere? Would avoid the dependency.

Choose a reason for hiding this comment

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

Or maybe solana-sdk-ids?

Copy link

Choose a reason for hiding this comment

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

Maybe worth adding the ID to sdk in this PR.

Copy link
Author

Choose a reason for hiding this comment

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

Oh sorry I missed this. I replace all solana_secp256r1_program with solana_sdk_ids::secp256r1_program.

@samkim-crypto samkim-crypto force-pushed the runtime-transaction/secp256r1 branch from 49eabc4 to c47cfe6 Compare December 3, 2024 23:26
@samkim-crypto
Copy link
Author

Updated! and the CI seems to pass!

@samkim-crypto samkim-crypto merged commit df080d3 into anza-xyz:master Dec 4, 2024
52 checks passed
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.

4 participants