-
Notifications
You must be signed in to change notification settings - Fork 758
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
VM: EVM Opts Chaining for better DevEx #3481
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One comment but LGTM in general
packages/vm/src/vm.ts
Outdated
common: opts.evmOpts?.common ?? opts.common, | ||
stateManager: opts.evmOpts?.stateManager ?? opts.stateManager, | ||
blockchain: opts.evmOpts?.blockchain ?? opts.blockchain, | ||
profiler: opts.evmOpts?.profiler ?? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this:
common: opts.common,
stateManager: opts.stateManager,
blockchain: opts.blockchain,
profiler: {
enabled: enableProfiler,
},
...evmOpts
Achieve the same? Or did I miss some side effects? This will first setup the object with the opts
settings and then override (if those exist) in evmOpts
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(this is a nit, I agree the current state works as well)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needed to have a really careful look on this to be sure, but: yes, I think this works, nice optimization! 🙂
Have updated!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming CI will pass, will approve here! 😄 👍
I was a bit shocked when I wanted to integrate the new
bls
EVM option into the client here (e.g.) that we still have got this ugly DevEx in place that a full fledged EVM needs to be passed over to the VM, even if only a tiny setting or boolean flag should be adjusted (so this then draws this need in to re-provide a full round set of statemanager, blockchain, common,... to align with the VM).This PR solves this, by just chaining over the EVM options. Should have no side effects, I thought through (and provided with tests) a couple of scenarios, especially regarding blockchain, statemanager, common.
So, if these three are provided to
evmOpts
(for whatever reasons), one would assume that the calling user has some reason for it and knows what he/she is doing, so then these three take precendence (note that the whole scenario is rather theoretical, this is just "solved for completeness").Otherwise SM, blockchain, common from the normal VM options take precedense, so behavior from before.
Should be ready to be merged, this should ease the life of other users as well.