Skip to content
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

Merged
merged 10 commits into from
Mar 7, 2024

Conversation

ArtificialPB
Copy link
Contributor

Adds support for muxTracer. Depends on changes from alloy-rs/alloy#252

Once both PR's are good to go I'll open a PR also for reth.

Copy link
Contributor

@mattsse mattsse left a 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(
Copy link
Contributor

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

Copy link
Contributor Author

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


/// An inspector that can delegate to multiple inspector types.
#[derive(Clone, Debug)]
enum MultiInspector {
Copy link
Contributor

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

Copy link
Contributor Author

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

Comment on lines +106 to +108
if let Some(outcome) = inspector.call(context, inputs) {
return Some(outcome);
}
Copy link
Contributor

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

Copy link
Contributor Author

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

Comment on lines +136 to +138
if let Some(outcome) = inspector.create(context, inputs) {
return Some(outcome);
}
Copy link
Contributor

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()
Copy link
Contributor

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> {
Copy link
Contributor

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

@mattsse mattsse linked an issue Mar 6, 2024 that may be closed by this pull request
Copy link
Contributor

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mattsse mattsse merged commit f0551cb into paradigmxyz:main Mar 7, 2024
11 checks passed
@ArtificialPB ArtificialPB deleted the feat/mux-tracer branch March 7, 2024 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add muxTracer to use multiple tracers in a single trace call
2 participants