apply code changes from #1029

modules/apps/29-fee/ibc_module.go

@@ -148,21 +148,12 @@ func (im IBCModule) OnChanCloseInit(
  return err
  }
 
- // delete fee enabled on channel
- // and refund any remaining fees escrowed on channel
- im.keeper.DeleteFeeEnabled(ctx, portID, channelID)
- err := im.keeper.RefundFeesOnChannel(ctx, portID, channelID)
- // error should only be non-nil if there is a bug in the code
- // that causes module account to have insufficient funds to refund
- // all escrowed fees on the channel.
- // Disable all channels to allow for coordinated fix to the issue
- // and mitigate/reverse damage.
- // NOTE: Underlying application's packets will still go through, but
- // fee module will be disabled for all channels
- if err != nil {
- im.keeper.DisableAllChannels(ctx)
+ if err := im.keeper.RefundFeesOnChannelClosure(ctx, portID, channelID); err != nil {
+ return err
  }
 
+ im.keeper.DeleteFeeEnabled(ctx, portID, channelID)
+
  return nil
 }
 
@@ -172,21 +163,17 @@ func (im IBCModule) OnChanCloseConfirm(
  portID,
  channelID string,
 ) error {
- // delete fee enabled on channel
- // and refund any remaining fees escrowed on channel
- im.keeper.DeleteFeeEnabled(ctx, portID, channelID)
- err := im.keeper.RefundFeesOnChannel(ctx, portID, channelID)
- // error should only be non-nil if there is a bug in the code
- // that causes module account to have insufficient funds to refund
- // all escrowed fees on the channel.
- // Disable all channels to allow for coordinated fix to the issue
- // and mitigate/reverse damage.
- // NOTE: Underlying application's packets will still go through, but
- // fee module will be disabled for all channels
- if err != nil {
- im.keeper.DisableAllChannels(ctx)
+ if err := im.app.OnChanCloseConfirm(ctx, portID, channelID); err != nil {
+ return err
  }
- return im.app.OnChanCloseConfirm(ctx, portID, channelID)
+
+ if err := im.keeper.RefundFeesOnChannelClosure(ctx, portID, channelID); err != nil {
+ return err
+ }
+
+ im.keeper.DeleteFeeEnabled(ctx, portID, channelID)
+
+ return nil
 }
 
 // OnRecvPacket implements the IBCModule interface.

modules/apps/29-fee/keeper/escrow.go

@@ -112,37 +112,57 @@ func (k Keeper) distributeFee(ctx sdk.Context, receiver sdk.AccAddress, fee sdk.
  }
 }
 
-func (k Keeper) RefundFeesOnChannel(ctx sdk.Context, portID, channelID string) error {
+// RefundFeesOnChannelClosure will refund all fees associated with the given port and channel identifiers.
+// If the escrow account runs out of balance then fee logic will be disabled for all channels as this
+// implies a severe bug.
+func (k Keeper) RefundFeesOnChannelClosure(ctx sdk.Context, portID, channelID string) error {
+ identifiedPacketFees := k.GetIdentifiedPacketFeesForChannel(ctx, portID, channelID)
 
- var refundErr error
+ // 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()
+
+ for _, identifiedPacketFee := range identifiedPacketFees {
+ for _, packetFee := range identifiedPacketFee.PacketFees {
+
+ if !k.EscrowAccountHasBalance(cacheCtx, packetFee.Fee) {
+ // if the escrow account does not have sufficient funds then there must exist a severe bug
+ // the fee module should be locked until manual intervention fixes the issue
+ // a locked fee module will simply skip fee logic, all channels will temporarily function as
+ // fee disabled channels
+ // NOTE: we use the uncached context to lock the fee module so that the state changes from
+ // locking the fee module are persisted
+ lockFeeModule(ctx)
+
+ // return a nil error so state changes are committed but distribution stops
+ return nil
+ }
 
- k.IteratePacketFeesInEscrow(ctx, portID, channelID, func(packetFees types.PacketFees) (stop bool) {
- for _, identifiedFee := range packetFees.PacketFees {
- refundAccAddr, err := sdk.AccAddressFromBech32(identifiedFee.RefundAddress)
+ refundAccAddr, err := sdk.AccAddressFromBech32(packetFee.RefundAddress)
  if err != nil {
- refundErr = err
- return true
+ return err
  }
 
  // refund all fees to refund address
  // Use SendCoins rather than the module account send functions since refund address may be a user account or module address.
  // if any `SendCoins` call returns an error, we return error and stop iteration
- if err = k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, refundAccAddr, identifiedFee.Fee.RecvFee); err != nil {
- refundErr = err
- return true
+ if err = k.bankKeeper.SendCoinsFromModuleToAccount(cacheCtx, types.ModuleName, refundAccAddr, packetFee.Fee.RecvFee); err != nil {
+ return err
  }
- if err = k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, refundAccAddr, identifiedFee.Fee.AckFee); err != nil {
- refundErr = err
- return true
+ if err = k.bankKeeper.SendCoinsFromModuleToAccount(cacheCtx, types.ModuleName, refundAccAddr, packetFee.Fee.AckFee); err != nil {
+ return err
  }
- if err = k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, refundAccAddr, identifiedFee.Fee.TimeoutFee); err != nil {
- refundErr = err
- return true
+ if err = k.bankKeeper.SendCoinsFromModuleToAccount(cacheCtx, types.ModuleName, refundAccAddr, packetFee.Fee.TimeoutFee); err != nil {
+ return err
  }
+
  }
 
- return false
- })
+ k.DeleteFeesInEscrow(cacheCtx, identifiedPacketFee.PacketId)
+ }
 
- return refundErr
+ // write the cache
+ writeFn()
+
+ return nil
 }

modules/apps/29-fee/keeper/escrow_test.go

@@ -261,7 +261,7 @@ func (suite *KeeperTestSuite) TestDistributeTimeoutFee() {
  suite.Require().True(hasBalance)
 }
 
-func (suite *KeeperTestSuite) TestRefundFeesOnChannel() {
+func (suite *KeeperTestSuite) TestRefundFeesOnChannelClosure() {
  suite.coordinator.Setup(suite.path)
 
  // setup
@@ -298,7 +298,7 @@ func (suite *KeeperTestSuite) TestRefundFeesOnChannel() {
  suite.Require().NoError(err)
 
  // check that refunding all fees on channel-0 refunds all fees except for fee on channel-1
- err = suite.chainA.GetSimApp().IBCFeeKeeper.RefundFeesOnChannel(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID)
+ err = suite.chainA.GetSimApp().IBCFeeKeeper.RefundFeesOnChannelClosure(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID)
  suite.Require().NoError(err, "refund fees returned unexpected error")
 
  // add fee sent to channel-1 to after balance to recover original balance
@@ -314,6 +314,6 @@ func (suite *KeeperTestSuite) TestRefundFeesOnChannel() {
 
  suite.chainA.GetSimApp().BankKeeper.SendCoinsFromModuleToAccount(suite.chainA.GetContext(), types.ModuleName, refundAcc, fee.TimeoutFee)
 
- err = suite.chainA.GetSimApp().IBCFeeKeeper.RefundFeesOnChannel(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID)
+ err = suite.chainA.GetSimApp().IBCFeeKeeper.RefundFeesOnChannelClosure(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID)
  suite.Require().Error(err, "refund fees returned no error with insufficient balance on module account")
 }

modules/apps/29-fee/keeper/keeper.go

@@ -69,7 +69,7 @@ func (k Keeper) GetNextSequenceSend(ctx sdk.Context, portID, channelID string) (
  return k.channelKeeper.GetNextSequenceSend(ctx, portID, channelID)
 }
 
-// GetFeeAccount returns the ICS29 Fee ModuleAccount address
+// GetFeeModuleAddress returns the ICS29 Fee ModuleAccount address
 func (k Keeper) GetFeeModuleAddress() sdk.AccAddress {
  return k.authKeeper.GetModuleAddress(types.ModuleName)
 }
@@ -128,21 +128,6 @@ func (k Keeper) GetAllFeeEnabledChannels(ctx sdk.Context) []types.FeeEnabledChan
  return enabledChArr
 }
 
-// DisableAllChannels will disable the fee module for all channels.
-// Only called if the module enters into an invalid state
-// e.g. ModuleAccount has insufficient balance to refund users.
-// In this case, chain developers should investigate the issue, fix it,
-// and then re-enable the fee module in a coordinated upgrade.
-func (k Keeper) DisableAllChannels(ctx sdk.Context) {
- store := ctx.KVStore(k.storeKey)
- iterator := sdk.KVStorePrefixIterator(store, []byte(types.FeeEnabledKeyPrefix))
-
- defer iterator.Close()
- for ; iterator.Valid(); iterator.Next() {
- store.Delete(iterator.Key())
- }
-}
-
 // SetCounterpartyAddress maps the destination chain relayer address to the source relayer address
 // The receiving chain must store the mapping from: address -> counterpartyAddress for the given channel
 func (k Keeper) SetCounterpartyAddress(ctx sdk.Context, address, counterpartyAddress, channelID string) {
@@ -271,19 +256,27 @@ func (k Keeper) DeleteFeesInEscrow(ctx sdk.Context, packetID channeltypes.Packet
  store.Delete(key)
 }
 
-// IteratePacketFeesInEscrow iterates over all the fees on the given channel currently escrowed and calls the provided callback
-// if the callback returns true, then iteration is stopped.
-func (k Keeper) IteratePacketFeesInEscrow(ctx sdk.Context, portID, channelID string, cb func(packetFees types.PacketFees) (stop bool)) {
+// GetIdentifiedPacketFeesForChannel returns all the currently escrowed fees on a given channel.
+func (k Keeper) GetIdentifiedPacketFeesForChannel(ctx sdk.Context, portID, channelID string) []types.IdentifiedPacketFees {
+ var identifiedPacketFees []types.IdentifiedPacketFees
+
  store := ctx.KVStore(k.storeKey)
  iterator := sdk.KVStorePrefixIterator(store, types.KeyFeesInEscrowChannelPrefix(portID, channelID))
 
  defer iterator.Close()
  for ; iterator.Valid(); iterator.Next() {
- packetFees := k.MustUnmarshalFees(iterator.Value())
- if cb(packetFees) {
- break
+ packetId, err := types.ParseKeyFeesInEscrow(string(iterator.Key()))
+ if err != nil {
+ panic(err)
  }
+
+ packetFees := k.MustUnmarshalFees(iterator.Value())
+
+ identifiedFee := types.NewIdentifiedPacketFees(packetId, packetFees.PacketFees)
+ identifiedPacketFees = append(identifiedPacketFees, identifiedFee)
  }
+
+ return identifiedPacketFees
 }
 
 // GetAllIdentifiedPacketFees returns a list of all IdentifiedPacketFees that are stored in state

modules/apps/29-fee/keeper/keeper_test.go

@@ -111,21 +111,6 @@ func (suite *KeeperTestSuite) TestFeesInEscrow() {
  suite.Require().False(hasFeesInEscrow)
 }
 
-func (suite *KeeperTestSuite) TestDisableAllChannels() {
- suite.chainA.GetSimApp().IBCFeeKeeper.SetFeeEnabled(suite.chainA.GetContext(), "port1", "channel1")
- suite.chainA.GetSimApp().IBCFeeKeeper.SetFeeEnabled(suite.chainA.GetContext(), "port2", "channel2")
- suite.chainA.GetSimApp().IBCFeeKeeper.SetFeeEnabled(suite.chainA.GetContext(), "port3", "channel3")
-
- suite.chainA.GetSimApp().IBCFeeKeeper.DisableAllChannels(suite.chainA.GetContext())
-
- suite.Require().False(suite.chainA.GetSimApp().IBCFeeKeeper.IsFeeEnabled(suite.chainA.GetContext(), "port1", "channel1"),
- "fee is still enabled on channel-1 after DisableAllChannels call")
- suite.Require().False(suite.chainA.GetSimApp().IBCFeeKeeper.IsFeeEnabled(suite.chainA.GetContext(), "port2", "channel2"),
- "fee is still enabled on channel-2 after DisableAllChannels call")
- suite.Require().False(suite.chainA.GetSimApp().IBCFeeKeeper.IsFeeEnabled(suite.chainA.GetContext(), "port3", "channel3"),
- "fee is still enabled on channel-3 after DisableAllChannels call")
-}
-
 func (suite *KeeperTestSuite) TestGetAllIdentifiedPacketFees() {
  suite.coordinator.Setup(suite.path)