feat: allow seamless switching between evm and zk-vm #271
Conversation
nbaztec
changed the title
nbaztec
changed the title
nbaztec
changed the title
| fn call_with_evm(&mut self, mut env: Env) -> eyre::Result<ResultAndState> { | ||
| let mut db = self.clone(); | ||
| db.initialize(&env); | ||
| let result = match revm::evm_inner::<Self>(&mut env, &mut db, None).transact() { |
There was a problem hiding this comment.
do you think that having inspector is valuable here?
There was a problem hiding this comment.
I don't think any deployed code (or that we call) will have cheatcodes or anything else for that matter, so it seems like a safe bet.
| let is_zk_url = foundry_common::try_get_http_provider(fork_url) | ||
| .map(|provider| { | ||
| let is_zk_url = tokio::runtime::Builder::new_multi_thread() | ||
| .enable_all() | ||
| .build() | ||
| .unwrap() | ||
| .block_on(provider.request("zks_L1ChainId", ())) | ||
| .map(|_: String| true) | ||
| .unwrap_or_default(); | ||
|
|
||
| is_zk_url | ||
| }) | ||
| .unwrap_or_default(); |
There was a problem hiding this comment.
what if rpc is not available or just a network lag?
There was a problem hiding this comment.
There's a retry mechanism, so that would take care at least of this partially. If the RPC is really not available then we wouldn't even be able to obtain the information for the fork to actually happen
| @@ -58,4 +58,4 @@ command -v git &>/dev/null || { | |||
| build_zkforge "${REPO_ROOT}" | |||
|
|
|||
| echo "Running tests..." | |||
| RUST_LOG=debug "${BINARY_PATH}" test --use "./${SOLC}" | |||
| RUST_LOG=warn "${BINARY_PATH}" test --use "./${SOLC}" | |||
There was a problem hiding this comment.
Changing the default level to warn as the log output is too verbose currently, and we usually only want the reasons for failure to investigate.
| fn maybe_switch_vm<S: DatabaseExt + DatabaseCommit + Send>( | ||
| &mut self, | ||
| mut db: MutexGuard<'_, Box<S>>, | ||
| fork_id: LocalForkId, | ||
| force_initialized: bool, | ||
| ) { |
There was a problem hiding this comment.
I'd love to change the target of this function and name it switch_to_evm and check everything outside.
It's always difficut to understand what is going on, when you check one main condition at the top of the function and do almost the whole logic under the bracket.
Please consider separate this functionality
There was a problem hiding this comment.
Replaced maybe_switch_vm with select_fork_vm as this better conveys the relationship between selecting a fork (select_fork) and immediately selecting a vm for it.
What 💻
vm.selectForkwith the check if the target chain supportszks_L1ChainIdrpc method or notCheatcodeTracerand executed on the EVM with the current backend.block.number, oraddress(...).balanceare handled to correctly return the values from theAccountInfoinstead of the system contracts.Why ✋
Evidence 📷
Notes 📝
There are errors happening withcode_by_hashwhen the switch is made that need to be investigated. These don't cause any issues and happen during storage_modification recording.success()check. These don't cause any issues and can be fixed by simply reverting to zk fork at the end.TODO
code_by_hasherrors. These don't cause any issues but should be addressed.