Return error if fee module is locked before distributing fees on ackn…

…owledgement and timeout (cosmos#1340) ## Description - Return error if fee module is locked - Move `DeleteFeesInEscrow` into `DistributePacketFees` func closes: cosmos#1320 ---
dymensionxyz · May 16, 2022 · b7b4400 · b7b4400
1 parent 2709c24
commit b7b4400
Show file tree

Hide file tree

Showing 4 changed files with 89 additions and 22 deletions.
diff --git a/modules/apps/29-fee/ibc_middleware.go b/modules/apps/29-fee/ibc_middleware.go
@@ -268,10 +268,7 @@ func (im IBCMiddleware) OnAcknowledgementPacket(
 	packetID := channeltypes.NewPacketId(packet.SourcePort, packet.SourceChannel, packet.Sequence)
 	feesInEscrow, found := im.keeper.GetFeesInEscrow(ctx, packetID)
 	if found {
-		im.keeper.DistributePacketFeesOnAcknowledgement(ctx, ack.ForwardRelayerAddress, relayer, feesInEscrow.PacketFees)
-
-		// removes the fees from the store as fees are now paid
-		im.keeper.DeleteFeesInEscrow(ctx, packetID)
+		im.keeper.DistributePacketFeesOnAcknowledgement(ctx, ack.ForwardRelayerAddress, relayer, feesInEscrow.PacketFees, packetID)
 	}
 
 	// call underlying callback
@@ -297,10 +294,7 @@ func (im IBCMiddleware) OnTimeoutPacket(
 	packetID := channeltypes.NewPacketId(packet.SourcePort, packet.SourceChannel, packet.Sequence)
 	feesInEscrow, found := im.keeper.GetFeesInEscrow(ctx, packetID)
 	if found {
-		im.keeper.DistributePacketFeesOnTimeout(ctx, relayer, feesInEscrow.PacketFees)
-
-		// removes the fee from the store as fee is now paid
-		im.keeper.DeleteFeesInEscrow(ctx, packetID)
+		im.keeper.DistributePacketFeesOnTimeout(ctx, relayer, feesInEscrow.PacketFees, packetID)
 	}
 
 	// call underlying callback

diff --git a/modules/apps/29-fee/ibc_middleware_test.go b/modules/apps/29-fee/ibc_middleware_test.go
@@ -20,6 +20,7 @@ var (
 	defaultRecvFee    = sdk.Coins{sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: sdk.NewInt(100)}}
 	defaultAckFee     = sdk.Coins{sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: sdk.NewInt(200)}}
 	defaultTimeoutFee = sdk.Coins{sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: sdk.NewInt(300)}}
+	smallAmount       = sdk.Coins{sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: sdk.NewInt(50)}}
 )
 
 // Tests OnChanOpenInit on ChainA
@@ -615,19 +616,22 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() {
 		originalBalance        sdk.Coins
 		expectedBalance        sdk.Coins
 		expectedRelayerBalance sdk.Coins
+		expLocked              bool
 	)
 
 	testCases := []struct {
-		name     string
-		malleate func()
-		expPass  bool
+		name               string
+		malleate           func()
+		expFeesDistributed bool
+		expPass            bool
 	}{
 		{
 			"success",
 			func() {
 				expectedRelayerBalance = packetFee.Fee.RecvFee.Add(packetFee.Fee.AckFee[0])
 			},
 			true,
+			true,
 		},
 		{
 			"no op success without a packet fee",
@@ -643,6 +647,7 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() {
 				expectedBalance = originalBalance
 			},
 			true,
+			true,
 		},
 		{
 			"ack wrong format",
@@ -652,6 +657,7 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() {
 				expectedBalance = originalBalance
 			},
 			false,
+			false,
 		},
 		{
 			"channel is not fee not enabled, success",
@@ -661,15 +667,18 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() {
 
 				expectedBalance = originalBalance
 			},
+			false,
 			true,
 		},
 		{
 			"success: fee module is disabled, skip fee logic",
 			func() {
 				lockFeeModule(suite.chainA)
+				expLocked = true
 
 				expectedBalance = originalBalance
 			},
+			false,
 			true,
 		},
 		{
@@ -685,6 +694,25 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() {
 				expectedRelayerBalance = packetFee.Fee.AckFee
 				expectedBalance = expectedBalance.Add(packetFee.Fee.RecvFee...)
 			},
+			false,
+			true,
+		},
+		{
+			"fail on no distribution by escrow account out of balance",
+			func() {
+				packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, suite.chainA.SenderAccount.GetSequence())
+				err := suite.chainA.GetSimApp().BankKeeper.SendCoinsFromModuleToAccount(suite.chainA.GetContext(), types.ModuleName, suite.chainA.SenderAccount.GetAddress(), smallAmount)
+				suite.Require().NoError(err)
+
+				// expectedBalance and expectedRelayerBalance don't change
+				expectedBalance = originalBalance.Add(smallAmount...)
+				expectedRelayerBalance = sdk.NewCoins()
+
+				suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, types.NewPacketFees([]types.PacketFee{packetFee}))
+
+				expLocked = true
+			},
+			false,
 			true,
 		},
 	}
@@ -693,6 +721,8 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() {
 		tc := tc
 		suite.Run(tc.name, func() {
 			suite.SetupTest()
+			expLocked = false
+
 			suite.coordinator.Setup(suite.path)
 			packet := suite.CreateMockPacket()
 
@@ -747,12 +777,20 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() {
 				suite.Require().Error(err)
 			}
 
+			suite.Require().Equal(expLocked, suite.chainA.GetSimApp().IBCFeeKeeper.IsLocked(suite.chainA.GetContext()))
+
 			suite.Require().Equal(
 				expectedBalance,
 				sdk.NewCoins(suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress(), ibctesting.TestCoin.Denom)),
 			)
 
 			relayerBalance := sdk.NewCoins(suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), relayerAddr, ibctesting.TestCoin.Denom))
+			if tc.expFeesDistributed {
+				// there should no longer be a fee in escrow for this packet
+				found := suite.chainA.GetSimApp().IBCFeeKeeper.HasFeesInEscrow(suite.chainA.GetContext(), packetID)
+				suite.Require().False(found)
+			}
+
 			suite.Require().Equal(
 				expectedRelayerBalance,
 				relayerBalance,
@@ -767,6 +805,7 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() {
 		packetFee       types.PacketFee
 		originalBalance sdk.Coins
 		expectedBalance sdk.Coins
+		expLocked       bool
 	)
 	testCases := []struct {
 		name              string
@@ -791,6 +830,7 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() {
 			"fee module is disabled, skip fee logic",
 			func() {
 				lockFeeModule(suite.chainA)
+				expLocked = true
 
 				expectedBalance = originalBalance
 			},
@@ -819,12 +859,30 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() {
 			},
 			false,
 		},
+		{
+			"fail on no distribution by escrow account out of balance",
+			func() {
+				packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, suite.chainA.SenderAccount.GetSequence())
+				err := suite.chainA.GetSimApp().BankKeeper.SendCoinsFromModuleToAccount(suite.chainA.GetContext(), types.ModuleName, suite.chainA.SenderAccount.GetAddress(), smallAmount)
+				suite.Require().NoError(err)
+
+				// expectedBalance don't change
+				expectedBalance = originalBalance.Add(smallAmount...)
+
+				suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, types.NewPacketFees([]types.PacketFee{packetFee}))
+
+				expLocked = true
+			},
+			false,
+		},
 	}
 
 	for _, tc := range testCases {
 		tc := tc
 		suite.Run(tc.name, func() {
 			suite.SetupTest()
+			expLocked = false
+
 			suite.coordinator.Setup(suite.path)
 			packet := suite.CreateMockPacket()
 
@@ -869,6 +927,8 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() {
 			err = cbs.OnTimeoutPacket(suite.chainA.GetContext(), packet, relayerAddr)
 			suite.Require().NoError(err)
 
+			suite.Require().Equal(expLocked, suite.chainA.GetSimApp().IBCFeeKeeper.IsLocked(suite.chainA.GetContext()))
+
 			suite.Require().Equal(
 				expectedBalance,
 				sdk.NewCoins(suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress(), ibctesting.TestCoin.Denom)),

diff --git a/modules/apps/29-fee/keeper/escrow.go b/modules/apps/29-fee/keeper/escrow.go
@@ -45,7 +45,7 @@ func (k Keeper) escrowPacketFee(ctx sdk.Context, packetID channeltypes.PacketId,
 }
 
 // DistributePacketFeesOnAcknowledgement pays all the acknowledgement & receive fees for a given packetID while refunding the timeout fees to the refund account.
-func (k Keeper) DistributePacketFeesOnAcknowledgement(ctx sdk.Context, forwardRelayer string, reverseRelayer sdk.AccAddress, packetFees []types.PacketFee) {
+func (k Keeper) DistributePacketFeesOnAcknowledgement(ctx sdk.Context, forwardRelayer string, reverseRelayer sdk.AccAddress, packetFees []types.PacketFee, packetID channeltypes.PacketId) {
 	// cache context before trying to distribute fees
 	// if the escrow account has insufficient balance then we want to avoid partially distributing fees
 	cacheCtx, writeFn := ctx.CacheContext()
@@ -61,7 +61,6 @@ func (k Keeper) DistributePacketFeesOnAcknowledgement(ctx sdk.Context, forwardRe
 			// NOTE: we use the uncached context to lock the fee module so that the state changes from
 			// locking the fee module are persisted
 			k.lockFeeModule(ctx)
-
 			return
 		}
 
@@ -74,11 +73,14 @@ func (k Keeper) DistributePacketFeesOnAcknowledgement(ctx sdk.Context, forwardRe
 		k.distributePacketFeeOnAcknowledgement(cacheCtx, refundAddr, forwardAddr, reverseRelayer, packetFee)
 	}
 
+	// NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context.
+	ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events())
+
 	// write the cache
 	writeFn()
 
-	// NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context.
-	ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events())
+	// removes the fees from the store as fees are now paid
+	k.DeleteFeesInEscrow(ctx, packetID)
 }
 
 // distributePacketFeeOnAcknowledgement pays the receive fee for a given packetID while refunding the timeout fee to the refund account associated with the Fee.
@@ -102,7 +104,7 @@ func (k Keeper) distributePacketFeeOnAcknowledgement(ctx sdk.Context, refundAddr
 }
 
 // DistributePacketsFeesOnTimeout pays all the timeout fees for a given packetID while refunding the acknowledgement & receive fees to the refund account.
-func (k Keeper) DistributePacketFeesOnTimeout(ctx sdk.Context, timeoutRelayer sdk.AccAddress, packetFees []types.PacketFee) {
+func (k Keeper) DistributePacketFeesOnTimeout(ctx sdk.Context, timeoutRelayer sdk.AccAddress, packetFees []types.PacketFee, packetID channeltypes.PacketId) {
 	// cache context before trying to distribute fees
 	// if the escrow account has insufficient balance then we want to avoid partially distributing fees
 	cacheCtx, writeFn := ctx.CacheContext()
@@ -116,7 +118,6 @@ func (k Keeper) DistributePacketFeesOnTimeout(ctx sdk.Context, timeoutRelayer sd
 			// NOTE: we use the uncached context to lock the fee module so that the state changes from
 			// locking the fee module are persisted
 			k.lockFeeModule(ctx)
-
 			return
 		}
 
@@ -129,11 +130,14 @@ func (k Keeper) DistributePacketFeesOnTimeout(ctx sdk.Context, timeoutRelayer sd
 		k.distributePacketFeeOnTimeout(cacheCtx, refundAddr, timeoutRelayer, packetFee)
 	}
 
+	// NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context.
+	ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events())
+
 	// write the cache
 	writeFn()
 
-	// NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context.
-	ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events())
+	// removing the fee from the store as the fee is now paid
+	k.DeleteFeesInEscrow(ctx, packetID)
 }
 
 // distributePacketFeeOnTimeout pays the timeout fee to the timeout relayer and refunds the acknowledgement & receive fee.

diff --git a/modules/apps/29-fee/keeper/escrow_test.go b/modules/apps/29-fee/keeper/escrow_test.go
@@ -30,6 +30,10 @@ func (suite *KeeperTestSuite) TestDistributeFee() {
 			"success",
 			func() {},
 			func() {
+				// check if fees has been deleted
+				packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, 1)
+				suite.Require().False(suite.chainA.GetSimApp().IBCFeeKeeper.HasFeesInEscrow(suite.chainA.GetContext(), packetID))
+
 				// check if the reverse relayer is paid
 				expectedReverseAccBal := reverseRelayerBal.Add(defaultAckFee[0]).Add(defaultAckFee[0])
 				balance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), reverseRelayer, sdk.DefaultBondDenom)
@@ -59,7 +63,10 @@ func (suite *KeeperTestSuite) TestDistributeFee() {
 				packetFees = append(packetFees, packetFee)
 			},
 			func() {
+				packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, 1)
+
 				suite.Require().True(suite.chainA.GetSimApp().IBCFeeKeeper.IsLocked(suite.chainA.GetContext()))
+				suite.Require().True(suite.chainA.GetSimApp().IBCFeeKeeper.HasFeesInEscrow(suite.chainA.GetContext(), packetID))
 
 				// check if the module acc contains all the fees
 				expectedModuleAccBal := packetFee.Fee.Total().Add(packetFee.Fee.Total()...)
@@ -149,8 +156,7 @@ func (suite *KeeperTestSuite) TestDistributeFee() {
 			reverseRelayerBal = suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), reverseRelayer, sdk.DefaultBondDenom)
 			refundAccBal = suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom)
 
-			suite.chainA.GetSimApp().IBCFeeKeeper.DistributePacketFeesOnAcknowledgement(suite.chainA.GetContext(), forwardRelayer, reverseRelayer, packetFees)
-
+			suite.chainA.GetSimApp().IBCFeeKeeper.DistributePacketFeesOnAcknowledgement(suite.chainA.GetContext(), forwardRelayer, reverseRelayer, packetFees, packetID)
 			tc.expResult()
 		})
 	}
@@ -196,7 +202,10 @@ func (suite *KeeperTestSuite) TestDistributePacketFeesOnTimeout() {
 				packetFees = append(packetFees, packetFee)
 			},
 			func() {
+				packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, 1)
+
 				suite.Require().True(suite.chainA.GetSimApp().IBCFeeKeeper.IsLocked(suite.chainA.GetContext()))
+				suite.Require().True(suite.chainA.GetSimApp().IBCFeeKeeper.HasFeesInEscrow(suite.chainA.GetContext(), packetID))
 
 				// check if the module acc contains all the fees
 				expectedModuleAccBal := packetFee.Fee.Total().Add(packetFee.Fee.Total()...)
@@ -259,7 +268,7 @@ func (suite *KeeperTestSuite) TestDistributePacketFeesOnTimeout() {
 			timeoutRelayerBal = suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), timeoutRelayer, sdk.DefaultBondDenom)
 			refundAccBal = suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom)
 
-			suite.chainA.GetSimApp().IBCFeeKeeper.DistributePacketFeesOnTimeout(suite.chainA.GetContext(), timeoutRelayer, packetFees)
+			suite.chainA.GetSimApp().IBCFeeKeeper.DistributePacketFeesOnTimeout(suite.chainA.GetContext(), timeoutRelayer, packetFees, packetID)
 
 			tc.expResult()
 		})