Problem: logs in callback contract are lost

[yihuang]

Closes: #1231

Solution:

• re-emit those logs

👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻

PR Checklist:

• Have you read the CONTRIBUTING.md?
• Does your PR follow the C4 patch requirements?
• Have you rebased your work on top of the latest master?
• Have you checked your code compiles? (make)
• Have you included tests for any non-trivial functionality?
• Have you checked your code passes the unit tests? (make test)
• Have you checked your code formatting is correct? (go fmt)
• Have you checked your basic code style is fine? (golangci-lint run)
• If you added any dependencies, have you checked they do not contain any known vulnerabilities? (go list -json -m all | nancy sleuth)
• If your changes affect the client infrastructure, have you run the integration test?
• If your changes affect public APIs, does your PR follow the C4 evolution of public contracts?
• If your code changes public APIs, have you incremented the crate version numbers and documented your changes in the CHANGELOG.md?
• If you are contributing for the first time, please read the agreement in CONTRIBUTING.md now and add a comment to this pull request stating that your PR is in accordance with the Developer's Certificate of Origin.

Thank you for your code, it's appreciated! :)

Summary by CodeRabbit

Bug Fixes:

• Fixed issues related to the default chain ID behavior and redundant parsing of chain ID from genesis, improving system stability.
• Addressed a problem with the memory store in the versiondb multistore, enhancing data storage efficiency.

New Features:

• Enhanced the onPacketResult function to handle the response from the EVM call and add logs to the stateDB, improving data tracking.
• Added a feature to re-emit logs in a callback contract, improving log management and traceability.

Tests:

• Added a print statement in the integration tests to display the logs of the OnPacketResult events, aiding in debugging and test validation.
• Introduced a new test function test_sc_call and a helper function assert_logs_equal for more comprehensive testing.

Refactor:

• Modified the context in the exec function to carry additional information related to the state DB, enhancing context data management.

[coderabbitai]

Walkthrough

The changes encompass updates to the crypto-org-chain/cronos repository, primarily focusing on the handling of Ethereum Virtual Machine (EVM) calls and the re-emission of logs in a callback contract. The modifications enhance the code's functionality by improving error handling, adding print statements for event logs, and modifying the context to carry additional information.

Changes

File(s)	Summary
CHANGELOG.md	Updates and fixes related to the default chain ID behavior, redundant parsing of chain ID from genesis, and fixing the memory store in the versiondb multistore.
integration_tests/test_ica_precompile.py	Addition of a print statement to display the logs of the OnPacketResult events.
x/cronos/keeper/keeper.go	Import statement added for the "github.com/ethereum/go-ethereum/core/vm" package. Modifications to the onPacketResult function to handle the response from the EVM call and add logs to the stateDB. Error handling added for EVM call failures.
x/cronos/keeper/precompiles/utils.go	Modification of the context (ctx) to carry the stateDB value throughout the execution.
x/cronos/types/keys.go	Addition of a constant StateDBContextKey with the value "statedb".
x/cronos/keeper/evm.go	Import statement added for the "github.com/ethereum/go-ethereum/core/vm" package. Addition of code to re-emit logs into the original stateDB for precompiled contract calls.
x/cronos/keeper/evm.go, x/cronos/keeper/keeper.go	Modifications to handle the response from the EVM call, add logs to the stateDB, and improve error handling.
integration_tests/test_ica_precompile.py	Various changes including imports, modifications to fixtures and test functions, and assertions for packet events.

🐇🍂

As the leaves fall, so do the bugs in our code,
With each pull request, we lighten the load.
In the spirit of autumn, we prune and we tweak,
Making our software less prone to a leak.
So here's to the coders, in sweaters so snug,
Sipping their lattes, squashing each bug. 🍁☕️🐇

---

Tips

Chat with CodeRabbit Bot (@coderabbitai)

• If you reply to a review comment from CodeRabbit, the bot will automatically respond.
• To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
• Note: Review comments are made on code diffs or files, not on the PR overview.
• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai help to get help.
• @coderabbitai resolve to resolve all the CodeRabbit review comments.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• The JSON schema for the configuration file is available here.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.json

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between dbd86c4 and bad0847.

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 onPacketResult function 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())
+		}
+	}

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between bad0847 and 3a4335c.

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]

Codecov Report

Merging #1233 (3a2201e) into main (dbd86c4) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

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     

Files	Coverage Δ
x/cronos/keeper/evm.go	52.94% <0.00%> (-1.61%)	⬇️

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 3a4335c and 8a3e3d4.

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 onPacketResult function 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())
+		}
+	}

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 8a3e3d4 and 59f0a53.

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 ibc fixture has been updated to include the relayer parameter when calling the prepare_network function. Ensure that the prepare_network function can handle this new argument.

• 190-196: The test_sc_call function is setting up the test environment correctly. Ensure that the CONTRACT and CONTRACTS["TestICA"] values are correct and that the contract is deployed successfully.

• 312-386: The test_sc_call function 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_equal function is a helper function to compare logs. It seems to be correctly implemented using itertools.zip_longest to handle cases where the two lists are not of the same length.

[yihuang]

integration test passed.

[coderabbitai]

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 59f0a53 and 24750f1.

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 StateDBContextKey is correctly set in the context before this function is called.

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 24750f1 and 9cb8be6.

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

[JayT106]

LGTM

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 9cb8be6 and 25c3fee.

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.cluster has been added. Ensure that the pystarport package is installed and available in the environment where these tests will be run.

• 48-57: The prepare_network function now accepts an additional argument relayer. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.

• 187-189: The assert_packet_event function 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.

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 25c3fee and 561361d.

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.cluster has been added. Ensure that the pystarport package is installed and available in the environment where this script will be run.

• 48-57: The prepare_network function now accepts an additional argument relayer. 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_result has 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.

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 561361d and 3a2201e.

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.cluster has been added. Ensure that this module is available in the project's dependencies.

• 46-57: The ibc fixture now accepts an additional argument relayer with a default value of cluster.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_result function is added. This function asserts that the logs of an event have the expected sequence number and status.

• 317-321: The assert_packet_result function is used to assert the logs of the OnPacketResult event in the test_sc_call function. This is a good practice as it ensures that the event logs are as expected.