Skip to content

Commit

Permalink
fix: delete already refunded fees from state if some fee cannot be re…
Browse files Browse the repository at this point in the history 
…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
  • Loading branch information
@mergify
Carlos Rodriguez authored and mergify[bot] committed May 7, 2024
1 parent 3aa76ad commit 19f1eba
Show file tree
Hide file tree
Showing 3 changed files with 46 additions and 57 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
### Bug Fixes

* (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.
* (app/29-fee) [\#6255](https://github.com/cosmos/ibc-go/pull/6255) Delete refunded fees from state if some fee(s) cannot be refunded on channel closure.

## [v8.2.0](https://github.com/cosmos/ibc-go/releases/tag/v8.2.0) - 2024-04-05

Expand Down
12 changes: 8 additions & 4 deletions modules/apps/29-fee/keeper/escrow.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ func (k Keeper) RefundFeesOnChannelClosure(ctx sdk.Context, portID, channelID st
cacheCtx, writeFn := ctx.CacheContext()

for _, identifiedPacketFee := range identifiedPacketFees {
var failedToSendCoins bool
var unRefundedFees []types.PacketFee
for _, packetFee := range identifiedPacketFee.PacketFees {

if !k.EscrowAccountHasBalance(cacheCtx, packetFee.Fee.Total()) {
Expand All @@ -207,18 +207,22 @@ func (k Keeper) RefundFeesOnChannelClosure(ctx sdk.Context, portID, channelID st

refundAddr, err := sdk.AccAddressFromBech32(packetFee.RefundAddress)
if err != nil {
failedToSendCoins = true
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 {
failedToSendCoins = true
unRefundedFees = append(unRefundedFees, packetFee)
continue
}
}

if !failedToSendCoins {
if len(unRefundedFees) > 0 {
// update packet fees to keep only the unrefunded fees
packetFees := types.NewPacketFees(unRefundedFees)
k.SetFeesInEscrow(cacheCtx, identifiedPacketFee.PacketId, packetFees)
} else {
k.DeleteFeesInEscrow(cacheCtx, identifiedPacketFee.PacketId)
}
}
Expand Down
90 changes: 37 additions & 53 deletions modules/apps/29-fee/keeper/escrow_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -395,51 +395,42 @@ func (suite *KeeperTestSuite) TestDistributePacketFeesOnTimeout() {

func (suite *KeeperTestSuite) TestRefundFeesOnChannelClosure() {
var (
expIdentifiedPacketFees []types.IdentifiedPacketFees
expEscrowBal sdk.Coins
expRefundBal sdk.Coins
refundAcc sdk.AccAddress
fee types.Fee
locked bool
expectEscrowFeesToBeDeleted bool
expIdentifiedPacketFees []types.IdentifiedPacketFees
expEscrowBal sdk.Coins
expRefundBal sdk.Coins
refundAcc sdk.AccAddress
fee types.Fee
)

testCases := []struct {
name string
malleate func()
expPass bool
locked bool
}{
{
"success", func() {
for i := 1; i < 6; i++ {
// store the fee in state & update escrow account balance
packetID := channeltypes.NewPacketID(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, uint64(i))
packetFees := types.NewPacketFees([]types.PacketFee{types.NewPacketFee(fee, refundAcc.String(), nil)})
identifiedPacketFees := types.NewIdentifiedPacketFees(packetID, packetFees.PacketFees)

suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, packetFees)

err := suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, fee.Total())
suite.Require().NoError(err)

expIdentifiedPacketFees = append(expIdentifiedPacketFees, identifiedPacketFees)
}
}, true,
}, false,
},
{
"success with undistributed packet fees on a different channel", func() {
for i := 1; i < 6; i++ {
// store the fee in state & update escrow account balance
packetID := channeltypes.NewPacketID(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, uint64(i))
packetFees := types.NewPacketFees([]types.PacketFee{types.NewPacketFee(fee, refundAcc.String(), nil)})
identifiedPacketFees := types.NewIdentifiedPacketFees(packetID, packetFees.PacketFees)

suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, packetFees)

err := suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, fee.Total())
suite.Require().NoError(err)

expIdentifiedPacketFees = append(expIdentifiedPacketFees, identifiedPacketFees)
}

// set packet fee for a different channel
Expand All @@ -453,12 +444,10 @@ func (suite *KeeperTestSuite) TestRefundFeesOnChannelClosure() {

expEscrowBal = fee.Total()
expRefundBal = expRefundBal.Sub(fee.Total()...)
}, true,
}, false,
},
{
"escrow account empty, module should become locked", func() {
locked = true

// store the fee in state without updating escrow account balance
packetID := channeltypes.NewPacketID(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, uint64(1))
packetFees := types.NewPacketFees([]types.PacketFee{types.NewPacketFee(fee, refundAcc.String(), nil)})
Expand All @@ -467,13 +456,10 @@ func (suite *KeeperTestSuite) TestRefundFeesOnChannelClosure() {
suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, packetFees)

expIdentifiedPacketFees = []types.IdentifiedPacketFees{identifiedPacketFees}
},
true,
}, true,
},
{
"escrow account goes negative on second packet, module should become locked", func() {
locked = true

// store 2 fees in state
packetID1 := channeltypes.NewPacketID(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, uint64(1))
packetID2 := channeltypes.NewPacketID(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, uint64(2))
Expand All @@ -494,55 +480,63 @@ func (suite *KeeperTestSuite) TestRefundFeesOnChannelClosure() {
{
"invalid refund acc address", func() {
// store the fee in state & update escrow account balance
expectEscrowFeesToBeDeleted = false
packetID := channeltypes.NewPacketID(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, uint64(1))
packetFees := types.NewPacketFees([]types.PacketFee{types.NewPacketFee(fee, "invalid refund address", nil)})
identifiedPacketFees := types.NewIdentifiedPacketFees(packetID, packetFees.PacketFees)

packetFees := types.NewPacketFees([]types.PacketFee{
types.NewPacketFee(fee, refundAcc.String(), nil), // this packet fee will be refunded, and will be deleted from state
types.NewPacketFee(fee, "invalid refund address", nil),
})
suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, packetFees)

err := suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, fee.Total())
// escrow twice the fee amount to account for the packet to have been incentivized twice
err := suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, fee.Total().MulInt(sdkmath.NewInt(2)))
suite.Require().NoError(err)

expIdentifiedPacketFees = []types.IdentifiedPacketFees{identifiedPacketFees}
// only the first packet fee should be refunded, the second should remain in state
expIdentifiedPacketFees = []types.IdentifiedPacketFees{types.NewIdentifiedPacketFees(packetID, packetFees.PacketFees[1:])}

expEscrowBal = fee.Total()
expRefundBal = expRefundBal.Sub(fee.Total()...)
}, true,
}, false,
},
{
"distributing to blocked address is skipped", func() {
expectEscrowFeesToBeDeleted = false
blockedAddr := suite.chainA.GetSimApp().AccountKeeper.GetModuleAccount(suite.chainA.GetContext(), transfertypes.ModuleName).GetAddress().String()

// store the fee in state & update escrow account balance
packetID := channeltypes.NewPacketID(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, uint64(1))
packetFees := types.NewPacketFees([]types.PacketFee{types.NewPacketFee(fee, blockedAddr, nil)})
identifiedPacketFees := types.NewIdentifiedPacketFees(packetID, packetFees.PacketFees)

packetFees := types.NewPacketFees([]types.PacketFee{
types.NewPacketFee(fee, refundAcc.String(), nil), // this packet fee will be refunded, and will be deleted from state
types.NewPacketFee(fee, blockedAddr, nil),
})
suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, packetFees)

err := suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, fee.Total())
// escrow twice the fee amount to account for the packet to have been incentivized twice
err := suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, fee.Total().MulInt(sdkmath.NewInt(2)))
suite.Require().NoError(err)

expIdentifiedPacketFees = []types.IdentifiedPacketFees{identifiedPacketFees}
// only the first packet fee should be refunded, the second should remain in state
expIdentifiedPacketFees = []types.IdentifiedPacketFees{types.NewIdentifiedPacketFees(packetID, packetFees.PacketFees[1:])}

expEscrowBal = fee.Total()
expRefundBal = expRefundBal.Sub(fee.Total()...)
}, true,
}, false,
},
}

for _, tc := range testCases {
tc := tc

suite.Run(tc.name, func() {
<<<<<<< HEAD

Check failure on line 530 in modules/apps/29-fee/keeper/escrow_test.go

View workflow job for this annotation

GitHub Actions / lint 

syntax error: unexpected <<, expected }

Check failure on line 530 in modules/apps/29-fee/keeper/escrow_test.go

View workflow job for this annotation

GitHub Actions / lint 

expected statement, found '<<' (typecheck)

Check failure on line 530 in modules/apps/29-fee/keeper/escrow_test.go

View workflow job for this annotation

GitHub Actions / lint 

expected statement, found '<<' (and 4 more errors)

Check failure on line 530 in modules/apps/29-fee/keeper/escrow_test.go

View workflow job for this annotation

GitHub Actions / lint 

syntax error: unexpected <<, expected }

Check failure on line 530 in modules/apps/29-fee/keeper/escrow_test.go

View workflow job for this annotation

GitHub Actions / lint 

expected statement, found '<<' (typecheck)

Check failure on line 530 in modules/apps/29-fee/keeper/escrow_test.go

View workflow job for this annotation

GitHub Actions / lint 

expected statement, found '<<' (and 4 more errors)

Check failure on line 530 in modules/apps/29-fee/keeper/escrow_test.go

View workflow job for this annotation

GitHub Actions / tests (01) 

expected statement, found '<<'
suite.SetupTest() // reset
suite.coordinator.Setup(suite.path) // setup channel
expIdentifiedPacketFees = []types.IdentifiedPacketFees{}
=======

Check failure on line 534 in modules/apps/29-fee/keeper/escrow_test.go

View workflow job for this annotation

GitHub Actions / lint 

syntax error: unexpected ==, expected }

Check failure on line 534 in modules/apps/29-fee/keeper/escrow_test.go

View workflow job for this annotation

GitHub Actions / lint 

syntax error: unexpected ==, expected }
suite.SetupTest() // reset
suite.path.Setup() // setup channel
expIdentifiedPacketFees = []types.IdentifiedPacketFees(nil)
>>>>>>> 500765e4 (fix: delete already refunded fees from state if some fee cannot be refunded on channel closure (#6255))

Check failure on line 538 in modules/apps/29-fee/keeper/escrow_test.go

View workflow job for this annotation

GitHub Actions / lint 

syntax error: unexpected >>, expected }

Check failure on line 538 in modules/apps/29-fee/keeper/escrow_test.go

View workflow job for this annotation

GitHub Actions / lint 

invalid character U+0023 '#' (typecheck)

Check failure on line 538 in modules/apps/29-fee/keeper/escrow_test.go

View workflow job for this annotation

GitHub Actions / lint 

expected ';', found fee (typecheck)

Check failure on line 538 in modules/apps/29-fee/keeper/escrow_test.go

View workflow job for this annotation

GitHub Actions / lint 

syntax error: unexpected >>, expected }

Check failure on line 538 in modules/apps/29-fee/keeper/escrow_test.go

View workflow job for this annotation

GitHub Actions / lint 

invalid character U+0023 '#' (typecheck)

Check failure on line 538 in modules/apps/29-fee/keeper/escrow_test.go

View workflow job for this annotation

GitHub Actions / lint 

expected ';', found fee (typecheck)
expEscrowBal = sdk.Coins{}
locked = false
expectEscrowFeesToBeDeleted = true

// setup
refundAcc = suite.chainA.SenderAccount.GetAddress()
Expand All @@ -565,32 +559,22 @@ func (suite *KeeperTestSuite) TestRefundFeesOnChannelClosure() {
originalEscrowBal := suite.chainA.GetSimApp().BankKeeper.GetAllBalances(suite.chainA.GetContext(), moduleAcc)

err := suite.chainA.GetSimApp().IBCFeeKeeper.RefundFeesOnChannelClosure(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID)
suite.Require().NoError(err)

// refundAcc balance after RefundFeesOnChannelClosure
refundBal := suite.chainA.GetSimApp().BankKeeper.GetAllBalances(suite.chainA.GetContext(), refundAcc)
escrowBal := suite.chainA.GetSimApp().BankKeeper.GetAllBalances(suite.chainA.GetContext(), moduleAcc)

if tc.expPass {
suite.Require().NoError(err)
} else {
suite.Require().Error(err)
}
suite.Require().Equal(tc.locked, suite.chainA.GetSimApp().IBCFeeKeeper.IsLocked(suite.chainA.GetContext()))
suite.Require().Equal(expIdentifiedPacketFees, suite.chainA.GetSimApp().IBCFeeKeeper.GetIdentifiedPacketFeesForChannel(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID))

suite.Require().Equal(locked, suite.chainA.GetSimApp().IBCFeeKeeper.IsLocked(suite.chainA.GetContext()))

if locked || !tc.expPass {
if tc.locked {
// refund account and escrow account balances should remain unchanged
suite.Require().Equal(originalRefundBal, refundBal)
suite.Require().Equal(originalEscrowBal, escrowBal)

// ensure none of the fees were deleted
suite.Require().Equal(expIdentifiedPacketFees, suite.chainA.GetSimApp().IBCFeeKeeper.GetIdentifiedPacketFeesForChannel(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID))
} else {
suite.Require().Equal(expEscrowBal, escrowBal) // escrow balance should be empty
suite.Require().Equal(expRefundBal, refundBal) // all packets should have been refunded

// all fees in escrow should be deleted if expected for this channel
suite.Require().Equal(expectEscrowFeesToBeDeleted, len(suite.chainA.GetSimApp().IBCFeeKeeper.GetIdentifiedPacketFeesForChannel(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID)) == 0)
}
})

Check failure on line 579 in modules/apps/29-fee/keeper/escrow_test.go

View workflow job for this annotation

GitHub Actions / lint 

expected statement, found ')' (typecheck)

Check failure on line 579 in modules/apps/29-fee/keeper/escrow_test.go

View workflow job for this annotation

GitHub Actions / lint 

expected statement, found ')' (typecheck)
}
Expand Down

0 comments on commit 19f1eba

Please sign in to comment.