Implement the debug_traceCall JSON-RPC endpoint
#605
Conversation
m-Peter
requested review from
janezpodhostnik,
peterargue,
zhangchiqing and
ramtinms
as code owners
m-Peter
force-pushed
the
implement-debug-trace-call-endpoint
branch
from
b12e4f7 to
a01ac34
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
api/debug.go
Outdated
| ) | ||
| } | ||
|
|
||
| func applyStateOverrides(config *tracers.TraceCallConfig, stateDB types.StateDB) error { |
There was a problem hiding this comment.
This is an attempt to implement the original Geth method from here: https://github.com/onflow/go-ethereum/blob/c9c8e533f2f3ae4cad858749edde01b28c789215/internal/ethapi/api.go#L981-L1019
There was a problem hiding this comment.
This is problematic, blockExecutor.StateDB is shared and it should not. I don't think there is any need for the blockExecutor to have a global StateDB and we can use the one from the emulator when running.
There was a problem hiding this comment.
Just some context on this. Both BlockExecutor and its StateDB are short-lived objects. They are created each time debug_traceCall is called, and they have no side-effects outside the scope of debug_traceCall. Any commitments done through the StateDB are not persisted, neither is the BlockExecutor.
We cannot use the emulator, because neither ReadOnlyBlockView nor BlockView expose their StateDB.
The BlockExecutor operates on a BlockView as it needs to execute direct calls, transactions, and dry-run transactions.
We could encapsulate the StateDB of BlockExecutor and move the applyStateOverrides there though.
I have added a test scenario in b7491f9, which verifies that state commitments are not persisted between requests on debug_traceCall.
There was a problem hiding this comment.
I feel like this should be a method on the executor.
There was a problem hiding this comment.
I feel like this should be a method on the executor.
Good point 👍 Updated in 09a1a96
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
m-Peter
force-pushed
the
implement-debug-trace-call-endpoint
branch
from
ae2c9d6 to
ed8284e
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…d contract calls with the remote registers API
…at mutates the state
m-Peter
force-pushed
the
implement-debug-trace-call-endpoint
branch
from
5ea0937 to
772cc5a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
api/debug.go (1)
174-177: Remove the unused variabletxafter unmarshallingThe variable
txis declared and unmarshalled fromtxEncodedbut is not used afterwards. This is unnecessary and could be removed to clean up the code.Apply this diff to remove the unused variable:
-def tx := &gethTypes.Transaction{} -if err := tx.UnmarshalBinary(txEncoded); err != nil { +if err := new(gethTypes.Transaction).UnmarshalBinary(txEncoded); err != nil { return nil, err }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- api/debug.go (3 hunks)
🧰 Additional context used
🔇 Additional comments (4)
api/debug.go (4)
36-64: UpdatedDebugAPIstruct and constructor are well-definedThe
DebugAPIstruct has been appropriately updated with new fields to support theTraceCallfunctionality, and the constructorNewDebugAPIcorrectly initializes these fields. The initialization of fields likeclient,receipts, andratelimiterenhances the capability of theDebugAPI.
128-190: Implementation ofTraceCallmethod is correctThe
TraceCallmethod is implemented correctly, handling rate limiting, transaction encoding, block resolution, executor creation, state overrides, and transaction execution with tracing. Error handling is properly managed at each step. This method enhances the debugging capabilities by allowing tracing of calls with state overrides.
231-260:tracerForReceiptfunction properly initializes tracersThe
tracerForReceiptfunction correctly sets up the tracer context and initializes the appropriate tracer based on the provided configuration. It handles default tracer initialization whenconfig.Traceris nil, ensuring that tracing functionalities work as expected.
191-230:executorAtBlockmethod is well-implementedThe
executorAtBlockmethod accurately retrieves the client for the specified block height, creates a remote ledger, and initializes the block executor. Error handling is thorough and informative, providing clear context in case of failures. This implementation ensures that the execution environment aligns with the specified block state.
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Outside diff range and nitpick comments (13)
services/requester/pool.go (2)
31-31: LGTM: Updated client type to EVM-specific implementationThe changes to the
TxPoolstruct andNewTxPoolfunction, updating the client type to*evm.CrossSporkClient, are consistent with the PR objectives. This modification enhances the specificity of the client used for transaction operations, which is crucial for implementing thedebug_traceCallendpoint and improving EVM support.Consider adding a comment above the
TxPoolstruct to explain the reason for using the EVM-specific client type. This would improve code readability and make the intent clearer for future developers. For example:// TxPool uses an EVM-specific client to support advanced EVM operations, // including the debug_traceCall functionality. type TxPool struct { // ... (existing fields) }Also applies to: 38-38
Line range hint
1-114: Summary: EVM-specific client integration enhances transaction pool capabilitiesThe changes in this file, while minimal, represent a significant step towards supporting advanced EVM operations, including the
debug_traceCallfunctionality. By updating the client type to*evm.CrossSporkClient, theTxPoolis now better equipped to handle EVM-specific transactions and debugging tasks.These modifications align well with the PR objectives and the requirements outlined in the linked issue #530. They lay the groundwork for integrating ERC-4337 and enhancing the EVM Gateway API's capabilities.
As the system evolves to support more advanced EVM features, consider the following:
- Implement comprehensive unit tests for the
TxPoolwith the new EVM-specific client to ensure all functionalities work as expected.- Update relevant documentation to reflect the changes in the transaction pool's behavior and capabilities.
- Monitor the performance impact of these changes, especially in high-load scenarios, to ensure the new client type doesn't introduce any bottlenecks.
- Consider implementing the caching mechanism suggested by user janezpodhostnik in the PR comments to optimize the handling of remote ledger requests.
services/evm/cross-spork_client_test.go (1)
Line range hint
1-145: Consider enhancing test coverage.The current test suite covers essential functionality of the CrossSporkClient, including range validation, continuity checks, and height-based client retrieval. However, consider the following suggestions to enhance test coverage:
- Add edge case tests for boundary conditions (e.g., exactly at the transition points between sporks).
- Include more negative test cases to ensure robust error handling.
- Consider adding tests for concurrent access if the CrossSporkClient is intended to be used in a multi-threaded environment.
- If not already present elsewhere, add integration tests that verify the CrossSporkClient works correctly with the actual blockchain client implementations.
tests/helpers.go (1)
160-160: LGTM. Consider adding flexibility for trace enabling.The addition of
TracesEnabled: trueto theConfigstruct is appropriate for implementing thedebug_traceCallJSON-RPC endpoint. This change aligns well with the PR objectives and enhances the testing capabilities for tracing functionality.For improved flexibility in testing scenarios, consider parameterizing the
TracesEnabledfield. This would allow for easier testing of both trace-enabled and trace-disabled configurations. For example:- TracesEnabled: true, + TracesEnabled: os.Getenv("ENABLE_TRACES") != "false",This change would enable traces by default while allowing them to be disabled via an environment variable when needed.
api/api.go (5)
Line range hint
645-664: LGTM with a suggestion: Consider adding a safety check for int64 castingThe change to use
resolveBlockNumberOrHashis consistent with other methods and improves code consistency. However, castingevmHeighttoint64could potentially lead to issues if the block height exceeds the maximum value of int64 (9,223,372,036,854,775,807).Consider adding a safety check before the casting:
if evmHeight > math.MaxInt64 { return handleError[hexutil.Bytes](fmt.Errorf("block height exceeds maximum int64 value"), l, b.collector) } res, err := b.evm.Call(ctx, tx, from, int64(evmHeight))This will prevent potential overflow issues in the future as the blockchain grows.
829-837: LGTM with a repeated suggestion: Consider adding a safety check for int64 castingThe change to use
resolveBlockNumberOrHashis consistent with other methods and improves code consistency. However, as mentioned in a previous comment, castingevmHeighttoint64could potentially lead to issues if the block height exceeds the maximum value of int64.Consider adding a safety check before the casting, similar to the suggestion for the Call method:
if evmHeight > math.MaxInt64 { return handleError[hexutil.Uint64](fmt.Errorf("block height exceeds maximum int64 value"), l, b.collector) } estimatedGas, err := b.evm.EstimateGas(ctx, tx, from, int64(evmHeight))This will prevent potential overflow issues in the future as the blockchain grows.
858-866: LGTM with a repeated suggestion: Consider adding a safety check for int64 castingThe change to use
resolveBlockNumberOrHashis consistent with other methods and improves code consistency. However, as mentioned in previous comments, castingevmHeighttoint64could potentially lead to issues if the block height exceeds the maximum value of int64.Consider adding a safety check before the casting, similar to the suggestions for the Call and EstimateGas methods:
if evmHeight > math.MaxInt64 { return handleError[hexutil.Bytes](fmt.Errorf("block height exceeds maximum int64 value"), l, b.collector) } code, err := b.evm.GetCode(ctx, address, int64(evmHeight))This will prevent potential overflow issues in the future as the blockchain grows.
983-991: LGTM with a repeated suggestion: Consider adding a safety check for int64 castingThe change to use
resolveBlockNumberOrHashis consistent with other methods and improves code consistency. However, as mentioned in previous comments, castingevmHeighttoint64could potentially lead to issues if the block height exceeds the maximum value of int64.Consider adding a safety check before the casting, similar to the suggestions for other methods:
if evmHeight > math.MaxInt64 { return handleError[hexutil.Bytes](fmt.Errorf("block height exceeds maximum int64 value"), l, b.collector) } result, err := b.evm.GetStorageAt(ctx, address, key, int64(evmHeight))This will prevent potential overflow issues in the future as the blockchain grows.
Nonce comparison still active; recommend tracking removal
The nonce comparison between the network and db values is still present in
api/api.goat line 783. To ensure this temporary safeguard is addressed once confidence in db nonce tracking is achieved, please create a ticket to track its eventual removal.🔗 Analysis chain
Line range hint
763-785: LGTM: Consistent changes and interesting nonce comparisonThe changes to use
resolveBlockNumberOrHashare consistent with other methods, which is good for maintainability. The addition of the nonce comparison between the network and db values is an interesting safeguard.Question: Is there a plan to remove this comparison once we gain confidence in the db nonce tracking? It might be worth adding a TODO comment or creating a ticket to revisit this in the future.
To help track the nonce discrepancies, consider adding the following logging:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Log nonce discrepancies for further analysis grep -n "network nonce does not equal db nonce" api/api.go if [ $? -eq 0 ]; then echo "Nonce comparison is still present in the code. Consider creating a ticket to track its removal." fiLength of output: 331
services/evm/executor.go (3)
26-26: Address the TODO: Consider changingtypes.StateDBtotypes.ReadOnlyViewThe comment suggests updating
types.StateDBtotypes.ReadOnlyView. Implementing this change could enhance safety by enforcing read-only access where mutation is not required.Would you like assistance in implementing this change or opening a GitHub issue to track this task?
149-149: Address the TODO: VerifyDirectCallBaseGasUsagevalueThe comment indicates that the value of
DirectCallBaseGasUsageshould be checked. Verifying this value ensures accurate gas calculations during execution.Would you like assistance in verifying or updating this value?
182-185: Address the TODO: Handle failures when calling Cadence arch precompilesThe TODO highlights that calls using Cadence architecture precompiles may fail when precompiled contracts are not set due to the absence of a receipt. This failure should be detected, and in such cases, a call should be executed against the Execution Node using an Access Node.
Would you like help in implementing this failure detection logic or opening a GitHub issue to track this task?
tests/web3js/debug_traces_test.js (1)
6-243: Consider organizing tests within a 'describe' block for better structureCurrently, the test cases are directly under the global scope. Wrapping them within a
describeblock improves readability and organization, especially as the test suite grows.Suggested change:
describe('Debug Traces Tests', () => { it('should retrieve call traces', async () => { // existing test code }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
- api/api.go (7 hunks)
- api/debug.go (3 hunks)
- api/utils.go (1 hunks)
- bootstrap/bootstrap.go (8 hunks)
- go.mod (3 hunks)
- services/evm/cross-spork_client.go (6 hunks)
- services/evm/cross-spork_client_test.go (2 hunks)
- services/evm/executor.go (1 hunks)
- services/evm/remote_ledger.go (3 hunks)
- services/evm/remote_ledger_test.go (3 hunks)
- services/ingestion/subscriber.go (3 hunks)
- services/ingestion/subscriber_test.go (6 hunks)
- services/requester/pool.go (2 hunks)
- services/requester/requester.go (4 hunks)
- services/requester/requester_test.go (2 hunks)
- tests/e2e_web3js_test.go (1 hunks)
- tests/go.mod (3 hunks)
- tests/helpers.go (1 hunks)
- tests/web3js/debug_traces_test.js (1 hunks)
🧰 Additional context used
🔇 Additional comments (48)
services/evm/remote_ledger_test.go (4)
98-98: Function call updated for consistency.The change from
newRemoteLedgertoNewRemoteLedgeris consistent with the previous change, reflecting the new public nature of theNewRemoteLedgerfunction.
70-70: FunctionNewRemoteLedgeris now exported, verify intention.The change from
newRemoteLedgertoNewRemoteLedgermakes the function public. This is consistent with Go naming conventions for exported functions.Please confirm that making this function public was intentional. If so, ensure that the function's documentation (if any) reflects its public nature and usage. You may want to run the following command to check for any documentation:
#!/bin/bash # Description: Check for documentation of the NewRemoteLedger function # Test: Search for NewRemoteLedger function definition and any preceding comments rg --type go -B 5 'func NewRemoteLedger'
Line range hint
1-98: Summary of changes and potential impact.The changes in this file are minimal and focused on improving naming conventions:
- The package name has been updated to
evm, aligning with the file path.- The
NewRemoteLedgerfunction is now exported (public).These changes improve code consistency and follow Go conventions. However, the newly exported
NewRemoteLedgerfunction may impact how it's used in other parts of the codebase. Ensure that this change is intentional and that any necessary documentation or usage guidelines are updated accordingly.To ensure these changes don't introduce any unintended side effects, please run the following command to check for any other occurrences of
newRemoteLedgerthat might need to be updated:#!/bin/bash # Description: Check for any remaining occurrences of newRemoteLedger # Test: Search for newRemoteLedger in all Go files rg --type go 'newRemoteLedger'
1-1: Package name change looks good, verify imports.The package name change from
requestertoevmaligns well with the file path. This is a good practice for better code organization.Please run the following script to check if there are any remaining imports of the old package name:
✅ Verification successful
Package name change verified, no remaining imports of 'requester' found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining imports of the old package name # Test: Search for imports of the old package name rg --type go 'import.*".*requester"'Length of output: 363
services/requester/pool.go (1)
17-17: LGTM: Import statement added for EVM supportThe addition of the
evmpackage import is consistent with the changes made to theTxPoolstruct andNewTxPoolfunction. This change aligns well with the PR objectives of implementing thedebug_traceCallJSON-RPC endpoint and enhancing EVM support.services/evm/cross-spork_client_test.go (2)
1-1: Package name change looks good.The package name has been updated from
requestertoevm, which aligns with the file path and likely reflects a broader restructuring of the codebase.
Line range hint
145-145: Error message check looks appropriate.The error message being verified ("invalid height not in available range: 10") is consistent with the expected behavior when an out-of-range height is provided to the client. This test case helps ensure that the client properly handles invalid input.
services/evm/cross-spork_client.go (6)
169-169: LGTM! Method call update is correct.The update from
getClientForHeighttoGetClientForHeightis consistent with the earlier method renaming. The logic of the method remains unchanged, which is good.
185-185: LGTM! Method call update is correct.The update from
getClientForHeighttoGetClientForHeightis consistent with the earlier method renaming. The logic of the method remains unchanged, which is good.
198-198: LGTM! Method call updates are correct.The updates from
getClientForHeighttoGetClientForHeightin bothExecuteScriptAtBlockHeightandSubscribeEventsByBlockHeightmethods are consistent with the earlier method renaming. The logic of both methods remains unchanged, which is good.Also applies to: 211-211
149-149: LGTM! Method visibility change looks good.The renaming of
getClientForHeighttoGetClientForHeightmakes the method publicly accessible, which is consistent with Go naming conventions for exported methods. The logic of the method remains unchanged, which is good.Please run the following script to ensure all usages of this method have been updated:
#!/bin/bash # Description: Check for any remaining usages of the old method name # Test: Search for usages of the old method name rg --type go 'getClientForHeight'
Line range hint
1-211: Overall changes look good, but consider potential impacts.The changes in this file are consistent and well-implemented. The main modifications are:
- Changing the package name from
requestertoevm.- Renaming
getClientForHeighttoGetClientForHeight, making it an exported method.- Updating all internal calls to use the new method name.
These changes improve the package structure and method visibility. However, please consider the following:
- Ensure that all files importing the old package name (
requester) are updated to use the new package name (evm).- Verify that any external packages or services that might have been using
getClientForHeightare updated to use the new exported methodGetClientForHeight.- Update any documentation or API references to reflect these changes.
To ensure a smooth transition, please run the following script to check for any remaining references to the old package name or method name:
#!/bin/bash # Description: Check for any remaining references to old package or method names # Test 1: Search for imports of the old package name echo "Checking for old package imports:" rg --type go 'import.*".*requester"' # Test 2: Search for usages of the old method name echo "Checking for old method name usage:" rg --type go 'getClientForHeight'
1-1: LGTM! Package name change looks good.The package name change from
requestertoevmis appropriate for the content of this file. It better reflects the EVM-related functionality.Please run the following script to ensure all import statements referencing this package have been updated:
✅ Verification successful
Import Statements Updated Successfully
All import statements referencing the old package name
requesterhave been successfully updated toevm. The old method namegetClientForHeightis only found in comments, which does not affect the functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining imports of the old package name # Test: Search for imports of the old package name rg --type go 'import.*".*requester"'Length of output: 359
tests/e2e_web3js_test.go (1)
Line range hint
1-34: Comprehensive test suite with well-integrated new test case.The test suite in
TestWeb3_E2Eis well-structured and covers a wide range of Web3 functionalities. The addition of the "test transaction traces" case enhances the coverage by including debugging capabilities, which aligns well with the PR objectives. The new test case is seamlessly integrated without disrupting the existing tests.This comprehensive approach to testing will help ensure the reliability and correctness of the Web3 implementation, including the new
debug_traceCallendpoint.services/requester/requester_test.go (2)
18-18: LGTM: Import statement added correctlyThe new import for the
evmpackage is correctly added and is necessary for the changes in thecreateEVMfunction.
Line range hint
1-230: Note: Existing tests remain validThe changes made to the import statement and the
createEVMfunction do not affect the existing test cases. All tests should continue to function as before, as they depend on the behavior of the returned EVM object, not its internal creation process.services/ingestion/subscriber.go (4)
13-13: LGTM: Import statement added for evm packageThe addition of the import statement for the
evmpackage is consistent with the change fromrequester.CrossSporkClienttoevm.CrossSporkClient. This import is necessary for using the new client type.
33-33: LGTM: RPCSubscriber struct updated to use evm.CrossSporkClientThe
clientfield in theRPCSubscriberstruct has been updated to use*evm.CrossSporkClient. This change is consistent with the import modification and aligns with the refactoring to use theevmpackage.
43-43: LGTM: NewRPCSubscriber function updated to accept evm.CrossSporkClientThe
NewRPCSubscriberfunction has been updated to accept*evm.CrossSporkClientinstead of*requester.CrossSporkClient. This change is consistent with the struct field modification and the overall refactoring to use theevmpackage.To ensure that this change has been consistently applied throughout the codebase, please run the following verification script:
✅ Verification successful
Verified: Consistent use of evm.CrossSporkClient confirmed
All instances of
NewRPCSubscribernow correctly use*evm.CrossSporkClient, and there are no remaining references torequester.CrossSporkClientin the codebase. The function signature change has been successfully applied throughout.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all calls to NewRPCSubscriber use evm.CrossSporkClient # Test: Search for NewRPCSubscriber calls echo "Searching for NewRPCSubscriber calls:" rg --type go -n 'NewRPCSubscriber\s*\(' -A 3 # Test: Check if there are any remaining references to requester.CrossSporkClient echo "Checking for any remaining references to requester.CrossSporkClient:" rg --type go -n 'requester\.CrossSporkClient'Length of output: 1183
Line range hint
1-385: Overall LGTM: Consistent refactoring to use evm.CrossSporkClientThe changes in this file are part of a larger refactoring to use the
evmpackage instead ofrequester. The modifications are consistent and well-applied within this file. However, to ensure the integrity of the entire codebase, it's crucial to verify that this refactoring has been consistently implemented across all relevant files.To ensure the consistency of this refactoring across the codebase, please run the following verification script:
This script will help identify any inconsistencies in the refactoring process and ensure that the change has been applied uniformly across the project.
✅ Verification successful
Verification Successful: Consistent Refactoring to
evm.CrossSporkClientConfirmedThe refactoring to replace the
requesterpackage with theevmpackage has been successfully applied across the codebase. No remaining imports or usages of therequesterpackage were found. Theevm.CrossSporkClientis consistently used in the following files:
services/requester/pool.goservices/ingestion/subscriber.gobootstrap/bootstrap.goapi/debug.go🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the consistency of the evm package refactoring across the codebase # Test: Check for any remaining imports of the requester package echo "Checking for any remaining imports of the requester package:" rg --type go -n '^import\s+\(.*"github.com/onflow/flow-evm-gateway/services/requester".*\)' -A 10 # Test: Check for any remaining uses of requester.CrossSporkClient echo "Checking for any remaining uses of requester.CrossSporkClient:" rg --type go -n 'requester\.CrossSporkClient' # Test: Verify that evm.CrossSporkClient is used consistently echo "Verifying consistent use of evm.CrossSporkClient:" rg --type go -n 'evm\.CrossSporkClient'Length of output: 1183
go.mod (3)
10-10: Approval: Addition of uint256 libraryThe addition of
github.com/holiman/uint256 v1.3.0as a direct dependency is appropriate for this PR. This library provides efficient 256-bit integer operations, which are crucial for implementing Ethereum-related functionality like thedebug_traceCallendpoint. It aligns well with the PR objectives and will likely improve the handling of large numbers in blockchain operations.
63-64: Note: New indirect dependencies addedSeveral new indirect dependencies have been introduced:
github.com/dlclark/regexp2 v1.7.0github.aaakk.us.kg/dop251/goja v0.0.0-20230806174421-c933cf95e127github.aaakk.us.kg/go-sourcemap/sourcemap v2.1.3+incompatiblegithub.aaakk.us.kg/google/pprof v0.0.0-20231229205709-960ae82b1e42These additions suggest that the project now includes functionality related to advanced regular expressions, JavaScript execution, source mapping, and profiling. While these don't require direct action, be aware that they may impact the project's size and build time. Ensure that these dependencies are necessary for the implementation of the
debug_traceCallendpoint or other related features.Also applies to: 80-80, 87-87
Line range hint
1-233: Verify compatibility and run tests with new dependenciesThe changes to the
go.modfile are focused and align well with the PR objectives of implementing thedebug_traceCallendpoint. The additions are minimal and targeted, which is good for maintaining project stability. However, to ensure these changes don't introduce any conflicts or issues:
- Verify that all new and updated dependencies are compatible with the existing codebase.
- Run the full test suite to catch any potential integration issues.
- Consider updating the project's documentation to reflect the new capabilities enabled by these dependencies.
To help verify the impact of these changes, you can run the following commands:
This script will ensure that the
go.modfile is up to date, verify the integrity of the dependencies, and run all tests in the project.tests/helpers.go (1)
Line range hint
1-489: Overall assessment: The change enhances testing capabilities without disrupting existing functionality.The addition of the
TracesEnabledfield to theConfigstruct is the only change in this file. It aligns well with the PR objectives of implementing thedebug_traceCallJSON-RPC endpoint. The rest of the file, containing various helper functions for test setup and execution, remains unchanged and appears to provide robust support for testing the EVM Gateway.services/ingestion/subscriber_test.go (6)
15-15: LGTM: Import statement updated correctly.The import statement for the
evmpackage has been added, which is consistent with the refactoring mentioned in the summary. This change aligns with the shift in the source of theCrossSporkClient.
Line range hint
78-82: LGTM: CrossSporkClient instantiation updated consistently.The
CrossSporkClientinstantiation in theTest_MissingBlockEventfunction has been updated to use theevmpackage, maintaining consistency with the previous changes. The method signature and parameters remain unchanged, preserving the expected functionality.
Line range hint
180-184: LGTM: CrossSporkClient instantiation updated consistently.The
CrossSporkClientinstantiation in theTest_SubscribingWithRetryOnErrorfunction has been updated to use theevmpackage, maintaining consistency with the previous changes. The method signature and parameters remain unchanged, preserving the expected functionality.
Line range hint
1-500: Summary: Consistent refactoring of CrossSporkClient instantiationThe changes in this file are part of a larger refactoring effort to move the
CrossSporkClientfrom therequesterpackage to theevmpackage. All instances ofCrossSporkClientinstantiation have been updated consistently throughout the file. The changes are straightforward and do not alter the underlying logic or functionality of the tests.Key points:
- Import statement updated to include the
evmpackage.- All
NewCrossSporkClientcalls now use theevmpackage instead ofrequester.- Method signatures and parameters remain unchanged, preserving the expected functionality.
These changes align with the PR objectives and the AI-generated summary. The refactoring should improve the overall structure of the codebase without introducing any functional changes to the tests.
Line range hint
38-42: LGTM: CrossSporkClient instantiation updated correctly.The
CrossSporkClientis now instantiated from theevmpackage, which is consistent with the import statement change. The method signature and parameters remain the same, suggesting that the functionality should be preserved.To ensure that this change doesn't introduce any unintended side effects, please run the following verification script:
✅ Verification successful
Verified: All instances of
NewCrossSporkClientare consistently updated to use theevmpackage.No issues found with the
CrossSporkClientinstantiation across theservices/directory.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all instances of CrossSporkClient are updated consistently rg --type go 'NewCrossSporkClient' services/Length of output: 808
Line range hint
243-247: LGTM: CrossSporkClient instantiation updated consistently.The
CrossSporkClientinstantiation in theTest_SubscribingWithRetryOnErrorMultipleBlocksfunction has been updated to use theevmpackage, maintaining consistency with the previous changes. The method signature and parameters remain unchanged, preserving the expected functionality.To ensure that all instances of
CrossSporkClienthave been updated consistently throughout the codebase, please run the following verification script:api/api.go (3)
283-291: LGTM: Improved consistency in block number/hash resolutionThe change to use
resolveBlockNumberOrHashinstead ofgetBlockNumberis a good refactoring that improves consistency across the API. This standardization will likely make the code more maintainable and reduce the chance of inconsistencies between different methods.
Line range hint
1-1200: LGTM: Improved consistency with resolveBlockNumberOrHash refactoringThe removal of the
getBlockNumbermethod and the introduction ofresolveBlockNumberOrHashacross multiple methods is a positive refactoring. This change improves consistency in handling block numbers and hashes throughout the API, which will likely lead to better maintainability and reduced code duplication.Great job on this refactoring effort!
Line range hint
1-1200: Summary of changes and suggestions
- The refactoring to use
resolveBlockNumberOrHashacross multiple methods greatly improves consistency and maintainability.- There's a potential issue with casting
evmHeighttoint64in several methods, which could lead to overflow problems as the blockchain grows. Consider adding safety checks as suggested in individual comments.- The nonce comparison in
GetTransactionCountis an interesting addition. Consider adding a plan or ticket to revisit this once confidence in db nonce tracking is established.Overall, these changes represent a significant improvement in code quality and consistency. Great work on the refactoring effort!
api/utils.go (1)
1-31: Implementation ofresolveBlockNumberOrHashlooks goodThe
resolveBlockNumberOrHashfunction correctly resolves the block number or hash and handles the error cases appropriately.api/debug.go (3)
189-190: Handle the error returned bytracer.GetResult()The
GetResult()method may return an error, which is currently being ignored. To ensure proper error handling, capture and check the error returned byGetResult().
179-182: Ensureconfigis not nil before applying state overridesPassing a
nilconfigtoblockExecutor.ApplyStateOverrides(config)may cause a nil pointer dereference within the method. Ensure that the method handlesconfigbeingnilor add a nil check before calling it.
143-147: Defaultingfromaddress to zero addressDefaulting the
fromaddress to the zero address when it's not provided is consistent with the Geth implementation. Ensure that this behavior is intentional and documented, as transactions executed from the zero address may have different implications.tests/web3js/debug_traces_test.js (1)
179-181: Verify the correctness of the 'stateOverrides' configurationThe
stateOverridesobject usesstateDiffto override state values. Ensure that the key corresponds to the correct storage slot and that the value is correctly formatted. Additionally, confirm that the client being used supportsstateOverridesindebug_traceCall.Run the following script to check if
stateOverridesare correctly applied:bootstrap/bootstrap.go (6)
Line range hint
14-25: Added necessary imports for updated types and error handling.The imports for
gethTypesanderrspackages have been added to support the use of Ethereum transaction and log types, as well as custom error handling.
51-51: Updatedclientfield inBootstrapstruct to new client type.The
clientfield in theBootstrapstruct has been updated to use the new*evm.CrossSporkClienttype, aligning with the updated client implementation.
284-293: UpdateddebugAPIinitialization with additional parameters.The
debugAPIis now initialized with additional parameters, ensuring proper setup of theDebugAPIwith all required dependencies. This change supports the newdebug_traceCallfunctionality.
443-443: Initializedevm.CrossSporkClientinsetupCrossSporkClient.The creation of the
clientusingevm.NewCrossSporkClientis appropriate and aligns with the new client structure. This change ensures that cross-spork capabilities are correctly handled.
460-460: UpdatedsetupStoragefunction signature with new client type.The
setupStoragefunction now acceptsclient *evm.CrossSporkClientas a parameter, reflecting the updated client type. This change ensures compatibility with the new client implementation.
517-519: Conditionally disable trace downloader for emulator environment.The added conditional check correctly disables the trace downloader when running in the emulator environment (
flow.Emulator), preventing E2E test failures as noted in the comment.services/requester/requester.go (4)
36-36: LGTMThe import statement for
gwEvmis correct and necessary for the updated functionality.
456-456: Duplicate Comment: Enhance Error Handling forNewRemoteLedgerThe existing review comment regarding error handling for
gwEvm.NewRemoteLedgeris still valid. Please refer to the previous feedback and ensure that specific errors are properly handled.
139-139: Update calls toNewEVMwith the newclienttypeEnsure that all calls to the
NewEVMfunction pass the new*gwEvm.CrossSporkClienttype as theclientparameter.#!/bin/bash # Description: Find all calls to `NewEVM` and verify the correct `client` type is used. # Test: Search for calls to `NewEVM` in the codebase and display the arguments. rg --type go 'NewEVM\(' -A 3
122-122: Verify all references toclientare updatedEnsure that all references to the
clientfield within theEVMstruct and its methods have been updated to use the new*gwEvm.CrossSporkClienttype.
| @@ -89,19 +89,19 @@ func Test_CrossSpork(t *testing.T) { | |||
| ) | |||
| require.NoError(t, err) | |||
|
|
|||
| c, err := client.getClientForHeight(150) | |||
| c, err := client.GetClientForHeight(150) | |||
There was a problem hiding this comment.
💡 Codebase verification
Remaining instances of getClientForHeight found. Please update to GetClientForHeight.
services/evm/cross-spork_client.goservices/evm/cross-spork_client_test.goapi/debug.go
🔗 Analysis chain
Method call updates look good, verify impact on other parts of the codebase.
The method calls have been consistently updated from getClientForHeight to GetClientForHeight, reflecting the change in the method's visibility from private to public. This change aligns with the updated method signature mentioned in the summary.
To ensure this change has been applied consistently across the codebase, please run the following script:
Also applies to: 96-96, 100-100, 104-104
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining instances of getClientForHeight and verify GetClientForHeight usage
# Search for any remaining instances of getClientForHeight
echo "Searching for any remaining instances of getClientForHeight:"
rg --type go 'getClientForHeight'
# Verify usage of GetClientForHeight
echo "Verifying usage of GetClientForHeight:"
rg --type go 'GetClientForHeight'
Length of output: 1270
| t.Run("test transaction traces", func(t *testing.T) { | ||
| runWeb3Test(t, "debug_traces_test") | ||
| }) |
There was a problem hiding this comment.
💡 Codebase verification
Implementation of debug_traces_test Not Found
The test case "test transaction traces" invokes runWeb3Test(t, "debug_traces_test"), but the implementation of debug_traces_test was not found in the codebase. Please ensure that the corresponding test implementation is added to avoid potential test failures.
🔗 Analysis chain
LGTM! Verify the implementation of debug_traces_test.
The addition of the "test transaction traces" test case is well-placed within the existing test suite and follows the established pattern. This new test aligns with the PR objective of adding support for the debug_traceCall endpoint.
To ensure the completeness of this test, please run the following script to verify the implementation of debug_traces_test:
This will help confirm that the actual test implementation exists and is properly defined.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of debug_traces_test
# Test: Search for the implementation of debug_traces_test
rg --type js -A 10 $'debug_traces_test'
Length of output: 38
Script:
#!/bin/bash
# Description: Verify the implementation of debug_traces_test in Go files
# Search for the implementation of debug_traces_test in Go files
rg --type go -A 10 debug_traces_test
Length of output: 563
Script:
#!/bin/bash
# Description: Search for the implementation of debug_traces_test across all files
# Search for the implementation of debug_traces_test in the entire codebase
rg debug_traces_test
Length of output: 85
| @@ -214,7 +215,7 @@ func createEVM(t *testing.T, cache *expirable.LRU[string, cadence.Value], mockCl | |||
| networkID := flowGo.Emulator | |||
| log := zerolog.New(zerolog.NewTestWriter(t)) | |||
|
|
|||
| client, err := NewCrossSporkClient(mockClient, nil, log, networkID) | |||
| client, err := evm.NewCrossSporkClient(mockClient, nil, log, networkID) | |||
There was a problem hiding this comment.
💡 Codebase verification
Update Required: Inconsistent usage of NewCrossSporkClient
The search identified that NewCrossSporkClient is still used without the evm package prefix in the following files:
services/evm/cross-spork_client_test.goservices/evm/cross-spork_client.go
Please update these instances to use evm.NewCrossSporkClient to ensure consistency across the codebase.
🔗 Analysis chain
LGTM: Updated client creation to use evm package
The modification to use evm.NewCrossSporkClient is correct and consistent with the added import statement. This change appears to be part of a refactoring to improve the package structure.
To ensure this change is consistent throughout the codebase, please run the following script:
This will help identify any remaining instances of NewCrossSporkClient that haven't been updated to use the evm package.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of NewCrossSporkClient across the codebase
# Test: Search for any remaining usage of NewCrossSporkClient without the evm package
echo "Searching for potentially outdated NewCrossSporkClient usage:"
rg --type go 'NewCrossSporkClient\(' --glob '!**/requester_test.go'
# Test: Confirm the usage of evm.NewCrossSporkClient in other files
echo "Confirming usage of evm.NewCrossSporkClient:"
rg --type go 'evm\.NewCrossSporkClient\('
Length of output: 1512
| github.com/dlclark/regexp2 v1.7.0 // indirect | ||
| github.com/docker/go-units v0.5.0 // indirect | ||
| github.com/dop251/goja v0.0.0-20230806174421-c933cf95e127 // indirect |
There was a problem hiding this comment.
💡 Codebase verification
Remove Unused Dependencies to Clean Up go.mod.
The following new dependencies are not used in the project and can be safely removed:
github.com/dlclark/regexp2 v1.7.0 // indirectgithub.aaakk.us.kg/dop251/goja v0.0.0-20230806174421-c933cf95e127 // indirectgithub.aaakk.us.kg/go-sourcemap/sourcemap v2.1.3+incompatible // indirect
Removing these will help keep the go.mod file clean and reduce unnecessary dependencies.
🔗 Analysis chain
New dependencies added: Approve with observations.
The changes to the go.mod file look good. Four new indirect dependencies have been added:
github.com/dlclark/regexp2 v1.7.0github.aaakk.us.kg/dop251/goja v0.0.0-20230806174421-c933cf95e127github.aaakk.us.kg/go-sourcemap/sourcemap v2.1.3+incompatiblegithub.aaakk.us.kg/google/pprof v0.0.0-20231229205709-960ae82b1e42
These additions seem to be related to the implementation of the debug_traceCall JSON-RPC endpoint, which is the main objective of this PR. The goja library, for instance, is a JavaScript engine for Go, which could be useful for executing and tracing smart contracts. The pprof package is likely added for profiling purposes, which aligns with the debugging capabilities being introduced.
To ensure these dependencies are actually used in the project, let's run a quick check:
This will help verify that the new dependencies are indeed being utilized in the project.
Also applies to: 80-80, 89-89
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for usage of new dependencies in the project
echo "Checking for usage of new dependencies:"
rg --type go "regexp2|goja|sourcemap|pprof" ..
Length of output: 317
| height := number.Int64() | ||
|
|
||
| // if special values (latest) we return latest executed height | ||
| if height < 0 { | ||
| executed, err := blocksDB.LatestEVMHeight() | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
| height = int64(executed) | ||
| } | ||
|
|
||
| return uint64(height), nil |
There was a problem hiding this comment.
Avoid potential integer overflow when casting executed from uint64 to int64
In the resolveBlockNumber function, casting executed (of type uint64) to int64 can cause integer overflow if executed exceeds math.MaxInt64. This could lead to incorrect block heights being returned.
Apply this diff to prevent the integer overflow:
func resolveBlockNumber(
number rpc.BlockNumber,
blocksDB storage.BlockIndexer,
) (uint64, error) {
- height := number.Int64()
+ var height uint64
// if special values (latest) we return latest executed height
- if height < 0 {
+ if number.Int64() < 0 {
executed, err := blocksDB.LatestEVMHeight()
if err != nil {
return 0, err
}
- height = int64(executed)
+ height = executed
} else {
- height = number.Int64()
+ if number.Int64() < 0 {
+ return 0, fmt.Errorf("%w: invalid negative block number", errs.ErrInvalid)
+ }
+ height = uint64(number.Int64())
}
return height, nil
}This change ensures that:
- The
heightvariable is of typeuint64, matching the expected return type and avoiding unnecessary casting. - Negative block numbers (other than special cases) are explicitly checked and an error is returned if encountered.
Committable suggestion was skipped due to low confidence.
| let updateTrace = response.body.result | ||
| assert.equal(updateTrace.from, '0xfacf71692421039876a5bb4f10ef7a439d8ef61e') | ||
| assert.equal(updateTrace.gas, '0x95ab') | ||
| assert.equal(updateTrace.gasUsed, '0x72c3') | ||
| assert.equal(updateTrace.to, '0x99a64c993965f8d69f985b5171bc20065cc32fab') | ||
| assert.equal( | ||
| updateTrace.input, | ||
| '0x6057361d0000000000000000000000000000000000000000000000000000000000000064' | ||
| ) | ||
| assert.equal(updateTrace.value, '0x0') | ||
| assert.equal(updateTrace.type, 'CALL') | ||
|
|
There was a problem hiding this comment.
Avoid hardcoding addresses and dynamic values in assertions
Hardcoding addresses, gas values, and other dynamic data may cause tests to fail in different environments or when configurations change. Consider using variables or retrieving these values from the context to make tests more robust.
Suggested change:
Replace hardcoded values with variables derived from the test setup. For example:
assert.equal(updateTrace.from, conf.eoa.address);
assert.equal(updateTrace.gas, traceCall.gas);
assert.equal(updateTrace.gasUsed, response.body.result.gasUsed); // or store expected gasUsed if consistent
assert.equal(updateTrace.to, contractAddress);
assert.equal(updateTrace.input, callData);| assert.equal(callTrace.gas, '0x75ab') | ||
| assert.equal(callTrace.gasUsed, '0x664b') | ||
| assert.equal(callTrace.to, '0x99a64c993965f8d69f985b5171bc20065cc32fab') | ||
| assert.equal(callTrace.input, '0x2e64cec1') | ||
| assert.equal( | ||
| callTrace.output, | ||
| '0x0000000000000000000000000000000000000000000000000000000000000539' | ||
| ) | ||
| assert.equal(callTrace.value, '0x0') | ||
| assert.equal(callTrace.type, 'CALL') | ||
|
|
There was a problem hiding this comment.
Avoid hardcoding dynamic values in assertions
Similar to previous comments, hardcoding values like gas, gasUsed, output, and value can lead to brittle tests. Use variables or constants that are dynamically obtained during the test execution.
Suggested change:
Use variables from earlier in the code:
assert.equal(callTrace.gas, traceCall.gas);
assert.equal(callTrace.gasUsed, response.body.result.gasUsed);
assert.equal(callTrace.to, contractAddress);
assert.equal(callTrace.input, callData);For the output, if the expected output can be predicted based on inputs, calculate it within the test rather than hardcoding the value.
Also applies to: 192-202, 224-228, 238-242
| let jsTracer = '{hist: {}, nops: 0, step: function(log, db) { var op = log.op.toString(); if (this.hist[op]){ this.hist[op]++; } else { this.hist[op] = 1; } this.nops++; }, fault: function(log, db) {}, result: function(ctx) { return this.hist; }}' | ||
| response = await helpers.callRPCMethod( | ||
| 'debug_traceCall', | ||
| [traceCall, 'latest', { tracer: jsTracer }] | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor the JavaScript tracer for improved readability
Embedding complex JavaScript code as a string reduces readability and maintainability. Define the tracer as a JavaScript object or function and serialize it when needed.
Suggested change:
const jsTracerObj = {
hist: {},
nops: 0,
step: function (log, db) {
var op = log.op.toString();
if (this.hist[op]) {
this.hist[op]++;
} else {
this.hist[op] = 1;
}
this.nops++;
},
fault: function (log, db) {},
result: function (ctx) {
return this.hist;
},
};
const jsTracer = JSON.stringify(jsTracerObj);| let latestHeight = await web3.eth.getBlockNumber() | ||
|
|
||
| // Assert value on previous block | ||
| response = await helpers.callRPCMethod( | ||
| 'debug_traceCall', | ||
| [traceCall, web3.utils.toHex(latestHeight - 1n), callTracer] | ||
| ) |
There was a problem hiding this comment.
Ensure consistent data types when handling block numbers
The latestHeight obtained from web3.eth.getBlockNumber() may be a Number or a String. Subtracting 1n (a BigInt) from it could cause a type error. Ensure that arithmetic operations are performed on numbers of the same type.
Suggested change:
If latestHeight is a Number:
traceCallParams = [traceCall, web3.utils.toHex(latestHeight - 1), callTracer];If latestHeight is a String:
const previousBlock = parseInt(latestHeight, 10) - 1;
traceCallParams = [traceCall, web3.utils.toHex(previousBlock), callTracer];| let res = await helpers.signAndSend({ | ||
| from: conf.eoa.address, | ||
| to: contractAddress, | ||
| data: updateData, | ||
| value: '0', | ||
| gasPrice: conf.minGasPrice, | ||
| }) | ||
| assert.equal(res.receipt.status, conf.successStatus) |
There was a problem hiding this comment.
Handle asynchronous operations appropriately
When sending a transaction using helpers.signAndSend, ensure that any necessary awaits are correctly applied and that you handle potential errors.
Suggested change:
Ensure that the transaction receipt is awaited and errors are caught:
try {
let res = await helpers.signAndSend({
from: conf.eoa.address,
to: contractAddress,
data: updateData,
value: '0',
gasPrice: conf.minGasPrice,
});
assert.equal(res.receipt.status, conf.successStatus);
} catch (error) {
assert.fail(`Transaction failed: ${error.message}`);
}
|
Closing in favor of: #635 |
Closes: #530
Description
Example usage:
{ "jsonrpc": "2.0", "id": 4, "result": { "from": "0x658bdf435d810c91414ec09147daa6db62406379", "gas": "0x76c0", "gasUsed": "0x624d", "to": "0x99466ed2e37b892a2ee3e9cd55a98b68f5735db2", "input": "0xc6888fa10000000000000000000000000000000000000000000000000000000000000006", "output": "0x000000000000000000000000000000000000000000000000000000000000002a", "value": "0x0", "type": "CALL" } }And:
{ "jsonrpc": "2.0", "id": 4, "result": { "gas": 25165, "failed": false, "returnValue": "000000000000000000000000000000000000000000000000000000000000002a", "structLogs": [ { "pc": 0, "op": "PUSH1", "gas": 9196, "gasCost": 3, "depth": 1, "stack": [] }, { "pc": 2, "op": "PUSH1", "gas": 9193, "gasCost": 3, "depth": 1, "stack": [ "0x60" ] }, { "pc": 4, "op": "MSTORE", "gas": 9190, "gasCost": 12, "depth": 1, "stack": [ "0x60", "0x40" ] }, ... ] } }For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
evmpackage.Tests