diff --git a/modules/apps/29-fee/ibc_middleware.go b/modules/apps/29-fee/ibc_middleware.go index bc1933486bb..7178c14ab4b 100644 --- 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 index 5abda336e47..da58e1ae627 100644 --- 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,12 +616,14 @@ 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", @@ -628,6 +631,7 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() { 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 index 2c5b570320e..7333b36353d 100644 --- 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 index e2a43afe586..4ac6e2ed03f 100644 --- 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() })