ethereumjs-monorepo icon indicating copy to clipboard operation
ethereumjs-monorepo copied to clipboard

EVM refactor considerations

Open jochem-brouwer opened this issue 3 years ago • 1 comments
trafficstars

Upon EVM refactor I encountered the following things; (should be addressed in a separate PR)

This issue will most likely get edited a lot due to things I find as well (or feel free to add tiny items if you find fishy-looking things, or stuff which is out-of-place)

  • [ ] Move dynamic gas usage of EXP from functions.ts into gas.ts (this also ensures the step event produces the right dynamic gas once it is invoked)
  • [ ] https://github.com/ethereumjs/ethereumjs-monorepo/blob/3e4e7bed681bfa6d1c95abbcc28f6508e828339d/packages/vm/src/evm/evm.ts#L484 this looks weird, should lte be replaced by lt? -> Also check if this is covered by ethereum/tests
  • [ ] Figure out if we can merge _baseCall and create methods in interpreter.ts; code looks duplicate for most parts
  • [ ] Same for Message, it seems like we can use this class/type as a default Environment
  • [ ] runInterpreter (evm.ts) also seems to have copied some logic for post-processing a call/create from interpreter.ts; check if all this post-call/create journaling can be moved into runInterpreter and be removed from Interpreter)

ethereum/tests related;

  • [ ] The journaling of gas refunds seem to be untested in some cases (only one failing test if the gas refund journal is not carried on in call frames in the London fork; SstoreCallToSelfSubRefundBelowZero_d0g0v0_London)

jochem-brouwer avatar Jun 06 '22 19:06 jochem-brouwer

@jochem-brouwer Can you give this issue another look and see what is done here?

If a lot of things are done I think it makes sense to take this issues out of the refactoring context and close the issue here and either add tasks to the v7 issue or open dedicated new issues (e.g. for the testing ones) with a bit more description so that people can tackle independently and with a bit more guidance.

holgerd77 avatar Jul 04 '22 15:07 holgerd77

Can this get an update and eventually be closed? (or replaced by a new issue laying out a single thing which has been left here if there is just one or two)

holgerd77 avatar Sep 20 '22 08:09 holgerd77

This were mostly internal notes to me, I should not have opened an issue for this but just written it down on some paper.

jochem-brouwer avatar Sep 20 '22 12:09 jochem-brouwer