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
Sequence DiagramsequenceDiagram
participant Client
participant IBCModule
participant MemoHandler
participant Blockchain
Client->>IBCModule: Send IBC Packet with Memo
IBCModule->>MemoHandler: Validate Memo
alt Invalid Memo Format
MemoHandler-->>IBCModule: Return Error
IBCModule-->>Client: Reject Packet
else Valid Memo
MemoHandler->>Blockchain: Process Memo
Blockchain-->>IBCModule: Acknowledgment
IBCModule-->>Client: Confirm Transaction
end
Possibly related PRs
Poem
✨ Finishing Touches
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.
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