Skip to content

feat: add Precompiles trait #689

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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

feat: add Precompiles trait #689

wants to merge 11 commits into from

Conversation

0xNeshi
Copy link
Collaborator

@0xNeshi 0xNeshi commented Jun 5, 2025

Proposal for unifying all precompiles in a single extension trait that can be more ergonomically invoked on contracts.

PR Checklist

  • Tests
  • Documentation
  • Changelog

@0xNeshi 0xNeshi self-assigned this Jun 5, 2025
Copy link

netlify bot commented Jun 5, 2025

Deploy Preview for contracts-stylus canceled.

Name Link
🔨 Latest commit 9da43e0
🔍 Latest deploy log https://app.netlify.com/projects/contracts-stylus/deploys/685a945faab3d10008bc52f5

Copy link

codecov bot commented Jun 5, 2025

Codecov Report

Attention: Patch coverage is 0% with 14 lines in your changes missing coverage. Please review.

Project coverage is 85.8%. Comparing base (e15e579) to head (9da43e0).

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
contracts/src/utils/precompiles.rs 0.0% 9 Missing ⚠️
contracts/src/token/erc20/extensions/permit.rs 0.0% 5 Missing ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
contracts/src/utils/cryptography/ecdsa.rs 48.4% <ø> (ø)
contracts/src/token/erc20/extensions/permit.rs 0.0% <0.0%> (ø)
contracts/src/utils/precompiles.rs 0.0% <0.0%> (ø)

Comment on lines +8 to +24
/// Precompile primitives.
pub mod primitives {
/// The ecrecover precompile primitives.
///
/// This module provides the cryptographic primitives needed for the
/// `ecrecover` precompile, which recovers the signer address from an
/// ECDSA signature and message hash.
///
/// Re-exports selected ECDSA types and constants specifically relevant
/// to the ecrecover operation.
pub mod ecrecover {
pub use crate::utils::cryptography::ecdsa::{
ECDSAInvalidSignature, ECDSAInvalidSignatureS, EcRecoverData,
Error, ECRECOVER_ADDR, SIGNATURE_S_UPPER_BOUND,
};
}
}
Copy link
Collaborator Author

@0xNeshi 0xNeshi Jun 5, 2025

Choose a reason for hiding this comment

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

Didn't remove utils::cryptography::ecdsa for two reasons:

  1. The original crate can be expanded to include additional ECDSA-related operations, which have nothing to do with ecrecover precompile; the crate would be useful to devs as a standalone.
  2. Backward compatibility.

The mod primitives module is used to make it clear to devs where to find relevant precompile-related types, constants, enums etc.
In this specific case, the reason for this reexport is that it should be easier for the average Stylus developer to understand that the relevant ecrecover primitives are in the same module, i.e. precompiles::primitives::ecrecover, than that they should actually import them from a completely different module utils::cryptography::ecdsa.

@0xNeshi
Copy link
Collaborator Author

0xNeshi commented Jun 5, 2025

CI should be fixed by #690

Copy link
Collaborator

@bidzyyys bidzyyys left a comment

Choose a reason for hiding this comment

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

I like the idea, but my main concern is that we are not able to build common interface (trait) for all the precompiles. IMHO each precompile is specific, and I do not see a way how we can handle it.

@0xNeshi
Copy link
Collaborator Author

0xNeshi commented Jun 5, 2025

I like the idea, but my main concern is that we are not able to build common interface (trait) for all the precompiles. IMHO each precompile is specific, and I do not see a way how we can handle it.

The idea is that each new precompile has its own function within trait Precompiles; the dev then imports utils::precompiles::Precompiles and they have access to all the wrapper functions.

Can you think of any precompile that would not work with this approach?

@0xNeshi 0xNeshi requested a review from bidzyyys June 5, 2025 14:02
Copy link
Collaborator

@bidzyyys bidzyyys left a comment

Choose a reason for hiding this comment

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

I think I may like the solution but please provide a refactor to our contracts too with the new precompile (Erc20Permit).

0xNeshi and others added 2 commits June 24, 2025 13:50
@0xNeshi
Copy link
Collaborator Author

0xNeshi commented Jun 24, 2025

I think I may like the solution but please provide a refactor to our contracts too with the new precompile (Erc20Permit).

That's already done:

@bidzyyys if both you and @qalisander support this approach, I'll update CHANGELOG and docs

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.

2 participants