-
Notifications
You must be signed in to change notification settings - Fork 72
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
feat(tracing): implement muxTracer #57
Conversation
4ab328c
to
6abd4ff
Compare
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 looks great,
a few nits and suggestions
fn selfdestruct<DB: Database>(&mut self, contract: Address, target: Address, value: U256) { | ||
match self { | ||
MultiInspector::FourByteInspector(inspector) => { | ||
<FourByteInspector as Inspector<DB>>::selfdestruct( |
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.
I faced the same issue with the selfdestruct call but couldn't figure out why this is necessary here
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.
I think it's because the FourByteInspector
implements the Inspector<DB>
trait, where selfdestruct
function has no reference to the DB
type, and compiler can't infer the Inspector
trait type
src/tracing/mux.rs
Outdated
|
||
/// An inspector that can delegate to multiple inspector types. | ||
#[derive(Clone, Debug)] | ||
enum MultiInspector { |
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.
I think calling this multi is potentially misleading, because this only wraps a single inspector.
maybe we can find a more appropriate name?
if not I'm fine with that as well because private type
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.
I've renamed it to DelegatingInspector
, seems like a better name
if let Some(outcome) = inspector.call(context, inputs) { | ||
return Some(outcome); | ||
} |
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.
I believe the TracingInspector
does not override the response so we can ignore this some check
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.
Just noticed that I forgot to add support also for the JsInspector
, and that inspector overrides the outcome of create
. I suggest we keep this as-is for now and add support for JsInspector
via separate PR
if let Some(outcome) = inspector.create(context, inputs) { | ||
return Some(outcome); | ||
} |
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.
same
.geth_call_traces(config, result.result.gas_used()) | ||
.into(), | ||
MultiInspector::Prestate(config, inspector) => { | ||
inspector.into_geth_builder().geth_prestate_traces(result, config, db)?.into() |
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.
making a note for myself here that I need to check if we can get rid of the db
argument
|
||
impl MuxInspector { | ||
/// Try creating a new instance of [MuxInspector] from the given [MuxConfig]. | ||
pub fn try_from_config(config: MuxConfig) -> Result<MuxInspector, Error> { |
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.
hmm, maybe we should actually make MultiInspector pub so this type can be configured easily manually without needing MuxConfig with an additional new function
pub new(impl IntoIter<MultiInspector>) -> Self
but not required for this pr
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.
lgtm
Adds support for
muxTracer
. Depends on changes from alloy-rs/alloy#252Once both PR's are good to go I'll open a PR also for
reth
.