From a4ecd559410c24320688ff160eb76ed8b4916a6d Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Fri, 10 May 2024 10:00:24 +0200 Subject: [PATCH] fix: delete already refunded fees from state if some fee cannot be refunded 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 500765e43a2059a80ebac2d596adeaf12d1883c5) # Conflicts: # CHANGELOG.md # modules/apps/29-fee/keeper/escrow_test.go * fix conflicts * add import --------- Co-authored-by: Carlos Rodriguez --- modules/apps/29-fee/keeper/escrow.go | 12 ++-- modules/apps/29-fee/keeper/escrow_test.go | 87 +++++++++-------------- 2 files changed, 41 insertions(+), 58 deletions(-) diff --git a/modules/apps/29-fee/keeper/escrow.go b/modules/apps/29-fee/keeper/escrow.go index d32ec6baae4..5976b58728d 100644 --- a/modules/apps/29-fee/keeper/escrow.go +++ b/modules/apps/29-fee/keeper/escrow.go @@ -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()) { @@ -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) } } diff --git a/modules/apps/29-fee/keeper/escrow_test.go b/modules/apps/29-fee/keeper/escrow_test.go index bcb6c473596..eceb2ffb113 100644 --- a/modules/apps/29-fee/keeper/escrow_test.go +++ b/modules/apps/29-fee/keeper/escrow_test.go @@ -1,6 +1,7 @@ package keeper_test import ( + sdkmath "cosmossdk.io/math" "github.com/cometbft/cometbft/crypto/secp256k1" sdk "github.com/cosmos/cosmos-sdk/types" @@ -313,19 +314,17 @@ 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() { @@ -333,16 +332,13 @@ func (suite *KeeperTestSuite) TestRefundFeesOnChannelClosure() { // 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() { @@ -350,14 +346,10 @@ func (suite *KeeperTestSuite) TestRefundFeesOnChannelClosure() { // 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 @@ -371,12 +363,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)}) @@ -385,13 +375,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)) @@ -412,42 +399,46 @@ 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, }, } @@ -457,10 +448,8 @@ func (suite *KeeperTestSuite) TestRefundFeesOnChannelClosure() { suite.Run(tc.name, func() { suite.SetupTest() // reset suite.coordinator.Setup(suite.path) // setup channel - expIdentifiedPacketFees = []types.IdentifiedPacketFees{} + expIdentifiedPacketFees = []types.IdentifiedPacketFees(nil) expEscrowBal = sdk.Coins{} - locked = false - expectEscrowFeesToBeDeleted = true // setup refundAcc = suite.chainA.SenderAccount.GetAddress() @@ -483,32 +472,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) } }) }