fix: delete already refunded fees from state if some fee cannot be re…

…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 <[email protected]>
cosmos · May 10, 2024 · 0cece8d · 0cece8d
1 parent 740532d
commit 0cece8d
Show file tree

Hide file tree

Showing 3 changed files with 41 additions and 58 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -57,6 +57,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
 

diff --git 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
@@ -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
@@ -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)})
@@ -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))
@@ -494,42 +480,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,
 		},
 	}
 
@@ -539,10 +529,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()
@@ -565,32 +553,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)
 			}
 		})
 	}