From f1b196b5067c7aed9e33002c21e6f96f3699f1ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Mon, 28 Feb 2022 17:32:11 +0100 Subject: [PATCH 1/8] feat: allow fee module to become locked --- modules/apps/29-fee/ibc_module.go | 4 ++-- modules/apps/29-fee/keeper/escrow.go | 4 ++++ modules/apps/29-fee/keeper/keeper.go | 13 +++++++++++++ modules/apps/29-fee/keeper/msg_server.go | 8 ++++++++ modules/apps/29-fee/types/errors.go | 1 + modules/apps/29-fee/types/keys.go | 6 ++++++ 6 files changed, 34 insertions(+), 2 deletions(-) diff --git a/modules/apps/29-fee/ibc_module.go b/modules/apps/29-fee/ibc_module.go index e8a4a45fd40..9afe61cb399 100644 --- a/modules/apps/29-fee/ibc_module.go +++ b/modules/apps/29-fee/ibc_module.go @@ -216,7 +216,7 @@ func (im IBCModule) OnAcknowledgementPacket( acknowledgement []byte, relayer sdk.AccAddress, ) error { - if !im.keeper.IsFeeEnabled(ctx, packet.SourcePort, packet.SourceChannel) { + if !im.keeper.IsFeeEnabled(ctx, packet.SourcePort, packet.SourceChannel) || im.keeper.IsLocked(ctx) { return im.app.OnAcknowledgementPacket(ctx, packet, acknowledgement, relayer) } @@ -248,7 +248,7 @@ func (im IBCModule) OnTimeoutPacket( packet channeltypes.Packet, relayer sdk.AccAddress, ) error { - if !im.keeper.IsFeeEnabled(ctx, packet.SourcePort, packet.SourceChannel) { + if !im.keeper.IsFeeEnabled(ctx, packet.SourcePort, packet.SourceChannel) || im.keeper.IsLocked(ctx) { return im.app.OnTimeoutPacket(ctx, packet, relayer) } diff --git a/modules/apps/29-fee/keeper/escrow.go b/modules/apps/29-fee/keeper/escrow.go index d3e96bad312..f1330fa2d90 100644 --- a/modules/apps/29-fee/keeper/escrow.go +++ b/modules/apps/29-fee/keeper/escrow.go @@ -96,6 +96,10 @@ func (k Keeper) DistributePacketFeesOnTimeout(ctx sdk.Context, timeoutRelayer sd // If the distribution fails for any reason (such as the receiving address being blocked), // the state changes will be discarded. func (k Keeper) distributeFee(ctx sdk.Context, receiver sdk.AccAddress, fee sdk.Coins) { + if k.IsLocked(ctx) { + return + } + // cache context before trying to distribute fees cacheCtx, writeFn := ctx.CacheContext() diff --git a/modules/apps/29-fee/keeper/keeper.go b/modules/apps/29-fee/keeper/keeper.go index 5b2e7810447..3e36770132e 100644 --- a/modules/apps/29-fee/keeper/keeper.go +++ b/modules/apps/29-fee/keeper/keeper.go @@ -77,6 +77,19 @@ func (k Keeper) GetFeeModuleAddress() sdk.AccAddress { return k.authKeeper.GetModuleAddress(types.ModuleName) } +// lockFeeModule sets a flag to determine if fee handling logic should run for the given channel +// identified by channel and port identifiers. +func (k Keeper) lockFeeModule(ctx sdk.Context) { + store := ctx.KVStore(k.storeKey) + store.Set(types.KeyLocked(), []byte{1}) +} + +// IsLocked indicates if the fee module is locked +func (k Keeper) IsLocked(ctx sdk.Context) bool { + store := ctx.KVStore(k.storeKey) + return store.Has(types.KeyLocked()) +} + // SetFeeEnabled sets a flag to determine if fee handling logic should run for the given channel // identified by channel and port identifiers. func (k Keeper) SetFeeEnabled(ctx sdk.Context, portID, channelID string) { diff --git a/modules/apps/29-fee/keeper/msg_server.go b/modules/apps/29-fee/keeper/msg_server.go index fdac1d27874..c8ffb853799 100644 --- a/modules/apps/29-fee/keeper/msg_server.go +++ b/modules/apps/29-fee/keeper/msg_server.go @@ -31,6 +31,10 @@ func (k Keeper) RegisterCounterpartyAddress(goCtx context.Context, msg *types.Ms func (k Keeper) PayPacketFee(goCtx context.Context, msg *types.MsgPayPacketFee) (*types.MsgPayPacketFeeResponse, error) { ctx := sdk.UnwrapSDKContext(goCtx) + if k.IsLocked(ctx) { + return nil, types.ErrFeeModuleLocked + } + // get the next sequence sequence, found := k.GetNextSequenceSend(ctx, msg.SourcePortId, msg.SourceChannelId) if !found { @@ -57,6 +61,10 @@ func (k Keeper) PayPacketFee(goCtx context.Context, msg *types.MsgPayPacketFee) func (k Keeper) PayPacketFeeAsync(goCtx context.Context, msg *types.MsgPayPacketFeeAsync) (*types.MsgPayPacketFeeAsyncResponse, error) { ctx := sdk.UnwrapSDKContext(goCtx) + if k.IsLocked(ctx) { + return nil, types.ErrFeeModuleLocked + } + if err := k.EscrowPacketFee(ctx, msg.PacketId, msg.PacketFee); err != nil { return nil, err } diff --git a/modules/apps/29-fee/types/errors.go b/modules/apps/29-fee/types/errors.go index 75fdd436c91..1e35c92e2b1 100644 --- a/modules/apps/29-fee/types/errors.go +++ b/modules/apps/29-fee/types/errors.go @@ -15,4 +15,5 @@ var ( ErrForwardRelayerAddressNotFound = sdkerrors.Register(ModuleName, 8, "forward relayer address not found") ErrFeeNotEnabled = sdkerrors.Register(ModuleName, 9, "fee module is not enabled for this channel. If this error occurs after channel setup, fee module may not be enabled") ErrRelayerNotFoundForAsyncAck = sdkerrors.Register(ModuleName, 10, "relayer address must be stored for async WriteAcknowledgement") + ErrFeeModuleLocked = sdkerrors.Register(ModuleName, 11, "the fee module is currently locked") ) diff --git a/modules/apps/29-fee/types/keys.go b/modules/apps/29-fee/types/keys.go index 8f23a4e7688..188191bc8cd 100644 --- a/modules/apps/29-fee/types/keys.go +++ b/modules/apps/29-fee/types/keys.go @@ -45,6 +45,12 @@ const ( AttributeKeyTimeoutFee = "timeout_fee" ) +// KeyLocked returns the key used to lock and unlock the fee module. This key is used +// in the presence of a severe bug. +func KeyLocked() []byte { + return []byte("locked") +} + // FeeEnabledKey returns the key that stores a flag to determine if fee logic should // be enabled for the given port and channel identifiers. func FeeEnabledKey(portID, channelID string) []byte { From 1ff4fa9d2bf9f7f799626c832a8a591cbc2ec7fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 2 Mar 2022 09:56:32 +0100 Subject: [PATCH 2/8] add test for IsLocked --- modules/apps/29-fee/keeper/keeper_test.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/modules/apps/29-fee/keeper/keeper_test.go b/modules/apps/29-fee/keeper/keeper_test.go index c9ad4a5f10b..944aad5cd41 100644 --- a/modules/apps/29-fee/keeper/keeper_test.go +++ b/modules/apps/29-fee/keeper/keeper_test.go @@ -92,6 +92,18 @@ func (suite *KeeperTestSuite) TestFeeInEscrow() { suite.Require().Equal(expectedArr, arr, "did not retrieve expected fees during iteration") } +func (suite *KeeperTestSuite) TestIsLocked() { + ctx := suite.chainA.GetContext() + suite.Require().False(suite.chainA.GetSimApp().IBCFeeKeeper.IsLocked(ctx)) + + // lock fee module + storeKey := suite.chainA.GetSimApp().GetKey(types.ModuleName) + store := ctx.KVStore(storeKey) + store.Set(types.KeyLocked(), []byte{1}) + + 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") From b1402550c6aedc1a76bd4fc5c17811711b0ba597 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 2 Mar 2022 10:04:08 +0100 Subject: [PATCH 3/8] add test for msg server --- modules/apps/29-fee/keeper/keeper_test.go | 13 +++++++---- modules/apps/29-fee/keeper/msg_server_test.go | 22 +++++++++++++++++-- 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/modules/apps/29-fee/keeper/keeper_test.go b/modules/apps/29-fee/keeper/keeper_test.go index 944aad5cd41..092a9a30477 100644 --- a/modules/apps/29-fee/keeper/keeper_test.go +++ b/modules/apps/29-fee/keeper/keeper_test.go @@ -66,6 +66,14 @@ func TestKeeperTestSuite(t *testing.T) { suite.Run(t, new(KeeperTestSuite)) } +// helper function +func lockFeeModule(chain *ibctesting.TestChain) { + ctx := chain.GetContext() + storeKey := chain.GetSimApp().GetKey(types.ModuleName) + store := ctx.KVStore(storeKey) + store.Set(types.KeyLocked(), []byte{1}) +} + func (suite *KeeperTestSuite) TestFeeInEscrow() { suite.coordinator.Setup(suite.path) @@ -96,10 +104,7 @@ func (suite *KeeperTestSuite) TestIsLocked() { ctx := suite.chainA.GetContext() suite.Require().False(suite.chainA.GetSimApp().IBCFeeKeeper.IsLocked(ctx)) - // lock fee module - storeKey := suite.chainA.GetSimApp().GetKey(types.ModuleName) - store := ctx.KVStore(storeKey) - store.Set(types.KeyLocked(), []byte{1}) + lockFeeModule(suite.chainA) suite.Require().True(suite.chainA.GetSimApp().IBCFeeKeeper.IsLocked(ctx)) } diff --git a/modules/apps/29-fee/keeper/msg_server_test.go b/modules/apps/29-fee/keeper/msg_server_test.go index 1b371a2369f..65861e218b1 100644 --- a/modules/apps/29-fee/keeper/msg_server_test.go +++ b/modules/apps/29-fee/keeper/msg_server_test.go @@ -1,6 +1,8 @@ package keeper_test import ( + sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/cosmos/ibc-go/v3/modules/apps/29-fee/types" channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types" ibctesting "github.com/cosmos/ibc-go/v3/testing" @@ -62,6 +64,13 @@ func (suite *KeeperTestSuite) TestPayPacketFee() { true, func() {}, }, + { + "fee module is locked", + false, + func() { + lockFeeModule(suite.chainA) + }, + }, } for _, tc := range testCases { @@ -78,7 +87,8 @@ func (suite *KeeperTestSuite) TestPayPacketFee() { msg := types.NewMsgPayPacketFee(fee, suite.path.EndpointA.ChannelConfig.PortID, channelID, refundAcc.String(), []string{}) tc.malleate() - _, err := suite.chainA.SendMsgs(msg) + + _, err := suite.chainA.GetSimApp().IBCFeeKeeper.PayPacketFee(sdk.WrapSDKContext(suite.chainA.GetContext()), msg) if tc.expPass { suite.Require().NoError(err) // message committed @@ -99,6 +109,13 @@ func (suite *KeeperTestSuite) TestPayPacketFeeAsync() { true, func() {}, }, + { + "fee module is locked", + false, + func() { + lockFeeModule(suite.chainA) + }, + }, } for _, tc := range testCases { @@ -125,7 +142,8 @@ func (suite *KeeperTestSuite) TestPayPacketFeeAsync() { tc.malleate() msg := types.NewMsgPayPacketFeeAsync(packetID, packetFee) - _, err := suite.chainA.SendMsgs(msg) + + _, err := suite.chainA.GetSimApp().IBCFeeKeeper.PayPacketFeeAsync(sdk.WrapSDKContext(suite.chainA.GetContext()), msg) if tc.expPass { suite.Require().NoError(err) // message committed From 5466343d0f35bb9453950ba2497ecac560158626 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 2 Mar 2022 10:07:23 +0100 Subject: [PATCH 4/8] add tests for onacknowledgement and ontimeout --- modules/apps/29-fee/fee_test.go | 10 +++++++++- modules/apps/29-fee/ibc_module_test.go | 19 +++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/modules/apps/29-fee/fee_test.go b/modules/apps/29-fee/fee_test.go index 9369fc16743..d2445adef22 100644 --- a/modules/apps/29-fee/fee_test.go +++ b/modules/apps/29-fee/fee_test.go @@ -21,7 +21,7 @@ type FeeTestSuite struct { chainB *ibctesting.TestChain chainC *ibctesting.TestChain - path *ibctesting.Path + path *ibctesting.Path pathAToC *ibctesting.Path } @@ -63,3 +63,11 @@ func (suite *FeeTestSuite) CreateMockPacket() channeltypes.Packet { 0, ) } + +// helper function +func lockFeeModule(chain *ibctesting.TestChain) { + ctx := chain.GetContext() + storeKey := chain.GetSimApp().GetKey(types.ModuleName) + store := ctx.KVStore(storeKey) + store.Set(types.KeyLocked(), []byte{1}) +} diff --git a/modules/apps/29-fee/ibc_module_test.go b/modules/apps/29-fee/ibc_module_test.go index 6e0202a887e..e03e5066d5b 100644 --- a/modules/apps/29-fee/ibc_module_test.go +++ b/modules/apps/29-fee/ibc_module_test.go @@ -611,6 +611,16 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() { }, true, }, + { + "success: fee module is disabled, skip fee logic", + func() { + lockFeeModule(suite.chainA) + + ack = ibcmock.MockAcknowledgement.Acknowledgement() + expectedBalance = originalBalance + }, + true, + }, { "fail on distribute receive fee (blocked address)", func() { @@ -724,6 +734,15 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() { }, false, }, + { + "fee module is disabled, skip fee logic", + func() { + lockFeeModule(suite.chainA) + + expectedBalance = originalBalance + }, + false, + }, { "no op if identified packet fee doesn't exist", func() { From fbce5703b540f46bcd4d58999a1f2554d107315f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 2 Mar 2022 10:19:02 +0100 Subject: [PATCH 5/8] add test for distributeFee --- modules/apps/29-fee/keeper/escrow_test.go | 85 +++++++++++++---------- 1 file changed, 49 insertions(+), 36 deletions(-) diff --git a/modules/apps/29-fee/keeper/escrow_test.go b/modules/apps/29-fee/keeper/escrow_test.go index 81103197aa5..f0786238aa3 100644 --- a/modules/apps/29-fee/keeper/escrow_test.go +++ b/modules/apps/29-fee/keeper/escrow_test.go @@ -124,22 +124,62 @@ func (suite *KeeperTestSuite) TestDistributeFee() { reverseRelayer sdk.AccAddress forwardRelayer string refundAcc sdk.AccAddress + refundAccBal sdk.Coin + fee types.Fee + packetID channeltypes.PacketId ) validSeq := uint64(1) testCases := []struct { - name string - malleate func() - expPass bool + name string + malleate func() + expResult func() }{ { - "success", func() {}, true, + "success", func() {}, func() { + // check if the reverse relayer is paid + hasBalance := suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), reverseRelayer, fee.AckFee[0].Add(fee.AckFee[0])) + suite.Require().True(hasBalance) + + // check if the forward relayer is paid + forward, err := sdk.AccAddressFromBech32(forwardRelayer) + suite.Require().NoError(err) + hasBalance = suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), forward, fee.RecvFee[0].Add(fee.RecvFee[0])) + suite.Require().True(hasBalance) + + // check if the refund acc has been refunded the timeoutFee + expectedRefundAccBal := refundAccBal.Add(fee.TimeoutFee[0].Add(fee.TimeoutFee[0])) + hasBalance = suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), refundAcc, expectedRefundAccBal) + suite.Require().True(hasBalance) + + // check the module acc wallet is now empty + hasBalance = suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress(), sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: sdk.NewInt(0)}) + suite.Require().True(hasBalance) + }, }, { "invalid forward address", func() { forwardRelayer = "invalid address" - }, false, + }, + func() { + // check if the refund acc has been refunded the timeoutFee & onRecvFee + expectedRefundAccBal := refundAccBal.Add(fee.TimeoutFee[0]).Add(fee.RecvFee[0]).Add(fee.TimeoutFee[0]).Add(fee.RecvFee[0]) + hasBalance := suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), refundAcc, expectedRefundAccBal) + suite.Require().True(hasBalance) + + }, + }, + { + "fee module is locked, no distribution occurs", func() { + lockFeeModule(suite.chainA) + }, + func() { + // check the module acc wallet still has the fee + hasBalance := suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress(), fee.Total()[0]) + suite.Require().True(hasBalance) + + }, }, } @@ -155,8 +195,8 @@ func (suite *KeeperTestSuite) TestDistributeFee() { reverseRelayer = sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()) forwardRelayer = sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()).String() - packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelID, suite.path.EndpointA.ChannelConfig.PortID, validSeq) - fee := types.Fee{ + packetID = channeltypes.NewPacketId(suite.path.EndpointA.ChannelID, suite.path.EndpointA.ChannelConfig.PortID, validSeq) + fee = types.Fee{ RecvFee: defaultReceiveFee, AckFee: defaultAckFee, TimeoutFee: defaultTimeoutFee, @@ -174,39 +214,12 @@ func (suite *KeeperTestSuite) TestDistributeFee() { tc.malleate() // refundAcc balance after escrow - refundAccBal := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom) + refundAccBal = suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom) suite.chainA.GetSimApp().IBCFeeKeeper.DistributePacketFees(suite.chainA.GetContext(), forwardRelayer, reverseRelayer, []types.PacketFee{packetFee, packetFee}) - if tc.expPass { - // there should no longer be a fee in escrow for this packet - found := suite.chainA.GetSimApp().IBCFeeKeeper.HasFeeInEscrow(suite.chainA.GetContext(), packetID) - suite.Require().False(found) - - // check if the reverse relayer is paid - hasBalance := suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), reverseRelayer, fee.AckFee[0].Add(fee.AckFee[0])) - suite.Require().True(hasBalance) - - // check if the forward relayer is paid - forward, err := sdk.AccAddressFromBech32(forwardRelayer) - suite.Require().NoError(err) - hasBalance = suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), forward, fee.RecvFee[0].Add(fee.RecvFee[0])) - suite.Require().True(hasBalance) + tc.expResult() - // check if the refund acc has been refunded the timeoutFee - expectedRefundAccBal := refundAccBal.Add(fee.TimeoutFee[0].Add(fee.TimeoutFee[0])) - hasBalance = suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), refundAcc, expectedRefundAccBal) - suite.Require().True(hasBalance) - - // check the module acc wallet is now empty - hasBalance = suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress(), sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: sdk.NewInt(0)}) - suite.Require().True(hasBalance) - } else { - // check if the refund acc has been refunded the timeoutFee & onRecvFee - expectedRefundAccBal := refundAccBal.Add(fee.TimeoutFee[0]).Add(fee.RecvFee[0]).Add(fee.TimeoutFee[0]).Add(fee.RecvFee[0]) - hasBalance := suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), refundAcc, expectedRefundAccBal) - suite.Require().True(hasBalance) - } }) } } From 7399b52e32b0a0a84078776086bbe92e485bf0b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Thu, 3 Mar 2022 11:44:18 +0100 Subject: [PATCH 6/8] address review comments --- modules/apps/29-fee/ibc_module.go | 12 +++++++++++- modules/apps/29-fee/ibc_module_test.go | 1 - 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/modules/apps/29-fee/ibc_module.go b/modules/apps/29-fee/ibc_module.go index 7e0cfce424f..7b2d2e52b97 100644 --- a/modules/apps/29-fee/ibc_module.go +++ b/modules/apps/29-fee/ibc_module.go @@ -221,7 +221,7 @@ func (im IBCModule) OnAcknowledgementPacket( acknowledgement []byte, relayer sdk.AccAddress, ) error { - if !im.keeper.IsFeeEnabled(ctx, packet.SourcePort, packet.SourceChannel) || im.keeper.IsLocked(ctx) { + if !im.keeper.IsFeeEnabled(ctx, packet.SourcePort, packet.SourceChannel) { return im.app.OnAcknowledgementPacket(ctx, packet, acknowledgement, relayer) } @@ -230,6 +230,13 @@ func (im IBCModule) OnAcknowledgementPacket( return sdkerrors.Wrapf(err, "cannot unmarshal ICS-29 incentivized packet acknowledgement: %v", ack) } + if im.keeper.IsLocked(ctx) { + // if the fee keeper is locked then fee logic should be skipped + // this may occur in the presence of a severe bug which leads invalid state + // the fee keeper will be unlocked after manual intervention + return im.app.OnAcknowledgementPacket(ctx, packet, ack.Result, relayer) + } + packetID := channeltypes.NewPacketId(packet.SourceChannel, packet.SourcePort, packet.Sequence) feesInEscrow, found := im.keeper.GetFeesInEscrow(ctx, packetID) if !found { @@ -253,6 +260,9 @@ func (im IBCModule) OnTimeoutPacket( packet channeltypes.Packet, relayer sdk.AccAddress, ) error { + // if the fee keeper is locked then fee logic should be skipped + // this may occur in the presence of a severe bug which leads invalid state + // the fee keeper will be unlocked after manual intervention if !im.keeper.IsFeeEnabled(ctx, packet.SourcePort, packet.SourceChannel) || im.keeper.IsLocked(ctx) { return im.app.OnTimeoutPacket(ctx, packet, relayer) } diff --git a/modules/apps/29-fee/ibc_module_test.go b/modules/apps/29-fee/ibc_module_test.go index e03e5066d5b..68ff3207443 100644 --- a/modules/apps/29-fee/ibc_module_test.go +++ b/modules/apps/29-fee/ibc_module_test.go @@ -616,7 +616,6 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() { func() { lockFeeModule(suite.chainA) - ack = ibcmock.MockAcknowledgement.Acknowledgement() expectedBalance = originalBalance }, true, From e81e4e51464a8a054be9389d505c5bd75dff1ea8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Thu, 3 Mar 2022 12:58:26 +0100 Subject: [PATCH 7/8] add more documentation as per review request --- modules/apps/29-fee/ibc_module.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/modules/apps/29-fee/ibc_module.go b/modules/apps/29-fee/ibc_module.go index 7b2d2e52b97..f97423377bc 100644 --- a/modules/apps/29-fee/ibc_module.go +++ b/modules/apps/29-fee/ibc_module.go @@ -234,6 +234,9 @@ func (im IBCModule) OnAcknowledgementPacket( // if the fee keeper is locked then fee logic should be skipped // this may occur in the presence of a severe bug which leads invalid state // the fee keeper will be unlocked after manual intervention + // the acknowledgement has been unmarshalled into an ics29 acknowledgement + // since the counterparty is still sending incentivized acknowledgements + // for fee enabled channels return im.app.OnAcknowledgementPacket(ctx, packet, ack.Result, relayer) } From 8e59f130c1be9e693c9b29313beb9f74addecfa4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Thu, 3 Mar 2022 15:57:23 +0100 Subject: [PATCH 8/8] remove IsLocked check from distributeFee --- modules/apps/29-fee/keeper/escrow.go | 4 ---- modules/apps/29-fee/keeper/escrow_test.go | 11 ----------- 2 files changed, 15 deletions(-) diff --git a/modules/apps/29-fee/keeper/escrow.go b/modules/apps/29-fee/keeper/escrow.go index f1330fa2d90..d3e96bad312 100644 --- a/modules/apps/29-fee/keeper/escrow.go +++ b/modules/apps/29-fee/keeper/escrow.go @@ -96,10 +96,6 @@ func (k Keeper) DistributePacketFeesOnTimeout(ctx sdk.Context, timeoutRelayer sd // If the distribution fails for any reason (such as the receiving address being blocked), // the state changes will be discarded. func (k Keeper) distributeFee(ctx sdk.Context, receiver sdk.AccAddress, fee sdk.Coins) { - if k.IsLocked(ctx) { - return - } - // cache context before trying to distribute fees cacheCtx, writeFn := ctx.CacheContext() diff --git a/modules/apps/29-fee/keeper/escrow_test.go b/modules/apps/29-fee/keeper/escrow_test.go index f0786238aa3..8cdf4a9413e 100644 --- a/modules/apps/29-fee/keeper/escrow_test.go +++ b/modules/apps/29-fee/keeper/escrow_test.go @@ -170,17 +170,6 @@ func (suite *KeeperTestSuite) TestDistributeFee() { }, }, - { - "fee module is locked, no distribution occurs", func() { - lockFeeModule(suite.chainA) - }, - func() { - // check the module acc wallet still has the fee - hasBalance := suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress(), fee.Total()[0]) - suite.Require().True(hasBalance) - - }, - }, } for _, tc := range testCases {