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

imp(evm): stateless custom precompiles#1272

Merged
fedekunze merged 18 commits intomainfrom
fedekunze/vm-interface
Sep 15, 2022
Merged

imp(evm): stateless custom precompiles#1272
fedekunze merged 18 commits intomainfrom
fedekunze/vm-interface

Conversation

@fedekunze
Copy link
Contributor

@fedekunze fedekunze commented Aug 15, 2022 •
edited
Loading

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 •
edited
Loading

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
Vvaradinov
Vvaradinov reviewed Aug 23, 2022
View reviewed changes
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.

CoderShiun
CoderShiun reviewed Aug 28, 2022
View reviewed changes
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.

emailtovamos
emailtovamos reviewed Aug 29, 2022
View reviewed changes
4rgon4ut
4rgon4ut suggested changes Aug 30, 2022
View reviewed changes
@facs95 facs95 mentioned this pull request Sep 1, 2022
Closed
11 tasks
@fedekunze fedekunze requested a review from a team as a code owner September 14, 2022 06:53
danburck
danburck approved these changes Sep 14, 2022
View reviewed changes
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.

@loredanacirstea
Copy link
Contributor

loredanacirstea commented Sep 15, 2022

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.

loredanacirstea
loredanacirstea approved these changes Sep 15, 2022
View reviewed changes
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.

@fedekunze fedekunze merged commit acf1547 into main Sep 15, 2022
@fedekunze fedekunze deleted the fedekunze/vm-interface branch September 15, 2022 11:54
@loredanacirstea loredanacirstea mentioned this pull request Nov 5, 2022
Closed
@danburck danburck mentioned this pull request Nov 30, 2022
Closed
@linear linear bot mentioned this pull request Feb 6, 2023
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@facs95 facs95 Awaiting requested review from facs95

7 more reviewers

@Vvaradinov Vvaradinov Vvaradinov left review comments

@CoderShiun CoderShiun CoderShiun left review comments

@emailtovamos emailtovamos emailtovamos left review comments

@4rgon4ut 4rgon4ut 4rgon4ut requested changes

@itsdevbear itsdevbear itsdevbear left review comments

@loredanacirstea loredanacirstea loredanacirstea approved these changes

@danburck danburck danburck approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@fedekunze fedekunze

Labels

None yet

Projects

No open projects
Ethermint Board
Status: ✅ Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@fedekunze @loredanacirstea @Vvaradinov @CoderShiun @emailtovamos @danburck @4rgon4ut @itsdevbear