-
Notifications
You must be signed in to change notification settings - Fork 235
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
Problem: acknowledgement is not aligned with underlying_app_success #1633
Conversation
check underlying_app_success
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1633 +/- ##
===========================================
+ Coverage 17.87% 36.30% +18.42%
===========================================
Files 72 123 +51
Lines 5168 9717 +4549
===========================================
+ Hits 924 3528 +2604
- Misses 4121 5775 +1654
- Partials 123 414 +291
|
Signed-off-by: mmsqe <mavis@crypto.com>
WalkthroughThe changes in this pull request primarily involve updates to the Changes
Possibly related PRs
Suggested reviewers
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 (
|
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.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
x/cronos/keeper/keeper.go (2)
320-322
: Approve changes with a minor suggestion for clarityThe new conditional check for
ack.UnderlyingAppSuccess
is a good addition. It ensures that failed underlying application responses are properly handled, improving the robustness of the acknowledgment processing logic.Consider adding a comment explaining the purpose of this check for better code readability:
+ // Handle cases where the underlying application was unsuccessful if !ack.UnderlyingAppSuccess { return k.onPacketResult(ctx, packet, false, relayer, contractAddress, packetSenderAddress) }
320-322
: Summary of changes and final considerationsThe changes in this file are focused on improving the handling of unsuccessful underlying app responses in the
IBCOnAcknowledgementPacketCallback
function. This directly addresses the PR objective of aligning acknowledgment with underlying app success.Consider the following final points:
- Ensure that this change is reflected in the CHANGELOG.md as it modifies the behavior of IBC packet handling.
- Update any relevant documentation that describes the IBC acknowledgment process.
- Consider adding or updating unit tests to cover both successful and unsuccessful underlying app scenarios.
As this change affects the IBC packet handling process, which is a critical part of the inter-blockchain communication, it's important to thoroughly test this change in a staging environment that simulates various IBC scenarios before merging to production.
integration_tests/test_ica_precompile.py (2)
321-322
: Remove redundant variable assignmentsAt lines 321-322,
balance
andstart
are reassigned. If these variables have not changed since their last assignment, reassigning them is unnecessary and could confuse readers.Consider removing the redundant assignments:
- balance = funds_ica(cli_host, ica_address, signer=signer) - start = w3.eth.get_block_number()
323-333
: Improve readability ofsubmit_msgs
function call argumentsThe arguments passed to
submit_msgs
from lines 323 to 333 can be formatted for better readability and maintainability.Apply this diff to reformat the arguments:
str, diff = submit_msgs( ibc, tcontract.functions.callSubmitMsgs, data, ica_address, False, expected_seq, contract.events.SubmitMsgsResult, channel_id, signer=signer, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- CHANGELOG.md (1 hunks)
- integration_tests/test_ica_precompile.py (2 hunks)
- x/cronos/keeper/keeper.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (5)
x/cronos/keeper/keeper.go (1)
320-322
: Verify the impact of this change across the codebaseThe new condition for handling unsuccessful underlying app responses addresses the PR objective. To ensure this change doesn't have unintended consequences, it's important to verify its impact across the codebase.
Please run the following script to check for any other occurrences of
UnderlyingAppSuccess
and ensure they are handled consistently:This will help ensure that the change is consistent with other parts of the codebase and that appropriate test cases exist.
CHANGELOG.md (1)
27-27
: LGTM: New bug fix added to UNRELEASED sectionThe new line correctly documents a bug fix related to acknowledgement alignment with underlying app success when the acknowledgement packet does not succeed. This is properly linked to the corresponding pull request (#1633).
integration_tests/test_ica_precompile.py (3)
Line range hint
308-337
: Confirm that all function calls are updated with the correct parametersThroughout the changes from lines 308 to 337, ensure that all calls to
submit_msgs
and related functions use the correct and updated parameters, especially after any refactoring.Run the following script to check for parameter usage:
#!/bin/bash # Description: Find all calls to `submit_msgs` and list their parameters. # Test: Extract `submit_msgs` calls and display the parameters. rg --type python 'submit_msgs\(' -A 10
317-318
:⚠️ Potential issueCorrect indentation and ensure proper assertion placement
Lines 317-318 contain assertion statements that are not indented properly. This could lead to syntax errors or logical issues in the test execution.
Apply this diff to fix the indentation:
- assert expected_seq == last_seq - assert status == Status.FAIL + assert expected_seq == last_seq + assert status == Status.FAILLikely invalid or redundant comment.
308-308
: Clarifyneed_wait=False
parameterIn line 308, when calling
submit_msgs
, the parameterneed_wait
is set toFalse
. This means the function will not wait for the transaction confirmation, which might lead to race conditions or flaky tests if subsequent code depends on the transaction's completion. Ensure that this is intentional and that the test accounts for the asynchronous nature of the call.Consider verifying the necessity of
need_wait=False
:✅ Verification successful
Clarify
need_wait=False
ParameterAfter analyzing the codebase, there are no subsequent assertions or operations that depend on the transaction's confirmation following
submit_msgs
calls withneed_wait=False
. This setup appears intentional and the tests account for the asynchronous nature of the call.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if any other parts of the test depend on the transaction confirmation. # Test: Search for usage of variables that might be affected. rg --type python 'last_seq|status|balance' -A 5Length of output: 165
Script:
#!/bin/bash # Description: Verify if other parts of the test depend on the transaction confirmation by searching for relevant variables. # Corrected rg command with proper file type and syntax rg --type py 'submit_msgs\(' -A 10Length of output: 9017
Script:
#!/bin/bash # Description: Identify if there are any assertions or operations following submit_msgs calls that depend on transaction confirmation. # Search for submit_msgs calls with need_wait=False and check the next few lines for assertions or state-dependent operations. rg --type py 'submit_msgs\([^)]*need_wait=False[^)]*\)' -A 10Length of output: 63
Co-authored-by: mmsqe <mavis@crypto.com> Signed-off-by: yihuang <huang@crypto.com>
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.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- integration_tests/test_ica_precompile.py (2 hunks)
- x/cronos/keeper/keeper.go (1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
x/cronos/keeper/keeper.go
[warning] 320-321: x/cronos/keeper/keeper.go#L320-L321
Added lines #L320 - L321 were not covered by tests
🔇 Additional comments (4)
integration_tests/test_ica_precompile.py (4)
309-309
: LGTM: Improved flexibility in message submissionThe addition of
need_wait=False
allows for more flexible handling of message submission without waiting for confirmation. This change is consistent with the updatedsubmit_msgs
function signature and provides better control over the test flow.
318-320
: LGTM: Improved error handling and log verificationThe addition of the assertion
assert status == Status.FAIL
and thewait_for_packet_log
function call significantly improves the test's robustness. These changes ensure that the test properly verifies the failure status and waits for the corresponding log, addressing the previous concern about error handling for unexpected statuses.
338-338
: LGTM: Improved sequence verificationThe addition of
assert expected_seq == last_seq
is a valuable improvement to the test. This assertion ensures that the expected sequence number matches the last sequence number returned by the contract, verifying the correct sequencing of operations in the ICA functionality.
321-338
: 🛠️ Refactor suggestionConsider refactoring repetitive code patterns
While the added code successfully performs the necessary steps for testing ICA functionality, there are still repetitive patterns that could benefit from refactoring. Consider creating a helper function to encapsulate the common operations of funding, submitting messages, and verifying status. This would improve code maintainability and readability.
Example refactoring:
def perform_ica_operation(ibc, tcontract, ica_address, expected_seq, signer): balance = funds_ica(ibc.chainmain.cosmos_cli(), ica_address, signer=signer) start = ibc.cronos.w3.eth.get_block_number() str, diff = submit_msgs( ibc, tcontract.functions.callSubmitMsgs, {"from": ADDRS[signer], "gas": 500000}, ica_address, False, expected_seq, tcontract.events.SubmitMsgsResult, get_next_channel(ibc.cronos.cosmos_cli(), connid), signer=signer, ) last_seq = tcontract.caller.getLastSeq() wait_for_status_change(tcontract, channel_id, last_seq) status = tcontract.caller.getStatus(channel_id, last_seq) assert expected_seq == last_seq assert status == Status.SUCCESS wait_for_packet_log(start, tcontract.events.OnPacketResult, channel_id, last_seq, status) return balance - diffThis refactoring would make the test more maintainable and easier to read.
8425423
👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻
when packet does not succeed
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
New Features
Improvements
Bug Fixes
Documentation