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(precompiles): EIP-7212: secp256r1 curve #1922

Merged
merged 15 commits into from Oct 23, 2023
Merged

feat(precompiles): EIP-7212: secp256r1 curve #1922

merged 15 commits into from Oct 23, 2023

Conversation

fedekunze
Copy link
Contributor

@fedekunze fedekunze commented Oct 22, 2023

Description

Integrates EIP-7212 as a precompile. Uses ethereum/go-etherum#27540 as reference


Closes #XXX

@github-actions github-actions bot added the proto label Oct 22, 2023
@fedekunze fedekunze changed the title "feat(precompiles): EIP-7212: secp256r1 curve" feat(precompiles): EIP-7212: secp256r1 curve Oct 22, 2023
@github-actions github-actions bot removed the proto label Oct 22, 2023
@fedekunze fedekunze marked this pull request as ready for review October 22, 2023 22:15
@fedekunze fedekunze requested a review from a team as a code owner October 22, 2023 22:15
@fedekunze fedekunze requested review from Vvaradinov and facs95 and removed request for a team October 22, 2023 22:15
Copy link
Contributor

@Vvaradinov Vvaradinov left a comment

Choose a reason for hiding this comment

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

LGTM! One last thing I did is add the bech32 address to the slice so the address can't accept transfers. Added in precompiles/common/types.go

app/upgrades/v16/upgrades.go Show resolved Hide resolved
@fedekunze fedekunze merged commit 2ad91e2 into main Oct 23, 2023
22 of 26 checks passed
@fedekunze fedekunze deleted the fedekunze/p256 branch October 23, 2023 08:43
@@ -189,6 +191,11 @@ func (n *IntegrationNetwork) GetChainID() string {
return n.cfg.chainID
}

// GetEIP155ChainID returns the network EIp-155 chainID number
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// GetEIP155ChainID returns the network EIp-155 chainID number
// GetEIP155ChainID returns the network EIP-155 chainID number

@@ -25,6 +28,7 @@ func DefaultConfig() Config {
account, _ := testtx.NewAccAddressAndKey()
return Config{
chainID: utils.MainnetChainID + "-1",
eip155ChainID: big.NewInt(9001),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this hardcoded number? The const used at the line above has this docstring:

// MainnetChainID defines the Evmos EIP155 chain ID for mainnet
MainnetChainID = "evmos_9001"

Isn't this one the eip155 chain id?

@@ -0,0 +1,82 @@
// Copyright 2014 The go-ethereum Authors
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it correct this copyright?

// See https://eips.ethereum.org/EIPS/eip-7212 for details
type Precompile struct{}

// Address defines the address of the staking compile contract.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Address defines the address of the staking compile contract.
// Address defines the address of the p256 compile contract.

// Address defines the address of the staking compile contract.
// address: 0x0000000000000000000000000000000000000013
func (Precompile) Address() common.Address {
return common.BytesToAddress([]byte{19})
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't we use the address as a const instead of passing []byte{19}?

precompiles/p256/p256.go Show resolved Hide resolved
precompiles/p256/p256.go Show resolved Hide resolved
@ulerdogan
Copy link

Hello! Thanks and congrats on the very first implementations on the EIP-7212. I am not sure if it's available on the mainnet now, I couldn't see in the documentation, but I just wanted to take attention of the current status of the EIP-7212.

The original EIP is currently on the Review phase which means that it can have significant changes on the implementation. At the same time, the proposal is being discussed in the RollCall's to standardize it as an RIP to be integrated by the EVM-like rollups. Then, most of the rollups are planning implement it to their protocol without waiting the EIP. We are planning to finalize the decision in the next RollCall at the 13th of the December. I recommend you to follow this discussion to have an identical version with most of the other EVMs.

Also, we have a PR to change the implementation address, it's not merged. I think that it's good to follow the RIP discussions for the address selection as the address will be blocked for this specific precompile and will prevent overlaps in the general ecosystem.

@ulerdogan
Copy link

Hello! I am happy to share that the specification of the proposal has been finalized by moving into the RIPs repository and the last changes will be applied with this PR. The 0x100 address range has been attached for the RIP precompiles and it will be first of them.

@MalteHerrmann
Copy link
Contributor

Thanks for the update @ulerdogan! 🙏 we'll adjust the implementation address accordingly.

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.

None yet

5 participants