refactor(provider): introduce ObservedExecution to encapsulate EVM observer lifecycle#1461
Conversation
|
There was a problem hiding this comment.
Pull request overview
This PR refactors EDR provider EVM observability and gas estimation flow by introducing an ObservedExecution<T> wrapper and observe_execution helper to centralize the EVM observer lifecycle (create → execute → collect/report), while also relocating estimate_gas into data/gas.rs alongside its helpers.
Changes:
- Added
observe_execution+ObservedExecution<T>to standardize EVM observer lifecycle handling and trace extraction. - Introduced consuming “filter-to-traces” helpers (
EvmObservedData::into_call_traces,CallResultWithMetadata::into_call_traces) to replace repeated inlineif include_call_traces { ... }blocks. - Moved/refactored
estimate_gasintodata/gas.rsand introduced a sharedContextto reduce duplication and avoid unnecessary cloning.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| crates/edr_provider/src/requests/hardhat/miner.rs | Refactors call trace extraction to use EvmObservedData::into_call_traces. |
| crates/edr_provider/src/requests/eth/evm.rs | Refactors call trace extraction to use EvmObservedData::into_call_traces. |
| crates/edr_provider/src/observability.rs | Introduces ObservedExecution<T> and observe_execution to encapsulate observer lifecycle and filtered trace retrieval. |
| crates/edr_provider/src/debug_trace.rs | Switches debug tracing flow to use observe_execution and into_result_and_traces. |
| crates/edr_provider/src/data/gas.rs | Moves/refactors gas estimation into gas.rs with shared Context and observer lifecycle consolidation. |
| crates/edr_provider/src/data.rs | Adopts observe_execution at call sites, adds conversion from ObservedExecution to CallResultWithMetadata, and delegates gas estimation into gas.rs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1461 +/- ##
==========================================
+ Coverage 79.34% 79.37% +0.02%
==========================================
Files 445 445
Lines 76465 76480 +15
Branches 76465 76480 +15
==========================================
+ Hits 60673 60707 +34
+ Misses 13660 13651 -9
+ Partials 2132 2122 -10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Unify gas structs into gas::Context Unify observe_execution logic, simplify check_gas function Organize and cleanup code Fix refactor: avoid cloning traces
cfe3428 to
2be6236
Compare
Wodann
left a comment
There was a problem hiding this comment.
Thanks for simplifying the code. I have one major concern, re. performance impact of the change.
Context
While implementing gas estimation improvements (#1455), several structural issues in the existing code became apparent: the EVM observer lifecycle was repeated manually at every call site, and
estimate_gaslogic was spread acrossdata.rsrather than living ingas.rsalongside its helpers. This PR addresses those improvements independently to avoid mixing concerns and enable focused reviewChanges
Introduces
ObservedExecution<T>andobserve_executioninobservability.rsto encapsulate the observer lifecycle (create → run → collect_and_report), replacing the manual pattern repeated at every EVM call site.Key changes:
observe_executionwraps observer creation and collection into a single callObservedExecution::into_result_and_tracesprovides a consuming accessor for the execution result and filtered call tracesEvmObservedData::into_call_traces/CallResultWithMetadata::into_call_tracesare consuming filter methods that replace inline if/else blocksestimate_gasis moved fromdata.rsintogas.rs, withcheck_gas_limitandbinary_search_estimationrefactored to take a sharedContextstruct; this also eliminates the onecall_trace_arena.clone()that existed in the previous implementation