generic Executor and EvmEventLogger#5640
Conversation
|
Please look for the NOTE: in crates/evm/src/executor/inspector/logs.rs diff |
| Some(token) => { | ||
| node_info!( | ||
| " Error: reverted with custom error: {:?}", | ||
| " Error: reverted with custom error (1): {:?}", |
There was a problem hiding this comment.
| " Error: reverted with custom error (1): {:?}", | |
| " Error: reverted with custom error: {:?}", |
There was a problem hiding this comment.
FYI the point of this is to differentiate the source of the error. anvil's errors are sometimes formatted exactly the same but originate in different places.
| None => { | ||
| node_info!( | ||
| " Error: reverted with custom error: {}", | ||
| " Error: reverted with custom error (2): {}", |
There was a problem hiding this comment.
| " Error: reverted with custom error (2): {}", | |
| " Error: reverted with custom error: {}", |
There was a problem hiding this comment.
FYI the point of this is to differentiate the source of the error. anvil's errors are sometimes formatted exactly the same but originate in different places.
|
@Evalir I incorporated your requests. Listen, I've spent way too much time rebasing this pull request over and over. Please go ahead and make any trivial changes or comment suggestions you have and either accept or reject the PR. Waiting for me to respond to fairly trivial comments and having 40 commits happen in the meantime, such that I have to review all the deltas again is just not worth it. As I mentioned in the original PR this is really about causing Executor to be generic, and having spent a little more time with the code, it's really The new commits I pulled show that you guys have now added an ExecutorBuilder. And really there already was what amounted to an InspectorStackBuilder (that's what InspectorStackConfig.stack() was doing) ... What I'm getting at is that the specific inspectors and what their behaviors are is an integral part of definition the Executor, and what the specific behavior of each inspector in the inspector stack is, on a per tool basis (anvil, chisel, etc) should probably be embodied in a generic InspectorStack<... inspectors...>, which in my instant PR takes only the ONLOG generic which in turn modifies only the EvmEventLogger (formerly LogCollector). The reason the current (and former) design is good 'nuf is that all the tools tend to share inspectors that all behave the same way across all the tools. EDIT: To clatify, this instant change is a step in the right direction towards what is probably coming soon anyway i.e. generic for InspectorStack, so that inspector behavior within the Executor can be selected on a per-tool basis. |
|
Hey, so, understood. Will close this PR for now—I think these are changes we definitely want, but we're undergoing a types migration and rebasing will just be a pain for either of us. Definitely agree this is a step in the right direction, and what we'll end up doing is probably getting these changes in after the migration. Thanks for the contrib! |
2 participants
Motivation
See #5505
Solution