diff --git a/CHANGELOG.md b/CHANGELOG.md index 31ca5c1539f..ff0d73923e3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -79,7 +79,8 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Bug Fixes * (modules/core/04-channel) [\#1130](https://github.com/cosmos/ibc-go/pull/1130) Call `packet.GetSequence()` rather than passing func in `WriteAcknowledgement` log output -* (apps/29-fee) [\#1278](https://github.com/cosmos/ibc-go/pull/1278) The URI path for the query to get all incentivized packets for a specifc channel did not follow the same format as the rest of queries. +* (apps/29-fee) [\#1278](https://github.com/cosmos/ibc-go/pull/1278) The URI path for the query to get all incentivized packets for a specific channel did not follow the same format as the rest of queries. +* (apps/29-fee)[\#1343](https://github.com/cosmos/ibc-go/pull/1523) Fixed an issue where a bad refund address would prevent channel closure. ## [v3.0.0](https://github.com/cosmos/ibc-go/releases/tag/v3.0.0) - 2022-03-15 diff --git a/modules/apps/29-fee/ibc_middleware_test.go b/modules/apps/29-fee/ibc_middleware_test.go index 17a961d0074..37cfaf2f948 100644 --- a/modules/apps/29-fee/ibc_middleware_test.go +++ b/modules/apps/29-fee/ibc_middleware_test.go @@ -345,7 +345,7 @@ func (suite *FeeTestSuite) TestOnChanCloseInit() { }, false, }, { - "RefundFeesOnChannelClosure fails - invalid refund address", func() { + "RefundFeesOnChannelClosure continues - 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)}) @@ -354,7 +354,7 @@ func (suite *FeeTestSuite) TestOnChanCloseInit() { err := suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, fee.Total()) suite.Require().NoError(err) }, - false, + true, }, { "fee module locked", func() { @@ -434,7 +434,7 @@ func (suite *FeeTestSuite) TestOnChanCloseConfirm() { }, false, }, { - "RefundChannelFeesOnClosure fails - refund address is invalid", func() { + "RefundChannelFeesOnClosure continues - 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)}) @@ -443,7 +443,7 @@ func (suite *FeeTestSuite) TestOnChanCloseConfirm() { err := suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, fee.Total()) suite.Require().NoError(err) }, - false, + true, }, { "fee module locked", func() { diff --git a/modules/apps/29-fee/keeper/escrow.go b/modules/apps/29-fee/keeper/escrow.go index 130ac18e1b3..9755fb45e8d 100644 --- a/modules/apps/29-fee/keeper/escrow.go +++ b/modules/apps/29-fee/keeper/escrow.go @@ -194,6 +194,7 @@ func (k Keeper) RefundFeesOnChannelClosure(ctx sdk.Context, portID, channelID st cacheCtx, writeFn := ctx.CacheContext() for _, identifiedPacketFee := range identifiedPacketFees { + var failedToSendCoins bool for _, packetFee := range identifiedPacketFee.PacketFees { if !k.EscrowAccountHasBalance(cacheCtx, packetFee.Fee.Total()) { @@ -211,24 +212,20 @@ func (k Keeper) RefundFeesOnChannelClosure(ctx sdk.Context, portID, channelID st refundAddr, err := sdk.AccAddressFromBech32(packetFee.RefundAddress) if err != nil { - return err - } - - // if the refund address is blocked, skip and continue distribution - if k.bankKeeper.BlockedAddr(refundAddr) { + failedToSendCoins = true 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. - moduleAcc := k.GetFeeModuleAddress() - if err = k.bankKeeper.SendCoins(cacheCtx, moduleAcc, refundAddr, packetFee.Fee.Total()); err != nil { - return err + if err = k.bankKeeper.SendCoinsFromModuleToAccount(cacheCtx, types.ModuleName, refundAddr, packetFee.Fee.Total()); err != nil { + failedToSendCoins = true + continue } - } - k.DeleteFeesInEscrow(cacheCtx, identifiedPacketFee.PacketId) + if !failedToSendCoins { + k.DeleteFeesInEscrow(cacheCtx, identifiedPacketFee.PacketId) + } } // NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context. diff --git a/modules/apps/29-fee/keeper/escrow_test.go b/modules/apps/29-fee/keeper/escrow_test.go index 9b67934c647..7ccf87aa91b 100644 --- a/modules/apps/29-fee/keeper/escrow_test.go +++ b/modules/apps/29-fee/keeper/escrow_test.go @@ -2,11 +2,10 @@ package keeper_test import ( sdk "github.com/cosmos/cosmos-sdk/types" - "github.com/tendermint/tendermint/crypto/secp256k1" - "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" + "github.com/tendermint/tendermint/crypto/secp256k1" ) func (suite *KeeperTestSuite) TestDistributeFee() { @@ -277,12 +276,13 @@ func (suite *KeeperTestSuite) TestDistributePacketFeesOnTimeout() { func (suite *KeeperTestSuite) TestRefundFeesOnChannelClosure() { var ( - expIdentifiedPacketFees []types.IdentifiedPacketFees - expEscrowBal sdk.Coins - expRefundBal sdk.Coins - refundAcc sdk.AccAddress - fee types.Fee - locked bool + expIdentifiedPacketFees []types.IdentifiedPacketFees + expEscrowBal sdk.Coins + expRefundBal sdk.Coins + refundAcc sdk.AccAddress + fee types.Fee + locked bool + expectEscrowFeesToBeDeleted bool ) testCases := []struct { @@ -375,6 +375,7 @@ func (suite *KeeperTestSuite) TestRefundFeesOnChannelClosure() { { "invalid refund acc address", func() { // store the fee in state & update escrow account balance + expectEscrowFeesToBeDeleted = false 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) @@ -385,10 +386,14 @@ func (suite *KeeperTestSuite) TestRefundFeesOnChannelClosure() { suite.Require().NoError(err) expIdentifiedPacketFees = []types.IdentifiedPacketFees{identifiedPacketFees} - }, false, + + expEscrowBal = fee.Total() + expRefundBal = expRefundBal.Sub(fee.Total()) + }, true, }, { "distributing to blocked address is skipped", func() { + expectEscrowFeesToBeDeleted = false blockedAddr := suite.chainA.GetSimApp().AccountKeeper.GetModuleAccount(suite.chainA.GetContext(), transfertypes.ModuleName).GetAddress().String() // store the fee in state & update escrow account balance @@ -418,6 +423,7 @@ func (suite *KeeperTestSuite) TestRefundFeesOnChannelClosure() { expIdentifiedPacketFees = []types.IdentifiedPacketFees{} expEscrowBal = sdk.Coins{} locked = false + expectEscrowFeesToBeDeleted = true // setup refundAcc = suite.chainA.SenderAccount.GetAddress() @@ -464,8 +470,8 @@ func (suite *KeeperTestSuite) TestRefundFeesOnChannelClosure() { 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)) + // all fees in escrow should be deleted if expected for this channel + suite.Require().Equal(expectEscrowFeesToBeDeleted, len(suite.chainA.GetSimApp().IBCFeeKeeper.GetIdentifiedPacketFeesForChannel(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID)) == 0) } }) }