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

Split executeMessage into executeCall, executeCreate #499

Merged
merged 15 commits into from Apr 25, 2019
Merged

Conversation

s1na
Copy link
Contributor

@s1na s1na commented Apr 25, 2019

This PR:

  • Brings all state snapshotting-reverting to executeMessage and moves call or create specific logic to _executeCall and _executeCreate. I think the differences between the two are more than the similarities, and it warrants two separate methods. It also prepares for moving call/create logic from EEI to Interpreter
  • For now _executeCreate is a relatively big function, this will be further broken down in future
  • It also changes how create collisions are handled. Previously reset the code to an invalid code which would throw. It took me a while to understand how exactly this was working. Cool trick, but confusing. So it now directly returns an error instead
  • Also re-orders functions and moves _reduceSenderBalance and _addToBalance to the bottom (they'll have to eventually be removed and moved to stateManager or something similar)

(To be merged into #479)

@holgerd77
Copy link
Member

Just as a note: this still includes the commits from the previous 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.

Apart from the repeated commits: directly did the review, looks great, Interpreter is beautifully readable now, also had some laid back all-file look on this to get a bit the broader picture.

Feel free to merge, thanks Sina.

@s1na s1na merged commit bcc0f69 into v4 Apr 25, 2019
@s1na s1na deleted the refactor/create branch April 25, 2019 11:20
@s1na s1na mentioned this pull request Apr 25, 2019
23 tasks
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.

None yet

3 participants