From 3d86e6d0bb248d5e689ca73aa5becf79e2f7672a Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Mon, 11 Apr 2022 15:51:13 +0200 Subject: [PATCH 1/4] attempt to refund fees if distribution fails --- modules/apps/29-fee/ibc_module_test.go | 6 +++-- modules/apps/29-fee/keeper/escrow.go | 34 +++++++++++++++----------- 2 files changed, 24 insertions(+), 16 deletions(-) diff --git a/modules/apps/29-fee/ibc_module_test.go b/modules/apps/29-fee/ibc_module_test.go index 8bdc92458f7..ca8a4ab1f04 100644 --- a/modules/apps/29-fee/ibc_module_test.go +++ b/modules/apps/29-fee/ibc_module_test.go @@ -622,6 +622,7 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() { }.Acknowledgement() expectedRelayerBalance = packetFee.Fee.AckFee + expectedBalance = expectedBalance.Add(packetFee.Fee.RecvFee...) }, true, }, @@ -741,8 +742,9 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() { relayerAddr = suite.chainA.GetSimApp().AccountKeeper.GetModuleAccount(suite.chainA.GetContext(), transfertypes.ModuleName).GetAddress() expectedBalance = originalBalance. - Add(packetFee.Fee.RecvFee[0]). - Add(packetFee.Fee.AckFee[0]) + Add(packetFee.Fee.RecvFee...). + Add(packetFee.Fee.AckFee...). + Add(packetFee.Fee.TimeoutFee...) }, false, }, diff --git a/modules/apps/29-fee/keeper/escrow.go b/modules/apps/29-fee/keeper/escrow.go index 37c9dd7e624..a2c2e744053 100644 --- a/modules/apps/29-fee/keeper/escrow.go +++ b/modules/apps/29-fee/keeper/escrow.go @@ -1,6 +1,7 @@ package keeper import ( + "bytes" "fmt" sdk "github.com/cosmos/cosmos-sdk/types" @@ -58,17 +59,17 @@ func (k Keeper) DistributePacketFees(ctx sdk.Context, forwardRelayer string, rev // distribute fee to valid forward relayer address otherwise refund the fee if !forwardAddr.Empty() { // distribute fee for forward relaying - k.distributeFee(ctx, forwardAddr, packetFee.Fee.RecvFee) + k.distributeFee(ctx, forwardAddr, refundAddr, packetFee.Fee.RecvFee) } else { // refund onRecv fee as forward relayer is not valid address - k.distributeFee(ctx, refundAddr, packetFee.Fee.RecvFee) + k.distributeFee(ctx, refundAddr, refundAddr, packetFee.Fee.RecvFee) } // distribute fee for reverse relaying - k.distributeFee(ctx, reverseRelayer, packetFee.Fee.AckFee) + k.distributeFee(ctx, reverseRelayer, refundAddr, packetFee.Fee.AckFee) // refund timeout fee for unused timeout - k.distributeFee(ctx, refundAddr, packetFee.Fee.TimeoutFee) + k.distributeFee(ctx, refundAddr, refundAddr, packetFee.Fee.TimeoutFee) } } @@ -82,31 +83,36 @@ func (k Keeper) DistributePacketFeesOnTimeout(ctx sdk.Context, timeoutRelayer sd } // refund receive fee for unused forward relaying - k.distributeFee(ctx, refundAddr, feeInEscrow.Fee.RecvFee) + k.distributeFee(ctx, refundAddr, refundAddr, feeInEscrow.Fee.RecvFee) // refund ack fee for unused reverse relaying - k.distributeFee(ctx, refundAddr, feeInEscrow.Fee.AckFee) + k.distributeFee(ctx, refundAddr, refundAddr, feeInEscrow.Fee.AckFee) // distribute fee for timeout relaying - k.distributeFee(ctx, timeoutRelayer, feeInEscrow.Fee.TimeoutFee) + k.distributeFee(ctx, timeoutRelayer, refundAddr, feeInEscrow.Fee.TimeoutFee) } } // distributeFee will attempt to distribute the escrowed fee to the receiver address. // If the distribution fails for any reason (such as the receiving address being blocked), // the state changes will be discarded. -func (k Keeper) distributeFee(ctx sdk.Context, receiver sdk.AccAddress, fee sdk.Coins) { +func (k Keeper) distributeFee(ctx sdk.Context, receiver, refundAccAddress sdk.AccAddress, fee sdk.Coins) { // cache context before trying to distribute fees cacheCtx, writeFn := ctx.CacheContext() err := k.bankKeeper.SendCoinsFromModuleToAccount(cacheCtx, types.ModuleName, receiver, fee) - if err == nil { - // 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()) + if err != nil && !bytes.Equal(receiver, refundAccAddress) { + err := k.bankKeeper.SendCoinsFromModuleToAccount(cacheCtx, types.ModuleName, refundAccAddress, fee) + if err != nil { + return + } } + + // 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()) } func (k Keeper) RefundFeesOnChannel(ctx sdk.Context, portID, channelID string) error { From 4e388d5f607bd0460cd12aea8418dfe011ec7412 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Tue, 12 Apr 2022 13:53:57 +0200 Subject: [PATCH 2/4] adapting logic and updating testcases --- modules/apps/29-fee/keeper/escrow.go | 11 +- modules/apps/29-fee/keeper/escrow_test.go | 138 ++++++++++++++-------- 2 files changed, 97 insertions(+), 52 deletions(-) diff --git a/modules/apps/29-fee/keeper/escrow.go b/modules/apps/29-fee/keeper/escrow.go index 5acf2e9036b..6625e8d1db2 100644 --- a/modules/apps/29-fee/keeper/escrow.go +++ b/modules/apps/29-fee/keeper/escrow.go @@ -104,10 +104,17 @@ func (k Keeper) distributeFee(ctx sdk.Context, receiver, refundAccAddress sdk.Ac cacheCtx, writeFn := ctx.CacheContext() err := k.bankKeeper.SendCoinsFromModuleToAccount(cacheCtx, types.ModuleName, receiver, fee) - if err != nil && !bytes.Equal(receiver, refundAccAddress) { + if err != nil { + if bytes.Equal(receiver, refundAccAddress) { + // if sending has already failed to the refund address, then return (no-op) + return + } + + // if an error is returned from x/bank and the receiver is not the refundAccAddress + // then attempt to refund the fee to the original sender err := k.bankKeeper.SendCoinsFromModuleToAccount(cacheCtx, types.ModuleName, refundAccAddress, fee) if err != nil { - return + return // if sending to the refund address fails, no-op } } diff --git a/modules/apps/29-fee/keeper/escrow_test.go b/modules/apps/29-fee/keeper/escrow_test.go index 0b9e3045542..f6fe204bf11 100644 --- a/modules/apps/29-fee/keeper/escrow_test.go +++ b/modules/apps/29-fee/keeper/escrow_test.go @@ -122,31 +122,94 @@ func (suite *KeeperTestSuite) TestEscrowPacketFee() { func (suite *KeeperTestSuite) TestDistributeFee() { var ( - reverseRelayer sdk.AccAddress - forwardRelayer string - refundAcc sdk.AccAddress + forwardRelayer string + forwardRelayerBal sdk.Coin + reverseRelayer sdk.AccAddress + reverseRelayerBal sdk.Coin + refundAcc sdk.AccAddress + refundAccBal sdk.Coin ) - validSeq := uint64(1) - testCases := []struct { - name string - malleate func() - expPass bool + name string + malleate func() + expResult func() }{ { - "success", func() {}, true, + "success", + func() {}, + func() { + // check if the reverse relayer is paid + expectedReverseAccBal := reverseRelayerBal.Add(defaultAckFee[0]).Add(defaultAckFee[0]) + balance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), reverseRelayer, sdk.DefaultBondDenom) + suite.Require().Equal(expectedReverseAccBal, balance) + + // check if the forward relayer is paid + forward, err := sdk.AccAddressFromBech32(forwardRelayer) + suite.Require().NoError(err) + + expectedForwardAccBal := forwardRelayerBal.Add(defaultReceiveFee[0]).Add(defaultReceiveFee[0]) + balance = suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), forward, sdk.DefaultBondDenom) + suite.Require().Equal(expectedForwardAccBal, balance) + + // check if the refund acc has been refunded the timeoutFee + expectedRefundAccBal := refundAccBal.Add(defaultTimeoutFee[0].Add(defaultTimeoutFee[0])) + balance = suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom) + suite.Require().Equal(expectedRefundAccBal, balance) + + // 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) + }, }, { - "invalid forward address", func() { + "invalid forward address", + func() { forwardRelayer = "invalid address" - }, false, + }, + 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]) + balance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom) + suite.Require().Equal(expectedRefundAccBal, balance) + }, }, { - "invalid forward address: blocked address", func() { + "invalid forward address: blocked address", + func() { forwardRelayer = suite.chainA.GetSimApp().AccountKeeper.GetModuleAccount(suite.chainA.GetContext(), transfertypes.ModuleName).GetAddress().String() - }, false, + }, + 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]) + balance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom) + suite.Require().Equal(expectedRefundAccBal, balance) + }, }, + { + "invalid receiver address: ack fee returned to sender", + func() { + reverseRelayer = suite.chainA.GetSimApp().AccountKeeper.GetModuleAccount(suite.chainA.GetContext(), transfertypes.ModuleName).GetAddress() + }, + func() { + // check if the refund acc has been refunded the timeoutFee & ackFee + expectedRefundAccBal := refundAccBal.Add(defaultTimeoutFee[0]).Add(defaultAckFee[0]).Add(defaultTimeoutFee[0]).Add(defaultAckFee[0]) + balance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom) + suite.Require().Equal(expectedRefundAccBal, balance) + }, + }, + // { + // "invalid refund address: no-op, timeout fee remains in escrow", + // func() { + // refundAcc = suite.chainA.GetSimApp().AccountKeeper.GetModuleAccount(suite.chainA.GetContext(), transfertypes.ModuleName).GetAddress() + // }, + // func() { + // // check if the module acc contains the timeoutFee + // expectedModuleAccBal := sdk.NewCoin(sdk.DefaultBondDenom, defaultTimeoutFee.Add(defaultTimeoutFee...).AmountOf(sdk.DefaultBondDenom)) + // balance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress(), sdk.DefaultBondDenom) + // suite.Require().Equal(expectedModuleAccBal, balance) + // }, + // }, } for _, tc := range testCases { @@ -156,59 +219,34 @@ func (suite *KeeperTestSuite) TestDistributeFee() { suite.SetupTest() // reset suite.coordinator.Setup(suite.path) // setup channel - // setup - refundAcc = suite.chainA.SenderAccount.GetAddress() - reverseRelayer = sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()) + // setup accounts forwardRelayer = sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()).String() + reverseRelayer = sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()) + refundAcc = suite.chainA.SenderAccount.GetAddress() - packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, validSeq) - fee := types.Fee{ - RecvFee: defaultReceiveFee, - AckFee: defaultAckFee, - TimeoutFee: defaultTimeoutFee, - } + packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, 1) + fee := types.NewFee(defaultReceiveFee, defaultAckFee, defaultTimeoutFee) // escrow the packet fee & store the fee in state packetFee := types.NewPacketFee(fee, refundAcc.String(), []string{}) - 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) tc.malleate() - // refundAcc balance after escrow - refundAccBal := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom) + // fetch the account balances before fee distribution (forward, reverse, refund) + forwardAccAddress, _ := sdk.AccAddressFromBech32(forwardRelayer) + forwardRelayerBal = suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), forwardAccAddress, sdk.DefaultBondDenom) + 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}) - if tc.expPass { - // check if the reverse relayer is paid - hasBalance := suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), reverseRelayer, fee.AckFee[0].Add(fee.AckFee[0])) - suite.Require().True(hasBalance) - - // check if the forward relayer is paid - forward, err := sdk.AccAddressFromBech32(forwardRelayer) - suite.Require().NoError(err) - hasBalance = suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), forward, fee.RecvFee[0].Add(fee.RecvFee[0])) - suite.Require().True(hasBalance) - - // check if the refund acc has been refunded the timeoutFee - expectedRefundAccBal := refundAccBal.Add(fee.TimeoutFee[0].Add(fee.TimeoutFee[0])) - hasBalance = suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), refundAcc, expectedRefundAccBal) - suite.Require().True(hasBalance) - - // check the module acc wallet is now empty - hasBalance = suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress(), sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: sdk.NewInt(0)}) - suite.Require().True(hasBalance) - } else { - // check if the refund acc has been refunded the timeoutFee & onRecvFee - expectedRefundAccBal := refundAccBal.Add(fee.TimeoutFee[0]).Add(fee.RecvFee[0]).Add(fee.TimeoutFee[0]).Add(fee.RecvFee[0]) - hasBalance := suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), refundAcc, expectedRefundAccBal) - suite.Require().True(hasBalance) - } + tc.expResult() }) } } From 34c7506d79bad2c31ea02eff55e408a7c5ce49bb Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Tue, 12 Apr 2022 14:06:26 +0200 Subject: [PATCH 3/4] updating inline comments --- modules/apps/29-fee/keeper/escrow.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/modules/apps/29-fee/keeper/escrow.go b/modules/apps/29-fee/keeper/escrow.go index 6625e8d1db2..f0c18e379ba 100644 --- a/modules/apps/29-fee/keeper/escrow.go +++ b/modules/apps/29-fee/keeper/escrow.go @@ -106,8 +106,7 @@ func (k Keeper) distributeFee(ctx sdk.Context, receiver, refundAccAddress sdk.Ac err := k.bankKeeper.SendCoinsFromModuleToAccount(cacheCtx, types.ModuleName, receiver, fee) if err != nil { if bytes.Equal(receiver, refundAccAddress) { - // if sending has already failed to the refund address, then return (no-op) - return + return // if sending to the refund address already failed, then return (no-op) } // if an error is returned from x/bank and the receiver is not the refundAccAddress From 063287308b57cceee275f21c9b71fcc13de8269e Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Tue, 12 Apr 2022 14:14:52 +0200 Subject: [PATCH 4/4] updating refund address testcase --- modules/apps/29-fee/keeper/escrow_test.go | 27 ++++++++++++----------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/modules/apps/29-fee/keeper/escrow_test.go b/modules/apps/29-fee/keeper/escrow_test.go index f6fe204bf11..052a8cdc9c1 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() { reverseRelayerBal sdk.Coin refundAcc sdk.AccAddress refundAccBal sdk.Coin + packetFee types.PacketFee ) testCases := []struct { @@ -198,18 +199,18 @@ func (suite *KeeperTestSuite) TestDistributeFee() { suite.Require().Equal(expectedRefundAccBal, balance) }, }, - // { - // "invalid refund address: no-op, timeout fee remains in escrow", - // func() { - // refundAcc = suite.chainA.GetSimApp().AccountKeeper.GetModuleAccount(suite.chainA.GetContext(), transfertypes.ModuleName).GetAddress() - // }, - // func() { - // // check if the module acc contains the timeoutFee - // expectedModuleAccBal := sdk.NewCoin(sdk.DefaultBondDenom, defaultTimeoutFee.Add(defaultTimeoutFee...).AmountOf(sdk.DefaultBondDenom)) - // balance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress(), sdk.DefaultBondDenom) - // suite.Require().Equal(expectedModuleAccBal, balance) - // }, - // }, + { + "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() + }, + func() { + // check if the module acc contains the timeoutFee + expectedModuleAccBal := sdk.NewCoin(sdk.DefaultBondDenom, defaultTimeoutFee.Add(defaultTimeoutFee...).AmountOf(sdk.DefaultBondDenom)) + balance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress(), sdk.DefaultBondDenom) + suite.Require().Equal(expectedModuleAccBal, balance) + }, + }, } for _, tc := range testCases { @@ -228,7 +229,7 @@ func (suite *KeeperTestSuite) TestDistributeFee() { fee := types.NewFee(defaultReceiveFee, defaultAckFee, defaultTimeoutFee) // escrow the packet fee & store the fee in state - packetFee := types.NewPacketFee(fee, refundAcc.String(), []string{}) + packetFee = types.NewPacketFee(fee, refundAcc.String(), []string{}) err := suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee) suite.Require().NoError(err)