From 8c312f63c82a246d2b97b8d9d09982a918ab3adf Mon Sep 17 00:00:00 2001 From: vuong Date: Thu, 5 May 2022 11:09:01 +0700 Subject: [PATCH 01/18] return err && move DeleteFees in DistributePacketFee func --- modules/apps/29-fee/ibc_middleware.go | 16 ++++++++-------- modules/apps/29-fee/keeper/escrow.go | 18 ++++++++++++++---- modules/apps/29-fee/keeper/escrow_test.go | 4 ++-- 3 files changed, 24 insertions(+), 14 deletions(-) diff --git a/modules/apps/29-fee/ibc_middleware.go b/modules/apps/29-fee/ibc_middleware.go index 96bb8be4030..386edca8724 100644 --- a/modules/apps/29-fee/ibc_middleware.go +++ b/modules/apps/29-fee/ibc_middleware.go @@ -232,10 +232,10 @@ 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) + err := im.keeper.DistributePacketFeesOnAcknowledgement(ctx, ack.ForwardRelayerAddress, relayer, feesInEscrow.PacketFees, packetID) + if err != nil { + return err + } } // call underlying callback @@ -261,10 +261,10 @@ 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) + err := im.keeper.DistributePacketFeesOnTimeout(ctx, relayer, feesInEscrow.PacketFees, packetID) + if err != nil { + return err + } } // call underlying callback diff --git a/modules/apps/29-fee/keeper/escrow.go b/modules/apps/29-fee/keeper/escrow.go index 0ca84684440..737779ee05d 100644 --- a/modules/apps/29-fee/keeper/escrow.go +++ b/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) 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() @@ -62,7 +62,7 @@ func (k Keeper) DistributePacketFeesOnAcknowledgement(ctx sdk.Context, forwardRe // locking the fee module are persisted k.lockFeeModule(ctx) - return + return types.ErrFeeModuleLocked } // check if refundAcc address works @@ -74,11 +74,16 @@ func (k Keeper) DistributePacketFeesOnAcknowledgement(ctx sdk.Context, forwardRe k.distributePacketFeeOnAcknowledgement(cacheCtx, refundAddr, forwardAddr, reverseRelayer, packetFee) } + // removes the fees from the store as fees are now paid + k.DeleteFeesInEscrow(ctx, packetID) + // 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()) + + return nil } // distributePacketFeeOnAcknowledgement pays the receive fee for a given packetID while refunding the timeout fee to the refund account associated with the Fee. @@ -102,7 +107,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) 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() @@ -117,7 +122,7 @@ func (k Keeper) DistributePacketFeesOnTimeout(ctx sdk.Context, timeoutRelayer sd // locking the fee module are persisted k.lockFeeModule(ctx) - return + return types.ErrFeeModuleLocked } // check if refundAcc address works @@ -129,11 +134,16 @@ func (k Keeper) DistributePacketFeesOnTimeout(ctx sdk.Context, timeoutRelayer sd k.distributePacketFeeOnTimeout(cacheCtx, refundAddr, timeoutRelayer, packetFee) } + // removes the fee from the store as fee is now paid + k.DeleteFeesInEscrow(ctx, packetID) + // 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()) + + return nil } // distributePacketFeeOnTimeout pays the timeout fee to the timeout relayer and refunds the acknowledgement & receive fee. diff --git a/modules/apps/29-fee/keeper/escrow_test.go b/modules/apps/29-fee/keeper/escrow_test.go index e2a43afe586..41c5c765cf0 100644 --- a/modules/apps/29-fee/keeper/escrow_test.go +++ b/modules/apps/29-fee/keeper/escrow_test.go @@ -149,7 +149,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() }) @@ -259,7 +259,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() }) From 9e8841cc419cf87e6bafd2e4084af044f65d3d2f Mon Sep 17 00:00:00 2001 From: vuong Date: Thu, 5 May 2022 11:25:12 +0700 Subject: [PATCH 02/18] add test --- modules/apps/29-fee/keeper/escrow_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/modules/apps/29-fee/keeper/escrow_test.go b/modules/apps/29-fee/keeper/escrow_test.go index 41c5c765cf0..4948c4e3f02 100644 --- a/modules/apps/29-fee/keeper/escrow_test.go +++ b/modules/apps/29-fee/keeper/escrow_test.go @@ -59,7 +59,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()...) @@ -196,7 +199,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()...) From 5dfc57affe659874c62271456af157788ed6ad06 Mon Sep 17 00:00:00 2001 From: vuong Date: Thu, 5 May 2022 18:46:37 +0700 Subject: [PATCH 03/18] TestDistributeFee: check if fees has been deleted --- modules/apps/29-fee/keeper/escrow_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/modules/apps/29-fee/keeper/escrow_test.go b/modules/apps/29-fee/keeper/escrow_test.go index 4948c4e3f02..6cb4bd994f5 100644 --- a/modules/apps/29-fee/keeper/escrow_test.go +++ b/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) From af268d96a75d8eb080dbdf0e769e7ca04f4f165a Mon Sep 17 00:00:00 2001 From: vuong Date: Fri, 6 May 2022 00:15:41 +0700 Subject: [PATCH 04/18] add distributePacketError field in testCases --- modules/apps/29-fee/keeper/escrow_test.go | 31 ++++++++++++++++------- 1 file changed, 22 insertions(+), 9 deletions(-) diff --git a/modules/apps/29-fee/keeper/escrow_test.go b/modules/apps/29-fee/keeper/escrow_test.go index 6cb4bd994f5..f9c10c88c89 100644 --- a/modules/apps/29-fee/keeper/escrow_test.go +++ b/modules/apps/29-fee/keeper/escrow_test.go @@ -22,9 +22,10 @@ func (suite *KeeperTestSuite) TestDistributeFee() { ) testCases := []struct { - name string - malleate func() - expResult func() + name string + malleate func() + expResult func() + distributePacketError error }{ { "success", @@ -56,6 +57,7 @@ func (suite *KeeperTestSuite) TestDistributeFee() { balance = suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress(), sdk.DefaultBondDenom) suite.Require().Equal(sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(0)), balance) }, + nil, }, { "escrow account out of balance, fee module becomes locked - no distribution", func() { @@ -73,6 +75,7 @@ func (suite *KeeperTestSuite) TestDistributeFee() { balance := suite.chainA.GetSimApp().BankKeeper.GetAllBalances(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress()) suite.Require().Equal(expectedModuleAccBal, balance) }, + types.ErrFeeModuleLocked, }, { "invalid forward address", @@ -85,6 +88,7 @@ func (suite *KeeperTestSuite) TestDistributeFee() { balance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom) suite.Require().Equal(expectedRefundAccBal, balance) }, + nil, }, { "invalid forward address: blocked address", @@ -97,6 +101,7 @@ func (suite *KeeperTestSuite) TestDistributeFee() { balance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom) suite.Require().Equal(expectedRefundAccBal, balance) }, + nil, }, { "invalid receiver address: ack fee returned to sender", @@ -109,6 +114,7 @@ func (suite *KeeperTestSuite) TestDistributeFee() { balance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom) suite.Require().Equal(expectedRefundAccBal, balance) }, + nil, }, { "invalid refund address: no-op, timeout fee remains in escrow", @@ -122,6 +128,7 @@ func (suite *KeeperTestSuite) TestDistributeFee() { balance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress(), sdk.DefaultBondDenom) suite.Require().Equal(expectedModuleAccBal, balance) }, + nil, }, } @@ -156,8 +163,8 @@ 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, packetID) - + distributePacketError := suite.chainA.GetSimApp().IBCFeeKeeper.DistributePacketFeesOnAcknowledgement(suite.chainA.GetContext(), forwardRelayer, reverseRelayer, packetFees, packetID) + suite.Require().Equal(tc.distributePacketError, distributePacketError) tc.expResult() }) } @@ -174,9 +181,10 @@ func (suite *KeeperTestSuite) TestDistributePacketFeesOnTimeout() { ) testCases := []struct { - name string - malleate func() - expResult func() + name string + malleate func() + expResult func() + distributePacketError error }{ { "success", @@ -196,6 +204,7 @@ func (suite *KeeperTestSuite) TestDistributePacketFeesOnTimeout() { balance = suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress(), sdk.DefaultBondDenom) suite.Require().Equal(sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(0)), balance) }, + nil, }, { "escrow account out of balance, fee module becomes locked - no distribution", func() { @@ -213,6 +222,7 @@ func (suite *KeeperTestSuite) TestDistributePacketFeesOnTimeout() { balance := suite.chainA.GetSimApp().BankKeeper.GetAllBalances(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress()) suite.Require().Equal(expectedModuleAccBal, balance) }, + types.ErrFeeModuleLocked, }, { "invalid timeout relayer address: timeout fee returned to sender", @@ -225,6 +235,7 @@ func (suite *KeeperTestSuite) TestDistributePacketFeesOnTimeout() { balance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom) suite.Require().Equal(expectedRefundAccBal, balance) }, + nil, }, { "invalid refund address: no-op, recv and ack fees remain in escrow", @@ -238,6 +249,7 @@ func (suite *KeeperTestSuite) TestDistributePacketFeesOnTimeout() { balance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress(), sdk.DefaultBondDenom) suite.Require().Equal(expectedModuleAccBal, balance) }, + nil, }, } @@ -269,7 +281,8 @@ 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, packetID) + distributePacketError := suite.chainA.GetSimApp().IBCFeeKeeper.DistributePacketFeesOnTimeout(suite.chainA.GetContext(), timeoutRelayer, packetFees, packetID) + suite.Require().Equal(tc.distributePacketError, distributePacketError) tc.expResult() }) From 6532a39d0ea53637a11efd1a6566d1fa4733d535 Mon Sep 17 00:00:00 2001 From: vuong Date: Sun, 8 May 2022 22:14:18 +0700 Subject: [PATCH 05/18] update testcase when ack and timeout in middleware test --- modules/apps/29-fee/ibc_middleware_test.go | 45 +++++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-) diff --git a/modules/apps/29-fee/ibc_middleware_test.go b/modules/apps/29-fee/ibc_middleware_test.go index 8c76720ad3e..7ea7e682ad5 100644 --- a/modules/apps/29-fee/ibc_middleware_test.go +++ b/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 @@ -635,6 +636,22 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() { }, 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})) + }, + false, + }, } for _, tc := range testCases { @@ -720,11 +737,13 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() { name string malleate func() expFeeDistributed bool + expPass bool }{ { "success", func() {}, true, + true, }, { "fee not enabled", @@ -734,6 +753,7 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() { expectedBalance = originalBalance }, false, + true, }, { "fee module is disabled, skip fee logic", @@ -743,6 +763,7 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() { expectedBalance = originalBalance }, false, + true, }, { "no op if identified packet fee doesn't exist", @@ -754,6 +775,7 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() { expectedBalance = originalBalance }, false, + true, }, { "distribute fee fails for timeout fee (blocked address)", @@ -766,6 +788,23 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() { Add(packetFee.Fee.TimeoutFee...) }, 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 don't change + expectedBalance = originalBalance.Add(smallAmount...) + + suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, types.NewPacketFees([]types.PacketFee{packetFee})) + }, + false, + false, }, } @@ -815,7 +854,11 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() { tc.malleate() err = cbs.OnTimeoutPacket(suite.chainA.GetContext(), packet, relayerAddr) - suite.Require().NoError(err) + if tc.expPass { + suite.Require().NoError(err) + } else { + suite.Require().Error(err) + } suite.Require().Equal( expectedBalance, From a9dd568906454bdca8a8b500d76e2f523757d6e2 Mon Sep 17 00:00:00 2001 From: vuong <56973102+vuong177@users.noreply.github.com> Date: Mon, 9 May 2022 18:24:25 +0700 Subject: [PATCH 06/18] Update modules/apps/29-fee/ibc_middleware.go Co-authored-by: Sean King --- modules/apps/29-fee/ibc_middleware.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/modules/apps/29-fee/ibc_middleware.go b/modules/apps/29-fee/ibc_middleware.go index 386edca8724..f3d57167636 100644 --- a/modules/apps/29-fee/ibc_middleware.go +++ b/modules/apps/29-fee/ibc_middleware.go @@ -232,8 +232,7 @@ func (im IBCMiddleware) OnAcknowledgementPacket( packetID := channeltypes.NewPacketId(packet.SourcePort, packet.SourceChannel, packet.Sequence) feesInEscrow, found := im.keeper.GetFeesInEscrow(ctx, packetID) if found { - err := im.keeper.DistributePacketFeesOnAcknowledgement(ctx, ack.ForwardRelayerAddress, relayer, feesInEscrow.PacketFees, packetID) - if err != nil { + if err := im.keeper.DistributePacketFeesOnAcknowledgement(ctx, ack.ForwardRelayerAddress, relayer, feesInEscrow.PacketFees, packetID); err != nil { return err } } From 61e134310b80aef100be753bafffa85d94b614aa Mon Sep 17 00:00:00 2001 From: vuong <56973102+vuong177@users.noreply.github.com> Date: Mon, 9 May 2022 18:26:05 +0700 Subject: [PATCH 07/18] Update modules/apps/29-fee/keeper/escrow.go Co-authored-by: Sean King --- modules/apps/29-fee/keeper/escrow.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/apps/29-fee/keeper/escrow.go b/modules/apps/29-fee/keeper/escrow.go index 737779ee05d..c4ba2ea885c 100644 --- a/modules/apps/29-fee/keeper/escrow.go +++ b/modules/apps/29-fee/keeper/escrow.go @@ -134,7 +134,7 @@ func (k Keeper) DistributePacketFeesOnTimeout(ctx sdk.Context, timeoutRelayer sd k.distributePacketFeeOnTimeout(cacheCtx, refundAddr, timeoutRelayer, packetFee) } - // removes the fee from the store as fee is now paid + // removing the fee from the store as the fee is now paid k.DeleteFeesInEscrow(ctx, packetID) // write the cache From e9a6a28e6af02a0b372a86a3a862edca4d8bb89a Mon Sep 17 00:00:00 2001 From: vuong <56973102+vuong177@users.noreply.github.com> Date: Mon, 9 May 2022 18:26:15 +0700 Subject: [PATCH 08/18] Update modules/apps/29-fee/ibc_middleware_test.go Co-authored-by: Sean King --- modules/apps/29-fee/ibc_middleware_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/modules/apps/29-fee/ibc_middleware_test.go b/modules/apps/29-fee/ibc_middleware_test.go index 7ea7e682ad5..e2f08490996 100644 --- a/modules/apps/29-fee/ibc_middleware_test.go +++ b/modules/apps/29-fee/ibc_middleware_test.go @@ -791,7 +791,6 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() { 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()) From 9d0b8528173d0ec7c7b028e3f8a7e5d93612e28f Mon Sep 17 00:00:00 2001 From: vuong <56973102+vuong177@users.noreply.github.com> Date: Mon, 9 May 2022 18:26:21 +0700 Subject: [PATCH 09/18] Update modules/apps/29-fee/ibc_middleware_test.go Co-authored-by: Sean King --- modules/apps/29-fee/ibc_middleware_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/modules/apps/29-fee/ibc_middleware_test.go b/modules/apps/29-fee/ibc_middleware_test.go index e2f08490996..f21785b8167 100644 --- a/modules/apps/29-fee/ibc_middleware_test.go +++ b/modules/apps/29-fee/ibc_middleware_test.go @@ -637,7 +637,6 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() { 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()) From 714dd4c1d084ede8be39389bb8bce455dbdd2049 Mon Sep 17 00:00:00 2001 From: vuong Date: Tue, 10 May 2022 13:16:44 +0700 Subject: [PATCH 10/18] update CHANGELOG --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 65274b8fa95..956d100c47d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -55,6 +55,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (modules/core/04-channel) [\#1232](https://github.com/cosmos/ibc-go/pull/1232) Updating params on `NewPacketId` and moving to bottom of file. * (modules/core/04-channel) [\#1279](https://github.com/cosmos/ibc-go/pull/1279) Add selected channel version to MsgChanOpenInitResponse and MsgChanOpenTryResponse. Emit channel version during OpenInit/OpenTry * (app/29-fee) [\#1305](https://github.com/cosmos/ibc-go/pull/1305) Change version string for fee module to `ics29-1` +* (app/29-fee) [\#1340](https://github.com/cosmos/ibc-go/pull/1340) Return error if fee module is locked before distributing fees on acknowledgement and timeout ### Features From 880a5d0fd1ffa866e72f75e326d13525e27f3178 Mon Sep 17 00:00:00 2001 From: vuong Date: Tue, 10 May 2022 13:19:01 +0700 Subject: [PATCH 11/18] nits --- modules/apps/29-fee/ibc_middleware.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/modules/apps/29-fee/ibc_middleware.go b/modules/apps/29-fee/ibc_middleware.go index f3d57167636..d0d3b26661e 100644 --- a/modules/apps/29-fee/ibc_middleware.go +++ b/modules/apps/29-fee/ibc_middleware.go @@ -260,8 +260,7 @@ func (im IBCMiddleware) OnTimeoutPacket( packetID := channeltypes.NewPacketId(packet.SourcePort, packet.SourceChannel, packet.Sequence) feesInEscrow, found := im.keeper.GetFeesInEscrow(ctx, packetID) if found { - err := im.keeper.DistributePacketFeesOnTimeout(ctx, relayer, feesInEscrow.PacketFees, packetID) - if err != nil { + if err := im.keeper.DistributePacketFeesOnTimeout(ctx, relayer, feesInEscrow.PacketFees, packetID); err != nil { return err } } From 035b53f88fa9f8a1afc9d1497357f2e1c3f40c93 Mon Sep 17 00:00:00 2001 From: vuong Date: Thu, 12 May 2022 08:28:37 +0700 Subject: [PATCH 12/18] remove error in DistributePacketFeesOnAcknowledgement --- modules/apps/29-fee/ibc_middleware.go | 4 +--- modules/apps/29-fee/keeper/escrow.go | 12 +++++------- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/modules/apps/29-fee/ibc_middleware.go b/modules/apps/29-fee/ibc_middleware.go index d0d3b26661e..9914d8bc481 100644 --- a/modules/apps/29-fee/ibc_middleware.go +++ b/modules/apps/29-fee/ibc_middleware.go @@ -232,9 +232,7 @@ func (im IBCMiddleware) OnAcknowledgementPacket( packetID := channeltypes.NewPacketId(packet.SourcePort, packet.SourceChannel, packet.Sequence) feesInEscrow, found := im.keeper.GetFeesInEscrow(ctx, packetID) if found { - if err := im.keeper.DistributePacketFeesOnAcknowledgement(ctx, ack.ForwardRelayerAddress, relayer, feesInEscrow.PacketFees, packetID); err != nil { - return err - } + im.keeper.DistributePacketFeesOnAcknowledgement(ctx, ack.ForwardRelayerAddress, relayer, feesInEscrow.PacketFees, packetID) } // call underlying callback diff --git a/modules/apps/29-fee/keeper/escrow.go b/modules/apps/29-fee/keeper/escrow.go index c4ba2ea885c..77be53a8db0 100644 --- a/modules/apps/29-fee/keeper/escrow.go +++ b/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, packetID channeltypes.PacketId) error { +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,8 +61,7 @@ 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 types.ErrFeeModuleLocked + return } // check if refundAcc address works @@ -74,16 +73,15 @@ func (k Keeper) DistributePacketFeesOnAcknowledgement(ctx sdk.Context, forwardRe k.distributePacketFeeOnAcknowledgement(cacheCtx, refundAddr, forwardAddr, reverseRelayer, packetFee) } - // removes the fees from the store as fees are now paid - k.DeleteFeesInEscrow(ctx, packetID) - // write the cache writeFn() + // removes the fees from the store as fees are now paid + k.DeleteFeesInEscrow(ctx, packetID) + // 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()) - return nil } // distributePacketFeeOnAcknowledgement pays the receive fee for a given packetID while refunding the timeout fee to the refund account associated with the Fee. From 8ae668cbf2036029d3bc731c81bb95eed1bc619f Mon Sep 17 00:00:00 2001 From: vuong Date: Thu, 12 May 2022 08:28:48 +0700 Subject: [PATCH 13/18] update test --- modules/apps/29-fee/ibc_middleware_test.go | 12 ++++++------ modules/apps/29-fee/keeper/escrow_test.go | 16 ++++------------ 2 files changed, 10 insertions(+), 18 deletions(-) diff --git a/modules/apps/29-fee/ibc_middleware_test.go b/modules/apps/29-fee/ibc_middleware_test.go index f21785b8167..0463a656624 100644 --- a/modules/apps/29-fee/ibc_middleware_test.go +++ b/modules/apps/29-fee/ibc_middleware_test.go @@ -649,7 +649,7 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() { suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, types.NewPacketFees([]types.PacketFee{packetFee})) }, - false, + true, }, } @@ -736,7 +736,7 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() { name string malleate func() expFeeDistributed bool - expPass bool + expLocked bool }{ { "success", @@ -762,7 +762,7 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() { expectedBalance = originalBalance }, false, - true, + false, }, { "no op if identified packet fee doesn't exist", @@ -852,10 +852,10 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() { tc.malleate() err = cbs.OnTimeoutPacket(suite.chainA.GetContext(), packet, relayerAddr) - if tc.expPass { - suite.Require().NoError(err) + if tc.expLocked { + suite.Require().False(suite.chainA.GetSimApp().IBCFeeKeeper.IsLocked(suite.chainA.GetContext())) } else { - suite.Require().Error(err) + suite.Require().True(suite.chainA.GetSimApp().IBCFeeKeeper.IsLocked(suite.chainA.GetContext())) } suite.Require().Equal( diff --git a/modules/apps/29-fee/keeper/escrow_test.go b/modules/apps/29-fee/keeper/escrow_test.go index f9c10c88c89..ff322b13445 100644 --- a/modules/apps/29-fee/keeper/escrow_test.go +++ b/modules/apps/29-fee/keeper/escrow_test.go @@ -22,10 +22,9 @@ func (suite *KeeperTestSuite) TestDistributeFee() { ) testCases := []struct { - name string - malleate func() - expResult func() - distributePacketError error + name string + malleate func() + expResult func() }{ { "success", @@ -57,7 +56,6 @@ func (suite *KeeperTestSuite) TestDistributeFee() { balance = suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress(), sdk.DefaultBondDenom) suite.Require().Equal(sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(0)), balance) }, - nil, }, { "escrow account out of balance, fee module becomes locked - no distribution", func() { @@ -75,7 +73,6 @@ func (suite *KeeperTestSuite) TestDistributeFee() { balance := suite.chainA.GetSimApp().BankKeeper.GetAllBalances(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress()) suite.Require().Equal(expectedModuleAccBal, balance) }, - types.ErrFeeModuleLocked, }, { "invalid forward address", @@ -88,7 +85,6 @@ func (suite *KeeperTestSuite) TestDistributeFee() { balance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom) suite.Require().Equal(expectedRefundAccBal, balance) }, - nil, }, { "invalid forward address: blocked address", @@ -101,7 +97,6 @@ func (suite *KeeperTestSuite) TestDistributeFee() { balance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom) suite.Require().Equal(expectedRefundAccBal, balance) }, - nil, }, { "invalid receiver address: ack fee returned to sender", @@ -114,7 +109,6 @@ func (suite *KeeperTestSuite) TestDistributeFee() { balance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom) suite.Require().Equal(expectedRefundAccBal, balance) }, - nil, }, { "invalid refund address: no-op, timeout fee remains in escrow", @@ -128,7 +122,6 @@ func (suite *KeeperTestSuite) TestDistributeFee() { balance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress(), sdk.DefaultBondDenom) suite.Require().Equal(expectedModuleAccBal, balance) }, - nil, }, } @@ -163,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) - distributePacketError := suite.chainA.GetSimApp().IBCFeeKeeper.DistributePacketFeesOnAcknowledgement(suite.chainA.GetContext(), forwardRelayer, reverseRelayer, packetFees, packetID) - suite.Require().Equal(tc.distributePacketError, distributePacketError) + suite.chainA.GetSimApp().IBCFeeKeeper.DistributePacketFeesOnAcknowledgement(suite.chainA.GetContext(), forwardRelayer, reverseRelayer, packetFees, packetID) tc.expResult() }) } From a11869bbd325dd008b8e365abdbf9ba28c8b9c8d Mon Sep 17 00:00:00 2001 From: vuong Date: Thu, 12 May 2022 08:29:13 +0700 Subject: [PATCH 14/18] CHANGELOG.MD --- CHANGELOG.md | 1 - 1 file changed, 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f270fd65836..b38ee315afc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -56,7 +56,6 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (modules/core/04-channel) [\#1232](https://github.com/cosmos/ibc-go/pull/1232) Updating params on `NewPacketId` and moving to bottom of file. * (modules/core/04-channel) [\#1279](https://github.com/cosmos/ibc-go/pull/1279) Add selected channel version to MsgChanOpenInitResponse and MsgChanOpenTryResponse. Emit channel version during OpenInit/OpenTry * (app/29-fee) [\#1305](https://github.com/cosmos/ibc-go/pull/1305) Change version string for fee module to `ics29-1` -* (app/29-fee) [\#1340](https://github.com/cosmos/ibc-go/pull/1340) Return error if fee module is locked before distributing fees on acknowledgement and timeout ### Features From e4adbd9f835cd7156a8dc76925607ba17061a60e Mon Sep 17 00:00:00 2001 From: vuong Date: Sun, 15 May 2022 16:15:14 +0700 Subject: [PATCH 15/18] DistributePacketFeesOnTimeout not return error --- modules/apps/29-fee/ibc_middleware.go | 4 +--- modules/apps/29-fee/keeper/escrow.go | 2 +- modules/apps/29-fee/keeper/escrow_test.go | 14 ++++---------- 3 files changed, 6 insertions(+), 14 deletions(-) diff --git a/modules/apps/29-fee/ibc_middleware.go b/modules/apps/29-fee/ibc_middleware.go index 9914d8bc481..dde334f533c 100644 --- a/modules/apps/29-fee/ibc_middleware.go +++ b/modules/apps/29-fee/ibc_middleware.go @@ -258,9 +258,7 @@ func (im IBCMiddleware) OnTimeoutPacket( packetID := channeltypes.NewPacketId(packet.SourcePort, packet.SourceChannel, packet.Sequence) feesInEscrow, found := im.keeper.GetFeesInEscrow(ctx, packetID) if found { - if err := im.keeper.DistributePacketFeesOnTimeout(ctx, relayer, feesInEscrow.PacketFees, packetID); err != nil { - return err - } + im.keeper.DistributePacketFeesOnTimeout(ctx, relayer, feesInEscrow.PacketFees, packetID) } // call underlying callback diff --git a/modules/apps/29-fee/keeper/escrow.go b/modules/apps/29-fee/keeper/escrow.go index 77be53a8db0..e609c734f5e 100644 --- a/modules/apps/29-fee/keeper/escrow.go +++ b/modules/apps/29-fee/keeper/escrow.go @@ -120,7 +120,7 @@ func (k Keeper) DistributePacketFeesOnTimeout(ctx sdk.Context, timeoutRelayer sd // locking the fee module are persisted k.lockFeeModule(ctx) - return types.ErrFeeModuleLocked + return nil } // check if refundAcc address works diff --git a/modules/apps/29-fee/keeper/escrow_test.go b/modules/apps/29-fee/keeper/escrow_test.go index ff322b13445..4ac6e2ed03f 100644 --- a/modules/apps/29-fee/keeper/escrow_test.go +++ b/modules/apps/29-fee/keeper/escrow_test.go @@ -173,10 +173,9 @@ func (suite *KeeperTestSuite) TestDistributePacketFeesOnTimeout() { ) testCases := []struct { - name string - malleate func() - expResult func() - distributePacketError error + name string + malleate func() + expResult func() }{ { "success", @@ -196,7 +195,6 @@ func (suite *KeeperTestSuite) TestDistributePacketFeesOnTimeout() { balance = suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress(), sdk.DefaultBondDenom) suite.Require().Equal(sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(0)), balance) }, - nil, }, { "escrow account out of balance, fee module becomes locked - no distribution", func() { @@ -214,7 +212,6 @@ func (suite *KeeperTestSuite) TestDistributePacketFeesOnTimeout() { balance := suite.chainA.GetSimApp().BankKeeper.GetAllBalances(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress()) suite.Require().Equal(expectedModuleAccBal, balance) }, - types.ErrFeeModuleLocked, }, { "invalid timeout relayer address: timeout fee returned to sender", @@ -227,7 +224,6 @@ func (suite *KeeperTestSuite) TestDistributePacketFeesOnTimeout() { balance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom) suite.Require().Equal(expectedRefundAccBal, balance) }, - nil, }, { "invalid refund address: no-op, recv and ack fees remain in escrow", @@ -241,7 +237,6 @@ func (suite *KeeperTestSuite) TestDistributePacketFeesOnTimeout() { balance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress(), sdk.DefaultBondDenom) suite.Require().Equal(expectedModuleAccBal, balance) }, - nil, }, } @@ -273,8 +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) - distributePacketError := suite.chainA.GetSimApp().IBCFeeKeeper.DistributePacketFeesOnTimeout(suite.chainA.GetContext(), timeoutRelayer, packetFees, packetID) - suite.Require().Equal(tc.distributePacketError, distributePacketError) + suite.chainA.GetSimApp().IBCFeeKeeper.DistributePacketFeesOnTimeout(suite.chainA.GetContext(), timeoutRelayer, packetFees, packetID) tc.expResult() }) From b420e4634a48d8587d7a14d9814c26ab5607de68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Mon, 16 May 2022 12:50:34 +0200 Subject: [PATCH 16/18] remove error from Distribute API and write cache before deleting fees --- modules/apps/29-fee/keeper/escrow.go | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/modules/apps/29-fee/keeper/escrow.go b/modules/apps/29-fee/keeper/escrow.go index e609c734f5e..e4d5af69fa8 100644 --- a/modules/apps/29-fee/keeper/escrow.go +++ b/modules/apps/29-fee/keeper/escrow.go @@ -73,15 +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() // removes the fees from the store as fees are now paid k.DeleteFeesInEscrow(ctx, packetID) - - // 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()) - } // distributePacketFeeOnAcknowledgement pays the receive fee for a given packetID while refunding the timeout fee to the refund account associated with the Fee. @@ -105,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, packetID channeltypes.PacketId) error { +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() @@ -119,8 +118,7 @@ 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 nil + return } // check if refundAcc address works @@ -132,16 +130,14 @@ func (k Keeper) DistributePacketFeesOnTimeout(ctx sdk.Context, timeoutRelayer sd k.distributePacketFeeOnTimeout(cacheCtx, refundAddr, timeoutRelayer, packetFee) } - // removing the fee from the store as the fee is now paid - k.DeleteFeesInEscrow(ctx, packetID) + // 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()) - - return nil + // 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. From 2c0ded7967e6a250b46a2fd9b145987f6b9d6a96 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Mon, 16 May 2022 12:51:20 +0200 Subject: [PATCH 17/18] adjust tests to check that fee module gets locked --- modules/apps/29-fee/ibc_middleware_test.go | 29 +++++++++++++--------- 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/modules/apps/29-fee/ibc_middleware_test.go b/modules/apps/29-fee/ibc_middleware_test.go index 0463a656624..54520f18e93 100644 --- a/modules/apps/29-fee/ibc_middleware_test.go +++ b/modules/apps/29-fee/ibc_middleware_test.go @@ -564,6 +564,7 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() { originalBalance sdk.Coins expectedBalance sdk.Coins expectedRelayerBalance sdk.Coins + expLocked bool ) testCases := []struct { @@ -616,6 +617,7 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() { "success: fee module is disabled, skip fee logic", func() { lockFeeModule(suite.chainA) + expLocked = true expectedBalance = originalBalance }, @@ -648,6 +650,8 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() { expectedRelayerBalance = sdk.NewCoins() suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, types.NewPacketFees([]types.PacketFee{packetFee})) + + expLocked = true }, true, }, @@ -657,6 +661,8 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() { tc := tc suite.Run(tc.name, func() { suite.SetupTest() + expLocked = false + suite.coordinator.Setup(suite.path) packet := suite.CreateMockPacket() @@ -711,6 +717,8 @@ 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)), @@ -731,18 +739,17 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() { packetFee types.PacketFee originalBalance sdk.Coins expectedBalance sdk.Coins + expLocked bool ) testCases := []struct { name string malleate func() expFeeDistributed bool - expLocked bool }{ { "success", func() {}, true, - true, }, { "fee not enabled", @@ -752,17 +759,16 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() { expectedBalance = originalBalance }, false, - true, }, { "fee module is disabled, skip fee logic", func() { lockFeeModule(suite.chainA) + expLocked = true expectedBalance = originalBalance }, false, - false, }, { "no op if identified packet fee doesn't exist", @@ -774,7 +780,6 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() { expectedBalance = originalBalance }, false, - true, }, { "distribute fee fails for timeout fee (blocked address)", @@ -787,7 +792,6 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() { Add(packetFee.Fee.TimeoutFee...) }, false, - true, }, { "fail on no distribution by escrow account out of balance", @@ -800,9 +804,10 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() { expectedBalance = originalBalance.Add(smallAmount...) suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, types.NewPacketFees([]types.PacketFee{packetFee})) + + expLocked = true }, false, - false, }, } @@ -810,6 +815,8 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() { tc := tc suite.Run(tc.name, func() { suite.SetupTest() + expLocked = false + suite.coordinator.Setup(suite.path) packet := suite.CreateMockPacket() @@ -852,11 +859,9 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() { tc.malleate() err = cbs.OnTimeoutPacket(suite.chainA.GetContext(), packet, relayerAddr) - if tc.expLocked { - suite.Require().False(suite.chainA.GetSimApp().IBCFeeKeeper.IsLocked(suite.chainA.GetContext())) - } else { - suite.Require().True(suite.chainA.GetSimApp().IBCFeeKeeper.IsLocked(suite.chainA.GetContext())) - } + suite.Require().NoError(err) + + suite.Require().Equal(expLocked, suite.chainA.GetSimApp().IBCFeeKeeper.IsLocked(suite.chainA.GetContext())) suite.Require().Equal( expectedBalance, From f48ab8b5b82c9ff5b5fa4b29840481f05abe1aae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Mon, 16 May 2022 12:57:43 +0200 Subject: [PATCH 18/18] add fee distribution check --- modules/apps/29-fee/ibc_middleware_test.go | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/modules/apps/29-fee/ibc_middleware_test.go b/modules/apps/29-fee/ibc_middleware_test.go index 54520f18e93..061415cca29 100644 --- a/modules/apps/29-fee/ibc_middleware_test.go +++ b/modules/apps/29-fee/ibc_middleware_test.go @@ -568,9 +568,10 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() { ) testCases := []struct { - name string - malleate func() - expPass bool + name string + malleate func() + expFeesDistributed bool + expPass bool }{ { "success", @@ -578,6 +579,7 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() { expectedRelayerBalance = packetFee.Fee.RecvFee.Add(packetFee.Fee.AckFee[0]) }, true, + true, }, { "no op success without a packet fee", @@ -593,6 +595,7 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() { expectedBalance = originalBalance }, true, + true, }, { "ack wrong format", @@ -602,6 +605,7 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() { expectedBalance = originalBalance }, false, + false, }, { "channel is not fee not enabled, success", @@ -611,6 +615,7 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() { expectedBalance = originalBalance }, + false, true, }, { @@ -621,6 +626,7 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() { expectedBalance = originalBalance }, + false, true, }, { @@ -636,6 +642,7 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() { expectedRelayerBalance = packetFee.Fee.AckFee expectedBalance = expectedBalance.Add(packetFee.Fee.RecvFee...) }, + false, true, }, { @@ -653,6 +660,7 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() { expLocked = true }, + false, true, }, } @@ -725,6 +733,12 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() { ) 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,