Skip to content
This repository has been archived by the owner on Apr 4, 2024. It is now read-only.

imp(evm): stateless custom precompiles #1272

Merged
merged 18 commits into from
Sep 15, 2022
Merged

imp(evm): stateless custom precompiles #1272

merged 18 commits into from
Sep 15, 2022

Conversation

fedekunze
Copy link
Contributor

@fedekunze fedekunze commented Aug 15, 2022

Description

This PR refactors the EVM module to support custom stateless precompiled contracts by wrapping the go-ethereum EVM into an interface.

This is the first step toward supporting modular EVM implementations with stateful precompiles.

Note: the concrete implementation and tests will be added in a follow-up PR, since the EVM wrapper is not hooked into the state transition at the moment.


For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@codecov
Copy link

codecov bot commented Aug 15, 2022

Codecov Report

Merging #1272 (be3e9d9) into main (723443a) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1272      +/-   ##
==========================================
+ Coverage   54.78%   54.80%   +0.01%     
==========================================
  Files         107      107              
  Lines        9984     9988       +4     
==========================================
+ Hits         5470     5474       +4     
  Misses       4243     4243              
  Partials      271      271              
Impacted Files Coverage Δ
app/ante/eth.go 80.05% <100.00%> (ø)
app/app.go 86.07% <100.00%> (ø)
x/evm/keeper/keeper.go 80.23% <100.00%> (+0.23%) ⬆️
x/evm/keeper/state_transition.go 77.03% <100.00%> (+0.13%) ⬆️
x/evm/statedb/journal.go 100.00% <100.00%> (ø)
x/evm/statedb/statedb.go 99.20% <100.00%> (ø)

@fedekunze fedekunze marked this pull request as ready for review August 19, 2022 11:09
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.

Overall good usage of the geth library for the purpose of precompiled contracts. Fixed a couple of typos in the comments above functions. The only other suggestion I see appropriate is the creation of an additional function that will handle RunStatefulPrecompiledContract.

x/evm/vm/geth/geth.go Outdated Show resolved Hide resolved
x/evm/vm/geth/geth.go Outdated Show resolved Hide resolved
x/evm/vm/geth/geth.go Outdated Show resolved Hide resolved
x/evm/vm/geth/precompiles.go Outdated Show resolved Hide resolved
x/evm/vm/interface.go Outdated Show resolved Hide resolved
fedekunze and others added 3 commits August 24, 2022 12:29
Copy link

@CoderShiun CoderShiun left a comment

Choose a reason for hiding this comment

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

LGTM.

x/evm/statedb/journal.go Outdated Show resolved Hide resolved
x/evm/vm/geth/geth.go Outdated Show resolved Hide resolved
x/evm/vm/geth/geth.go Outdated Show resolved Hide resolved
x/evm/vm/interface.go Outdated Show resolved Hide resolved
x/evm/vm/interface.go Show resolved Hide resolved
x/evm/statedb/journal.go Show resolved Hide resolved
x/evm/vm/geth/geth.go Outdated Show resolved Hide resolved
x/evm/vm/geth/geth.go Outdated Show resolved Hide resolved
x/evm/vm/geth/geth.go Show resolved Hide resolved
Copy link
Contributor

@danburck danburck left a comment

Choose a reason for hiding this comment

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

@fedekunze LGTM

It would be helpful to add more context to this PR placing it into the grand scheme of the overall spec.

x/evm/vm/interface.go Show resolved Hide resolved
@loredanacirstea
Copy link
Contributor

I based my stateful and state-using precompiles work on modifying geth to receive a custom precompile list.
But I like @fedekunze 's approach better. Wrapping the go-ethereum/core/vm constructor into an interface that remains compatible with itself is better and does not require changes from the go-ethereum team.
I wish I knew more about Go interfaces and this trick back in February:

type EVM struct {
	*vm.EVM
}

I have not tested this PR yet, but I will use this as a base (instead of #1131 (comment)), for my precompiles work and see how it works.

Copy link
Contributor

@loredanacirstea loredanacirstea left a comment

Choose a reason for hiding this comment

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

utACK
Wrapping the go-ethereum/core/vm constructor into an interface that remains compatible with itself is better and does not require changes from the go-ethereum team.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

8 participants