Class-based promisified rewrite of runCall and runCode

[s1na]

Summary This PR introduces two classes Interpreter and Loop as alternatives for respectively runCall and runCode. Most of the code is directly ported from runCall and runCode and has been promisified without much modifications to the logic. Two auxiliary data types Message and TxContext have also been used to help with structuring the data. The PR doesn't change external API.

More details:

• runTx instantiates an Interpreter and calls executeMessage on it, instead of calling runCall. Therefore runCall and runCode are removed from the main flow.
• To keep the API unchanged, runCall and runCode also use the classes internally. I personally would prefer however at some point to drop them.
• Both classes (and their names) are far from final, and will still change in future PRs. I didn't want to make this PR bigger than it already is. I will add documentation and unit tests later once they're more stable.
• Test cases which used a mocked failing runCall in runTx and runBlock tests have been removed.

(To be merged into #479)

[coveralls]

Coverage decreased (-0.8%) to 94.46% when pulling 46a771b on refactor/interpreter into c5b25c6 on v4.

[holgerd77]

Great stuff! 😀 For clarification: despite not in the final form, do you already consider this consistent and ready for review and want to iterate after merge or do you want to first continue to work on this along this PR?

[s1na]

I was thinking of doing more iterations in future PRs. This one provides the groundwork for more targeted PRs.

[holgerd77]

Ok, will try to do a review soon.

[holgerd77]

Starting slowly to grasp what's going on here! 😛

I'm totally impressed, how do you get the tests passing on such large PRs, do you have some certain strategy for that? Do you do incremental changes and run the tests again and again or do everything at once and then do fixes so long until tests pass again?

[holgerd77]

(some typos, edited the post, should always post-read, especially since I often post from mobile these days)

[s1na]

I think for each PR like this one I probably run the state tests like 20-30 times 😄 😄 it's different from case to case. For this PR some parts I did incrementally, but for the Interpreter and Loop classes, I copied the code from runCall and runCode and then tried to get a basic promisified version running, and then iterated on that.

[holgerd77]

Side note, because I stumbled upon this here: we should really add tests for the events we emit (not sure how complex this is though) - at least on a basic level (minimally: event received, but surely better with one example of the data structure) since this is crucial part of the API.

Seems you have thought on leaving the event on the VM (for now, I think later this will probably listened to on the EVM instance to reduce coupling), if I read the code correct.

[s1na]

Yeah I left event emitting on the VM now. Agree it'd be better to have it decoupled. It'd also reduce dependency of these classes on VM. But I'm still not sure where to do the events, and whether they should be all consolidated (as it is now), or each class emits events only for itself (and users would subscribe to any of them they wanted).

Regarding tests, yes the events are not being tested directly. Although indirectly since they're being used in the tests when jsontrace is enabled I sometimes notice when I break something!

[holgerd77]

I was also already thinking if Stack, Memory, gas?, other? should have their own events, together with some thought-through event name taxonomy, maybe let's open a separate issue on this and discuss this and have this formed out.

Side note: we should also think security questions along.

[holgerd77]

Ok, I've studied the code changes within this PR for over 2 hours now and while I can't guarantee for equivalency of every line of code I can say that this is a consistent rewrite preserving the existing functionality.

Would give this a go now to enable further refactoring work.

[s1na]

Thanks for taking the time Holger! :)