EVM/VM: update imports/exports, async-eventemitter -> eventemitter2 Switch

[acolytec3]

Fixes #2295

[codecov]

Codecov Report

Merging #2303 (f5f0642) into master (6eb5a85) will decrease coverage by 0.27%.
The diff coverage is 100.00%.

Additional details and impacted files

Flag	Coverage Δ
block	91.37% <ø> (ø)
blockchain	90.21% <ø> (ø)
client	87.00% <ø> (?)
common	98.39% <ø> (ø)
devp2p	91.63% <ø> (-0.03%)	⬇️
ethash	∅ <ø> (∅)
evm	79.17% <100.00%> (-0.07%)	⬇️
rlp	∅ <ø> (∅)
statemanager	89.46% <ø> (ø)
trie	90.36% <ø> (ø)
tx	97.79% <ø> (ø)
util	88.99% <ø> (ø)
vm	85.20% <100.00%> (-0.11%)	⬇️

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

[holgerd77]

Just read up on the associated issue, what is the status of this here? I would assume from your latest comment ("will pick up on Monday" or similar) not yet ready for review?

[acolytec3]

Correct. I need to figure out how to cleanly update the types for asyncEventEmitter. The hacks described in this thread all work in some sense but I haven't been able to make any of them without either casting to any or else having other weird side affects on the EVMInterface typing so far where it also comes for evm.codes

[acolytec3]

I ran the VM benchmarks with the new eventemitter2 dependency and below are the results vs current master (basically identical).

eventemitter2
Number of blocks to sample: 10
Block 9422905 x 27,769 ops/sec ±1.84% (85 runs sampled)
Block 9422906 x 26,526 ops/sec ±3.12% (87 runs sampled)
Block 9422907 x 26,901 ops/sec ±0.80% (91 runs sampled)
Block 9422908 x 25,902 ops/sec ±0.80% (88 runs sampled)
Block 9422910 x 24,201 ops/sec ±4.45% (86 runs sampled)

async-eventemitter
Number of blocks to sample: 10
Block 9422905 x 28,308 ops/sec ±1.21% (88 runs sampled)
Block 9422906 x 26,785 ops/sec ±2.05% (89 runs sampled)
Block 9422907 x 26,336 ops/sec ±1.01% (92 runs sampled)
Block 9422908 x 24,039 ops/sec ±3.81% (83 runs sampled)
Block 9422910 x 24,225 ops/sec ±0.82% (86 runs sampled)

So, on performance at least, looks good. Will rebase the PR on current master and we can verify everything is working ok.

[holgerd77]

Actually I just can't do much more on this review than sitting here before the monitor for 5 minutes and reflecting on the changes and impacts. 😀

I do think that changing on the event emitter class "on the go" has some risks and might trigger some not-thought-of consequenses for some use cases and we somewhat have to count in that some people might complain (or at least run into some issues).

On the other hand I think this is so much the right respectively an overdue and at the end good move to switch from this super outdated package to a modern, used and maintained library, that the benefits - at least mid-term - strongly outweight some short-term hickups.

So, long story short: will approve, also directly integrate into releases (will prepare right after merging in here).

[holgerd77]

Ugh.

@jochem-brouwer Seems we are loosing this typing here. 😬

I've tried to re-add, but seems evenemitter2 is not generic so this is not an option.

Hmm, sorry for that, I know this was some great improvement. Since we were so stuck on this import problem finally solved here and having this super-old dependency removed and updated would generally be really good I would have some stronger tendency for now to nevertheless go for it. Are you ok with this? 😬

I am currently not seeing an alternative.

Or are there eventual solutions here to get the typing back? Can't think of any right now?

[holgerd77]

Seems this is being worked on (respectively: already done) here? 🤩

EventEmitter2/EventEmitter2#294

So maybe we can just bring this back in a follow-up bugfix release once this is ready to be published?

[davidmurdoch]

This is a breaking change, if EthereumJS follows semver. Ganache uses the next method of EtheruemJS vm's usage of AsyncEventEmitter's emitter.on method. Example:

vm.evm.events.on("step", async (event, next) => {
  // ... some async things ...
  next();
});

It doesn't appear that the new library you use has this option. Can you revert this change in the v6 release? Then maybe re-release with this PR as a v7?

[holgerd77]

@davidmurdoch we'll discuss internally on how to proceed here and reach back here once we have settled.