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

Remove AsyncEventEmitter extension from EVM and VM #2235

Merged
merged 8 commits into from
Aug 25, 2022

Conversation

jochem-brouwer
Copy link
Member

Counter PR against #2229

I'm not sure anymore why we needed the generic types, especially now we can just access these fields from the interface directly (yes, the events are optional, but you can just vm.events!.on()

@codecov
Copy link

codecov bot commented Aug 25, 2022

Codecov Report

Merging #2235 (ca781c3) into master (2e9e493) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

Flag Coverage Δ
block 92.98% <ø> (ø)
blockchain 88.43% <ø> (ø)
client 87.00% <ø> (ø)
common 59.55% <ø> (ø)
devp2p 92.25% <ø> (+0.10%) ⬆️
ethash ∅ <ø> (∅)
evm 79.08% <100.00%> (+0.01%) ⬆️
rlp ∅ <ø> (∅)
statemanager 88.16% <ø> (ø)
trie 89.48% <ø> (ø)
tx 97.98% <ø> (ø)
util 92.33% <ø> (ø)
vm 85.27% <100.00%> (+0.02%) ⬆️

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

@jochem-brouwer
Copy link
Member Author

Note: the original problem we tried to solve in #2229, #2182 was that before this PR, each time if you wanted to use EVM events, you had to

(vm.evm as unknown as EVM).on("step", () => { ... });

This is not what we want. Instead, if we add the events optionally to the interface, one can now;

vm.evm.events!.on(

This does therefore not require the generic types as was introduced in the other PRs.

tape('Berlin: EIP 2315 tests', (t) => {
const common = new Common({ chain: Chain.Mainnet, hardfork: Hardfork.Berlin, eips: [2315] })

const runTest = async function (test: any, st: tape.Test) {
let i = 0
const vm = await VM.create({ common })

;(<EVM>vm.evm).on('step', function (step: any) {
vm.evm.events!.on('step', function (step: any) {
Copy link
Member

Choose a reason for hiding this comment

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

Have updated all these unnecessary castings in the tests, should be good to go now. 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep this is much cleaner, thanks 😄 👍

@holgerd77 holgerd77 marked this pull request as ready for review August 25, 2022 12:02
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.

None yet

2 participants