imp(apps): allow one sided fee middleware handshakes to complete #6253
Conversation
srdtrk
requested review from
AdityaSripal,
charleenfei,
colin-axner,
damiannolan,
chatton,
crodriguezvega and
DimitrisJim
as code owners
WalkthroughThe changes in the modules focus on enhancing error handling, logging, and middleware interactions within the IBC framework. These improvements involve refining handshake processes to handle version discrepancies effectively, optimizing middleware operations for better interchain communication, and introducing new test functions to enhance fee management capabilities. Changes
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 (
|
crodriguezvega
added
priority
There was a problem hiding this comment.
Nice work as always @srdtrk!
So we can init we a fee version but downgrade/fallback if the counterparty doesn't support 👍🏻
| return errorsmod.Wrapf(err, "failed to unmarshal ICS29 counterparty version metadata: %s", counterpartyVersion) | ||
| // we pass the entire version string onto the underlying application. | ||
| // and disable fees for this channel | ||
| im.keeper.DeleteFeeEnabled(ctx, portID, channelID) |
There was a problem hiding this comment.
Not feeling super strong about it, but we could also alternatively not call SetFeeEnabled in OnChanOpenInit, and call here SetFeeEnabled instead if err == nil.
There was a problem hiding this comment.
I'm ok with this since this line is inside the larger im.keeper.IsFeeEnabled(ctx, portID, channelID) if case. This would require a larger refactor.
![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
Out of diff range and nitpick comments (2)
CHANGELOG.md (2)
Line range hint
169-169: Link bare URL to descriptive text
The URL on this line should be linked to descriptive text for better readability and to provide context. For example,[GitHub Pull Request #1234](https://github.com/cosmos/ibc-go/pull/1234).
Line range hint
184-184: Remove trailing spaces
There are unnecessary trailing spaces on this line. It's a good practice to remove trailing spaces as they can complicate version control diffs.
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional Context Used
Path-based Instructions (1)
CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (3)
- modules/apps/29-fee/fee_test.go (2 hunks)
- modules/apps/transfer/ibc_module.go (1 hunks)
- modules/apps/transfer/ibc_module_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- modules/apps/29-fee/fee_test.go
- modules/apps/transfer/ibc_module.go
Additional Context Used
Path-based Instructions (1)
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"
Additional comments not posted (3)
modules/apps/transfer/ibc_module_test.go (3)
135-139: Ensure the test case description matches the logic implemented.Verification successful
The test case description "success: invalid counterparty version proposes new version" is associated with a test function that sets
counterpartyVersionto "version" and includes a comment stating "transfer module will propose the default version." This suggests that the test is designed to handle a scenario where an invalid counterparty version triggers the transfer module to propose a default version, which aligns with the description provided.Given this information, the test case description appears to accurately reflect the logic implemented in the test function. Therefore, the review comment has been verified as accurate.
- The test function correctly implements the logic described by the test case description.
- Located in
modules/apps/transfer/ibc_module_test.go.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the test case description "success: invalid counterparty version proposes new version" accurately reflects the test logic. # Test: Search for the test case description in the codebase. Expect: Description should match the logic. rg --type go $'success: invalid counterparty version proposes new version'Length of output: 190
Script:
#!/bin/bash # Description: Examine the test function logic associated with the description "success: invalid counterparty version proposes new version" to ensure it matches the description. # Test: Extract the full test function to analyze the logic implemented. rg --type go -A 10 $'success: invalid counterparty version proposes new version' modules/apps/transfer/ibc_module_test.goLength of output: 486
141-146: Ensure error handling is robust in test cases.Verification successful
The verification script has confirmed the presence of the test case "failure: max channels reached" in the
ibc_module_test.gofile. However, the script only checked for the existence of the test case description and did not delve into the specifics of the error handling mechanisms implemented within the test case. This means we do not have enough information to fully verify the robustness of the error handling as mentioned in the review comment.To proceed, we need to examine the implementation details of this specific test case to ensure that it covers all necessary scenarios as expected. This involves checking the logic within the test case to see if it appropriately triggers and handles the
types.ErrMaxTransferChannelserror.
The script output provides the implementation details for the test case "failure: max channels reached". The test case sets the
ChannelIDto a value beyond the maximum allowed (math.MaxUint32 + 1) and expects the errortypes.ErrMaxTransferChannelsto be triggered. This indicates that the test case is specifically designed to test the scenario where the maximum number of transfer channels is exceeded, and it correctly anticipates the corresponding error.Given this information, it appears that the error handling for this specific test case is implemented as expected, focusing on the scenario of exceeding the maximum channel limit and checking for the correct error response.
Conclusion:
The error handling in the test case "failure: max channels reached" is robust and correctly implemented to cover the scenario of exceeding the maximum number of transfer channels.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the error handling in the test case "failure: max channels reached" is robust and covers all necessary scenarios. # Test: Search for the test case in the codebase. Expect: Comprehensive error handling. rg --type go $'failure: max channels reached'Length of output: 132
Script:
#!/bin/bash # Description: Examine the implementation details of the test case "failure: max channels reached" to ensure it appropriately handles the types.ErrMaxTransferChannels error. # Test: Extract the implementation details of the test case from the codebase. rg --type go -A 10 $'failure: max channels reached' modules/apps/transfer/ibc_module_test.goLength of output: 603
152-157: Check for consistency in error messages and handling.Verification successful
The verification script was executed to check the consistency of error handling in the test case "failure: invalid order - ORDERED" within the
ibc_module_test.gofile. The script successfully located the specific test case in the codebase, confirming its presence.However, the script output does not provide detailed information about how the error is handled or whether the error handling is consistent with the module's standards. To fully address the review comment, we need to examine the implementation details of the error handling in this test case and compare it with the standard error handling practices in the module.
Since the current script output is insufficient for a conclusive verification, further investigation is required.
The additional script output has provided the necessary context to evaluate the error handling for the test case "failure: invalid order - ORDERED" in the
ibc_module_test.gofile. The test case explicitly sets the channel ordering tochanneltypes.ORDEREDand expects an errorchanneltypes.ErrInvalidChannelOrderingto be triggered.This setup indicates that the test is designed to validate that the system correctly identifies and handles an invalid channel ordering scenario, which is consistent with the module's standards for error handling. The error type
ErrInvalidChannelOrderingis appropriately used here to indicate the specific nature of the error, aligning with the expected module behavior.Given this information, it can be concluded that the error handling in this test case is consistent with the module's standards.
- The test case correctly sets up a scenario for an invalid channel order.
- It checks for the specific error
channeltypes.ErrInvalidChannelOrdering, which is appropriate for this kind of failure.This confirms that the error handling is implemented correctly and consistently within the module's standards.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the error messages and handling in the test case "failure: invalid order - ORDERED" are consistent with the module's standards. # Test: Search for the test case in the codebase. Expect: Consistent error handling. rg --type go $'failure: invalid order - ORDERED'Length of output: 138
Script:
#!/bin/bash # Description: Examine the error handling details for the test case "failure: invalid order - ORDERED" in ibc_module_test.go # Extract the relevant test case block and check for error handling consistency. rg --type go -A 10 'failure: invalid order - ORDERED' modules/apps/transfer/ibc_module_test.goLength of output: 386
|
* imp: added counter version proposal to try step * test: fixed tests * test: tested removing fee middleware * test: added ica onesided test * add changelog * imp: improved RemoveFeeMiddleware * nit: damian --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io> (cherry picked from commit 3b3ecc5) # Conflicts: # CHANGELOG.md # modules/apps/27-interchain-accounts/host/keeper/handshake.go # modules/apps/29-fee/fee_test.go # modules/apps/29-fee/ibc_middleware.go # modules/apps/29-fee/transfer_test.go # modules/apps/transfer/ibc_module.go # modules/apps/transfer/ibc_module_test.go
* imp: added counter version proposal to try step * test: fixed tests * test: tested removing fee middleware * test: added ica onesided test * add changelog * imp: improved RemoveFeeMiddleware * nit: damian --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io> (cherry picked from commit 3b3ecc5) # Conflicts: # CHANGELOG.md # modules/apps/transfer/ibc_module_test.go
…kport #6253) (#6260) * imp(apps): allow one sided fee middleware handshakes to complete (#6253) * imp: added counter version proposal to try step * test: fixed tests * test: tested removing fee middleware * test: added ica onesided test * add changelog * imp: improved RemoveFeeMiddleware * nit: damian --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io> (cherry picked from commit 3b3ecc5) # Conflicts: # CHANGELOG.md # modules/apps/27-interchain-accounts/host/keeper/handshake.go # modules/apps/29-fee/fee_test.go # modules/apps/29-fee/ibc_middleware.go # modules/apps/29-fee/transfer_test.go # modules/apps/transfer/ibc_module.go # modules/apps/transfer/ibc_module_test.go * imp: iteration one * imp: iteration two * Update CHANGELOG.md --------- Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com> Co-authored-by: srdtrk <srdtrk@hotmail.com> Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
…kport #6253) (#6261) * imp(apps): allow one sided fee middleware handshakes to complete (#6253) * imp: added counter version proposal to try step * test: fixed tests * test: tested removing fee middleware * test: added ica onesided test * add changelog * imp: improved RemoveFeeMiddleware * nit: damian --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io> (cherry picked from commit 3b3ecc5) # Conflicts: # CHANGELOG.md # modules/apps/transfer/ibc_module_test.go * imp: first iteration * fix: test * nit --------- Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com> Co-authored-by: srdtrk <srdtrk@hotmail.com> Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
Description
closes: #6171
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
New Features
Bug Fixes
Tests
Documentation