- 
                Notifications
    
You must be signed in to change notification settings  - Fork 113
 
feat(rpc): add debug_traceCall api #711
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall nice, looks good! Added a few questions. Would also like to add some stricter validation on the tests.
| 
               | 
          ||
| // Verify response contains expected trace data | ||
| if tc.traceResponse != "" { | ||
| s.Require().Contains(string(res.Data), tc.traceResponse) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not get more specific and have strict equalities with the strings here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
applied (11e5dc5)
| } | ||
| 
               | 
          ||
| // Check for expected fields in callTracer format | ||
| requiredFields := []string{"from", "gas", "gasUsed", "to", "type"} | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any way we can validate the entire trace, or is there indeterminism?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably also check that there aren't any extra fields here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
applied (11e5dc5)
The response type comparison is like below. (evmd vs geth)
Actual logs
2025/10/11 19:00:10 Structure Comparison for debug_traceCall:
2025/10/11 19:00:10   Structure Match: true
2025/10/11 19:00:10   Type Match: true (object vs object)
2025/10/11 19:00:10   Errors Match: true
| 
               | 
          ||
| // Check gas fields exist and are hex strings | ||
| if gasStr, ok := traceResult["gas"].(string); ok { | ||
| if !strings.HasPrefix(gasStr, "0x") { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mostly just want more strict validation everywhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
applied (11e5dc5)
| } | ||
| 
               | 
          ||
| func DebugTraceCall(rCtx *types.RPCContext) (*types.RpcResult, error) { | ||
| if result := rCtx.AlreadyTested(MethodNameDebugTraceCall); result != nil { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain this line? What do you mean already tested?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was originally a design to prevent wasting resources by redundantly testing the same API multiple times.
I had some reasoning behind it back then, but now it looks unnecessary. I’ll remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
applied (f58bfa1)
| }, nil | ||
| } | ||
| 
               | 
          ||
| result := &types.RpcResult{ | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's run some validation on the data wtihin the trace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think comparison between return data from geth can be substitution for that.
applied (11e5dc5)
        
          
                x/vm/keeper/grpc_query.go
              
                Outdated
          
        
      | // get the context of block beginning | ||
| requestedHeight := req.BlockNumber | ||
| if requestedHeight < 1 { | ||
| // 0 is a special value in `ContextWithHeight` | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, need explanation for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto (#711 (comment))
| } | ||
| 
               | 
          ||
| ctx := sdk.UnwrapSDKContext(c) | ||
| // the caller sets the `ctx.BlockHeight()` to be `requestedHeight - 1`, so we can get the context of block beginning | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this documented anywhere? Don't quite understand this either
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto (#711 (comment))
| 
           Could we also add a simple timeout? #691 (comment)  | 
    
          
 Could you check this comment? Actually, timeout has already been applied.  | 
    
* add debug_traceBlock api * add test cases for debug_traceCall * chore: update CHANGELOG.md * add debug_traceBlock api * feat(rpc): add debug_getRawBlock api and fix debug_traceBlock api * test(rpc): fix test cases for debug apis * test(rpc): modify test case for debug_traceCall * chore: fix lint * fix test code * Merge branch 'main' into feat(rpc)-traceCall * chore: modify comments * fix(x/vm): apply default args to TraceCall method * test(jsonrpc): add type comparison of debug_traceCall response from evmd vs geth * chore: remove useless method call * fix: non-deterministic system test result * fix: non-deterministic system test result * fix: non-deterministic system test result * fix: non-deterministic system test result * fix: non-deterministic system test result * fix: non-deterministic system test result * fix: non-deterministic system test result * fix: non-deterministic system test result * fix: non-deterministic system test result * fix: return type of eth apis * fix: non-deterministic system test result * chore: non-deterministic calculation for query * fix: non-deterministic system test result * test: fix eth_feeHistory api test code * test: fix eth_feeHistory api test code --------- Co-authored-by: Vlad J <vladjdk@gmail.com>
Description
Add debug namespace apis below.
debug_traceCalldebug_traceBlockdebug_getRawBlockCloses: #692
Original PR: #691
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
mainbranch