From 4d5a7474ca971e6aeb6b1dab55727b66353ed361 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 13 Apr 2022 13:11:31 +0200 Subject: [PATCH 1/8] port code from #1056 --- modules/apps/29-fee/keeper/escrow.go | 65 +++++++++++++++++++++++----- 1 file changed, 55 insertions(+), 10 deletions(-) diff --git a/modules/apps/29-fee/keeper/escrow.go b/modules/apps/29-fee/keeper/escrow.go index 8f201293860..4727deee772 100644 --- a/modules/apps/29-fee/keeper/escrow.go +++ b/modules/apps/29-fee/keeper/escrow.go @@ -53,7 +53,23 @@ func (k Keeper) EscrowPacketFee(ctx sdk.Context, packetID channeltypes.PacketId, func (k Keeper) DistributePacketFees(ctx sdk.Context, forwardRelayer string, reverseRelayer sdk.AccAddress, feesInEscrow []types.PacketFee) { forwardAddr, _ := sdk.AccAddressFromBech32(forwardRelayer) + // 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 _, packetFee := range feesInEscrow { + 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) + + return + } + refundAddr, err := sdk.AccAddressFromBech32(packetFee.RefundAddress) if err != nil { panic(fmt.Sprintf("could not parse refundAcc %s to sdk.AccAddress", packetFee.RefundAddress)) @@ -62,38 +78,67 @@ func (k Keeper) DistributePacketFees(ctx sdk.Context, forwardRelayer string, rev // distribute fee to valid forward relayer address otherwise refund the fee if !forwardAddr.Empty() && !k.bankKeeper.BlockedAddr(forwardAddr) { // distribute fee for forward relaying - k.distributeFee(ctx, forwardAddr, refundAddr, packetFee.Fee.RecvFee) + k.distributeFee(cacheCtx, forwardAddr, refundAddr, packetFee.Fee.RecvFee) } else { // refund onRecv fee as forward relayer is not valid address - k.distributeFee(ctx, refundAddr, refundAddr, packetFee.Fee.RecvFee) + k.distributeFee(cacheCtx, refundAddr, refundAddr, packetFee.Fee.RecvFee) } // distribute fee for reverse relaying - k.distributeFee(ctx, reverseRelayer, refundAddr, packetFee.Fee.AckFee) + k.distributeFee(cacheCtx, reverseRelayer, refundAddr, packetFee.Fee.AckFee) // refund timeout fee for unused timeout - k.distributeFee(ctx, refundAddr, refundAddr, packetFee.Fee.TimeoutFee) + k.distributeFee(cacheCtx, refundAddr, refundAddr, packetFee.Fee.TimeoutFee) } + + // 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()) } // DistributePacketsFeesTimeout pays the timeout fee for a given packetID while refunding the acknowledgement fee & receive fee to the refund account associated with the Fee func (k Keeper) DistributePacketFeesOnTimeout(ctx sdk.Context, timeoutRelayer sdk.AccAddress, feesInEscrow []types.PacketFee) { - for _, feeInEscrow := range feesInEscrow { + + // 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 _, packetFee := range feesInEscrow { + 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) + + return + } + // check if refundAcc address works - refundAddr, err := sdk.AccAddressFromBech32(feeInEscrow.RefundAddress) + refundAddr, err := sdk.AccAddressFromBech32(packetFee.RefundAddress) if err != nil { - panic(fmt.Sprintf("could not parse refundAcc %s to sdk.AccAddress", feeInEscrow.RefundAddress)) + panic(fmt.Sprintf("could not parse refundAcc %s to sdk.AccAddress", packetFee.RefundAddress)) } // refund receive fee for unused forward relaying - k.distributeFee(ctx, refundAddr, refundAddr, feeInEscrow.Fee.RecvFee) + k.distributeFee(cacheCtx, refundAddr, refundAddr, packetFee.Fee.RecvFee) // refund ack fee for unused reverse relaying - k.distributeFee(ctx, refundAddr, refundAddr, feeInEscrow.Fee.AckFee) + k.distributeFee(cacheCtx, refundAddr, refundAddr, packetFee.Fee.AckFee) // distribute fee for timeout relaying - k.distributeFee(ctx, timeoutRelayer, refundAddr, feeInEscrow.Fee.TimeoutFee) + k.distributeFee(cacheCtx, timeoutRelayer, refundAddr, packetFee.Fee.TimeoutFee) } + + // 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()) } // distributeFee will attempt to distribute the escrowed fee to the receiver address. From ca65d4f0da3cdbc016595b8cd7cb21a1009075e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 13 Apr 2022 13:37:14 +0200 Subject: [PATCH 2/8] refactor: deduplicate distribution code --- modules/apps/29-fee/ibc_module.go | 4 +- modules/apps/29-fee/keeper/escrow.go | 83 +++++++++-------------- modules/apps/29-fee/keeper/escrow_test.go | 4 +- 3 files changed, 36 insertions(+), 55 deletions(-) diff --git a/modules/apps/29-fee/ibc_module.go b/modules/apps/29-fee/ibc_module.go index c542c5999e6..a6f4a2c07d8 100644 --- a/modules/apps/29-fee/ibc_module.go +++ b/modules/apps/29-fee/ibc_module.go @@ -229,7 +229,7 @@ func (im IBCModule) OnAcknowledgementPacket( packetID := channeltypes.NewPacketId(packet.SourcePort, packet.SourceChannel, packet.Sequence) feesInEscrow, found := im.keeper.GetFeesInEscrow(ctx, packetID) if found { - im.keeper.DistributePacketFees(ctx, ack.ForwardRelayerAddress, relayer, feesInEscrow.PacketFees) + im.keeper.DistributePacketFees(ctx, ack.ForwardRelayerAddress, relayer, feesInEscrow.PacketFees, false) // removes the fees from the store as fees are now paid im.keeper.DeleteFeesInEscrow(ctx, packetID) @@ -258,7 +258,7 @@ func (im IBCModule) 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) + im.keeper.DistributePacketFees(ctx, "", relayer, feesInEscrow.PacketFees, true) // timeouts have no forward relayer // removes the fee from the store as fee is now paid im.keeper.DeleteFeesInEscrow(ctx, packetID) diff --git a/modules/apps/29-fee/keeper/escrow.go b/modules/apps/29-fee/keeper/escrow.go index 4727deee772..455583a15ee 100644 --- a/modules/apps/29-fee/keeper/escrow.go +++ b/modules/apps/29-fee/keeper/escrow.go @@ -50,14 +50,14 @@ func (k Keeper) EscrowPacketFee(ctx sdk.Context, packetID channeltypes.PacketId, } // DistributePacketFees pays the acknowledgement fee & receive fee for a given packetID while refunding the timeout fee to the refund account associated with the Fee. -func (k Keeper) DistributePacketFees(ctx sdk.Context, forwardRelayer string, reverseRelayer sdk.AccAddress, feesInEscrow []types.PacketFee) { - forwardAddr, _ := sdk.AccAddressFromBech32(forwardRelayer) - +func (k Keeper) DistributePacketFees(ctx sdk.Context, forwardRelayer string, reverseRelayer sdk.AccAddress, packetFees []types.PacketFee, isTimeout bool) { // 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 _, packetFee := range feesInEscrow { + forwardAddr, _ := sdk.AccAddressFromBech32(forwardRelayer) + + for _, packetFee := range 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 @@ -70,25 +70,17 @@ func (k Keeper) DistributePacketFees(ctx sdk.Context, forwardRelayer string, rev return } + // check if refundAcc address works refundAddr, err := sdk.AccAddressFromBech32(packetFee.RefundAddress) if err != nil { panic(fmt.Sprintf("could not parse refundAcc %s to sdk.AccAddress", packetFee.RefundAddress)) } - // distribute fee to valid forward relayer address otherwise refund the fee - if !forwardAddr.Empty() && !k.bankKeeper.BlockedAddr(forwardAddr) { - // distribute fee for forward relaying - k.distributeFee(cacheCtx, forwardAddr, refundAddr, packetFee.Fee.RecvFee) + if isTimeout { + k.distributePacketFeeOnTimeout(ctx, refundAddr, reverseRelayer, packetFee) } else { - // refund onRecv fee as forward relayer is not valid address - k.distributeFee(cacheCtx, refundAddr, refundAddr, packetFee.Fee.RecvFee) + k.distributePacketFeeOnAcknowledgement(ctx, refundAddr, forwardAddr, reverseRelayer, packetFee) } - - // distribute fee for reverse relaying - k.distributeFee(cacheCtx, reverseRelayer, refundAddr, packetFee.Fee.AckFee) - - // refund timeout fee for unused timeout - k.distributeFee(cacheCtx, refundAddr, refundAddr, packetFee.Fee.TimeoutFee) } // write the cache @@ -96,49 +88,38 @@ func (k Keeper) DistributePacketFees(ctx sdk.Context, forwardRelayer string, rev // 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()) -} - -// DistributePacketsFeesTimeout pays the timeout fee for a given packetID while refunding the acknowledgement fee & receive fee to the refund account associated with the Fee -func (k Keeper) DistributePacketFeesOnTimeout(ctx sdk.Context, timeoutRelayer sdk.AccAddress, feesInEscrow []types.PacketFee) { - // 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 _, packetFee := range feesInEscrow { - 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) +} - return - } +// distributePacketFeeOnAcknowledgement pays the acknowledgement fee & receive fee for a given packetID while refunding the timeout fee to the refund account associated with the Fee. +func (k Keeper) distributePacketFeeOnAcknowledgement(ctx sdk.Context, refundAddr, forwardRelayer, reverseRelayer sdk.AccAddress, packetFee types.PacketFee) { + // distribute fee to valid forward relayer address otherwise refund the fee + if !forwardRelayer.Empty() && !k.bankKeeper.BlockedAddr(forwardRelayer) { + // distribute fee for forward relaying + k.distributeFee(ctx, forwardRelayer, refundAddr, packetFee.Fee.RecvFee) + } else { + // refund onRecv fee as forward relayer is not valid address + k.distributeFee(ctx, refundAddr, refundAddr, packetFee.Fee.RecvFee) + } - // check if refundAcc address works - refundAddr, err := sdk.AccAddressFromBech32(packetFee.RefundAddress) - if err != nil { - panic(fmt.Sprintf("could not parse refundAcc %s to sdk.AccAddress", packetFee.RefundAddress)) - } + // distribute fee for reverse relaying + k.distributeFee(ctx, reverseRelayer, refundAddr, packetFee.Fee.AckFee) - // refund receive fee for unused forward relaying - k.distributeFee(cacheCtx, refundAddr, refundAddr, packetFee.Fee.RecvFee) + // refund timeout fee for unused timeout + k.distributeFee(ctx, refundAddr, refundAddr, packetFee.Fee.TimeoutFee) - // refund ack fee for unused reverse relaying - k.distributeFee(cacheCtx, refundAddr, refundAddr, packetFee.Fee.AckFee) +} - // distribute fee for timeout relaying - k.distributeFee(cacheCtx, timeoutRelayer, refundAddr, packetFee.Fee.TimeoutFee) - } +// distributePacketFeeOnTimeout pays the timeout fee for a given packetID while refunding the acknowledgement fee & receive fee to the refund account associated with the Fee +func (k Keeper) distributePacketFeeOnTimeout(ctx sdk.Context, refundAddr sdk.AccAddress, timeoutRelayer sdk.AccAddress, packetFee types.PacketFee) { + // refund receive fee for unused forward relaying + k.distributeFee(ctx, refundAddr, refundAddr, packetFee.Fee.RecvFee) - // write the cache - writeFn() + // refund ack fee for unused reverse relaying + k.distributeFee(ctx, refundAddr, refundAddr, packetFee.Fee.AckFee) - // 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()) + // distribute fee for timeout relaying + k.distributeFee(ctx, timeoutRelayer, refundAddr, packetFee.Fee.TimeoutFee) } // distributeFee will attempt to distribute the escrowed fee to the receiver address. diff --git a/modules/apps/29-fee/keeper/escrow_test.go b/modules/apps/29-fee/keeper/escrow_test.go index 85747ae7acf..ec7ac50bc0d 100644 --- a/modules/apps/29-fee/keeper/escrow_test.go +++ b/modules/apps/29-fee/keeper/escrow_test.go @@ -244,7 +244,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.DistributePacketFees(suite.chainA.GetContext(), forwardRelayer, reverseRelayer, []types.PacketFee{packetFee, packetFee}) + suite.chainA.GetSimApp().IBCFeeKeeper.DistributePacketFees(suite.chainA.GetContext(), forwardRelayer, reverseRelayer, []types.PacketFee{packetFee, packetFee}, false) tc.expResult() }) @@ -282,7 +282,7 @@ func (suite *KeeperTestSuite) TestDistributeTimeoutFee() { // refundAcc balance after escrow refundAccBal := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom) - suite.chainA.GetSimApp().IBCFeeKeeper.DistributePacketFeesOnTimeout(suite.chainA.GetContext(), timeoutRelayer, []types.PacketFee{packetFee, packetFee}) + suite.chainA.GetSimApp().IBCFeeKeeper.DistributePacketFees(suite.chainA.GetContext(), "", timeoutRelayer, []types.PacketFee{packetFee, packetFee}, true) // check if the timeoutRelayer has been paid hasBalance := suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), timeoutRelayer, fee.TimeoutFee[0]) From e91f84900fde6bda7c9bce8b3bbdfb30941bd8f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 13 Apr 2022 13:54:24 +0200 Subject: [PATCH 3/8] add test case for on acknowledgement packet distribution --- modules/apps/29-fee/keeper/escrow.go | 4 ++-- modules/apps/29-fee/keeper/escrow_test.go | 21 +++++++++++++++++++-- 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/modules/apps/29-fee/keeper/escrow.go b/modules/apps/29-fee/keeper/escrow.go index 455583a15ee..51cb8105b48 100644 --- a/modules/apps/29-fee/keeper/escrow.go +++ b/modules/apps/29-fee/keeper/escrow.go @@ -77,9 +77,9 @@ func (k Keeper) DistributePacketFees(ctx sdk.Context, forwardRelayer string, rev } if isTimeout { - k.distributePacketFeeOnTimeout(ctx, refundAddr, reverseRelayer, packetFee) + k.distributePacketFeeOnTimeout(cacheCtx, refundAddr, reverseRelayer, packetFee) } else { - k.distributePacketFeeOnAcknowledgement(ctx, refundAddr, forwardAddr, reverseRelayer, packetFee) + k.distributePacketFeeOnAcknowledgement(cacheCtx, refundAddr, forwardAddr, reverseRelayer, packetFee) } } diff --git a/modules/apps/29-fee/keeper/escrow_test.go b/modules/apps/29-fee/keeper/escrow_test.go index ec7ac50bc0d..c240e5e30b7 100644 --- a/modules/apps/29-fee/keeper/escrow_test.go +++ b/modules/apps/29-fee/keeper/escrow_test.go @@ -128,6 +128,7 @@ func (suite *KeeperTestSuite) TestDistributeFee() { refundAcc sdk.AccAddress refundAccBal sdk.Coin packetFee types.PacketFee + packetFees []types.PacketFee ) testCases := []struct { @@ -162,6 +163,20 @@ func (suite *KeeperTestSuite) TestDistributeFee() { suite.Require().Equal(sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(0)), balance) }, }, + { + "escrow account out of balance, fee module becomes locked - no distribution", func() { + // pass in an extra packet fee + packetFees = append(packetFees, packetFee) + }, + func() { + suite.Require().True(suite.chainA.GetSimApp().IBCFeeKeeper.IsLocked(suite.chainA.GetContext())) + + // check if the module acc contains all the fees + expectedModuleAccBal := packetFee.Fee.Total().Add(packetFee.Fee.Total()...) + balance := suite.chainA.GetSimApp().BankKeeper.GetAllBalances(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress()) + suite.Require().Equal(expectedModuleAccBal, balance) + }, + }, { "invalid forward address", func() { @@ -201,7 +216,8 @@ func (suite *KeeperTestSuite) TestDistributeFee() { { "invalid refund address: no-op, timeout fee remains in escrow", func() { - packetFee.RefundAddress = suite.chainA.GetSimApp().AccountKeeper.GetModuleAccount(suite.chainA.GetContext(), transfertypes.ModuleName).GetAddress().String() + packetFees[0].RefundAddress = suite.chainA.GetSimApp().AccountKeeper.GetModuleAccount(suite.chainA.GetContext(), transfertypes.ModuleName).GetAddress().String() + packetFees[1].RefundAddress = suite.chainA.GetSimApp().AccountKeeper.GetModuleAccount(suite.chainA.GetContext(), transfertypes.ModuleName).GetAddress().String() }, func() { // check if the module acc contains the timeoutFee @@ -229,6 +245,7 @@ func (suite *KeeperTestSuite) TestDistributeFee() { // escrow the packet fee & store the fee in state packetFee = types.NewPacketFee(fee, refundAcc.String(), []string{}) + packetFees = []types.PacketFee{packetFee, packetFee} err := suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee) suite.Require().NoError(err) @@ -244,7 +261,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.DistributePacketFees(suite.chainA.GetContext(), forwardRelayer, reverseRelayer, []types.PacketFee{packetFee, packetFee}, false) + suite.chainA.GetSimApp().IBCFeeKeeper.DistributePacketFees(suite.chainA.GetContext(), forwardRelayer, reverseRelayer, packetFees, false) tc.expResult() }) From 317aaa191ee92f64549694fad8bb0660272ccf14 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 13 Apr 2022 13:56:29 +0200 Subject: [PATCH 4/8] add check for timeout packet distribution --- 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 c240e5e30b7..8c0905f1d34 100644 --- a/modules/apps/29-fee/keeper/escrow_test.go +++ b/modules/apps/29-fee/keeper/escrow_test.go @@ -314,6 +314,12 @@ func (suite *KeeperTestSuite) TestDistributeTimeoutFee() { // 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) + + // attempt to distribute with empty escrow balance + suite.chainA.GetSimApp().IBCFeeKeeper.DistributePacketFees(suite.chainA.GetContext(), "", timeoutRelayer, []types.PacketFee{packetFee, packetFee}, true) + + suite.Require().True(suite.chainA.GetSimApp().IBCFeeKeeper.IsLocked(suite.chainA.GetContext())) + } func (suite *KeeperTestSuite) TestRefundFeesOnChannelClosure() { From 8e73b5adfe66a648a5a1d26e98a04abcea4603d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Thu, 14 Apr 2022 12:29:49 +0200 Subject: [PATCH 5/8] revert to duplicate code approach --- modules/apps/29-fee/ibc_module.go | 4 +- modules/apps/29-fee/keeper/escrow.go | 45 +++++++++++++++++++---- modules/apps/29-fee/keeper/escrow_test.go | 4 +- 3 files changed, 41 insertions(+), 12 deletions(-) diff --git a/modules/apps/29-fee/ibc_module.go b/modules/apps/29-fee/ibc_module.go index a6f4a2c07d8..76a8bd64676 100644 --- a/modules/apps/29-fee/ibc_module.go +++ b/modules/apps/29-fee/ibc_module.go @@ -229,7 +229,7 @@ func (im IBCModule) OnAcknowledgementPacket( packetID := channeltypes.NewPacketId(packet.SourcePort, packet.SourceChannel, packet.Sequence) feesInEscrow, found := im.keeper.GetFeesInEscrow(ctx, packetID) if found { - im.keeper.DistributePacketFees(ctx, ack.ForwardRelayerAddress, relayer, feesInEscrow.PacketFees, false) + 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) @@ -258,7 +258,7 @@ func (im IBCModule) OnTimeoutPacket( packetID := channeltypes.NewPacketId(packet.SourcePort, packet.SourceChannel, packet.Sequence) feesInEscrow, found := im.keeper.GetFeesInEscrow(ctx, packetID) if found { - im.keeper.DistributePacketFees(ctx, "", relayer, feesInEscrow.PacketFees, true) // timeouts have no forward relayer + im.keeper.DistributePacketFeesOnTimeout(ctx, relayer, feesInEscrow.PacketFees) // timeouts have no forward relayer // removes the fee from the store as fee is now paid im.keeper.DeleteFeesInEscrow(ctx, packetID) diff --git a/modules/apps/29-fee/keeper/escrow.go b/modules/apps/29-fee/keeper/escrow.go index 1ec8e045ca8..2ab6e97dd3f 100644 --- a/modules/apps/29-fee/keeper/escrow.go +++ b/modules/apps/29-fee/keeper/escrow.go @@ -49,8 +49,8 @@ func (k Keeper) EscrowPacketFee(ctx sdk.Context, packetID channeltypes.PacketId, return nil } -// DistributePacketFees pays all the acknowledgement & receive fees for a given packetID while refunding the timeout fees to the refund account. -func (k Keeper) DistributePacketFees(ctx sdk.Context, forwardRelayer string, reverseRelayer sdk.AccAddress, packetFees []types.PacketFee, isTimeout bool) { +// 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) { // 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() @@ -76,11 +76,7 @@ func (k Keeper) DistributePacketFees(ctx sdk.Context, forwardRelayer string, rev panic(fmt.Sprintf("could not parse refundAcc %s to sdk.AccAddress", packetFee.RefundAddress)) } - if isTimeout { - k.distributePacketFeeOnTimeout(cacheCtx, refundAddr, reverseRelayer, packetFee) - } else { - k.distributePacketFeeOnAcknowledgement(cacheCtx, refundAddr, forwardAddr, reverseRelayer, packetFee) - } + k.distributePacketFeeOnAcknowledgement(cacheCtx, refundAddr, forwardAddr, reverseRelayer, packetFee) } // write the cache @@ -88,7 +84,6 @@ func (k Keeper) DistributePacketFees(ctx sdk.Context, forwardRelayer string, rev // 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. @@ -111,6 +106,40 @@ func (k Keeper) distributePacketFeeOnAcknowledgement(ctx sdk.Context, refundAddr } +func (k Keeper) DistributePacketFeesOnTimeout(ctx sdk.Context, timeoutRelayer sdk.AccAddress, packetFees []types.PacketFee) { + // 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 _, packetFee := range 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) + + return + } + + // check if refundAcc address works + refundAddr, err := sdk.AccAddressFromBech32(packetFee.RefundAddress) + if err != nil { + panic(fmt.Sprintf("could not parse refundAcc %s to sdk.AccAddress", packetFee.RefundAddress)) + } + + k.distributePacketFeeOnTimeout(cacheCtx, refundAddr, timeoutRelayer, packetFee) + } + + // 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()) +} + // distributePacketFeeOnTimeout pays the timeout fee to the timeout relayer and refunds the acknowledgement & receive fee. func (k Keeper) distributePacketFeeOnTimeout(ctx sdk.Context, refundAddr, timeoutRelayer sdk.AccAddress, packetFee types.PacketFee) { // refund receive fee for unused forward relaying diff --git a/modules/apps/29-fee/keeper/escrow_test.go b/modules/apps/29-fee/keeper/escrow_test.go index 8c0905f1d34..13e747e48d6 100644 --- a/modules/apps/29-fee/keeper/escrow_test.go +++ b/modules/apps/29-fee/keeper/escrow_test.go @@ -119,7 +119,7 @@ func (suite *KeeperTestSuite) TestEscrowPacketFee() { } } -func (suite *KeeperTestSuite) TestDistributeFee() { +func (suite *KeeperTestSuite) TestDistributePacketFeesOnAcknowledgement() { var ( forwardRelayer string forwardRelayerBal sdk.Coin @@ -261,7 +261,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.DistributePacketFees(suite.chainA.GetContext(), forwardRelayer, reverseRelayer, packetFees, false) + suite.chainA.GetSimApp().IBCFeeKeeper.DistributePacketFeesOnAcknowledgement(suite.chainA.GetContext(), forwardRelayer, reverseRelayer, packetFees) tc.expResult() }) From f968b786d973deb68df7e80dc4d133f33fa46d5c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Thu, 14 Apr 2022 12:45:14 +0200 Subject: [PATCH 6/8] add tests for DistributePacketFeesOnTimeout --- modules/apps/29-fee/keeper/escrow_test.go | 146 ++++++++++++------ modules/apps/29-fee/keeper/genesis_test.go | 4 +- modules/apps/29-fee/keeper/grpc_query_test.go | 12 +- modules/apps/29-fee/keeper/keeper_test.go | 10 +- modules/apps/29-fee/keeper/msg_server_test.go | 4 +- 5 files changed, 117 insertions(+), 59 deletions(-) diff --git a/modules/apps/29-fee/keeper/escrow_test.go b/modules/apps/29-fee/keeper/escrow_test.go index 13e747e48d6..ea915beedf6 100644 --- a/modules/apps/29-fee/keeper/escrow_test.go +++ b/modules/apps/29-fee/keeper/escrow_test.go @@ -78,7 +78,7 @@ func (suite *KeeperTestSuite) TestEscrowPacketFee() { // setup refundAcc = suite.chainA.SenderAccount.GetAddress() - receiveFee = defaultReceiveFee + receiveFee = defaultRecvFee ackFee = defaultAckFee timeoutFee = defaultTimeoutFee packetID = channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, uint64(1)) @@ -149,7 +149,7 @@ func (suite *KeeperTestSuite) TestDistributePacketFeesOnAcknowledgement() { forward, err := sdk.AccAddressFromBech32(forwardRelayer) suite.Require().NoError(err) - expectedForwardAccBal := forwardRelayerBal.Add(defaultReceiveFee[0]).Add(defaultReceiveFee[0]) + expectedForwardAccBal := forwardRelayerBal.Add(defaultRecvFee[0]).Add(defaultRecvFee[0]) balance = suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), forward, sdk.DefaultBondDenom) suite.Require().Equal(expectedForwardAccBal, balance) @@ -184,7 +184,7 @@ func (suite *KeeperTestSuite) TestDistributePacketFeesOnAcknowledgement() { }, func() { // check if the refund acc has been refunded the timeoutFee & recvFee - expectedRefundAccBal := refundAccBal.Add(defaultTimeoutFee[0]).Add(defaultReceiveFee[0]).Add(defaultTimeoutFee[0]).Add(defaultReceiveFee[0]) + expectedRefundAccBal := refundAccBal.Add(defaultTimeoutFee[0]).Add(defaultRecvFee[0]).Add(defaultTimeoutFee[0]).Add(defaultRecvFee[0]) balance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom) suite.Require().Equal(expectedRefundAccBal, balance) }, @@ -196,7 +196,7 @@ func (suite *KeeperTestSuite) TestDistributePacketFeesOnAcknowledgement() { }, func() { // check if the refund acc has been refunded the timeoutFee & recvFee - expectedRefundAccBal := refundAccBal.Add(defaultTimeoutFee[0]).Add(defaultReceiveFee[0]).Add(defaultTimeoutFee[0]).Add(defaultReceiveFee[0]) + expectedRefundAccBal := refundAccBal.Add(defaultTimeoutFee[0]).Add(defaultRecvFee[0]).Add(defaultTimeoutFee[0]).Add(defaultRecvFee[0]) balance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom) suite.Require().Equal(expectedRefundAccBal, balance) }, @@ -241,7 +241,7 @@ func (suite *KeeperTestSuite) TestDistributePacketFeesOnAcknowledgement() { refundAcc = suite.chainA.SenderAccount.GetAddress() packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, 1) - fee := types.NewFee(defaultReceiveFee, defaultAckFee, defaultTimeoutFee) + fee := types.NewFee(defaultRecvFee, defaultAckFee, defaultTimeoutFee) // escrow the packet fee & store the fee in state packetFee = types.NewPacketFee(fee, refundAcc.String(), []string{}) @@ -268,58 +268,116 @@ func (suite *KeeperTestSuite) TestDistributePacketFeesOnAcknowledgement() { } } -func (suite *KeeperTestSuite) TestDistributeTimeoutFee() { - suite.coordinator.Setup(suite.path) // setup channel +func (suite *KeeperTestSuite) TestDistributePacketFeesOnTimeout() { + var ( + timeoutRelayer sdk.AccAddress + timeoutRelayerBal sdk.Coin + refundAcc sdk.AccAddress + refundAccBal sdk.Coin + packetFee types.PacketFee + packetFees []types.PacketFee + ) - // setup - refundAcc := suite.chainA.SenderAccount.GetAddress() - timeoutRelayer := sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()) + testCases := []struct { + name string + malleate func() + expResult func() + }{ + { + "success", + func() {}, + func() { + // check if the timeout relayer is paid + expectedTimeoutAccBal := timeoutRelayerBal.Add(defaultTimeoutFee[0]).Add(defaultTimeoutFee[0]) + balance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), timeoutRelayer, sdk.DefaultBondDenom) + suite.Require().Equal(expectedTimeoutAccBal, balance) - packetID := channeltypes.NewPacketId( - suite.path.EndpointA.ChannelConfig.PortID, - suite.path.EndpointA.ChannelID, - 1, - ) + // check if the refund acc has been refunded the recv/ack fees + expectedRefundAccBal := refundAccBal.Add(defaultAckFee[0]).Add(defaultAckFee[0]).Add(defaultRecvFee[0]).Add(defaultRecvFee[0]) + balance = suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom) + suite.Require().Equal(expectedRefundAccBal, balance) - fee := types.Fee{ - RecvFee: defaultReceiveFee, - AckFee: defaultAckFee, - TimeoutFee: defaultTimeoutFee, + // check the module acc wallet is now empty + 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) + }, + }, + { + "escrow account out of balance, fee module becomes locked - no distribution", func() { + // pass in an extra packet fee + packetFees = append(packetFees, packetFee) + }, + func() { + suite.Require().True(suite.chainA.GetSimApp().IBCFeeKeeper.IsLocked(suite.chainA.GetContext())) + + // check if the module acc contains all the fees + expectedModuleAccBal := packetFee.Fee.Total().Add(packetFee.Fee.Total()...) + balance := suite.chainA.GetSimApp().BankKeeper.GetAllBalances(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress()) + suite.Require().Equal(expectedModuleAccBal, balance) + }, + }, + { + "invalid timeout relayer address: timeout fee returned to sender", + func() { + timeoutRelayer = suite.chainA.GetSimApp().AccountKeeper.GetModuleAccount(suite.chainA.GetContext(), transfertypes.ModuleName).GetAddress() + }, + func() { + // check if the refund acc has been refunded the all the fees + expectedRefundAccBal := sdk.Coins{refundAccBal}.Add(packetFee.Fee.Total()...).Add(packetFee.Fee.Total()...)[0] + balance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom) + suite.Require().Equal(expectedRefundAccBal, balance) + }, + }, + { + "invalid refund address: no-op, recv and ack fees remain in escrow", + func() { + packetFees[0].RefundAddress = suite.chainA.GetSimApp().AccountKeeper.GetModuleAccount(suite.chainA.GetContext(), transfertypes.ModuleName).GetAddress().String() + packetFees[1].RefundAddress = suite.chainA.GetSimApp().AccountKeeper.GetModuleAccount(suite.chainA.GetContext(), transfertypes.ModuleName).GetAddress().String() + }, + func() { + // check if the module acc contains the timeoutFee + expectedModuleAccBal := sdk.NewCoin(sdk.DefaultBondDenom, defaultRecvFee.Add(defaultRecvFee[0]).Add(defaultAckFee[0]).Add(defaultAckFee[0]).AmountOf(sdk.DefaultBondDenom)) + balance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress(), sdk.DefaultBondDenom) + suite.Require().Equal(expectedModuleAccBal, balance) + }, + }, } - // escrow the packet fee & store the fee in state - packetFee := types.NewPacketFee(fee, refundAcc.String(), []string{}) + for _, tc := range testCases { + tc := tc - err := suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee) - suite.Require().NoError(err) - // escrow a second packet fee to test with multiple fees distributed - err = suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee) - suite.Require().NoError(err) + suite.Run(tc.name, func() { + suite.SetupTest() // reset + suite.coordinator.Setup(suite.path) // setup channel - // refundAcc balance after escrow - refundAccBal := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom) + // setup accounts + timeoutRelayer = sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()) + refundAcc = suite.chainA.SenderAccount.GetAddress() - suite.chainA.GetSimApp().IBCFeeKeeper.DistributePacketFees(suite.chainA.GetContext(), "", timeoutRelayer, []types.PacketFee{packetFee, packetFee}, true) + packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, 1) + fee := types.NewFee(defaultRecvFee, defaultAckFee, defaultTimeoutFee) - // check if the timeoutRelayer has been paid - hasBalance := suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), timeoutRelayer, fee.TimeoutFee[0]) - suite.Require().True(hasBalance) + // escrow the packet fee & store the fee in state + packetFee = types.NewPacketFee(fee, refundAcc.String(), []string{}) + packetFees = []types.PacketFee{packetFee, packetFee} + err := suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee) + suite.Require().NoError(err) - // check if the refund acc has been refunded the recv & ack fees - expectedRefundAccBal := refundAccBal.Add(fee.AckFee[0]).Add(fee.AckFee[0]) - expectedRefundAccBal = refundAccBal.Add(fee.RecvFee[0]).Add(fee.RecvFee[0]) - hasBalance = suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), refundAcc, expectedRefundAccBal) - suite.Require().True(hasBalance) + // escrow a second packet fee to test with multiple fees distributed + err = suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee) + suite.Require().NoError(err) - // 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) + tc.malleate() - // attempt to distribute with empty escrow balance - suite.chainA.GetSimApp().IBCFeeKeeper.DistributePacketFees(suite.chainA.GetContext(), "", timeoutRelayer, []types.PacketFee{packetFee, packetFee}, true) + // fetch the account balances before fee distribution (forward, reverse, refund) + 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.Require().True(suite.chainA.GetSimApp().IBCFeeKeeper.IsLocked(suite.chainA.GetContext())) + suite.chainA.GetSimApp().IBCFeeKeeper.DistributePacketFeesOnTimeout(suite.chainA.GetContext(), timeoutRelayer, packetFees) + tc.expResult() + }) + } } func (suite *KeeperTestSuite) TestRefundFeesOnChannelClosure() { @@ -469,7 +527,7 @@ func (suite *KeeperTestSuite) TestRefundFeesOnChannelClosure() { expRefundBal = suite.chainA.GetSimApp().BankKeeper.GetAllBalances(suite.chainA.GetContext(), refundAcc) fee = types.Fee{ - RecvFee: defaultReceiveFee, + RecvFee: defaultRecvFee, AckFee: defaultAckFee, TimeoutFee: defaultTimeoutFee, } diff --git a/modules/apps/29-fee/keeper/genesis_test.go b/modules/apps/29-fee/keeper/genesis_test.go index ec9602d3a4f..8aa30385e58 100644 --- a/modules/apps/29-fee/keeper/genesis_test.go +++ b/modules/apps/29-fee/keeper/genesis_test.go @@ -11,7 +11,7 @@ func (suite *KeeperTestSuite) TestInitGenesis() { refundAcc := suite.chainA.SenderAccount.GetAddress() packetID := channeltypes.NewPacketId(ibctesting.MockFeePort, ibctesting.FirstChannelID, 1) fee := types.Fee{ - RecvFee: defaultReceiveFee, + RecvFee: defaultRecvFee, AckFee: defaultAckFee, TimeoutFee: defaultTimeoutFee, } @@ -73,7 +73,7 @@ func (suite *KeeperTestSuite) TestExportGenesis() { refundAcc := suite.chainA.SenderAccount.GetAddress() packetID := channeltypes.NewPacketId(ibctesting.MockFeePort, ibctesting.FirstChannelID, 1) fee := types.Fee{ - RecvFee: defaultReceiveFee, + RecvFee: defaultRecvFee, AckFee: defaultAckFee, TimeoutFee: defaultTimeoutFee, } diff --git a/modules/apps/29-fee/keeper/grpc_query_test.go b/modules/apps/29-fee/keeper/grpc_query_test.go index 446616ab176..43935490f2a 100644 --- a/modules/apps/29-fee/keeper/grpc_query_test.go +++ b/modules/apps/29-fee/keeper/grpc_query_test.go @@ -28,7 +28,7 @@ func (suite *KeeperTestSuite) TestQueryIncentivizedPackets() { func() { suite.chainA.GetSimApp().IBCFeeKeeper.SetFeeEnabled(suite.chainA.GetContext(), ibctesting.MockFeePort, ibctesting.FirstChannelID) - fee := types.NewFee(defaultReceiveFee, defaultAckFee, defaultTimeoutFee) + fee := types.NewFee(defaultRecvFee, defaultAckFee, defaultTimeoutFee) packetFee := types.NewPacketFee(fee, suite.chainA.SenderAccount.GetAddress().String(), []string(nil)) for i := 0; i < 3; i++ { @@ -113,7 +113,7 @@ func (suite *KeeperTestSuite) TestQueryIncentivizedPacket() { suite.chainA.GetSimApp().IBCFeeKeeper.SetFeeEnabled(suite.chainA.GetContext(), ibctesting.MockFeePort, ibctesting.FirstChannelID) packetID := channeltypes.NewPacketId(ibctesting.MockFeePort, ibctesting.FirstChannelID, 1) - fee := types.NewFee(defaultReceiveFee, defaultAckFee, defaultTimeoutFee) + fee := types.NewFee(defaultRecvFee, defaultAckFee, defaultTimeoutFee) packetFee := types.NewPacketFee(fee, suite.chainA.SenderAccount.GetAddress().String(), []string(nil)) for i := 0; i < 3; i++ { @@ -269,7 +269,7 @@ func (suite *KeeperTestSuite) TestQueryTotalRecvFees() { packetID := channeltypes.NewPacketId(ibctesting.MockFeePort, ibctesting.FirstChannelID, 1) - fee := types.NewFee(defaultReceiveFee, defaultAckFee, defaultTimeoutFee) + fee := types.NewFee(defaultRecvFee, defaultAckFee, defaultTimeoutFee) packetFee := types.NewPacketFee(fee, suite.chainA.SenderAccount.GetAddress().String(), []string(nil)) for i := 0; i < 3; i++ { @@ -292,7 +292,7 @@ func (suite *KeeperTestSuite) TestQueryTotalRecvFees() { suite.Require().NotNil(res) // expected total is three times the default recv fee - expectedFees := defaultReceiveFee.Add(defaultReceiveFee...).Add(defaultReceiveFee...) + expectedFees := defaultRecvFee.Add(defaultRecvFee...).Add(defaultRecvFee...) suite.Require().Equal(expectedFees, res.RecvFees) } else { suite.Require().Error(err) @@ -333,7 +333,7 @@ func (suite *KeeperTestSuite) TestQueryTotalAckFees() { packetID := channeltypes.NewPacketId(ibctesting.MockFeePort, ibctesting.FirstChannelID, 1) - fee := types.NewFee(defaultReceiveFee, defaultAckFee, defaultTimeoutFee) + fee := types.NewFee(defaultRecvFee, defaultAckFee, defaultTimeoutFee) packetFee := types.NewPacketFee(fee, suite.chainA.SenderAccount.GetAddress().String(), []string(nil)) for i := 0; i < 3; i++ { @@ -397,7 +397,7 @@ func (suite *KeeperTestSuite) TestQueryTotalTimeoutFees() { packetID := channeltypes.NewPacketId(ibctesting.MockFeePort, ibctesting.FirstChannelID, 1) - fee := types.NewFee(defaultReceiveFee, defaultAckFee, defaultTimeoutFee) + fee := types.NewFee(defaultRecvFee, defaultAckFee, defaultTimeoutFee) packetFee := types.NewPacketFee(fee, suite.chainA.SenderAccount.GetAddress().String(), []string(nil)) for i := 0; i < 3; i++ { diff --git a/modules/apps/29-fee/keeper/keeper_test.go b/modules/apps/29-fee/keeper/keeper_test.go index cf0d17ae494..c4be4553c5a 100644 --- a/modules/apps/29-fee/keeper/keeper_test.go +++ b/modules/apps/29-fee/keeper/keeper_test.go @@ -15,7 +15,7 @@ import ( ) var ( - defaultReceiveFee = sdk.Coins{sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: sdk.NewInt(100)}} + 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)}} invalidCoins = sdk.Coins{sdk.Coin{Denom: "invalidDenom", Amount: sdk.NewInt(100)}} @@ -76,7 +76,7 @@ func lockFeeModule(chain *ibctesting.TestChain) { } func (suite *KeeperTestSuite) TestEscrowAccountHasBalance() { - fee := types.NewFee(defaultReceiveFee, defaultAckFee, defaultTimeoutFee) + fee := types.NewFee(defaultRecvFee, defaultAckFee, defaultTimeoutFee) suite.Require().False(suite.chainA.GetSimApp().IBCFeeKeeper.EscrowAccountHasBalance(suite.chainA.GetContext(), fee.Total())) @@ -96,7 +96,7 @@ func (suite *KeeperTestSuite) TestFeesInEscrow() { // escrow five fees for packet sequence 1 packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, 1) - fee := types.NewFee(defaultReceiveFee, defaultAckFee, defaultTimeoutFee) + fee := types.NewFee(defaultRecvFee, defaultAckFee, defaultTimeoutFee) for i := 1; i < 6; i++ { packetFee := types.NewPacketFee(fee, suite.chainA.SenderAccount.GetAddress().String(), nil) @@ -132,7 +132,7 @@ func (suite *KeeperTestSuite) TestGetIdentifiedPacketFeesForChannel() { 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) + fee := types.NewFee(defaultRecvFee, defaultAckFee, defaultTimeoutFee) // escrow the packet fee packetFee := types.NewPacketFee(fee, refundAcc.String(), []string{}) @@ -193,7 +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.NewFee(defaultReceiveFee, defaultAckFee, defaultTimeoutFee) + fee := types.NewFee(defaultRecvFee, defaultAckFee, defaultTimeoutFee) // escrow the packet fee packetFee := types.NewPacketFee(fee, refundAcc.String(), []string{}) diff --git a/modules/apps/29-fee/keeper/msg_server_test.go b/modules/apps/29-fee/keeper/msg_server_test.go index 74c5342d674..2954ebcebaf 100644 --- a/modules/apps/29-fee/keeper/msg_server_test.go +++ b/modules/apps/29-fee/keeper/msg_server_test.go @@ -80,7 +80,7 @@ func (suite *KeeperTestSuite) TestPayPacketFee() { refundAcc := suite.chainA.SenderAccount.GetAddress() channelID := suite.path.EndpointA.ChannelID fee := types.Fee{ - RecvFee: defaultReceiveFee, + RecvFee: defaultRecvFee, AckFee: defaultAckFee, TimeoutFee: defaultTimeoutFee, } @@ -129,7 +129,7 @@ func (suite *KeeperTestSuite) TestPayPacketFeeAsync() { // build packetID channelID := suite.path.EndpointA.ChannelID fee := types.Fee{ - RecvFee: defaultReceiveFee, + RecvFee: defaultRecvFee, AckFee: defaultAckFee, TimeoutFee: defaultTimeoutFee, } From 558e8523be9ca51e5f0afa5c0f9b4fed17d06439 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Thu, 14 Apr 2022 12:47:31 +0200 Subject: [PATCH 7/8] chore: remove unnecessary comment --- modules/apps/29-fee/ibc_module.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/apps/29-fee/ibc_module.go b/modules/apps/29-fee/ibc_module.go index 76a8bd64676..e89b95cb4de 100644 --- a/modules/apps/29-fee/ibc_module.go +++ b/modules/apps/29-fee/ibc_module.go @@ -258,7 +258,7 @@ func (im IBCModule) 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) // timeouts have no forward relayer + im.keeper.DistributePacketFeesOnTimeout(ctx, relayer, feesInEscrow.PacketFees) // removes the fee from the store as fee is now paid im.keeper.DeleteFeesInEscrow(ctx, packetID) From b73b4392f665838d5acec07c4f9f1ebc08b82b06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Thu, 14 Apr 2022 14:11:11 +0200 Subject: [PATCH 8/8] add back godoc --- modules/apps/29-fee/keeper/escrow.go | 1 + 1 file changed, 1 insertion(+) diff --git a/modules/apps/29-fee/keeper/escrow.go b/modules/apps/29-fee/keeper/escrow.go index 2ab6e97dd3f..e2b29a439cc 100644 --- a/modules/apps/29-fee/keeper/escrow.go +++ b/modules/apps/29-fee/keeper/escrow.go @@ -106,6 +106,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) { // cache context before trying to distribute fees // if the escrow account has insufficient balance then we want to avoid partially distributing fees