fix: check ack success if only ack is not async #2662
Conversation
WalkthroughThis pull request introduces enhancements to IBC (Inter-Blockchain Communication) memo handling and testing across multiple files. The changes focus on improving error handling and test coverage for IBC token transfers and memo deserialization. Specifically, the modifications include adding test cases for invalid memo formats, refining acknowledgment processing in the IBC module, and expanding test scenarios for memo marshalling and deserialization. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
x/uibc/uics20/ibc_module.go (1)
101-103: LGTM! Consider enhancing the documentation.The null check for async acknowledgments is a good defensive programming practice. However, the comment could be more descriptive.
- // ack is nil if acknowledgement is asynchronous + // ack can be nil for asynchronous acknowledgments, which occurs when the receiving + // chain needs more time to process the packet and will acknowledge it latertests/e2e/e2e_ibc_memo_test.go (1)
89-95: Consider enhancing test coverage for acknowledgment handling.While the test verifies balances correctly, it would be beneficial to add assertions that verify the acknowledgment behavior, especially for the async acknowledgment case that was added in
ibc_module.go.s.SendIBC(setup.GaiaChainID, s.Chain.ID, accs.Alice.String(), atomFromGaia, "", invalidM, "") updatedIBCAtomBalance = updatedIBCAtomBalance.Add(atomFromGaia.Amount) s.checkSupply(umeeAPIEndpoint, uatomIBCHash, updatedIBCAtomBalance) s.checkLeverageAccountBalance(umeeAPIEndpoint, accs.Alice.String(), uatomIBCHash, atomFromGaia.Amount) + // Verify acknowledgment behavior + ack := s.GetLastAcknowledgment() + s.Assert().NotNil(ack, "Acknowledgment should be present for sync operations") + s.Assert().True(ack.Success(), "Acknowledgment should indicate success")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/e2e/e2e_ibc_memo_test.go(1 hunks)x/uibc/uics20/ibc_module.go(1 hunks)x/uibc/uics20/memo_handler_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test-app-after-import
- GitHub Check: test-app-import-export
- GitHub Check: test-app-multi-seed-short
- GitHub Check: test-e2e
- GitHub Check: Analyze
- GitHub Check: Summary
🔇 Additional comments (1)
x/uibc/uics20/memo_handler_test.go (1)
146-148: LGTM! Good test coverage.The test case properly validates error handling for malformed IBC forwarding messages.
Codecov ReportAttention: Patch coverage is
❌ Your project status has failed because the head coverage (61.97%) is below the target coverage (65.50%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2662 +/- ##
===========================================
- Coverage 75.38% 61.97% -13.42%
===========================================
Files 100 275 +175
Lines 8025 19644 +11619
===========================================
+ Hits 6050 12175 +6125
- Misses 1589 6693 +5104
- Partials 386 776 +390
|
Description
closes: #XXXX
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...
!to the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit
Tests
Bug Fixes