feat(evm): debug_traceCall method implemented #2022
Conversation
WalkthroughThe changes introduce the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DebugAPI
participant Backend
participant EVM
participant RPC
User->>DebugAPI: Call TraceCall
DebugAPI->>Backend: Forward TraceCall request
Backend->>EVM: Execute transaction in block context
EVM-->>Backend: Return structured logs
Backend-->>DebugAPI: Return logs to API
DebugAPI-->>User: Provide results
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2022 +/- ##
==========================================
+ Coverage 66.24% 66.26% +0.02%
==========================================
Files 265 265
Lines 16662 16746 +84
==========================================
+ Hits 11037 11097 +60
- Misses 4797 4810 +13
- Partials 828 839 +11
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Outside diff range, codebase verification and nitpick comments (4)
eth/rpc/backend/mocks/evm_query_client.go (4)
Line range hint
19-37: Review of Balance method:The method has been correctly updated to reflect its new functionality of querying balance information. The use of panic for no return value, while ensuring robustness, might be too aggressive. Consider using a more graceful error handling strategy to improve the user experience in production environments.
The code changes are approved.
Consider replacing the panic with a more user-friendly error handling approach, such as returning an error to the caller.
Line range hint
389-408: Review of TraceCall method:The method is implemented correctly and is consistent with other methods in terms of error handling. As with the
Balancemethod, consider a less aggressive error handling strategy.The code changes are approved.
Consider a consistent approach to error handling across all methods, potentially replacing the panic with error returns.
Line range hint
426-445: Review of TraceTx method:The method is implemented correctly and is consistent with other methods in terms of error handling. As with the
Balancemethod, consider a less aggressive error handling strategy.The code changes are approved.
Consider a consistent approach to error handling across all methods, potentially replacing the panic with error returns.
Line range hint
463-482: Review of ValidatorAccount method:The method is implemented correctly and is consistent with other methods in terms of error handling. As with the
Balancemethod, consider a less aggressive error handling strategy.The code changes are approved.
Consider a consistent approach to error handling across all methods, potentially replacing the panic with error returns.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
x/evm/query.pb.gois excluded by!**/*.pb.gox/evm/query.pb.gw.gois excluded by!**/*.pb.gw.go
Files selected for processing (10)
- CHANGELOG.md (1 hunks)
- e2e/evm/test/debug_queries.test.ts (3 hunks)
- eth/rpc/backend/backend.go (1 hunks)
- eth/rpc/backend/mocks/README.md (1 hunks)
- eth/rpc/backend/mocks/evm_query_client.go (27 hunks)
- eth/rpc/backend/tracing.go (1 hunks)
- eth/rpc/rpcapi/debugapi/api.go (1 hunks)
- proto/eth/evm/v1/query.proto (1 hunks)
- x/evm/keeper/grpc_query.go (4 hunks)
- x/evm/keeper/grpc_query_test.go (1 hunks)
Additional context used
Markdownlint
eth/rpc/backend/mocks/README.md
4-4: null
Bare URL used(MD034, no-bare-urls)
GitHub Check: codecov/patch
eth/rpc/backend/tracing.go
[warning] 222-226: eth/rpc/backend/tracing.go#L222-L226
Added lines #L222 - L226 were not covered by tests
[warning] 228-230: eth/rpc/backend/tracing.go#L228-L230
Added lines #L228 - L230 were not covered by tests
[warning] 233-235: eth/rpc/backend/tracing.go#L233-L235
Added lines #L233 - L235 were not covered by tests
[warning] 238-246: eth/rpc/backend/tracing.go#L238-L246
Added lines #L238 - L246 were not covered by tests
[warning] 249-250: eth/rpc/backend/tracing.go#L249-L250
Added lines #L249 - L250 were not covered by tests
[warning] 252-254: eth/rpc/backend/tracing.go#L252-L254
Added lines #L252 - L254 were not covered by tests
[warning] 256-259: eth/rpc/backend/tracing.go#L256-L259
Added lines #L256 - L259 were not covered by tests
[warning] 261-261: eth/rpc/backend/tracing.go#L261
Added line #L261 was not covered by testsx/evm/keeper/grpc_query.go
[warning] 538-538: x/evm/keeper/grpc_query.go#L538
Added line #L538 was not covered by tests
[warning] 544-544: x/evm/keeper/grpc_query.go#L544
Added line #L544 was not covered by tests
[warning] 549-549: x/evm/keeper/grpc_query.go#L549
Added line #L549 was not covered by tests
[warning] 587-587: x/evm/keeper/grpc_query.go#L587
Added line #L587 was not covered by tests
[warning] 601-601: x/evm/keeper/grpc_query.go#L601
Added line #L601 was not covered by tests
[warning] 608-608: x/evm/keeper/grpc_query.go#L608
Added line #L608 was not covered by tests
[warning] 698-699: x/evm/keeper/grpc_query.go#L698-L699
Added lines #L698 - L699 were not covered by tests
Additional comments not posted (9)
e2e/evm/test/debug_queries.test.ts (3)
32-32: Enabledebug_traceBlockByNumbertest case.The removal of the
.skipmodifier indicates that the implementation for this functionality has been completed. Ensure that the test case is fully functional and covers all necessary aspects of thedebug_traceBlockByNumbermethod.
40-40: Enabledebug_traceBlockByHashtest case.Similar to the previous test, the removal of the
.skipmodifier suggests that the implementation for this functionality is ready. Verify that the test case adequately tests thedebug_traceBlockByHashmethod.
53-67: Review new test casedebug_traceCall.This new test case is crucial as it tests the newly implemented
debug_traceCallmethod. Ensure that the test case constructs the transaction object correctly and that the expected output is validated properly. Consider adding more detailed assertions to verify specific properties of the trace result.eth/rpc/backend/backend.go (1)
125-129: Review new methodTraceCall.The addition of the
TraceCallmethod to theEVMBackendinterface is a significant enhancement. Ensure that the method's parameters (txArgs,contextHeight, andconfig) are correctly used within the method and that the return types (interface{},error) are appropriate for the expected functionality.proto/eth/evm/v1/query.proto (1)
69-72: Correctly defined RPC method.The
TraceCallmethod is correctly defined in the protobuf file with the appropriate request and response types. The inclusion of the HTTP GET option is a thoughtful addition, enhancing accessibility via RESTful API calls.The changes are approved as they meet the objectives and are correctly implemented.
eth/rpc/rpcapi/debugapi/api.go (1)
109-128: Well-implemented debugging method.The
TraceCallmethod in theDebugAPIstruct is well-implemented, enhancing the debugging capabilities by allowing the execution ofeth_callwithin the context of a specified block. The method logs useful debugging information and handles errors appropriately.The changes are approved as they meet the objectives and are correctly implemented.
x/evm/keeper/grpc_query_test.go (1)
920-1000: Review of the new functionTestTraceCallThe function
TestTraceCallis well-structured and covers various scenarios including error handling and successful transaction traces. Here are some specific observations and suggestions:
Error Handling (Lines 926-930): The test case for a nil query is correctly expecting an "InvalidArgument" error, which is good practice for robust error handling.
Successful Transaction Trace for Nibi Transfer (Lines 933-949): This test case simulates a simple Nibi transfer. The setup is straightforward and the expected response is correctly traced using
TraceNibiTransfer(). It's good to see that the test is isolated and uses mock data effectively.Trace ERC-20 Transfer Transaction (Lines 952-979): This test case is more complex, involving the deployment of a contract and a subsequent ERC-20 token transfer. The use of
DeployContractand packing the transaction data are well-handled. However, ensure that theTraceERC20Transfer()function accurately reflects the expected trace output for an ERC-20 transfer, as this is critical for the validity of the test.General Structure and Readability: The function uses a table-driven approach which is excellent for readability and maintainability. Each test case is clearly named and the scenarios are well-defined.
Mocking and Dependencies: The use of
evmtest.NewTestDeps()and the setup functions within some test cases help isolate the tests from external changes and focus on the functionality being tested.Overall, the implementation of
TestTraceCallappears to be thorough and well-considered. It's important to ensure that all edge cases and potential error conditions are covered in future tests, especially as the complexity of the transactions increases.The code changes are approved.
CHANGELOG.md (1)
118-118: Changelog entry fordebug_traceCallmethod is correctly added.The entry is well-placed under the "Unreleased" section and includes a proper link to the pull request, adhering to the project's changelog guidelines.
x/evm/keeper/grpc_query.go (1)
727-727: Review: TraceEthTxMsg Method Signature ChangeThe
TraceEthTxMsgmethod's signature has been updated to accept aMessageobject instead of aTransaction. This change is consistent with the overall shift towards usingMessageobjects for transaction tracing, as outlined in the PR objectives.
- Correctness: The method correctly handles the
Messageobject, ensuring that all necessary properties are set before tracing.- Simplicity: The updated signature simplifies the method's parameters and aligns with the new message-based approach, making the method easier to understand and use.
The changes to the
TraceEthTxMsgmethod's signature are approved as they enhance clarity and maintain consistency with the new approach.
| # Mocks Generation | ||
|
|
||
| To generate mocks, install `mockery` tool: | ||
| https://vektra.github.io/mockery/latest/installation/ |
There was a problem hiding this comment.
Address Markdownlint warning: Bare URL used.
To improve the markdown quality and comply with best practices, consider using a markdown link instead of a bare URL. For example:
[installation]: https://vektra.github.io/mockery/latest/installation/Tools
Markdownlint
4-4: null
Bare URL used(MD034, no-bare-urls)
| // TraceCall implements eth debug_traceCall method which lets you run an eth_call | ||
| // within the context of the given block execution using the final state of parent block as the base. | ||
| // Method returns the structured logs created during the execution of EVM. | ||
| // The method returns the same output as debug_traceTransaction. | ||
| // https://geth.ethereum.org/docs/interacting-with-geth/rpc/ns-debug#debugtracecall | ||
| func (b *Backend) TraceCall( | ||
| txArgs evm.JsonTxArgs, | ||
| contextBlock rpc.BlockNumber, | ||
| config *evm.TraceConfig, | ||
| ) (interface{}, error) { | ||
| blk, err := b.TendermintBlockByNumber(contextBlock) | ||
| if err != nil { | ||
| b.logger.Debug("block not found", "contextBlock", contextBlock) | ||
| return nil, err | ||
| } | ||
| nc, ok := b.clientCtx.Client.(tmrpcclient.NetworkClient) | ||
| if !ok { | ||
| return nil, errors.New("invalid rpc client") | ||
| } | ||
|
|
||
| cp, err := nc.ConsensusParams(b.ctx, &blk.Block.Height) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| traceTxRequest := evm.QueryTraceTxRequest{ | ||
| Msg: txArgs.ToMsgEthTx(), | ||
| Predecessors: nil, | ||
| BlockNumber: blk.Block.Height, | ||
| BlockTime: blk.Block.Time, | ||
| BlockHash: common.Bytes2Hex(blk.BlockID.Hash), | ||
| ProposerAddress: sdk.ConsAddress(blk.Block.ProposerAddress), | ||
| ChainId: b.chainID.Int64(), | ||
| BlockMaxGas: cp.ConsensusParams.Block.MaxGas, | ||
| } | ||
|
|
||
| if config != nil { | ||
| traceTxRequest.TraceConfig = config | ||
| } | ||
| traceResult, err := b.queryClient.TraceCall(rpc.NewContextWithHeight(contextBlock.Int64()), &traceTxRequest) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| var decodedResult interface{} | ||
| err = json.Unmarshal(traceResult.Data, &decodedResult) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return decodedResult, nil | ||
| } |
There was a problem hiding this comment.
Well-implemented method but lacks test coverage.
The TraceCall method is well-implemented and aligns with the PR objectives. However, it is crucial to add test coverage for the new lines to ensure the functionality works as expected and to catch any potential issues early.
Please consider adding unit tests to cover the new functionality. This will not only improve the code quality but also ensure that the method behaves as expected under different scenarios.
Tools
GitHub Check: codecov/patch
[warning] 222-226: eth/rpc/backend/tracing.go#L222-L226
Added lines #L222 - L226 were not covered by tests
[warning] 228-230: eth/rpc/backend/tracing.go#L228-L230
Added lines #L228 - L230 were not covered by tests
[warning] 233-235: eth/rpc/backend/tracing.go#L233-L235
Added lines #L233 - L235 were not covered by tests
[warning] 238-246: eth/rpc/backend/tracing.go#L238-L246
Added lines #L238 - L246 were not covered by tests
[warning] 249-250: eth/rpc/backend/tracing.go#L249-L250
Added lines #L249 - L250 were not covered by tests
[warning] 252-254: eth/rpc/backend/tracing.go#L252-L254
Added lines #L252 - L254 were not covered by tests
[warning] 256-259: eth/rpc/backend/tracing.go#L256-L259
Added lines #L256 - L259 were not covered by tests
[warning] 261-261: eth/rpc/backend/tracing.go#L261
Added line #L261 was not covered by tests
| if len(ret) == 0 { | ||
| panic("no return value specified for BaseFee") | ||
| } | ||
|
|
||
| var r0 *evm.QueryBaseFeeResponse | ||
| var r1 error | ||
| if rf, ok := ret.Get(0).(func(context.Context, *evm.QueryBaseFeeRequest, ...grpc.CallOption) (*evm.QueryBaseFeeResponse, error)); ok { | ||
| return rf(ctx, in, opts...) | ||
| } |
There was a problem hiding this comment.
Review of BaseFee method:
The method is implemented correctly and is consistent with other methods in terms of error handling. As with the Balance method, consider a less aggressive error handling strategy.
The code changes are approved.
Consider a consistent approach to error handling across all methods, potentially replacing the panic with error returns.
| if len(ret) == 0 { | ||
| panic("no return value specified for Code") | ||
| } | ||
|
|
||
| var r0 *evm.QueryCodeResponse | ||
| var r1 error | ||
| if rf, ok := ret.Get(0).(func(context.Context, *evm.QueryCodeRequest, ...grpc.CallOption) (*evm.QueryCodeResponse, error)); ok { | ||
| return rf(ctx, in, opts...) | ||
| } |
There was a problem hiding this comment.
Review of Code method:
The method is implemented correctly and is consistent with other methods in terms of error handling. As with the Balance method, consider a less aggressive error handling strategy.
The code changes are approved.
Consider a consistent approach to error handling across all methods, potentially replacing the panic with error returns.
| if len(ret) == 0 { | ||
| panic("no return value specified for EstimateGas") | ||
| } | ||
|
|
||
| var r0 *evm.EstimateGasResponse | ||
| var r1 error | ||
| if rf, ok := ret.Get(0).(func(context.Context, *evm.EthCallRequest, ...grpc.CallOption) (*evm.EstimateGasResponse, error)); ok { | ||
| return rf(ctx, in, opts...) | ||
| } |
There was a problem hiding this comment.
Review of EstimateGas method:
The method is implemented correctly and is consistent with other methods in terms of error handling. As with the Balance method, consider a less aggressive error handling strategy.
The code changes are approved.
Consider a consistent approach to error handling across all methods, potentially replacing the panic with error returns.
| if len(ret) == 0 { | ||
| panic("no return value specified for TraceBlock") | ||
| } | ||
|
|
||
| var r0 *evm.QueryTraceBlockResponse | ||
| var r1 error | ||
| if rf, ok := ret.Get(0).(func(context.Context, *evm.QueryTraceBlockRequest, ...grpc.CallOption) (*evm.QueryTraceBlockResponse, error)); ok { | ||
| return rf(ctx, in, opts...) | ||
| } |
There was a problem hiding this comment.
Review of TraceBlock method:
The method is implemented correctly and is consistent with other methods in terms of error handling. As with the Balance method, consider a less aggressive error handling strategy.
The code changes are approved.
Consider a consistent approach to error handling across all methods, potentially replacing the panic with error returns.
| // NewEVMQueryClient creates a new instance of EVMQueryClient. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. | ||
| // The first argument is typically a *testing.T value. | ||
| func NewEVMQueryClient(t interface { | ||
| mock.TestingT | ||
| Cleanup(func()) | ||
| } | ||
|
|
||
| // NewEVMQueryClient creates a new instance of EVMQueryClient. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. | ||
| func NewEVMQueryClient(t mockConstructorTestingTNewEVMQueryClient) *EVMQueryClient { | ||
| }) *EVMQueryClient { |
There was a problem hiding this comment.
Review of NewEVMQueryClient method:
The method is implemented correctly and is consistent with other methods in terms of error handling. As with the Balance method, consider a less aggressive error handling strategy.
The code changes are approved.
Consider a consistent approach to error handling across all methods, potentially replacing the panic with error returns.
| msg, err := ethTx.AsMessage(signer, cfg.BaseFee) | ||
| if err != nil { | ||
| result.Error = err.Error() | ||
| continue | ||
| } | ||
| traceResult, logIndex, err := k.TraceEthTxMsg(ctx, cfg, txConfig, msg, req.TraceConfig, true, nil) |
There was a problem hiding this comment.
Review: TraceBlock Method Error Handling
In the TraceBlock method, the error handling during the message conversion process (lines 698-699) is not covered by tests. This is a critical part of the method, as it ensures that any issues during the conversion are properly handled and do not propagate errors throughout the system.
Add test coverage for the error handling in the TraceBlock method to ensure that conversion errors are handled correctly.
+ // TODO: Add tests for error handling in TraceBlock methodCommittable 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.
| msg, err := ethTx.AsMessage(signer, cfg.BaseFee) | |
| if err != nil { | |
| result.Error = err.Error() | |
| continue | |
| } | |
| traceResult, logIndex, err := k.TraceEthTxMsg(ctx, cfg, txConfig, msg, req.TraceConfig, true, nil) | |
| msg, err := ethTx.AsMessage(signer, cfg.BaseFee) | |
| if err != nil { | |
| result.Error = err.Error() | |
| continue | |
| } | |
| traceResult, logIndex, err := k.TraceEthTxMsg(ctx, cfg, txConfig, msg, req.TraceConfig, true, nil) | |
| + // TODO: Add tests for error handling in TraceBlock method |
Tools
GitHub Check: codecov/patch
[warning] 698-699: x/evm/keeper/grpc_query.go#L698-L699
Added lines #L698 - L699 were not covered by tests
| msg, err := tx.AsMessage(signer, cfg.BaseFee) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| result, _, err := k.TraceEthTxMsg(ctx, cfg, txConfig, msg, req.TraceConfig, false, tracerConfig) | ||
| if err != nil { | ||
| // error will be returned with detail status from traceTx | ||
| return nil, err | ||
| } | ||
|
|
||
| resultData, err := json.Marshal(result) | ||
| if err != nil { | ||
| return nil, grpcstatus.Error(grpccodes.Internal, err.Error()) | ||
| } | ||
|
|
||
| return &evm.QueryTraceTxResponse{ | ||
| Data: resultData, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
Review: TraceTx Method Implementation
The TraceTx method has been modified to use the Message object instead of the Transaction object directly. This aligns with the PR's objective to enhance the EVM's debugging capabilities by ensuring that transactions are properly constructed and validated before processing.
- Correctness: The conversion from
TransactiontoMessageis handled correctly, and error handling is robust, ensuring that any issues during the conversion are caught and handled appropriately. - Performance: The method efficiently sets up the necessary context and configuration for tracing, which should not introduce significant overhead.
- Error Handling: Proper error handling is implemented, with clear error messages that aid in debugging.
- Test Coverage: According to static analysis warnings, lines 538 and 544 are not covered by tests. It's crucial to ensure that these parts of the code are tested, especially since they involve error handling.
Consider adding test coverage for the error handling paths in the TraceTx method to ensure robustness.
+ // TODO: Add tests for error handling in TraceTx methodCommittable 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.
| msg, err := tx.AsMessage(signer, cfg.BaseFee) | |
| if err != nil { | |
| return nil, err | |
| } | |
| result, _, err := k.TraceEthTxMsg(ctx, cfg, txConfig, msg, req.TraceConfig, false, tracerConfig) | |
| if err != nil { | |
| // error will be returned with detail status from traceTx | |
| return nil, err | |
| } | |
| resultData, err := json.Marshal(result) | |
| if err != nil { | |
| return nil, grpcstatus.Error(grpccodes.Internal, err.Error()) | |
| } | |
| return &evm.QueryTraceTxResponse{ | |
| Data: resultData, | |
| }, nil | |
| } | |
| msg, err := tx.AsMessage(signer, cfg.BaseFee) | |
| if err != nil { | |
| return nil, err | |
| } | |
| result, _, err := k.TraceEthTxMsg(ctx, cfg, txConfig, msg, req.TraceConfig, false, tracerConfig) | |
| if err != nil { | |
| // error will be returned with detail status from traceTx | |
| return nil, err | |
| } | |
| resultData, err := json.Marshal(result) | |
| if err != nil { | |
| return nil, grpcstatus.Error(grpccodes.Internal, err.Error()) | |
| } | |
| // TODO: Add tests for error handling in TraceTx method | |
| return &evm.QueryTraceTxResponse{ | |
| Data: resultData, | |
| }, nil | |
| } |
Tools
GitHub Check: codecov/patch
[warning] 538-538: x/evm/keeper/grpc_query.go#L538
Added line #L538 was not covered by tests
[warning] 544-544: x/evm/keeper/grpc_query.go#L544
Added line #L544 was not covered by tests
[warning] 549-549: x/evm/keeper/grpc_query.go#L549
Added line #L549 was not covered by tests
| // TraceCall configures a new tracer according to the provided configuration, and | ||
| // executes the given message in the provided environment. The return value will | ||
| // be tracer dependent. | ||
| func (k Keeper) TraceCall( | ||
| goCtx context.Context, req *evm.QueryTraceTxRequest, | ||
| ) (*evm.QueryTraceTxResponse, error) { | ||
| if err := req.Validate(); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // get the context of block beginning | ||
| contextHeight := req.BlockNumber | ||
| if contextHeight < 1 { | ||
| // 0 is a special value in `ContextWithHeight` | ||
| contextHeight = 1 | ||
| } | ||
|
|
||
| ctx := sdk.UnwrapSDKContext(goCtx) | ||
| ctx = ctx.WithBlockHeight(contextHeight) | ||
| ctx = ctx.WithBlockTime(req.BlockTime) | ||
| ctx = ctx.WithHeaderHash(gethcommon.Hex2Bytes(req.BlockHash)) | ||
|
|
||
| // to get the base fee we only need the block max gas in the consensus params | ||
| ctx = ctx.WithConsensusParams(&cmtproto.ConsensusParams{ | ||
| Block: &cmtproto.BlockParams{MaxGas: req.BlockMaxGas}, | ||
| }) | ||
|
|
||
| chainID := k.EthChainID(ctx) | ||
| cfg, err := k.GetEVMConfig(ctx, ParseProposerAddr(ctx, req.ProposerAddress), chainID) | ||
| if err != nil { | ||
| return nil, grpcstatus.Errorf(grpccodes.Internal, "failed to load evm config: %s", err.Error()) | ||
| } | ||
|
|
||
| // compute and use base fee of the height that is being traced | ||
| baseFee := k.GetBaseFee(ctx) | ||
| if baseFee != nil { | ||
| cfg.BaseFee = baseFee | ||
| } | ||
|
|
||
| txConfig := statedb.NewEmptyTxConfig(gethcommon.BytesToHash(ctx.HeaderHash().Bytes())) | ||
|
|
||
| var tracerConfig json.RawMessage | ||
| if req.TraceConfig != nil && req.TraceConfig.TracerJsonConfig != "" { | ||
| // ignore error. default to no traceConfig | ||
| _ = json.Unmarshal([]byte(req.TraceConfig.TracerJsonConfig), &tracerConfig) | ||
| } | ||
|
|
||
| // req.Msg is not signed, so to gethcore.Message because it's not signed and will fail on getting | ||
| msgEthTx := req.Msg | ||
| txData, err := evm.UnpackTxData(req.Msg.Data) | ||
| if err != nil { | ||
| return nil, grpcstatus.Errorf(grpccodes.Internal, "failed to unpack tx data: %s", err.Error()) | ||
| } | ||
| msg := gethcore.NewMessage( | ||
| gethcommon.HexToAddress(msgEthTx.From), | ||
| txData.GetTo(), | ||
| txData.GetNonce(), | ||
| txData.GetValueWei(), | ||
| txData.GetGas(), | ||
| txData.GetGasPrice(), | ||
| txData.GetGasFeeCapWei(), | ||
| txData.GetGasTipCapWei(), | ||
| txData.GetData(), | ||
| txData.GetAccessList(), | ||
| false, | ||
| ) | ||
| result, _, err := k.TraceEthTxMsg(ctx, cfg, txConfig, msg, req.TraceConfig, false, tracerConfig) |
There was a problem hiding this comment.
Review: TraceCall Method Implementation
The TraceCall method is a new addition that configures a tracer based on the provided configuration and executes the message in the specified environment. This method is crucial for the new debugging capabilities introduced in the PR.
- Correctness: The method correctly sets up the context and handles the conversion of transaction data to a
Messageobject. The use ofgethcore.NewMessageensures that the message is constructed correctly. - Performance: The method efficiently handles the setup and execution of the tracing, which should be performant given the context of its usage.
- Error Handling: Proper error handling is in place, with specific checks for configuration errors and transaction unpacking issues.
- Test Coverage: Lines 587 and 608, which involve error handling, are not covered by tests according to static analysis warnings. It's important to cover these scenarios to ensure the method's reliability.
Enhance test coverage for the TraceCall method, particularly focusing on error handling scenarios.
+ // TODO: Add tests for error handling in TraceCall methodCommittable 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.
| // TraceCall configures a new tracer according to the provided configuration, and | |
| // executes the given message in the provided environment. The return value will | |
| // be tracer dependent. | |
| func (k Keeper) TraceCall( | |
| goCtx context.Context, req *evm.QueryTraceTxRequest, | |
| ) (*evm.QueryTraceTxResponse, error) { | |
| if err := req.Validate(); err != nil { | |
| return nil, err | |
| } | |
| // get the context of block beginning | |
| contextHeight := req.BlockNumber | |
| if contextHeight < 1 { | |
| // 0 is a special value in `ContextWithHeight` | |
| contextHeight = 1 | |
| } | |
| ctx := sdk.UnwrapSDKContext(goCtx) | |
| ctx = ctx.WithBlockHeight(contextHeight) | |
| ctx = ctx.WithBlockTime(req.BlockTime) | |
| ctx = ctx.WithHeaderHash(gethcommon.Hex2Bytes(req.BlockHash)) | |
| // to get the base fee we only need the block max gas in the consensus params | |
| ctx = ctx.WithConsensusParams(&cmtproto.ConsensusParams{ | |
| Block: &cmtproto.BlockParams{MaxGas: req.BlockMaxGas}, | |
| }) | |
| chainID := k.EthChainID(ctx) | |
| cfg, err := k.GetEVMConfig(ctx, ParseProposerAddr(ctx, req.ProposerAddress), chainID) | |
| if err != nil { | |
| return nil, grpcstatus.Errorf(grpccodes.Internal, "failed to load evm config: %s", err.Error()) | |
| } | |
| // compute and use base fee of the height that is being traced | |
| baseFee := k.GetBaseFee(ctx) | |
| if baseFee != nil { | |
| cfg.BaseFee = baseFee | |
| } | |
| txConfig := statedb.NewEmptyTxConfig(gethcommon.BytesToHash(ctx.HeaderHash().Bytes())) | |
| var tracerConfig json.RawMessage | |
| if req.TraceConfig != nil && req.TraceConfig.TracerJsonConfig != "" { | |
| // ignore error. default to no traceConfig | |
| _ = json.Unmarshal([]byte(req.TraceConfig.TracerJsonConfig), &tracerConfig) | |
| } | |
| // req.Msg is not signed, so to gethcore.Message because it's not signed and will fail on getting | |
| msgEthTx := req.Msg | |
| txData, err := evm.UnpackTxData(req.Msg.Data) | |
| if err != nil { | |
| return nil, grpcstatus.Errorf(grpccodes.Internal, "failed to unpack tx data: %s", err.Error()) | |
| } | |
| msg := gethcore.NewMessage( | |
| gethcommon.HexToAddress(msgEthTx.From), | |
| txData.GetTo(), | |
| txData.GetNonce(), | |
| txData.GetValueWei(), | |
| txData.GetGas(), | |
| txData.GetGasPrice(), | |
| txData.GetGasFeeCapWei(), | |
| txData.GetGasTipCapWei(), | |
| txData.GetData(), | |
| txData.GetAccessList(), | |
| false, | |
| ) | |
| result, _, err := k.TraceEthTxMsg(ctx, cfg, txConfig, msg, req.TraceConfig, false, tracerConfig) | |
| // TraceCall configures a new tracer according to the provided configuration, and | |
| // executes the given message in the provided environment. The return value will | |
| // be tracer dependent. | |
| func (k Keeper) TraceCall( | |
| goCtx context.Context, req *evm.QueryTraceTxRequest, | |
| ) (*evm.QueryTraceTxResponse, error) { | |
| if err := req.Validate(); err != nil { | |
| return nil, err | |
| } | |
| // get the context of block beginning | |
| contextHeight := req.BlockNumber | |
| if contextHeight < 1 { | |
| // 0 is a special value in `ContextWithHeight` | |
| contextHeight = 1 | |
| } | |
| ctx := sdk.UnwrapSDKContext(goCtx) | |
| ctx = ctx.WithBlockHeight(contextHeight) | |
| ctx = ctx.WithBlockTime(req.BlockTime) | |
| ctx = ctx.WithHeaderHash(gethcommon.Hex2Bytes(req.BlockHash)) | |
| // to get the base fee we only need the block max gas in the consensus params | |
| ctx = ctx.WithConsensusParams(&cmtproto.ConsensusParams{ | |
| Block: &cmtproto.BlockParams{MaxGas: req.BlockMaxGas}, | |
| }) | |
| chainID := k.EthChainID(ctx) | |
| cfg, err := k.GetEVMConfig(ctx, ParseProposerAddr(ctx, req.ProposerAddress), chainID) | |
| if err != nil { | |
| return nil, grpcstatus.Errorf(grpccodes.Internal, "failed to load evm config: %s", err.Error()) | |
| } | |
| // compute and use base fee of the height that is being traced | |
| baseFee := k.GetBaseFee(ctx) | |
| if baseFee != nil { | |
| cfg.BaseFee = baseFee | |
| } | |
| txConfig := statedb.NewEmptyTxConfig(gethcommon.BytesToHash(ctx.HeaderHash().Bytes())) | |
| var tracerConfig json.RawMessage | |
| if req.TraceConfig != nil && req.TraceConfig.TracerJsonConfig != "" { | |
| // ignore error. default to no traceConfig | |
| _ = json.Unmarshal([]byte(req.TraceConfig.TracerJsonConfig), &tracerConfig) | |
| } | |
| // req.Msg is not signed, so to gethcore.Message because it's not signed and will fail on getting | |
| msgEthTx := req.Msg | |
| txData, err := evm.UnpackTxData(req.Msg.Data) | |
| if err != nil { | |
| return nil, grpcstatus.Errorf(grpccodes.Internal, "failed to unpack tx data: %s", err.Error()) | |
| } | |
| msg := gethcore.NewMessage( | |
| gethcommon.HexToAddress(msgEthTx.From), | |
| txData.GetTo(), | |
| txData.GetNonce(), | |
| txData.GetValueWei(), | |
| txData.GetGas(), | |
| txData.GetGasPrice(), | |
| txData.GetGasFeeCapWei(), | |
| txData.GetGasTipCapWei(), | |
| txData.GetData(), | |
| txData.GetAccessList(), | |
| false, | |
| ) | |
| result, _, err := k.TraceEthTxMsg(ctx, cfg, txConfig, msg, req.TraceConfig, false, tracerConfig) | |
| // TODO: Add tests for error handling in TraceCall method |
Tools
GitHub Check: codecov/patch
[warning] 587-587: x/evm/keeper/grpc_query.go#L587
Added line #L587 was not covered by tests
[warning] 601-601: x/evm/keeper/grpc_query.go#L601
Added line #L601 was not covered by tests
[warning] 608-608: x/evm/keeper/grpc_query.go#L608
Added line #L608 was not covered by tests
| func (s *BackendSuite) TestTraceCall() { | ||
| priv, _ := ethsecp256k1.GenerateKey() | ||
| from := common.BytesToAddress(priv.PubKey().Address().Bytes()) | ||
|
|
||
| txArgs := evm.JsonTxArgs{ | ||
| From: &from, | ||
| Value: (*hexutil.Big)(big.NewInt(1e12)), | ||
| } | ||
|
|
||
| testCases := []struct { | ||
| name string | ||
| registerMock func() | ||
| expResult interface{} | ||
| expPass bool | ||
| }{ | ||
| { | ||
| "pass - valid call", | ||
| func() { | ||
| var ( | ||
| queryClient = s.backend.queryClient.QueryClient.(*mocks.EVMQueryClient) | ||
| client = s.backend.clientCtx.Client.(*mocks.Client) | ||
| height int64 = 1 | ||
| ) | ||
|
|
||
| msgEthereumTx := txArgs.ToMsgEthTx() | ||
|
|
||
| _, err := RegisterBlock(client, height, nil) | ||
| s.Require().NoError(err) | ||
| RegisterTraceCall(queryClient, msgEthereumTx) | ||
| RegisterConsensusParams(client, height) | ||
| }, | ||
| map[string]interface{}{"test": "hello"}, | ||
| true, | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| s.Run(fmt.Sprintf("case %s", tc.name), func() { | ||
| s.SetupTest() // reset test and queries | ||
| tc.registerMock() | ||
|
|
||
| txResult, err := s.backend.TraceCall(txArgs, rpc.NewBlockNumber(big.NewInt(1)), &evm.TraceConfig{}) | ||
|
|
||
| if tc.expPass { | ||
| s.Require().NoError(err) | ||
| s.Require().Equal(tc.expResult, txResult) | ||
| } else { | ||
| s.Require().Error(err) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Review of the TestTraceCall function.
The TestTraceCall function is well-structured and covers a scenario for a valid call. However, there are a few areas that could be improved:
- Error Handling: The function uses
_to ignore errors in several places (e.g., line 271). It's generally a good practice to handle these errors or at least log them for debugging purposes. - Magic Numbers: The function uses a magic number
1e12(line 276). It would be more maintainable to define this as a constant at the beginning of the file, giving it a meaningful name. - Test Coverage: Currently, only one test case is defined. Consider adding more scenarios to cover different edge cases and potential error conditions.
- Mock Setup: The mock setup in lines 287-299 is quite dense and could benefit from some comments explaining each step, especially for those who might not be familiar with the codebase.
Consider handling errors instead of ignoring them, define magic numbers as constants, increase test coverage, and add comments to complex mock setups.
| func RegisterTraceCall( | ||
| queryClient *mocks.EVMQueryClient, msgEthTx *evm.MsgEthereumTx, | ||
| ) { | ||
| data := []byte{0x7b, 0x22, 0x74, 0x65, 0x73, 0x74, 0x22, 0x3a, 0x20, 0x22, 0x68, 0x65, 0x6c, 0x6c, 0x6f, 0x22, 0x7d} | ||
| queryClient.On( | ||
| "TraceCall", | ||
| rpc.NewContextWithHeight(1), | ||
| &evm.QueryTraceTxRequest{ | ||
| Msg: msgEthTx, | ||
| BlockNumber: 1, | ||
| ChainId: TEST_CHAIN_ID_NUMBER(), | ||
| BlockMaxGas: -1, | ||
| TraceConfig: &evm.TraceConfig{}, | ||
| }, | ||
| ). | ||
| Return(&evm.QueryTraceTxResponse{Data: data}, nil) | ||
| } |
There was a problem hiding this comment.
Review of the RegisterTraceCall function.
The RegisterTraceCall function is crucial for setting up the mock environment for testing the TraceCall method. Here are some observations and suggestions:
- Hardcoded Data: The JSON data used in the mock response is hardcoded (line 63). While this is acceptable for unit tests, ensure that this data accurately represents what the
TraceCallmethod would return in a real scenario. - Error Scenarios: The function currently does not test how the
TraceCallmethod behaves under error conditions. It would be beneficial to add tests that simulate various error scenarios to ensure robust error handling. - Documentation: Adding comments to explain the purpose of each parameter and the expected behavior of the mock could help other developers understand the test setup more quickly.
Consider adding error scenarios to the tests, and improve documentation within the function to enhance readability and maintainability.
Summary by CodeRabbit
New Features
debug_traceCallmethod for enhanced transaction tracing capabilities in the Ethereum Virtual Machine (EVM).TraceCallmethod to theDebugAPIandBackendfor executing calls within specified block contexts.TraceCalladded to theQueryservice, enabling non-intrusive transaction analysis.Bug Fixes
debug_traceBlockByNumberanddebug_traceBlockByHash.Documentation
mockerytool, improving usability for developers.Tests
TraceCall, enhancing coverage for various transaction scenarios.