Skip to content

Conversation

@developeruche
Copy link

Close #132

Solution

Introduced an InspectorInitializer which can init both cloneable and non-cloneable inspectors

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

Copy link
Member

@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.

nice, this looks great,

but after further evaluating this, we actually need this to be fallible.

the main use case for this is is so we can use new Inspectors that are not Clone that we need to reinitialize per tx, specifically:

https://github.com/paradigmxyz/revm-inspectors/blob/eb6ed5965050e8723ccf1ec02b0b53f7f0005af5/src/tracing/js/mod.rs#L56-L57

but creating this one is fallible:

https://github.com/paradigmxyz/revm-inspectors/blob/eb6ed5965050e8723ccf1ec02b0b53f7f0005af5/src/tracing/js/mod.rs#L109-L109

so we actually need the initalizer to also return a Result

and then the trace fn would also need to be fallible and we likely need a new error type for

-> Result<TraceOutput<E::HaltReason, E::Inspector>, E::Error>

or use either::Either<E::Error, Init::Error>

wdyt @klkvr

@developeruche
Copy link
Author

nice, this looks great,

but after further evaluating this, we actually need this to be fallible.

the main use case for this is is so we can use new Inspectors that are not Clone that we need to reinitialize per tx, specifically:

https://github.com/paradigmxyz/revm-inspectors/blob/eb6ed5965050e8723ccf1ec02b0b53f7f0005af5/src/tracing/js/mod.rs#L56-L57

but creating this one is fallible:

https://github.com/paradigmxyz/revm-inspectors/blob/eb6ed5965050e8723ccf1ec02b0b53f7f0005af5/src/tracing/js/mod.rs#L109-L109

so we actually need the initalizer to also return a Result

and then the trace fn would also need to be fallible and we likely need a new error type for

-> Result<TraceOutput<E::HaltReason, E::Inspector>, E::Error>

or use either::Either<E::Error, Init::Error>

wdyt @klkvr

If I get you right;

pub trait InspectorInitializer<T> {
    /// Create a new inspector instance.
    fn initialize(&mut self) -> T;
}

Initialize should return a result rather than the plain inspector?

@mattsse
Copy link
Member

mattsse commented Jul 17, 2025

Initialize should return a result rather than the plain inspector?

yep, for example if initializer is || JsInpesctor::new()

@developeruche
Copy link
Author

Initialize should return a result rather than the plain inspector?

yep, for example if initializer is || JsInpesctor::new()

Understood, this has been impl

Comment on lines 56 to 57
<E as Evm>::Inspector,
<E as Evm>::Error,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<E as Evm>::Inspector,
<E as Evm>::Error,
E::Inspector,
E::Error,

Copy link
Author

@developeruche developeruche Jul 22, 2025

Choose a reason for hiding this comment

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

I couldn't let this go. I needed to specify the associated type before I could proceed with compilation

@developeruche developeruche requested a review from klkvr July 22, 2025 12:34
@developeruche developeruche requested a review from mattsse August 14, 2025 14:53
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.

[Feature] Use initializer for Inspector reset

3 participants