evm/vm: reintroduce evm interface #2869
Conversation
|
So, just to give some feedback here: for me this looks pretty healthy, generally make things a lot more transparent to see at one unifed place what is actually exposed by the EVM (and used). And e.g. the journal then is used anyhow - being it implicit or named/stated in the interface - that's just how things are implemented right now and we just also do not want to change any more at this point (right?). So, tldr;: I would be a fan of adding this here. |
Ok, I wanted to just get this approval in order to continue, thanks 😄 Will finish today! |
|
Updated this via UI |
|
Client tests failing here. |
|
Re-triggered CI. If client miner tests fail, this seems to be a race condition, I had this on another PR which @gabrocheleau re-triggered and then it passed. |
packages/vm/src/vm.ts
Outdated
| @@ -20,6 +20,7 @@ import type { | |||
| } from './types.js' | |||
| import type { BlockchainInterface } from '@ethereumjs/blockchain' | |||
| import type { EVMStateManagerInterface } from '@ethereumjs/common' | |||
| import type { EVMInterface } from '@ethereumjs/evm/dist/cjs/types.js' | |||
There was a problem hiding this comment.
These imports are still a total no-go for merging, I guess that's obvious?
There was a problem hiding this comment.
Huh, how did this get introduced? 🤔 I think VSCode auto-imported this
There was a problem hiding this comment.
Nvm see my new comment on the PR, PR was not yet ready to be reviewed (sorry for marking it as ready-to-review)
|
When I got the approval I saw this comment on this PR (see first message):
I was a bit confused why I had this comment, but now if I review my own PR I see many TODOs. This is also why this PR looks dirty (CC @holgerd77 ) |
…ereumjs-monorepo into reintroduce-evm-interface
|
Have changed a method name ( |
|
Have updated this via UI |
|
Maybe just to throw in the ring: do we want to keep this interface on EVM or also move to Common? Side note: in case we move to Common I would be a fan of a slightly different structure there and do folder "interfaces/" and then have a dedicated named file for every major interface. This just scales better and is more transparent and very well worth to have an own directory for this, these interfaces now play a very central role in our monoprepo and can very well get a visible place. |
|
I indeed thought of moving this to Common and I think regarding cleani-ness this is great 😄 |
|
Ok, can you maybe tackle directly today or tomorrow? Would you also be ok with this slightly adopted folder structure (and could you integrate as well)? So I would then name the files after the packages for simplicity: so e.g. |
Addresses #2867
Will await an approval that this is OK before continuing.
Should we also introduce to use this interface in client?