Convert StatelessVerkleStateManager usage to type in vm#4021
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| stateManager.initVerkleExecutionWitness!(block.header.number, block.executionWitness) | ||
|
|
||
| if (stateManager instanceof StatelessVerkleStateManager) { | ||
| if ('verifyPostState' in stateManager) { |
There was a problem hiding this comment.
The line was previously very descriptive stateManager instanceof StatelessVerkleStateManager such that it is clear that stateManager is actually a StatelessVerkleStateManager. Could you add a comment here? Check if stateManager is of type StatelessVerkleStateManager for instance
| } | ||
|
|
||
| if (!(vm.stateManager instanceof StatelessVerkleStateManager)) { | ||
| if (!('verifyPostState' in vm.stateManager)) { |
There was a problem hiding this comment.
Here a comment for StatelessVerkleStateManager
| !(vm.stateManager instanceof StatefulVerkleStateManager) && | ||
| !(vm.stateManager instanceof StatelessVerkleStateManager) | ||
| ) { | ||
| if (!('verifyPostState' in vm.stateManager) && !('verifyPostState' in vm.stateManager)) { |
There was a problem hiding this comment.
Here thus a check if it's a *VerkleStateManager type (Stateful or Stateless)
acolytec3
left a comment
acolytec3
left a comment
There was a problem hiding this comment.
Update evm/vite.config.bundler.ts to point to ../vm/examples/runGoerliBlock.ts and then run npm run visualize:bundle from root/packages/evm and verify that only the MerkleStateManager appears under the statemanager in the bundle
| !(vm.stateManager instanceof StatefulVerkleStateManager) && | ||
| !(vm.stateManager instanceof StatelessVerkleStateManager) | ||
| ) { | ||
| if (!('verifyPostState' in vm.stateManager) && !('verifyPostState' in vm.stateManager)) { |
There was a problem hiding this comment.
Isn't this checking the same condition twice?
There was a problem hiding this comment.
Ah, good point, I did not catch this 😅 If verifyPostState is only a method of StatefulVerkleStateManager and StatelessVerkleStateManager and not of any other StateManager then we can indeed compactify it to one check (and not the same check twice)
There was a problem hiding this comment.
Hah, sorry I missed that. Will update!
| stateManager.initVerkleExecutionWitness!(block.header.number, block.executionWitness) | ||
|
|
||
| if (stateManager instanceof StatelessVerkleStateManager) { | ||
| if ('verifyPostState' in stateManager) { |
There was a problem hiding this comment.
I think it would be nice to have a code comment directly on the original verifyPostState method how the method is (mis-)used and that it should therefore not be renamed.
holgerd77
left a comment
holgerd77
left a comment
There was a problem hiding this comment.
Will take this in here, additional comments on the other PR(s).
4 participants
This modifies our usage of
StatelessVerkleStateManagerinvmto import it only as a type so it does not get included in a bundle where theStatelessVerkleStateManagerisn't actually used