fix: delete already refunded fees from state if some fee cannot be refunded on channel closure #6255
Conversation
…funds fees on channel closure
crodriguezvega
requested review from
AdityaSripal,
charleenfei,
colin-axner,
damiannolan and
chatton
as code owners
WalkthroughWalkthroughThe update in Changes
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
![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)
modules/apps/29-fee/keeper/escrow.go (1)
Line range hint
192-216: Refactor to improve error handling and reduce code duplication.- refundAddr, err := sdk.AccAddressFromBech32(packetFee.RefundAddress) - if err != nil { - unRefundedFees = append(unRefundedFees, packetFee) - continue - } - - // refund all fees to refund address - if err = k.bankKeeper.SendCoinsFromModuleToAccount(cacheCtx, types.ModuleName, refundAddr, packetFee.Fee.Total()); err != nil { - unRefundedFees = append(unRefundedFees, packetFee) - continue - } + if err := k.attemptRefund(cacheCtx, &unRefundedFees, packetFee); err != nil { + return err + }Consider extracting the refund logic into a separate method
attemptRefundto handle the parsing of the refund address and the actual refund. This reduces duplication and improves readability.modules/apps/29-fee/keeper/escrow_test.go (1)
Line range hint
398-563: Enhance test coverage for handling of unrefunded fees.It appears that the tests could be enhanced to specifically verify the handling of
unRefundedFees. Would you like me to help by adding detailed test cases or opening a GitHub issue to track this enhancement?
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- modules/apps/29-fee/keeper/escrow.go (2 hunks)
- modules/apps/29-fee/keeper/escrow_test.go (6 hunks)
Additional Context Used
Path-based Instructions (2)
modules/apps/29-fee/keeper/escrow.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.modules/apps/29-fee/keeper/escrow_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 (1)
modules/apps/29-fee/keeper/escrow.go (1)
221-225: Ensure proper handling of state updates for unrefunded fees.Please verify that the state updates using
SetFeesInEscrowandDeleteFeesInEscroware correctly implemented across the codebase, especially in scenarios where fees are partially refunded.
There was a problem hiding this comment.
Thanks @crodriguezvega! Code changes look good to me.
I think in future we can:
- RefundFeesOnChannelClosure(ctx sdk.Context, portID, channelID string) error {
+ RefundFeesOnChannelClosure(ctx sdk.Context, portID, channelID string) {It looks like the error return is not used here at all since #1523
There was a problem hiding this comment.
might be nice to refactor this test in future to be a bit more clear, maybe we some expResult func or something. But happy to move ahead with this for now
There was a problem hiding this comment.
Actionable comments posted: 0
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"
Maybe we could put a limit on the number of fees that can be escrowed for a single packet? |
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
170-170: Use Markdown link syntax for URLs.- * [\#6193](https://github.com/cosmos/ibc-go/pull/6193) Bump Cosmos SDK to v0.50.6. + * [\#6193](https://github.com/cosmos/ibc-go/pull/6193) [Bump Cosmos SDK to v0.50.6](https://github.com/cosmos/ibc-go/pull/6193).
Line range hint
185-185: Remove trailing spaces.- * (apps/27-interchain-accounts) [\#6167](https://github.com/cosmos/ibc-go/pull/6167) Fixed an edge case bug where migrating params for a pre-existing ica module which implemented controller functionality only could panic when migrating params for newly added host, and align controller param migration with host. + * (apps/27-interchain-accounts) [\#6167](https://github.com/cosmos/ibc-go/pull/6167) Fixed an edge case bug where migrating params for a pre-existing ica module which implemented controller functionality only could panic when migrating params for newly added host, and align controller param migration with host.
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
Out of diff range and nitpick comments (2)
CHANGELOG.md (2)
Line range hint
171-171: Use descriptive text for URLs to improve readability.- * [\#6193](https://github.com/cosmos/ibc-go/pull/6193) Bump Cosmos SDK to v0.50.6. + * [Bump Cosmos SDK to v0.50.6](https://github.com/cosmos/ibc-go/pull/6193).
Line range hint
186-186: Remove trailing spaces.- * (apps/27-interchain-accounts) [\#6167](https://github.com/cosmos/ibc-go/pull/6167) Fixed an edge case bug where migrating params for a pre-existing ica module which implemented controller functionality only could panic when migrating params for newly added host, and align controller param migration with host. + * (apps/27-interchain-accounts) [\#6167](https://github.com/cosmos/ibc-go/pull/6167) Fixed an edge case bug where migrating params for a pre-existing ica module which implemented controller functionality only could panic when migrating params for newly added host, and align controller param migration with host.
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"
|
…funded on channel closure (#6255) * delete the refunded fees in case an error happens in the loop that refunds fees on channel closure * test simplifications * fix typo * clean up code * fix logic * add changelog (cherry picked from commit 500765e) # Conflicts: # CHANGELOG.md # modules/apps/29-fee/keeper/escrow_test.go
…funded on channel closure (#6255) * delete the refunded fees in case an error happens in the loop that refunds fees on channel closure * test simplifications * fix typo * clean up code * fix logic * add changelog (cherry picked from commit 500765e) # Conflicts: # modules/apps/29-fee/keeper/escrow_test.go
…funded on channel closure (backport #6255) (#6269) * fix: delete already refunded fees from state if some fee cannot be refunded on channel closure (#6255) * delete the refunded fees in case an error happens in the loop that refunds fees on channel closure * test simplifications * fix typo * clean up code * fix logic * add changelog (cherry picked from commit 500765e) # Conflicts: # CHANGELOG.md # modules/apps/29-fee/keeper/escrow_test.go * fix conflicts * add import --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
…funded on channel closure (backport #6255) (#6270) * fix: delete already refunded fees from state if some fee cannot be refunded on channel closure (#6255) * delete the refunded fees in case an error happens in the loop that refunds fees on channel closure * test simplifications * fix typo * clean up code * fix logic * add changelog (cherry picked from commit 500765e) # Conflicts: # modules/apps/29-fee/keeper/escrow_test.go * fix conflicts --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
Description
While refunding fees for a packet on channel closure, some fees could be refunded and some not. Those fees that are refunded should be deleted from state, so that there is no possibility of them being refunded again if the packet fee information for a packet is used somehow.
closes: #XXXX
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
RefundFeesOnChannelClosurefunction to handle unrefunded fees by storing them in a new slice and updating packet fees accordingly.