Problem: logs in callback contract are lost #1233
Conversation
Closes: crypto-org-chain#1231 Solution: - re-emit those logs
yihuang
requested review from
JayT106 and
calvinaco
and removed request for
a team
WalkthroughThe changes encompass updates to the Changes
TipsChat with CodeRabbit Bot (
|
Signed-off-by: yihuang <huang@crypto.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- CHANGELOG.md (1 hunks)
- integration_tests/test_ica_precompile.py (1 hunks)
- x/cronos/keeper/keeper.go (2 hunks)
- x/cronos/keeper/precompiles/utils.go (1 hunks)
Files skipped from review due to trivial changes (2)
- integration_tests/test_ica_precompile.py
- x/cronos/keeper/precompiles/utils.go
Additional comments: 4
CHANGELOG.md (2)
12-14: The code for setting labels and title for the plot is correct and follows best practices.
8-14: > Note: This review was outside of the patch, but no patch was found that overlapped with it. Original lines [19-19]
The filter_data function is being used correctly to filter the data based on the 'Category' column value of 'Electronics'.
x/cronos/keeper/keeper.go (2)
23-28: The import statement for the "github.com/ethereum/go-ethereum/core/vm" package has been added. Ensure that this package is included in your project dependencies.
299-314: The
onPacketResultfunction has been modified to handle the response from the EVM call. If the call is successful, it retrieves the stateDB from the context and adds the logs from the response to the stateDB. The function now also returns an error if the EVM call fails. Ensure that the error handling is consistent with the rest of your codebase.+ if stateDB, ok := ctx.Value("statedb").(vm.StateDB); ok { + for _, l := range rsp.Logs { + stateDB.AddLog(l.ToEthereum()) + } + }
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional comments: 2
CHANGELOG.md (2)
12-14: The code for setting labels and title for the plot is correct and follows best practices.
8-14: > Note: This review was outside of the patch, but no patch was found that overlapped with it. Original lines [19-19]
The code correctly filters the data based on the 'Category' column value of 'Electronics'.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1233 +/- ##
==========================================
- Coverage 36.95% 36.93% -0.02%
==========================================
Files 115 115
Lines 10235 10239 +4
==========================================
Hits 3782 3782
- Misses 6080 6083 +3
- Partials 373 374 +1
|
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- integration_tests/test_ica_precompile.py (1 hunks)
- x/cronos/keeper/keeper.go (2 hunks)
- x/cronos/keeper/precompiles/utils.go (2 hunks)
- x/cronos/types/keys.go (1 hunks)
Files skipped from review due to trivial changes (3)
- integration_tests/test_ica_precompile.py
- x/cronos/keeper/precompiles/utils.go
- x/cronos/types/keys.go
Additional comments: 2
x/cronos/keeper/keeper.go (2)
23-28: The import statement for the "github.com/ethereum/go-ethereum/core/vm" package is added correctly.
299-314: The
onPacketResultfunction is updated to handle the response from the EVM call. It retrieves the stateDB from the context and adds the logs from the response to the stateDB. The function now returns an error if the EVM call fails. This is a good practice as it ensures that the logs from the EVM call are not lost.+ if stateDB, ok := ctx.Value(types.StateDBContextKey).(vm.StateDB); ok { + for _, l := range rsp.Logs { + stateDB.AddLog(l.ToEthereum()) + } + }
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- integration_tests/test_ica_precompile.py (4 hunks)
Additional comments: 5
integration_tests/test_ica_precompile.py (5)
1-10: The new imports seem to be used in the code and are correctly placed at the top of the file. Ensure that these modules are included in the project dependencies.
48-59: The
ibcfixture has been updated to include therelayerparameter when calling theprepare_networkfunction. Ensure that theprepare_networkfunction can handle this new argument.190-196: The
test_sc_callfunction is setting up the test environment correctly. Ensure that theCONTRACTandCONTRACTS["TestICA"]values are correct and that the contract is deployed successfully.312-386: The
test_sc_callfunction is checking the logs emitted in the callback. Ensure that the expected logs match the actual logs produced by the system.389-391: The
assert_logs_equalfunction is a helper function to compare logs. It seems to be correctly implemented usingitertools.zip_longestto handle cases where the two lists are not of the same length.
|
integration test passed. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- x/cronos/keeper/evm.go (2 hunks)
- x/cronos/keeper/keeper.go (1 hunks)
Additional comments: 2
x/cronos/keeper/evm.go (2)
8-14: The import statement for the "github.com/ethereum/go-ethereum/core/vm" package is added. Ensure that this package is included in your dependencies.
36-45: The new block of code checks if the call originates from an ibc-in precompiled message. If it does, it re-emits the logs into the original stateDB. This is a good practice to prevent the loss of logs. However, ensure that the
StateDBContextKeyis correctly set in the context before this function is called.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (3)
- go.mod
- go.sum
- gomod2nix.toml
Files selected for processing (1)
- x/cronos/keeper/evm.go (2 hunks)
Files skipped from review due to trivial changes (1)
- x/cronos/keeper/evm.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- integration_tests/test_ica_precompile.py (7 hunks)
Additional comments: 4
integration_tests/test_ica_precompile.py (4)
3-9: The import statement for
pystarport.clusterhas been added. Ensure that thepystarportpackage is installed and available in the environment where these tests will be run.48-57: The
prepare_networkfunction now accepts an additional argumentrelayer. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.187-189: The
assert_packet_eventfunction has been added. This function checks if the logs match the expected sequence and status. This is a good practice as it helps in validating the behavior of the system.319-324: The test cases have been updated to include assertions for packet events. This is a good practice as it helps in validating the behavior of the system.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- integration_tests/test_ica_precompile.py (7 hunks)
Additional comments: 4
integration_tests/test_ica_precompile.py (4)
3-8: The import statement for
pystarport.clusterhas been added. Ensure that thepystarportpackage is installed and available in the environment where this script will be run.48-57: The
prepare_networkfunction now accepts an additional argumentrelayer. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.187-190: The new function
assert_packet_resulthas been added. This function checks the logs of an event and asserts that the first log's arguments match the expected sequence number and status. This function seems to be used for testing and debugging purposes.317-321: Assertions have been added to check the result of packet submission and the status of the packet. These assertions are important for validating the behavior of the system under test.
This reverts commit 25c3fee.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- integration_tests/test_ica_precompile.py (7 hunks)
Additional comments: 4
integration_tests/test_ica_precompile.py (4)
3-9: The import statement for
pystarport.clusterhas been added. Ensure that this module is available in the project's dependencies.46-57: The
ibcfixture now accepts an additional argumentrelayerwith a default value ofcluster.Relayer.RLY.value. Ensure that all calls to this fixture throughout the codebase have been updated to match the new signature.187-190: The
assert_packet_resultfunction is added. This function asserts that the logs of an event have the expected sequence number and status.317-321: The
assert_packet_resultfunction is used to assert the logs of theOnPacketResultevent in thetest_sc_callfunction. This is a good practice as it ensures that the event logs are as expected.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
394bce5
Closes: #1231
Solution:
👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻
PR Checklist:
make)make test)go fmt)golangci-lint run)go list -json -m all | nancy sleuth)Thank you for your code, it's appreciated! :)
Summary by CodeRabbit
Bug Fixes:
New Features:
onPacketResultfunction to handle the response from the EVM call and add logs to the stateDB, improving data tracking.Tests:
printstatement in the integration tests to display the logs of theOnPacketResultevents, aiding in debugging and test validation.test_sc_calland a helper functionassert_logs_equalfor more comprehensive testing.Refactor:
execfunction to carry additional information related to the state DB, enhancing context data management.