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

EVM runCode/runCall type cleanup #2861

Merged
merged 8 commits into from
Jul 6, 2023
Merged

EVM runCode/runCall type cleanup #2861

merged 8 commits into from
Jul 6, 2023

Conversation

jochem-brouwer
Copy link
Member

Closes #2860

This PR unifies the types for the EVMRunCodeOpts and EVMRunCallOpts.

Discussion item: in order to not create a base class I had to unify the pc option from runCodeOpts. I cannot think of a practical reason why someone would want to runCode (which they supply) but they do not want to start at the starting point of the code. I suggest we remove it.

Also side-note: this whole runCode and runCall should at some point be merged. We can refactor EVM in such way that we allow the external method call to runCall (or maybe we should rename this, runEVM or something) to provide the complete system state. In these, almost all options are optional, but they provide access to the internal run state. This would extend to providing things like current stack, the current pc, etc. (This is an idea I had a long time ago, and seeing the current state of EVM it is not clean and we should clean it up, it is kind of messy)

@codecov
Copy link

codecov bot commented Jul 5, 2023

Codecov Report

Merging #2861 (35d094d) into master (4c6b4bf) will not change coverage.
The diff coverage is n/a.

❗ Current head 35d094d differs from pull request most recent head 563309a. Consider uploading reports for the commit 563309a to get more accurate results

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
vm 77.37% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

gabrocheleau
gabrocheleau previously approved these changes Jul 6, 2023
Copy link
Contributor

@gabrocheleau gabrocheleau left a comment

Choose a reason for hiding this comment

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

lgtm

@holgerd77
Copy link
Member

For the pc option I think there was a use case when this was introduced (can vaguely remember that someone explicitly requested).

So for these kind of options we should really be careful and/or really dig this out again. Normally no one is just introducing options "for fun" and had some argument why to do. 😜

@holgerd77
Copy link
Member

On this EVM refactor thing: maybe you want to do another quick round of reflection and estimate here. Is this at the end a rather somewhat small thing to do, in terms of the amount of work? Or is this more extensive? I personally completely can't judge.

So if this is rather small, we might even want to consider to just take this in now (but absolutely not the outcome I am pushing here for, totally in the open, just wanted to at least mention the possibility).

@jochem-brouwer
Copy link
Member Author

For the pc option I think there was a use case when this was introduced (can vaguely remember that someone explicitly requested).

So for these kind of options we should really be careful and/or really dig this out again. Normally no one is just introducing options "for fun" and had some argument why to do. stuck_out_tongue_winking_eye

Alright, I will block this PR and re-introduce the pc option.

Regarding the EVM refactor: this will not be a small task. Nevertheless, I will prioritize this.

@holgerd77
Copy link
Member

Regarding the EVM refactor: this will not be a small task. Nevertheless, I will prioritize this.

Ah, no, please no big refactorings any more.

@jochem-brouwer
Copy link
Member Author

jochem-brouwer commented Jul 6, 2023

@holgerd77 It's an internal refactor, see discord 😄

@jochem-brouwer
Copy link
Member Author

jochem-brouwer commented Jul 6, 2023

(and the side-note of this PR 😄 )

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EVM: Consolidate EVMRunCallOpts/EVMRunCodeOpts types
3 participants