diff --git a/modules/apps/29-fee/ibc_module.go b/modules/apps/29-fee/ibc_module.go index 9d026a9f032..c542c5999e6 100644 --- a/modules/apps/29-fee/ibc_module.go +++ b/modules/apps/29-fee/ibc_module.go @@ -148,19 +148,8 @@ 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 } return nil @@ -172,21 +161,15 @@ 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 + } + + return nil } // OnRecvPacket implements the IBCModule interface. diff --git a/modules/apps/29-fee/ibc_module_test.go b/modules/apps/29-fee/ibc_module_test.go index 4ca50d2c612..1e5743f7490 100644 --- a/modules/apps/29-fee/ibc_module_test.go +++ b/modules/apps/29-fee/ibc_module_test.go @@ -16,9 +16,9 @@ import ( ) var ( - validCoins = sdk.Coins{sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: sdk.NewInt(100)}} - validCoins2 = sdk.Coins{sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: sdk.NewInt(200)}} - validCoins3 = sdk.Coins{sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: sdk.NewInt(300)}} + 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)}} ) // Tests OnChanOpenInit on ChainA @@ -291,47 +291,39 @@ func (suite *FeeTestSuite) TestOnChanOpenAck() { } } -// Tests OnChanCloseInit on chainA func (suite *FeeTestSuite) TestOnChanCloseInit() { + var ( + refundAcc sdk.AccAddress + fee types.Fee + ) + testCases := []struct { name string - setup func(suite *FeeTestSuite) - disabled bool + malleate func() + expPass bool }{ { - "success", - func(suite *FeeTestSuite) { - packetID := channeltypes.NewPacketId( - suite.path.EndpointA.ChannelConfig.PortID, - suite.path.EndpointA.ChannelID, - 1, - ) - refundAcc := suite.chainA.SenderAccount.GetAddress() - packetFee := types.NewPacketFee(types.Fee{validCoins, validCoins2, validCoins3}, refundAcc.String(), []string{}) - err := suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee) - suite.Require().NoError(err) - }, - false, + "success", func() {}, true, }, { - "module account balance insufficient", - func(suite *FeeTestSuite) { - packetID := channeltypes.NewPacketId( - suite.path.EndpointA.ChannelConfig.PortID, - suite.path.EndpointA.ChannelID, - 1, - ) - refundAcc := suite.chainA.SenderAccount.GetAddress() - packetFee := types.NewPacketFee(types.Fee{validCoins, validCoins2, validCoins3}, refundAcc.String(), []string{}) - err := suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee) - suite.Require().NoError(err) - - suite.chainA.GetSimApp().BankKeeper.SendCoinsFromModuleToAccount(suite.chainA.GetContext(), types.ModuleName, refundAcc, validCoins3) + "application callback fails", func() { + suite.chainA.GetSimApp().FeeMockModule.IBCApp.OnChanCloseInit = func( + ctx sdk.Context, portID, channelID string, + ) error { + return fmt.Errorf("application callback fails") + } + }, false, + }, + { + "RefundFeesOnChannelClosure fails - invalid refund address", func() { + // store the fee in state & update escrow account balance + packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, uint64(1)) + packetFees := types.NewPacketFees([]types.PacketFee{types.NewPacketFee(fee, "invalid refund address", nil)}) - // set fee enabled on different channel - suite.chainA.GetSimApp().IBCFeeKeeper.SetFeeEnabled(suite.chainA.GetContext(), "portID7", "channel-7") + suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, packetFees) + suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, fee.Total()) }, - true, + false, }, } @@ -341,9 +333,19 @@ func (suite *FeeTestSuite) TestOnChanCloseInit() { suite.SetupTest() suite.coordinator.Setup(suite.path) // setup channel - origBal := suite.chainA.GetSimApp().BankKeeper.GetAllBalances(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress()) + packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, 1) + fee = types.Fee{ + RecvFee: defaultRecvFee, + AckFee: defaultAckFee, + TimeoutFee: defaultTimeoutFee, + } - tc.setup(suite) + refundAcc = suite.chainA.SenderAccount.GetAddress() + packetFee := types.NewPacketFee(fee, refundAcc.String(), []string{}) + err := suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee) + suite.Require().NoError(err) + + tc.malleate() module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), ibctesting.MockFeePort) suite.Require().NoError(err) @@ -351,19 +353,12 @@ func (suite *FeeTestSuite) TestOnChanCloseInit() { cbs, ok := suite.chainA.App.GetIBCKeeper().Router.GetRoute(module) suite.Require().True(ok) - if tc.disabled { - suite.Require().True( - suite.chainA.GetSimApp().IBCFeeKeeper.IsFeeEnabled(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID), - "fee is not disabled on original channel: %s", suite.path.EndpointA.ChannelID, - ) - suite.Require().True( - suite.chainA.GetSimApp().IBCFeeKeeper.IsFeeEnabled(suite.chainA.GetContext(), "portID7", "channel-7"), - "fee is not disabled on other channel: %s", "channel-7", - ) + err = cbs.OnChanCloseInit(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID) + + if tc.expPass { + suite.Require().NoError(err) } else { - cbs.OnChanCloseInit(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID) - afterBal := suite.chainA.GetSimApp().BankKeeper.GetAllBalances(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress()) - suite.Require().Equal(origBal, afterBal, "balances of refund account not equal after all fees refunded") + suite.Require().Error(err) } }) } @@ -371,57 +366,61 @@ func (suite *FeeTestSuite) TestOnChanCloseInit() { // Tests OnChanCloseConfirm on chainA func (suite *FeeTestSuite) TestOnChanCloseConfirm() { + var ( + refundAcc sdk.AccAddress + fee types.Fee + ) + testCases := []struct { name string - setup func(suite *FeeTestSuite) - disabled bool + malleate func() + expPass bool }{ { - "success", - func(suite *FeeTestSuite) { - packetID := channeltypes.PacketId{ - PortId: suite.path.EndpointA.ChannelConfig.PortID, - ChannelId: suite.path.EndpointA.ChannelID, - Sequence: 1, - } - refundAcc := suite.chainA.SenderAccount.GetAddress() - packetFee := types.NewPacketFee(types.Fee{validCoins, validCoins2, validCoins3}, refundAcc.String(), []string{}) - err := suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee) - suite.Require().NoError(err) - }, - false, + "success", func() {}, true, }, { - "module account balance insufficient", - func(suite *FeeTestSuite) { - packetID := channeltypes.PacketId{ - PortId: suite.path.EndpointA.ChannelConfig.PortID, - ChannelId: suite.path.EndpointA.ChannelID, - Sequence: 1, + "application callback fails", func() { + suite.chainA.GetSimApp().FeeMockModule.IBCApp.OnChanCloseConfirm = func( + ctx sdk.Context, portID, channelID string, + ) error { + return fmt.Errorf("application callback fails") } - refundAcc := suite.chainA.SenderAccount.GetAddress() - packetFee := types.NewPacketFee(types.Fee{validCoins, validCoins2, validCoins3}, refundAcc.String(), []string{}) - err := suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee) - suite.Require().NoError(err) - - suite.chainA.GetSimApp().BankKeeper.SendCoinsFromModuleToAccount(suite.chainA.GetContext(), types.ModuleName, refundAcc, validCoins3) + }, false, + }, + { + "RefundChannelFeesOnClosure fails - refund address is invalid", func() { + // store the fee in state & update escrow account balance + packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, uint64(1)) + packetFees := types.NewPacketFees([]types.PacketFee{types.NewPacketFee(fee, "invalid refund address", nil)}) - // set fee enabled on different channel - suite.chainA.GetSimApp().IBCFeeKeeper.SetFeeEnabled(suite.chainA.GetContext(), "portID7", "channel-7") + suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, packetFees) + suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, fee.Total()) }, - true, + false, }, } for _, tc := range testCases { tc := tc + suite.Run(tc.name, func() { suite.SetupTest() suite.coordinator.Setup(suite.path) // setup channel - origBal := suite.chainA.GetSimApp().BankKeeper.GetAllBalances(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress()) + packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, 1) + fee = types.Fee{ + RecvFee: defaultRecvFee, + AckFee: defaultAckFee, + TimeoutFee: defaultTimeoutFee, + } - tc.setup(suite) + refundAcc = suite.chainA.SenderAccount.GetAddress() + packetFee := types.NewPacketFee(fee, refundAcc.String(), []string{}) + err := suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee) + suite.Require().NoError(err) + + tc.malleate() module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), ibctesting.MockFeePort) suite.Require().NoError(err) @@ -429,20 +428,14 @@ func (suite *FeeTestSuite) TestOnChanCloseConfirm() { cbs, ok := suite.chainA.App.GetIBCKeeper().Router.GetRoute(module) suite.Require().True(ok) - if tc.disabled { - suite.Require().True( - suite.chainA.GetSimApp().IBCFeeKeeper.IsFeeEnabled(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID), - "fee is not disabled on original channel: %s", suite.path.EndpointA.ChannelID, - ) - suite.Require().True( - suite.chainA.GetSimApp().IBCFeeKeeper.IsFeeEnabled(suite.chainA.GetContext(), "portID7", "channel-7"), - "fee is not disabled on other channel: %s", "channel-7", - ) + err = cbs.OnChanCloseConfirm(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID) + + if tc.expPass { + suite.Require().NoError(err) } else { - cbs.OnChanCloseConfirm(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID) - afterBal := suite.chainA.GetSimApp().BankKeeper.GetAllBalances(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress()) - suite.Require().Equal(origBal, afterBal, "balances of refund account not equal after all fees refunded") + suite.Require().Error(err) } + }) } } @@ -657,9 +650,9 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() { packetID := channeltypes.NewPacketId(packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence()) packetFee = types.NewPacketFee( types.Fee{ - RecvFee: validCoins, - AckFee: validCoins2, - TimeoutFee: validCoins3, + RecvFee: defaultRecvFee, + AckFee: defaultAckFee, + TimeoutFee: defaultTimeoutFee, }, suite.chainA.SenderAccount.GetAddress().String(), []string{}, @@ -788,9 +781,9 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() { packetFee = types.NewPacketFee( types.Fee{ - RecvFee: validCoins, - AckFee: validCoins2, - TimeoutFee: validCoins3, + RecvFee: defaultRecvFee, + AckFee: defaultAckFee, + TimeoutFee: defaultTimeoutFee, }, suite.chainA.SenderAccount.GetAddress().String(), []string{}, diff --git a/modules/apps/29-fee/keeper/escrow.go b/modules/apps/29-fee/keeper/escrow.go index f0c18e379ba..8f201293860 100644 --- a/modules/apps/29-fee/keeper/escrow.go +++ b/modules/apps/29-fee/keeper/escrow.go @@ -124,37 +124,57 @@ func (k Keeper) distributeFee(ctx sdk.Context, receiver, refundAccAddress sdk.Ac ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events()) } -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 module will become locked as this implies the presence +// of a severe bug. When the fee module is locked, no fee distributions will be performed. +// Please see ADR 004 for more information. +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.Total()) { + // 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 + k.lockFeeModule(ctx) - k.IteratePacketFeesInEscrow(ctx, portID, channelID, func(packetFees types.PacketFees) (stop bool) { - for _, identifiedFee := range packetFees.PacketFees { - refundAccAddr, err := sdk.AccAddressFromBech32(identifiedFee.RefundAddress) + // return a nil error so state changes are committed but distribution stops + return nil + } + + refundAddr, err := sdk.AccAddressFromBech32(packetFee.RefundAddress) if err != nil { - refundErr = err - return true + return err + } + + // if the refund address is blocked, skip and continue distribution + if k.bankKeeper.BlockedAddr(refundAddr) { + continue } // 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(ctx, types.ModuleName, refundAccAddr, identifiedFee.Fee.AckFee); err != nil { - refundErr = err - return true - } - if err = k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, refundAccAddr, identifiedFee.Fee.TimeoutFee); err != nil { - refundErr = err - return true + moduleAcc := k.GetFeeModuleAddress() + if err = k.bankKeeper.SendCoins(cacheCtx, moduleAcc, refundAddr, packetFee.Fee.Total()); err != nil { + return err } + } - return false - }) + k.DeleteFeesInEscrow(cacheCtx, identifiedPacketFee.PacketId) + } + + // write the cache + writeFn() - return refundErr + return nil } diff --git a/modules/apps/29-fee/keeper/escrow_test.go b/modules/apps/29-fee/keeper/escrow_test.go index 052a8cdc9c1..85747ae7acf 100644 --- a/modules/apps/29-fee/keeper/escrow_test.go +++ b/modules/apps/29-fee/keeper/escrow_test.go @@ -7,7 +7,6 @@ import ( "github.com/cosmos/ibc-go/v3/modules/apps/29-fee/types" transfertypes "github.com/cosmos/ibc-go/v3/modules/apps/transfer/types" channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types" - ibctesting "github.com/cosmos/ibc-go/v3/testing" ) func (suite *KeeperTestSuite) TestEscrowPacketFee() { @@ -300,59 +299,193 @@ func (suite *KeeperTestSuite) TestDistributeTimeoutFee() { suite.Require().True(hasBalance) } -func (suite *KeeperTestSuite) TestRefundFeesOnChannel() { - suite.coordinator.Setup(suite.path) +func (suite *KeeperTestSuite) TestRefundFeesOnChannelClosure() { + var ( + expIdentifiedPacketFees []types.IdentifiedPacketFees + expEscrowBal sdk.Coins + expRefundBal sdk.Coins + refundAcc sdk.AccAddress + fee types.Fee + locked bool + ) - // setup - refundAcc := suite.chainA.SenderAccount.GetAddress() + testCases := []struct { + name string + malleate func() + expPass bool + }{ + { + "success", func() { + for i := 1; i < 6; i++ { + // store the fee in state & update escrow account balance + packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, uint64(i)) + packetFees := types.NewPacketFees([]types.PacketFee{types.NewPacketFee(fee, refundAcc.String(), nil)}) + identifiedPacketFees := types.NewIdentifiedPacketFees(packetID, packetFees.PacketFees) - // refundAcc balance before escrow - prevBal := suite.chainA.GetSimApp().BankKeeper.GetAllBalances(suite.chainA.GetContext(), refundAcc) - - for i := 0; i < 5; i++ { - packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, uint64(i)) - fee := types.Fee{ - RecvFee: defaultReceiveFee, - AckFee: defaultAckFee, - TimeoutFee: defaultTimeoutFee, - } - - packetFee := types.NewPacketFee(fee, refundAcc.String(), []string{}) - suite.chainA.GetSimApp().IBCFeeKeeper.SetFeeEnabled(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID) - err := suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee) - suite.Require().NoError(err) - } + suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, packetFees) - // send a packet over a different channel to ensure this fee is not refunded - packetID := channeltypes.NewPacketId(ibctesting.MockFeePort, "channel-1", 1) - fee := types.Fee{ - RecvFee: defaultReceiveFee, - AckFee: defaultAckFee, - TimeoutFee: defaultTimeoutFee, + suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, fee.Total()) + + expIdentifiedPacketFees = append(expIdentifiedPacketFees, identifiedPacketFees) + } + }, true, + }, + { + "success with undistributed packet fees on a different channel", func() { + for i := 1; i < 6; i++ { + // store the fee in state & update escrow account balance + packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, uint64(i)) + packetFees := types.NewPacketFees([]types.PacketFee{types.NewPacketFee(fee, refundAcc.String(), nil)}) + identifiedPacketFees := types.NewIdentifiedPacketFees(packetID, packetFees.PacketFees) + + suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, packetFees) + + suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, fee.Total()) + + expIdentifiedPacketFees = append(expIdentifiedPacketFees, identifiedPacketFees) + } + + // set packet fee for a different channel + packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, "channel-1", uint64(1)) + packetFees := types.NewPacketFees([]types.PacketFee{types.NewPacketFee(fee, refundAcc.String(), nil)}) + suite.chainA.GetSimApp().IBCFeeKeeper.SetFeeEnabled(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, "channel-1") + + suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, packetFees) + suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, fee.Total()) + + expEscrowBal = fee.Total() + expRefundBal = expRefundBal.Sub(fee.Total()) + }, true, + }, + { + "escrow account empty, module should become locked", func() { + locked = true + + // store the fee in state without updating escrow account balance + packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, uint64(1)) + packetFees := types.NewPacketFees([]types.PacketFee{types.NewPacketFee(fee, refundAcc.String(), nil)}) + identifiedPacketFees := types.NewIdentifiedPacketFees(packetID, packetFees.PacketFees) + + suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, packetFees) + + expIdentifiedPacketFees = []types.IdentifiedPacketFees{identifiedPacketFees} + }, + true, + }, + { + "escrow account goes negative on second packet, module should become locked", func() { + locked = true + + // store 2 fees in state + packetID1 := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, uint64(1)) + packetID2 := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, uint64(2)) + packetFees := types.NewPacketFees([]types.PacketFee{types.NewPacketFee(fee, refundAcc.String(), nil)}) + identifiedPacketFee1 := types.NewIdentifiedPacketFees(packetID1, packetFees.PacketFees) + identifiedPacketFee2 := types.NewIdentifiedPacketFees(packetID2, packetFees.PacketFees) + + suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID1, packetFees) + suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID2, packetFees) + + // update escrow account balance for 1 fee + suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, fee.Total()) + + expIdentifiedPacketFees = []types.IdentifiedPacketFees{identifiedPacketFee1, identifiedPacketFee2} + }, true, + }, + { + "invalid refund acc address", func() { + // store the fee in state & update escrow account balance + packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, uint64(1)) + packetFees := types.NewPacketFees([]types.PacketFee{types.NewPacketFee(fee, "invalid refund address", nil)}) + identifiedPacketFees := types.NewIdentifiedPacketFees(packetID, packetFees.PacketFees) + + suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, packetFees) + + suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, fee.Total()) + + expIdentifiedPacketFees = []types.IdentifiedPacketFees{identifiedPacketFees} + }, false, + }, + { + "distributing to blocked address is skipped", func() { + blockedAddr := suite.chainA.GetSimApp().AccountKeeper.GetModuleAccount(suite.chainA.GetContext(), transfertypes.ModuleName).GetAddress().String() + + // store the fee in state & update escrow account balance + packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, uint64(1)) + packetFees := types.NewPacketFees([]types.PacketFee{types.NewPacketFee(fee, blockedAddr, nil)}) + identifiedPacketFees := types.NewIdentifiedPacketFees(packetID, packetFees.PacketFees) + + suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, packetFees) + + suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, fee.Total()) + + expIdentifiedPacketFees = []types.IdentifiedPacketFees{identifiedPacketFees} + + expEscrowBal = fee.Total() + expRefundBal = expRefundBal.Sub(fee.Total()) + }, true, + }, } - packetFee := types.NewPacketFee(fee, refundAcc.String(), []string{}) - suite.chainA.GetSimApp().IBCFeeKeeper.SetFeeEnabled(suite.chainA.GetContext(), ibctesting.MockFeePort, "channel-1") - err := suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee) - suite.Require().NoError(err) + for _, tc := range testCases { + tc := tc - // 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) - suite.Require().NoError(err, "refund fees returned unexpected error") + suite.Run(tc.name, func() { + suite.SetupTest() // reset + suite.coordinator.Setup(suite.path) // setup channel + expIdentifiedPacketFees = []types.IdentifiedPacketFees{} + expEscrowBal = sdk.Coins{} + locked = false - // add fee sent to channel-1 to after balance to recover original balance - afterBal := suite.chainA.GetSimApp().BankKeeper.GetAllBalances(suite.chainA.GetContext(), refundAcc) - suite.Require().Equal(prevBal, afterBal.Add(fee.RecvFee...).Add(fee.AckFee...).Add(fee.TimeoutFee...), "refund account not back to original balance after refunding all tokens") + // setup + refundAcc = suite.chainA.SenderAccount.GetAddress() + moduleAcc := suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress() - // create escrow and then change module account balance to cause error on refund - packetID = channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, uint64(6)) + // expected refund balance if the refunds are successful + // NOTE: tc.malleate() should transfer from refund balance to correctly set the escrow balance + expRefundBal = suite.chainA.GetSimApp().BankKeeper.GetAllBalances(suite.chainA.GetContext(), refundAcc) - packetFee = types.NewPacketFee(fee, refundAcc.String(), []string{}) - err = suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee) - suite.Require().NoError(err) + fee = types.Fee{ + RecvFee: defaultReceiveFee, + AckFee: defaultAckFee, + TimeoutFee: defaultTimeoutFee, + } + + tc.malleate() + + // refundAcc balance before distribution + originalRefundBal := suite.chainA.GetSimApp().BankKeeper.GetAllBalances(suite.chainA.GetContext(), refundAcc) + originalEscrowBal := suite.chainA.GetSimApp().BankKeeper.GetAllBalances(suite.chainA.GetContext(), moduleAcc) - suite.chainA.GetSimApp().BankKeeper.SendCoinsFromModuleToAccount(suite.chainA.GetContext(), types.ModuleName, refundAcc, fee.TimeoutFee) + err := suite.chainA.GetSimApp().IBCFeeKeeper.RefundFeesOnChannelClosure(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID) - err = suite.chainA.GetSimApp().IBCFeeKeeper.RefundFeesOnChannel(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") + // refundAcc balance after RefundFeesOnChannelClosure + refundBal := suite.chainA.GetSimApp().BankKeeper.GetAllBalances(suite.chainA.GetContext(), refundAcc) + escrowBal := suite.chainA.GetSimApp().BankKeeper.GetAllBalances(suite.chainA.GetContext(), moduleAcc) + + if tc.expPass { + suite.Require().NoError(err) + } else { + suite.Require().Error(err) + } + + suite.Require().Equal(locked, suite.chainA.GetSimApp().IBCFeeKeeper.IsLocked(suite.chainA.GetContext())) + + if locked || !tc.expPass { + // refund account and escrow account balances should remain unchanged + suite.Require().Equal(originalRefundBal, refundBal) + suite.Require().Equal(originalEscrowBal, escrowBal) + + // ensure none of the fees were deleted + suite.Require().Equal(expIdentifiedPacketFees, suite.chainA.GetSimApp().IBCFeeKeeper.GetIdentifiedPacketFeesForChannel(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID)) + } else { + suite.Require().Equal(expEscrowBal, escrowBal) // escrow balance should be empty + suite.Require().Equal(expRefundBal, refundBal) // all packets should have been refunded + + // all fees in escrow should be deleted for this channel + suite.Require().Empty(suite.chainA.GetSimApp().IBCFeeKeeper.GetIdentifiedPacketFeesForChannel(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID)) + } + + }) + } } diff --git a/modules/apps/29-fee/keeper/keeper.go b/modules/apps/29-fee/keeper/keeper.go index 2ebec6aaf42..e0317d3660a 100644 --- a/modules/apps/29-fee/keeper/keeper.go +++ b/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) } @@ -143,21 +143,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) { @@ -286,19 +271,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 diff --git a/modules/apps/29-fee/keeper/keeper_test.go b/modules/apps/29-fee/keeper/keeper_test.go index 5c44e45c332..cf0d17ae494 100644 --- a/modules/apps/29-fee/keeper/keeper_test.go +++ b/modules/apps/29-fee/keeper/keeper_test.go @@ -76,11 +76,7 @@ func lockFeeModule(chain *ibctesting.TestChain) { } func (suite *KeeperTestSuite) TestEscrowAccountHasBalance() { - fee := types.Fee{ - AckFee: defaultAckFee, - RecvFee: defaultReceiveFee, - TimeoutFee: defaultTimeoutFee, - } + fee := types.NewFee(defaultReceiveFee, defaultAckFee, defaultTimeoutFee) suite.Require().False(suite.chainA.GetSimApp().IBCFeeKeeper.EscrowAccountHasBalance(suite.chainA.GetContext(), fee.Total())) @@ -127,19 +123,68 @@ func (suite *KeeperTestSuite) TestIsLocked() { suite.Require().True(suite.chainA.GetSimApp().IBCFeeKeeper.IsLocked(ctx)) } -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") +func (suite *KeeperTestSuite) TestGetIdentifiedPacketFeesForChannel() { + suite.coordinator.Setup(suite.path) + + // escrow a fee + refundAcc := suite.chainA.SenderAccount.GetAddress() + packetID1 := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, 1) + packetID2 := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, 2) + packetID5 := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, 51) + + fee := types.NewFee(defaultReceiveFee, defaultAckFee, defaultTimeoutFee) + + // escrow the packet fee + packetFee := types.NewPacketFee(fee, refundAcc.String(), []string{}) + suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID1, types.NewPacketFees([]types.PacketFee{packetFee})) + suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID2, types.NewPacketFees([]types.PacketFee{packetFee})) + suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID5, types.NewPacketFees([]types.PacketFee{packetFee})) + + // set fees in escrow for packetIDs on different channel + diffChannel := "channel-1" + diffPacketID1 := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, diffChannel, 1) + diffPacketID2 := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, diffChannel, 2) + diffPacketID5 := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, diffChannel, 5) + suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), diffPacketID1, types.NewPacketFees([]types.PacketFee{packetFee})) + suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), diffPacketID2, types.NewPacketFees([]types.PacketFee{packetFee})) + suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), diffPacketID5, types.NewPacketFees([]types.PacketFee{packetFee})) - suite.chainA.GetSimApp().IBCFeeKeeper.DisableAllChannels(suite.chainA.GetContext()) + expectedFees := []types.IdentifiedPacketFees{ + { + PacketId: packetID1, + PacketFees: []types.PacketFee{ + { + Fee: fee, + RefundAddress: refundAcc.String(), + Relayers: nil, + }, + }, + }, + { + PacketId: packetID2, + PacketFees: []types.PacketFee{ + { + Fee: fee, + RefundAddress: refundAcc.String(), + Relayers: nil, + }, + }, + }, + { + PacketId: packetID5, + PacketFees: []types.PacketFee{ + { + Fee: fee, + RefundAddress: refundAcc.String(), + Relayers: nil, + }, + }, + }, + } - 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") + identifiedFees := suite.chainA.GetSimApp().IBCFeeKeeper.GetIdentifiedPacketFeesForChannel(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID) + suite.Require().Len(identifiedFees, len(expectedFees)) + suite.Require().Equal(identifiedFees, expectedFees) } func (suite *KeeperTestSuite) TestGetAllIdentifiedPacketFees() { @@ -148,11 +193,7 @@ func (suite *KeeperTestSuite) TestGetAllIdentifiedPacketFees() { // escrow a fee refundAcc := suite.chainA.SenderAccount.GetAddress() packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, 1) - fee := types.Fee{ - AckFee: defaultAckFee, - RecvFee: defaultReceiveFee, - TimeoutFee: defaultTimeoutFee, - } + fee := types.NewFee(defaultReceiveFee, defaultAckFee, defaultTimeoutFee) // escrow the packet fee packetFee := types.NewPacketFee(fee, refundAcc.String(), []string{}) diff --git a/modules/apps/29-fee/transfer_test.go b/modules/apps/29-fee/transfer_test.go index a07d841d07c..9d7557fd6c4 100644 --- a/modules/apps/29-fee/transfer_test.go +++ b/modules/apps/29-fee/transfer_test.go @@ -23,9 +23,9 @@ func (suite *FeeTestSuite) TestFeeTransfer() { // set up coin & ics20 packet coin := ibctesting.TestCoin fee := types.Fee{ - RecvFee: validCoins, - AckFee: validCoins2, - TimeoutFee: validCoins3, + RecvFee: defaultRecvFee, + AckFee: defaultAckFee, + TimeoutFee: defaultTimeoutFee, } msgs := []sdk.Msg{