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
---

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

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)),

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.

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()
  })