test: cover more of transfer OnTimeoutPacket #6456
Conversation
gjermundgaraba
requested review from
colin-axner,
AdityaSripal,
damiannolan and
chatton
as code owners
WalkthroughThe updates enhance the Changes
Sequence Diagram(s) (Beta)No sequence diagrams are necessary for these changes as they primarily involve test modifications and minor adjustments. Assessment against linked issues
Possibly related issues
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
| name: "success", | ||
| malleate: func() {}, | ||
| expError: nil, | ||
| }, | ||
| { | ||
| name: "non-existent channel", | ||
| malleate: func() { | ||
| packet.SourceChannel = "channel-100" | ||
| }, | ||
| expError: ibcerrors.ErrNotFound, |
There was a problem hiding this comment.
You could remove the name, malleate and expError keys to be consistent with the two test cases below.
| // check that module account escrow address has locked the tokens | ||
| escrowAddress := types.GetEscrowAddress(packet.GetSourcePort(), packet.GetSourceChannel()) | ||
| escrowBalance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), escrowAddress, sdk.DefaultBondDenom) | ||
| suite.Require().Equal(coinToSendToB, escrowBalance) |
There was a problem hiding this comment.
Since this is already tested elsewhere, maybe we can remove it to narrow the scope of the test?
| @@ -885,7 +885,7 @@ func (suite *KeeperTestSuite) TestOnAcknowledgementPacketSetsTotalEscrowAmountFo | |||
| func (suite *KeeperTestSuite) TestOnTimeoutPacket() { | |||
There was a problem hiding this comment.
The changes in this file are unrelated to the main purpose of the PR to increase coverage of OnTtimeoutPacket, right? We would normally do this in a separate PR, so that reviews are easier; or at least would add a comment with the purpose of the changes to help the reviewers.
There was a problem hiding this comment.
Good point, this was somewhat orthogonal to the original issue. It was just a case I saw that was not covered when going through the coverage report. I can easily pull this out into a separate PR. Would you prefer an issue for it, a comment (I am adding that in any case now) or a PR without an issue?
There was a problem hiding this comment.
A separate PR would be fine by me. Thank you, @gjermundgaraba!
| @@ -885,7 +885,7 @@ func (suite *KeeperTestSuite) TestOnAcknowledgementPacketSetsTotalEscrowAmountFo | |||
| func (suite *KeeperTestSuite) TestOnTimeoutPacket() { | |||
| var ( | |||
| path *ibctesting.Path | |||
| amount sdkmath.Int | |||
| amount string | |||
There was a problem hiding this comment.
I changed this test to take string directly so that we can also test the case of a malformed string amount, which was not currently covered. I discovered this when going through the coverage report related to the original issue. I can also pull this out into a separate PR.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- modules/apps/transfer/ibc_module_test.go (4 hunks)
- modules/apps/transfer/keeper/relay_test.go (7 hunks)
Additional context used
Path-based instructions (2)
modules/apps/transfer/ibc_module_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"modules/apps/transfer/keeper/relay_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
Additional comments not posted (6)
modules/apps/transfer/ibc_module_test.go (4)
7-8: New importsdkmathfrom"cosmossdk.io/math"added.This import is necessary for handling mathematical operations with more precision, which is often required in blockchain applications.
19-19: New importibcerrorsfrom"github.com/cosmos/ibc-go/v8/modules/core/errors"added.This import is crucial for handling IBC-specific errors, ensuring that error handling is consistent with the IBC module's standards.
236-240: Modified test case to handle an "invalid counterparty version".This change enhances the robustness of the version handling in channel negotiations by explicitly testing for invalid versions.
276-367: Added new functionTestOnTimeoutPacketto test various timeout scenarios.This addition is crucial for improving test coverage on the
OnTimeoutPacketmethod, addressing the previously identified gap in coverage. The test scenarios are well thought out, covering cases like non-existent channels, invalid packet data, and already timed-out packets.modules/apps/transfer/keeper/relay_test.go (2)
888-888: Change in handlingamountfromsdkmath.Inttostringto test malformed string amounts.This change allows the test to cover scenarios where the amount is not properly formatted, increasing the robustness of the test suite.
904-906: Ensure proper parsing and error handling foramount.This change introduces string parsing for
amount, which is crucial for handling different input formats. Ensure that the parsing is robust and handles errors gracefully.Also applies to: 921-923, 935-937, 949-950
Verification successful
The code surrounding the
NewIntFromStringfunction calls includes checks to ensure that the parsing was successful by verifying theokvariable. This indicates that the error handling for the amount parsing is properly implemented.
- Lines 904-906:
suite.Require().True(ok)ensures that the parsing was successful.- Lines 921-923:
suite.Require().True(ok)ensures that the parsing was successful.- Lines 935-937:
suite.Require().True(ok)ensures that the parsing was successful.- Lines 949-950:
suite.Require().True(ok)ensures that the parsing was successful.These checks confirm that the code handles potential parsing errors appropriately.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the amount parsing is robust against various malformed inputs. # Test: Search for the function usage. Expect: Proper error handling for malformed inputs. rg --type go $'NewIntFromString' modules/apps/transfer/keeper/relay_test.goLength of output: 359
Script:
#!/bin/bash # Extract surrounding lines of each instance of NewIntFromString to verify error handling rg --type go -A 3 -B 3 $'NewIntFromString' modules/apps/transfer/keeper/relay_test.goLength of output: 1642
| }, | ||
| sdkerrors.ErrInsufficientFunds, | ||
| }, | ||
| { | ||
| "failure: cannot mint because sender address is invalid", | ||
| func() { | ||
| denom = types.NewDenom(sdk.DefaultBondDenom) | ||
| amount = sdkmath.OneInt() | ||
| amount = sdkmath.OneInt().String() |
There was a problem hiding this comment.
Handling of invalid sender address and amount.
- sender = "invalid address"
- amount = "invalid"
+ sender = suite.chainA.SenderAccount.GetAddress().String() # Use a valid address
+ amount = sdkmath.NewInt(100).String() # Use a valid amountThe test cases for invalid sender address and amount should ensure that the system behaves as expected under these error conditions. However, make sure that these are intentional and covered by assertions in the test.
Also applies to: 969-970
Committable suggestion was skipped due to low confidence.
|
| path = ibctesting.NewTransferPath(suite.chainA, suite.chainB) | ||
| path.Setup() | ||
|
|
||
| coinToSendToB := sdk.NewCoin(sdk.DefaultBondDenom, sdkmath.NewInt(42)) |
There was a problem hiding this comment.
should be able to use the ibctesting.TestCoin here but will open issue for that to be cleaned up in bulk.
DimitrisJim
deleted the
gjermund/6447-add-test-for-transfers-ontimeoutpacket
branch
Description
closes: #6447
Some relatively light unit tests for the transfer IBCModule OnTimeoutPacket method. Most of the critical functionality is dealt with in the relay_test file (I also added another test case there after going through the coverage report).
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/).godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.Summary by CodeRabbit
TestOnChanOpenAckto better handle counterparty version validation.TestOnTimeoutPacketto cover various packet timeout scenarios, improving reliability.TestOnTimeoutPacketinrelay_testto handleamountas a string, ensuring consistency in data handling.