fix(evm): debug calls with custom tracer and tracer options #2031
Conversation
WalkthroughThe changes introduce enhancements to the Ethereum Virtual Machine (EVM) functionality, including modifications to tracing configurations and improvements in debugging tests. A new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant EVM
participant Tracer
User->>App: Initiate Debug Call
App->>EVM: Request Debug Trace
EVM->>Tracer: Use TracerConfig
Tracer-->>EVM: Return Trace Result
EVM-->>App: Send Trace Data
App-->>User: Display Trace Data
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
x/evm/evm.pb.gois excluded by!**/*.pb.go
Files selected for processing (5)
- CHANGELOG.md (1 hunks)
- app/app.go (1 hunks)
- e2e/evm/test/debug_queries.test.ts (4 hunks)
- proto/eth/evm/v1/evm.proto (2 hunks)
- x/evm/keeper/grpc_query.go (4 hunks)
Files skipped from review due to trivial changes (1)
- CHANGELOG.md
Additional context used
GitHub Check: break-check
proto/eth/evm/v1/evm.proto
[failure] 182-182:
Field "13" with name "tracer_config" on message "TraceConfig" changed cardinality from "optional with implicit presence" to "optional with explicit presence".
[failure] 182-182:
Field "13" with name "tracer_config" on message "TraceConfig" changed option "json_name" from "tracerJsonConfig" to "tracerConfig".
[failure] 182-182:
Field "13" with name "tracer_config" on message "TraceConfig" changed type from "string" to "message".
[failure] 182-182:
Field "13" on message "TraceConfig" changed name from "tracer_json_config" to "tracer_config".
Additional comments not posted (9)
e2e/evm/test/debug_queries.test.ts (5)
35-39: LGTM!The changes to the
debug_traceBlockByNumbertest case are approved. The additional parameters in theprovider.sendcall improve the tracing functionality by allowing for more detailed configurations during the debugging process.
48-52: LGTM!The changes to the
debug_traceBlockByHashtest case are approved. The additional parameters in theprovider.sendcall improve the tracing functionality by allowing for more detailed configurations during the debugging process.
57-65: LGTM!The changes to the
debug_traceTransactiontest case are approved. The additional parameters in theprovider.sendcall improve the tracing functionality by allowing for more detailed configurations during the debugging process. It's great to see that the test case is now actively tested rather than marked as a TODO.
80-84: LGTM!The changes to the
debug_traceCalltest case are approved. The additional parameters in theprovider.sendcall improve the tracing functionality by allowing for more detailed configurations during the debugging process.
113-119: LGTM!The changes to the
expectTracefunction are approved. The new property checks improve the validation of the trace result by checking for the completeness and correctness of the trace data returned from the debugging queries.proto/eth/evm/v1/evm.proto (1)
149-151: LGTM!The
TracerConfigmessage is well-defined and serves a clear purpose of storing Geth-specific tracer configuration.app/app.go (1)
55-57: LGTM! The import statement is used to initialize the geth native tracers.The blank identifier
_is used to import thegithub.com/ethereum/go-ethereum/eth/tracers/nativepackage solely for its side effects, i.e., to ensure that theinit()function of the package is called during the initialization of the application.This is a common pattern in Go to force the initialization of a package without actually using any of its exported identifiers in the code.
The inclusion of this import suggests that the application is integrating Ethereum's native tracing capabilities, which can provide detailed execution traces of Ethereum transactions and smart contracts.
x/evm/keeper/grpc_query.go (2)
531-533: Approve switching to a structured tracer configuration object.The change to use a structured object for the tracer configuration instead of a JSON string improves the clarity and robustness of the code.
687-687: Handle the error from marshaling the tracer configuration.Ignoring the error from marshaling the
TraceConfig.TracerConfigcould lead to unexpected behavior if there's an error. Consider handling the error, for example:-tracerConfig, _ = json.Marshal(req.TraceConfig.TracerConfig) +tracerConfig, err = json.Marshal(req.TraceConfig.TracerConfig) +if err != nil { + return nil, grpcstatus.Error(grpccodes.Internal, err.Error()) +}
| // tracer_config configures the tracer options | ||
| TracerConfig tracer_config = 13 [ (gogoproto.jsontag) = "tracerConfig" ]; |
There was a problem hiding this comment.
Breaking change: tracer_json_config field replaced with tracer_config.
The tracer_json_config field of type string at tag 13 has been replaced with a new field tracer_config of type TracerConfig at the same tag. This is a breaking change that improves type safety and clarity but requires updates to the code that sets or reads this field.
Please consider the following recommendations:
- Update the documentation to highlight this breaking change.
- Add a release note about this breaking change.
Do you need any assistance in updating the code that sets or reads this field? I'd be happy to help!
Tools
GitHub Check: break-check
[failure] 182-182:
Field "13" with name "tracer_config" on message "TraceConfig" changed cardinality from "optional with implicit presence" to "optional with explicit presence".
[failure] 182-182:
Field "13" with name "tracer_config" on message "TraceConfig" changed option "json_name" from "tracerJsonConfig" to "tracerConfig".
[failure] 182-182:
Field "13" with name "tracer_config" on message "TraceConfig" changed type from "string" to "message".
[failure] 182-182:
Field "13" on message "TraceConfig" changed name from "tracer_json_config" to "tracer_config".
| // ignore error. default to no traceConfig | ||
| _ = json.Unmarshal([]byte(req.TraceConfig.TracerJsonConfig), &tracerConfig) | ||
| tracerConfig, _ = json.Marshal(req.TraceConfig.TracerConfig) |
There was a problem hiding this comment.
Handle the error from marshaling the tracer configuration.
Ignoring the error from marshaling the TracerConfig could lead to unexpected behavior if there's an error. Consider handling the error, for example:
-tracerConfig, _ = json.Marshal(req.TraceConfig.TracerConfig)
+tracerConfig, err = json.Marshal(req.TraceConfig.TracerConfig)
+if err != nil {
+ return nil, grpcstatus.Error(grpccodes.Internal, err.Error())
+}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| tracerConfig, _ = json.Marshal(req.TraceConfig.TracerConfig) | |
| tracerConfig, err = json.Marshal(req.TraceConfig.TracerConfig) | |
| if err != nil { | |
| return nil, grpcstatus.Error(grpccodes.Internal, err.Error()) | |
| } |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2031 +/- ##
==========================================
- Coverage 66.26% 66.26% -0.01%
==========================================
Files 265 265
Lines 16746 16749 +3
==========================================
+ Hits 11097 11098 +1
- Misses 4810 4811 +1
- Partials 839 840 +1
|
Can handle
debug_traceBlockByNumber(as well as traceTransaction / traceCall) with atracerandtraceConfigoptions like this: