Skip to content

Conversation

holgerd77
Copy link
Member

Addresses #3522

First round on replacing the VM core class based methods with function calls by remove the bindings.

Feel free to pick up if wanted!

Copy link

codecov bot commented Jul 22, 2024

Codecov Report

Attention: Patch coverage is 76.85950% with 28 lines in your changes missing coverage. Please review.

Project coverage is 56.80%. Comparing base (d7c2619) to head (5eb2316).
Report is 1 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
block 73.21% <ø> (ø)
blockchain 85.64% <ø> (?)
client ?
common 91.60% <ø> (?)
devp2p 0.00% <ø> (?)
evm 63.33% <40.00%> (?)
genesis ?
statemanager 64.52% <ø> (?)
trie 52.38% <ø> (?)
tx ?
util 69.68% <ø> (?)
vm 58.53% <78.44%> (?)
wallet ?

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

@jochem-brouwer
Copy link
Member

jochem-brouwer commented Jul 22, 2024

I think this should be ready for review now.

Small side note, upon checking vm.ts to see whats left, we have a huge emitEVMProfile method there, which does NOT call into this anywhere, so we can also remove that!

EDIT: will go ahead and do this

@holgerd77
Copy link
Member Author

Can you do in a separate PR please?

@jochem-brouwer
Copy link
Member

Ok, went ahead and also moved emitEVMProfile to a separate file.

This will only help if the end user only uses buildBlock (which is likely the least popular method of VM), but this was a simple optimization so I think this is OK.

Copy link
Member Author

@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.

Very cool! 👏

(will admin-merge)

@holgerd77 holgerd77 merged commit fb50628 into master Jul 23, 2024
@holgerd77 holgerd77 deleted the vm-standalone-methods branch July 23, 2024 07:54
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.

2 participants