imp(fee): capital efficient fee middleware (cosmos#5571)

* feat: initial implementation * test: fixed keeper tests * test: fixed types tests * test: all tests passing * test: fixed fee callbacks test * feat: implemented migration * test: added migration tests * docs: updated godocs * imp: review items * imp: review items * imp: review items * docs: updated godocs * test: added multiple denoms test case for total * specify what the migration that fails does in error message --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
notional-labs · Jan 17, 2024 · 6fc8159 · 6fc8159
1 parent cf7cc66
commit 6fc8159
Show file tree

Hide file tree

Showing 12 changed files with 525 additions and 78 deletions.
diff --git a/modules/apps/29-fee/ibc_middleware_test.go b/modules/apps/29-fee/ibc_middleware_test.go
@@ -25,7 +25,6 @@ var (
 	defaultRecvFee    = sdk.Coins{sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: sdkmath.NewInt(100)}}
 	defaultAckFee     = sdk.Coins{sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: sdkmath.NewInt(200)}}
 	defaultTimeoutFee = sdk.Coins{sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: sdkmath.NewInt(300)}}
-	smallAmount       = sdk.Coins{sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: sdkmath.NewInt(50)}}
 )
 
 // Tests OnChanOpenInit on ChainA
@@ -605,6 +604,8 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() {
 		packetFee           types.PacketFee
 		refundAddr          sdk.AccAddress
 		relayerAddr         sdk.AccAddress
+		escrowAmount        sdk.Coins
+		initialRefundAccBal sdk.Coins
 		expRefundAccBalance sdk.Coins
 		expPayeeAccBalance  sdk.Coins
 	)
@@ -621,10 +622,31 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() {
 				// retrieve the relayer acc balance and add the expected recv and ack fees
 				relayerAccBalance := sdk.NewCoins(suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), relayerAddr, sdk.DefaultBondDenom))
 				expPayeeAccBalance = relayerAccBalance.Add(packetFee.Fee.RecvFee...).Add(packetFee.Fee.AckFee...)
+			},
+			true,
+			func() {
+				// assert that the packet fees have been distributed
+				found := suite.chainA.GetSimApp().IBCFeeKeeper.HasFeesInEscrow(suite.chainA.GetContext(), packetID)
+				suite.Require().False(found)
 
-				// retrieve the refund acc balance and add the expected timeout fees
-				refundAccBalance := sdk.NewCoins(suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAddr, sdk.DefaultBondDenom))
-				expRefundAccBalance = refundAccBalance.Add(packetFee.Fee.TimeoutFee...)
+				relayerAccBalance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), relayerAddr, sdk.DefaultBondDenom)
+				suite.Require().Equal(expPayeeAccBalance, sdk.NewCoins(relayerAccBalance))
+
+				refundAccBalance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAddr, sdk.DefaultBondDenom)
+				suite.Require().Equal(initialRefundAccBal, sdk.NewCoins(refundAccBalance))
+			},
+		},
+		{
+			"success: some refunds",
+			func() {
+				// set timeout_fee > recv_fee + ack_fee
+				packetFee.Fee.TimeoutFee = packetFee.Fee.Total().Add(sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, sdkmath.NewInt(100)))...)
+
+				escrowAmount = packetFee.Fee.Total()
+
+				// retrieve the relayer acc balance and add the expected recv and ack fees
+				relayerAccBalance := sdk.NewCoins(suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), relayerAddr, sdk.DefaultBondDenom))
+				expPayeeAccBalance = relayerAccBalance.Add(packetFee.Fee.RecvFee...).Add(packetFee.Fee.AckFee...)
 			},
 			true,
 			func() {
@@ -635,6 +657,9 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() {
 				relayerAccBalance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), relayerAddr, sdk.DefaultBondDenom)
 				suite.Require().Equal(expPayeeAccBalance, sdk.NewCoins(relayerAccBalance))
 
+				// expect the correct refunds
+				refundCoins := packetFee.Fee.Total().Sub(packetFee.Fee.RecvFee...).Sub(packetFee.Fee.AckFee...)
+				expRefundAccBalance = initialRefundAccBal.Add(refundCoins...)
 				refundAccBalance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAddr, sdk.DefaultBondDenom)
 				suite.Require().Equal(expRefundAccBalance, sdk.NewCoins(refundAccBalance))
 			},
@@ -656,10 +681,6 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() {
 				// retrieve the payee acc balance and add the expected recv and ack fees
 				payeeAccBalance := sdk.NewCoins(suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), payeeAddr, sdk.DefaultBondDenom))
 				expPayeeAccBalance = payeeAccBalance.Add(packetFee.Fee.RecvFee...).Add(packetFee.Fee.AckFee...)
-
-				// retrieve the refund acc balance and add the expected timeout fees
-				refundAccBalance := sdk.NewCoins(suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAddr, sdk.DefaultBondDenom))
-				expRefundAccBalance = refundAccBalance.Add(packetFee.Fee.TimeoutFee...)
 			},
 			true,
 			func() {
@@ -671,8 +692,9 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() {
 				payeeAccBalance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), payeeAddr, sdk.DefaultBondDenom)
 				suite.Require().Equal(expPayeeAccBalance, sdk.NewCoins(payeeAccBalance))
 
+				// expect zero refunds
 				refundAccBalance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAddr, sdk.DefaultBondDenom)
-				suite.Require().Equal(expRefundAccBalance, sdk.NewCoins(refundAccBalance))
+				suite.Require().Equal(initialRefundAccBal, sdk.NewCoins(refundAccBalance))
 			},
 		},
 		{
@@ -721,10 +743,6 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() {
 				// retrieve the relayer acc balance and add the expected ack fees
 				relayerAccBalance := sdk.NewCoins(suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), relayerAddr, sdk.DefaultBondDenom))
 				expPayeeAccBalance = relayerAccBalance.Add(packetFee.Fee.AckFee...)
-
-				// retrieve the refund acc balance and add the expected recv fees and timeout fees
-				refundAccBalance := sdk.NewCoins(suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAddr, sdk.DefaultBondDenom))
-				expRefundAccBalance = refundAccBalance.Add(packetFee.Fee.RecvFee...).Add(packetFee.Fee.TimeoutFee...)
 			},
 			true,
 			func() {
@@ -735,15 +753,16 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() {
 				relayerAccBalance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), relayerAddr, sdk.DefaultBondDenom)
 				suite.Require().Equal(expPayeeAccBalance, sdk.NewCoins(relayerAccBalance))
 
+				// expect only recv fee to be refunded
+				expRefundAccBalance = initialRefundAccBal.Add(packetFee.Fee.RecvFee...)
 				refundAccBalance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAddr, sdk.DefaultBondDenom)
 				suite.Require().Equal(expRefundAccBalance, sdk.NewCoins(refundAccBalance))
 			},
 		},
 		{
 			"fail: fee distribution fails and fee module is locked when escrow account does not have sufficient funds",
 			func() {
-				err := suite.chainA.GetSimApp().BankKeeper.SendCoinsFromModuleToAccount(suite.chainA.GetContext(), types.ModuleName, suite.chainA.SenderAccount.GetAddress(), smallAmount)
-				suite.Require().NoError(err)
+				escrowAmount = sdk.NewCoins()
 			},
 			true,
 			func() {
@@ -796,15 +815,18 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() {
 			packet := suite.CreateMockPacket()
 			packetID = channeltypes.NewPacketID(packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence())
 			packetFee = types.NewPacketFee(types.NewFee(defaultRecvFee, defaultAckFee, defaultTimeoutFee), refundAddr.String(), nil)
+			escrowAmount = packetFee.Fee.Total()
+
+			ack = types.NewIncentivizedAcknowledgement(relayerAddr.String(), ibcmock.MockAcknowledgement.Acknowledgement(), true).Acknowledgement()
+
+			tc.malleate() // malleate mutates test data
 
 			suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, types.NewPacketFees([]types.PacketFee{packetFee}))
 
-			err := suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAddr, types.ModuleName, packetFee.Fee.Total())
+			err := suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAddr, types.ModuleName, escrowAmount)
 			suite.Require().NoError(err)
 
-			ack = types.NewIncentivizedAcknowledgement(relayerAddr.String(), ibcmock.MockAcknowledgement.Acknowledgement(), true).Acknowledgement()
-
-			tc.malleate() // malleate mutates test data
+			initialRefundAccBal = sdk.NewCoins(suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAddr, sdk.DefaultBondDenom))
 
 			// retrieve module callbacks
 			module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), ibctesting.MockFeePort)
@@ -828,12 +850,14 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() {
 
 func (suite *FeeTestSuite) TestOnTimeoutPacket() {
 	var (
-		packetID            channeltypes.PacketId
-		packetFee           types.PacketFee
-		refundAddr          sdk.AccAddress
-		relayerAddr         sdk.AccAddress
-		expRefundAccBalance sdk.Coins
-		expPayeeAccBalance  sdk.Coins
+		packetID             channeltypes.PacketId
+		packetFee            types.PacketFee
+		refundAddr           sdk.AccAddress
+		relayerAddr          sdk.AccAddress
+		escrowAmount         sdk.Coins
+		initialRelayerAccBal sdk.Coins
+		expRefundAccBalance  sdk.Coins
+		expPayeeAccBalance   sdk.Coins
 	)
 
 	testCases := []struct {
@@ -843,22 +867,46 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() {
 		expResult func()
 	}{
 		{
-			"success",
+			"success: no refund",
 			func() {
-				// retrieve the relayer acc balance and add the expected timeout fees
-				relayerAccBalance := sdk.NewCoins(suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), relayerAddr, sdk.DefaultBondDenom))
-				expPayeeAccBalance = relayerAccBalance.Add(packetFee.Fee.TimeoutFee...)
+				// expect zero refunds
+				refundAccBalance := sdk.NewCoins(suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAddr, sdk.DefaultBondDenom))
+				expRefundAccBalance = refundAccBalance
+			},
+			true,
+			func() {
+				// assert that the packet fees have been distributed
+				found := suite.chainA.GetSimApp().IBCFeeKeeper.HasFeesInEscrow(suite.chainA.GetContext(), packetID)
+				suite.Require().False(found)
+
+				expPayeeAccBalance = initialRelayerAccBal.Add(packetFee.Fee.TimeoutFee...)
+				relayerAccBalance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), relayerAddr, sdk.DefaultBondDenom)
+				suite.Require().Equal(expPayeeAccBalance, sdk.NewCoins(relayerAccBalance))
+
+				refundAccBalance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAddr, sdk.DefaultBondDenom)
+				suite.Require().Equal(expRefundAccBalance, sdk.NewCoins(refundAccBalance))
+			},
+		},
+		{
+			"success: refund (recv_fee + ack_fee) - timeout_fee",
+			func() {
+				// set recv_fee + ack_fee > timeout_fee
+				packetFee.Fee.RecvFee = packetFee.Fee.Total().Add(sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, sdkmath.NewInt(100)))...)
+
+				escrowAmount = packetFee.Fee.Total()
 
 				// retrieve the refund acc balance and add the expected recv and ack fees
+				refundCoins := packetFee.Fee.Total().Sub(packetFee.Fee.TimeoutFee...)
 				refundAccBalance := sdk.NewCoins(suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAddr, sdk.DefaultBondDenom))
-				expRefundAccBalance = refundAccBalance.Add(packetFee.Fee.RecvFee...).Add(packetFee.Fee.AckFee...)
+				expRefundAccBalance = refundAccBalance.Add(refundCoins...)
 			},
 			true,
 			func() {
 				// assert that the packet fees have been distributed
 				found := suite.chainA.GetSimApp().IBCFeeKeeper.HasFeesInEscrow(suite.chainA.GetContext(), packetID)
 				suite.Require().False(found)
 
+				expPayeeAccBalance = initialRelayerAccBal.Add(packetFee.Fee.TimeoutFee...)
 				relayerAccBalance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), relayerAddr, sdk.DefaultBondDenom)
 				suite.Require().Equal(expPayeeAccBalance, sdk.NewCoins(relayerAccBalance))
 
@@ -881,9 +929,9 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() {
 				payeeAccBalance := sdk.NewCoins(suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), payeeAddr, sdk.DefaultBondDenom))
 				expPayeeAccBalance = payeeAccBalance.Add(packetFee.Fee.TimeoutFee...)
 
-				// retrieve the refund acc balance and add the expected recv and ack fees
+				// expect zero refunds
 				refundAccBalance := sdk.NewCoins(suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAddr, sdk.DefaultBondDenom))
-				expRefundAccBalance = refundAccBalance.Add(packetFee.Fee.RecvFee...).Add(packetFee.Fee.AckFee...)
+				expRefundAccBalance = refundAccBalance
 			},
 			true,
 			func() {
@@ -936,8 +984,7 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() {
 		{
 			"fee distribution fails and fee module is locked when escrow account does not have sufficient funds",
 			func() {
-				err := suite.chainA.GetSimApp().BankKeeper.SendCoinsFromModuleToAccount(suite.chainA.GetContext(), types.ModuleName, suite.chainA.SenderAccount.GetAddress(), smallAmount)
-				suite.Require().NoError(err)
+				escrowAmount = sdk.NewCoins()
 			},
 			true,
 			func() {
@@ -982,12 +1029,15 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() {
 			packet := suite.CreateMockPacket()
 			packetID = channeltypes.NewPacketID(packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence())
 			packetFee = types.NewPacketFee(types.NewFee(defaultRecvFee, defaultAckFee, defaultTimeoutFee), refundAddr.String(), nil)
+			escrowAmount = packetFee.Fee.Total()
+
+			tc.malleate() // malleate mutates test data
 
 			suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, types.NewPacketFees([]types.PacketFee{packetFee}))
-			err := suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress(), types.ModuleName, packetFee.Fee.Total())
+			err := suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress(), types.ModuleName, escrowAmount)
 			suite.Require().NoError(err)
 
-			tc.malleate() // malleate mutates test data
+			initialRelayerAccBal = sdk.NewCoins(suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), relayerAddr, sdk.DefaultBondDenom))
 
 			// retrieve module callbacks
 			module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), ibctesting.MockFeePort)

diff --git a/modules/apps/29-fee/keeper/escrow.go b/modules/apps/29-fee/keeper/escrow.go
@@ -97,8 +97,9 @@ func (k Keeper) distributePacketFeeOnAcknowledgement(ctx sdk.Context, refundAddr
 	// distribute fee for reverse relaying
 	k.distributeFee(ctx, reverseRelayer, refundAddr, packetFee.Fee.AckFee)
 
-	// refund timeout fee for unused timeout
-	k.distributeFee(ctx, refundAddr, refundAddr, packetFee.Fee.TimeoutFee)
+	// refund unused amount from the escrowed fee
+	refundCoins := packetFee.Fee.Total().Sub(packetFee.Fee.RecvFee...).Sub(packetFee.Fee.AckFee...)
+	k.distributeFee(ctx, refundAddr, refundAddr, refundCoins)
 }
 
 // DistributePacketsFeesOnTimeout pays all the timeout fees for a given packetID while refunding the acknowledgement & receive fees to the refund account.
@@ -137,14 +138,12 @@ func (k Keeper) DistributePacketFeesOnTimeout(ctx sdk.Context, timeoutRelayer sd
 
 // distributePacketFeeOnTimeout pays the timeout fee to the timeout relayer and refunds the acknowledgement & receive fee.
 func (k Keeper) distributePacketFeeOnTimeout(ctx sdk.Context, refundAddr, timeoutRelayer sdk.AccAddress, packetFee types.PacketFee) {
-	// refund receive fee for unused forward relaying
-	k.distributeFee(ctx, refundAddr, refundAddr, packetFee.Fee.RecvFee)
-
-	// refund ack fee for unused reverse relaying
-	k.distributeFee(ctx, refundAddr, refundAddr, packetFee.Fee.AckFee)
-
 	// distribute fee for timeout relaying
 	k.distributeFee(ctx, timeoutRelayer, refundAddr, packetFee.Fee.TimeoutFee)
+
+	// refund unused amount from the escrowed fee
+	refundCoins := packetFee.Fee.Total().Sub(packetFee.Fee.TimeoutFee...)
+	k.distributeFee(ctx, refundAddr, refundAddr, refundCoins)
 }
 
 // distributeFee will attempt to distribute the escrowed fee to the receiver address.