From 08b7d05944a62fb3c2be91d1e252603855d9ce43 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Wed, 13 Apr 2022 11:19:34 +0200 Subject: [PATCH 1/6] making EsrowPacketFee private, adapting test cases to conform --- modules/apps/29-fee/ibc_module_test.go | 29 +- modules/apps/29-fee/keeper/escrow.go | 2 +- modules/apps/29-fee/keeper/escrow_test.go | 253 +++++++++--------- modules/apps/29-fee/keeper/grpc_query_test.go | 30 +-- modules/apps/29-fee/keeper/keeper_test.go | 5 +- modules/apps/29-fee/keeper/msg_server.go | 4 +- 6 files changed, 160 insertions(+), 163 deletions(-) diff --git a/modules/apps/29-fee/ibc_module_test.go b/modules/apps/29-fee/ibc_module_test.go index 4ca50d2c612..8292621cedf 100644 --- a/modules/apps/29-fee/ibc_module_test.go +++ b/modules/apps/29-fee/ibc_module_test.go @@ -308,8 +308,9 @@ func (suite *FeeTestSuite) TestOnChanCloseInit() { ) refundAcc := suite.chainA.SenderAccount.GetAddress() packetFee := types.NewPacketFee(types.Fee{validCoins, validCoins2, validCoins3}, refundAcc.String(), []string{}) - err := suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee) - suite.Require().NoError(err) + + suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, types.NewPacketFees([]types.PacketFee{packetFee})) + suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, packetFee.Fee.Total()) }, false, }, @@ -323,8 +324,9 @@ func (suite *FeeTestSuite) TestOnChanCloseInit() { ) refundAcc := suite.chainA.SenderAccount.GetAddress() packetFee := types.NewPacketFee(types.Fee{validCoins, validCoins2, validCoins3}, refundAcc.String(), []string{}) - err := suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee) - suite.Require().NoError(err) + + suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, types.NewPacketFees([]types.PacketFee{packetFee})) + suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, packetFee.Fee.Total()) suite.chainA.GetSimApp().BankKeeper.SendCoinsFromModuleToAccount(suite.chainA.GetContext(), types.ModuleName, refundAcc, validCoins3) @@ -386,8 +388,9 @@ func (suite *FeeTestSuite) TestOnChanCloseConfirm() { } refundAcc := suite.chainA.SenderAccount.GetAddress() packetFee := types.NewPacketFee(types.Fee{validCoins, validCoins2, validCoins3}, refundAcc.String(), []string{}) - err := suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee) - suite.Require().NoError(err) + + suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, types.NewPacketFees([]types.PacketFee{packetFee})) + suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, packetFee.Fee.Total()) }, false, }, @@ -401,8 +404,9 @@ func (suite *FeeTestSuite) TestOnChanCloseConfirm() { } refundAcc := suite.chainA.SenderAccount.GetAddress() packetFee := types.NewPacketFee(types.Fee{validCoins, validCoins2, validCoins3}, refundAcc.String(), []string{}) - err := suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee) - suite.Require().NoError(err) + + suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, types.NewPacketFees([]types.PacketFee{packetFee})) + suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, packetFee.Fee.Total()) suite.chainA.GetSimApp().BankKeeper.SendCoinsFromModuleToAccount(suite.chainA.GetContext(), types.ModuleName, refundAcc, validCoins3) @@ -664,8 +668,9 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() { suite.chainA.SenderAccount.GetAddress().String(), []string{}, ) - err = suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee) - suite.Require().NoError(err) + + suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, types.NewPacketFees([]types.PacketFee{packetFee})) + suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress(), types.ModuleName, packetFee.Fee.Total()) relayerAddr := suite.chainB.SenderAccount.GetAddress() @@ -796,8 +801,8 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() { []string{}, ) - err = suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee) - suite.Require().NoError(err) + suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, types.NewPacketFees([]types.PacketFee{packetFee})) + suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress(), types.ModuleName, packetFee.Fee.Total()) // log original sender balance // NOTE: balance is logged after escrowing tokens diff --git a/modules/apps/29-fee/keeper/escrow.go b/modules/apps/29-fee/keeper/escrow.go index f0c18e379ba..55c15b1566d 100644 --- a/modules/apps/29-fee/keeper/escrow.go +++ b/modules/apps/29-fee/keeper/escrow.go @@ -12,7 +12,7 @@ import ( ) // EscrowPacketFee sends the packet fee to the 29-fee module account to hold in escrow -func (k Keeper) EscrowPacketFee(ctx sdk.Context, packetID channeltypes.PacketId, packetFee types.PacketFee) error { +func (k Keeper) escrowPacketFee(ctx sdk.Context, packetID channeltypes.PacketId, packetFee types.PacketFee) error { if !k.IsFeeEnabled(ctx, packetID.PortId, packetID.ChannelId) { // users may not escrow fees on this channel. Must send packets without a fee message return sdkerrors.Wrap(types.ErrFeeNotEnabled, "cannot escrow fee for packet") diff --git a/modules/apps/29-fee/keeper/escrow_test.go b/modules/apps/29-fee/keeper/escrow_test.go index 052a8cdc9c1..8b90b2ce224 100644 --- a/modules/apps/29-fee/keeper/escrow_test.go +++ b/modules/apps/29-fee/keeper/escrow_test.go @@ -10,115 +10,117 @@ import ( ibctesting "github.com/cosmos/ibc-go/v3/testing" ) -func (suite *KeeperTestSuite) TestEscrowPacketFee() { - var ( - err error - refundAcc sdk.AccAddress - ackFee sdk.Coins - receiveFee sdk.Coins - timeoutFee sdk.Coins - packetID channeltypes.PacketId - ) - - testCases := []struct { - name string - malleate func() - expPass bool - }{ - { - "success", func() {}, true, - }, - { - "success with existing packet fee", func() { - fee := types.Fee{ - RecvFee: receiveFee, - AckFee: ackFee, - TimeoutFee: timeoutFee, - } - - packetFee := types.NewPacketFee(fee, refundAcc.String(), []string{}) - feesInEscrow := types.NewPacketFees([]types.PacketFee{packetFee}) - - suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, feesInEscrow) - }, true, - }, - { - "fee not enabled on this channel", func() { - packetID.ChannelId = "disabled_channel" - }, false, - }, - { - "refundAcc does not exist", func() { - // this acc does not exist on chainA - refundAcc = suite.chainB.SenderAccount.GetAddress() - }, false, - }, - { - "ackFee balance not found", func() { - ackFee = invalidCoins - }, false, - }, - { - "receive balance not found", func() { - receiveFee = invalidCoins - }, false, - }, - { - "timeout balance not found", func() { - timeoutFee = invalidCoins - }, false, - }, - } - - for _, tc := range testCases { - tc := tc - - suite.Run(tc.name, func() { - suite.SetupTest() // reset - suite.coordinator.Setup(suite.path) // setup channel - - // setup - refundAcc = suite.chainA.SenderAccount.GetAddress() - receiveFee = defaultReceiveFee - ackFee = defaultAckFee - timeoutFee = defaultTimeoutFee - packetID = channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, uint64(1)) - - tc.malleate() - fee := types.Fee{ - RecvFee: receiveFee, - AckFee: ackFee, - TimeoutFee: timeoutFee, - } - packetFee := types.NewPacketFee(fee, refundAcc.String(), []string{}) - - // refundAcc balance before escrow - originalBal := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom) - - // escrow the packet fee - err = suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee) - - if tc.expPass { - feesInEscrow, found := suite.chainA.GetSimApp().IBCFeeKeeper.GetFeesInEscrow(suite.chainA.GetContext(), packetID) - suite.Require().True(found) - // check if the escrowed fee is set in state - suite.Require().True(feesInEscrow.PacketFees[0].Fee.AckFee.IsEqual(fee.AckFee)) - suite.Require().True(feesInEscrow.PacketFees[0].Fee.RecvFee.IsEqual(fee.RecvFee)) - suite.Require().True(feesInEscrow.PacketFees[0].Fee.TimeoutFee.IsEqual(fee.TimeoutFee)) - // check if the fee is escrowed correctly - hasBalance := suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress(), sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: sdk.NewInt(600)}) - suite.Require().True(hasBalance) - expectedBal := originalBal.Amount.Sub(sdk.NewInt(600)) - // check if the refund acc has sent the fee - hasBalance = suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), refundAcc, sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: expectedBal}) - suite.Require().True(hasBalance) - suite.Require().NoError(err) - } else { - suite.Require().Error(err) - } - }) - } -} +// TODO: Move these tests to msg_server_test.go +// +// func (suite *KeeperTestSuite) TestEscrowPacketFee() { +// var ( +// err error +// refundAcc sdk.AccAddress +// ackFee sdk.Coins +// receiveFee sdk.Coins +// timeoutFee sdk.Coins +// packetID channeltypes.PacketId +// ) + +// testCases := []struct { +// name string +// malleate func() +// expPass bool +// }{ +// { +// "success", func() {}, true, +// }, +// { +// "success with existing packet fee", func() { +// fee := types.Fee{ +// RecvFee: receiveFee, +// AckFee: ackFee, +// TimeoutFee: timeoutFee, +// } + +// packetFee := types.NewPacketFee(fee, refundAcc.String(), []string{}) +// feesInEscrow := types.NewPacketFees([]types.PacketFee{packetFee}) + +// suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, feesInEscrow) +// }, true, +// }, +// { +// "fee not enabled on this channel", func() { +// packetID.ChannelId = "disabled_channel" +// }, false, +// }, +// { +// "refundAcc does not exist", func() { +// // this acc does not exist on chainA +// refundAcc = suite.chainB.SenderAccount.GetAddress() +// }, false, +// }, +// { +// "ackFee balance not found", func() { +// ackFee = invalidCoins +// }, false, +// }, +// { +// "receive balance not found", func() { +// receiveFee = invalidCoins +// }, false, +// }, +// { +// "timeout balance not found", func() { +// timeoutFee = invalidCoins +// }, false, +// }, +// } + +// for _, tc := range testCases { +// tc := tc + +// suite.Run(tc.name, func() { +// suite.SetupTest() // reset +// suite.coordinator.Setup(suite.path) // setup channel + +// // setup +// refundAcc = suite.chainA.SenderAccount.GetAddress() +// receiveFee = defaultReceiveFee +// ackFee = defaultAckFee +// timeoutFee = defaultTimeoutFee +// packetID = channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, uint64(1)) + +// tc.malleate() +// fee := types.Fee{ +// RecvFee: receiveFee, +// AckFee: ackFee, +// TimeoutFee: timeoutFee, +// } +// packetFee := types.NewPacketFee(fee, refundAcc.String(), []string{}) + +// // refundAcc balance before escrow +// originalBal := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom) + +// // escrow the packet fee +// err = suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee) + +// if tc.expPass { +// feesInEscrow, found := suite.chainA.GetSimApp().IBCFeeKeeper.GetFeesInEscrow(suite.chainA.GetContext(), packetID) +// suite.Require().True(found) +// // check if the escrowed fee is set in state +// suite.Require().True(feesInEscrow.PacketFees[0].Fee.AckFee.IsEqual(fee.AckFee)) +// suite.Require().True(feesInEscrow.PacketFees[0].Fee.RecvFee.IsEqual(fee.RecvFee)) +// suite.Require().True(feesInEscrow.PacketFees[0].Fee.TimeoutFee.IsEqual(fee.TimeoutFee)) +// // check if the fee is escrowed correctly +// hasBalance := suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress(), sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: sdk.NewInt(600)}) +// suite.Require().True(hasBalance) +// expectedBal := originalBal.Amount.Sub(sdk.NewInt(600)) +// // check if the refund acc has sent the fee +// hasBalance = suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), refundAcc, sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: expectedBal}) +// suite.Require().True(hasBalance) +// suite.Require().NoError(err) +// } else { +// suite.Require().Error(err) +// } +// }) +// } +// } func (suite *KeeperTestSuite) TestDistributeFee() { var ( @@ -230,12 +232,10 @@ func (suite *KeeperTestSuite) TestDistributeFee() { // 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) + packetFees := []types.PacketFee{packetFee, packetFee} + suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, types.NewPacketFees(packetFees)) + suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, packetFee.Fee.Total().Add(packetFee.Fee.Total()...)) tc.malleate() @@ -274,11 +274,9 @@ func (suite *KeeperTestSuite) TestDistributeTimeoutFee() { // 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) + packetFees := []types.PacketFee{packetFee, packetFee} + suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, types.NewPacketFees(packetFees)) + suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, packetFee.Fee.Total().Add(packetFee.Fee.Total()...)) // refundAcc balance after escrow refundAccBal := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom) @@ -319,8 +317,9 @@ func (suite *KeeperTestSuite) TestRefundFeesOnChannel() { packetFee := types.NewPacketFee(fee, refundAcc.String(), []string{}) suite.chainA.GetSimApp().IBCFeeKeeper.SetFeeEnabled(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID) - err := suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee) - suite.Require().NoError(err) + + suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, types.NewPacketFees([]types.PacketFee{packetFee})) + suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, packetFee.Fee.Total()) } // send a packet over a different channel to ensure this fee is not refunded @@ -333,11 +332,12 @@ func (suite *KeeperTestSuite) TestRefundFeesOnChannel() { packetFee := types.NewPacketFee(fee, refundAcc.String(), []string{}) suite.chainA.GetSimApp().IBCFeeKeeper.SetFeeEnabled(suite.chainA.GetContext(), ibctesting.MockFeePort, "channel-1") - err := suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee) - suite.Require().NoError(err) + + suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, types.NewPacketFees([]types.PacketFee{packetFee})) + suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, packetFee.Fee.Total()) // check that refunding all fees on channel-0 refunds all fees except for fee on channel-1 - err = suite.chainA.GetSimApp().IBCFeeKeeper.RefundFeesOnChannel(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID) + err := suite.chainA.GetSimApp().IBCFeeKeeper.RefundFeesOnChannel(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID) suite.Require().NoError(err, "refund fees returned unexpected error") // add fee sent to channel-1 to after balance to recover original balance @@ -348,8 +348,9 @@ func (suite *KeeperTestSuite) TestRefundFeesOnChannel() { packetID = channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, uint64(6)) packetFee = types.NewPacketFee(fee, refundAcc.String(), []string{}) - err = suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee) - suite.Require().NoError(err) + + suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, types.NewPacketFees([]types.PacketFee{packetFee})) + suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, packetFee.Fee.Total()) suite.chainA.GetSimApp().BankKeeper.SendCoinsFromModuleToAccount(suite.chainA.GetContext(), types.ModuleName, refundAcc, fee.TimeoutFee) diff --git a/modules/apps/29-fee/keeper/grpc_query_test.go b/modules/apps/29-fee/keeper/grpc_query_test.go index 446616ab176..4483b0d877c 100644 --- a/modules/apps/29-fee/keeper/grpc_query_test.go +++ b/modules/apps/29-fee/keeper/grpc_query_test.go @@ -34,7 +34,7 @@ func (suite *KeeperTestSuite) TestQueryIncentivizedPackets() { for i := 0; i < 3; i++ { // escrow packet fees for three different packets packetID := channeltypes.NewPacketId(ibctesting.MockFeePort, ibctesting.FirstChannelID, uint64(i+1)) - suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee) + suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, types.NewPacketFees([]types.PacketFee{packetFee})) expectedPackets = append(expectedPackets, types.NewIdentifiedPacketFees(packetID, []types.PacketFee{packetFee})) } @@ -116,11 +116,8 @@ func (suite *KeeperTestSuite) TestQueryIncentivizedPacket() { fee := types.NewFee(defaultReceiveFee, defaultAckFee, defaultTimeoutFee) packetFee := types.NewPacketFee(fee, suite.chainA.SenderAccount.GetAddress().String(), []string(nil)) - for i := 0; i < 3; i++ { - // escrow three packet fees for the same packet - err := suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee) - suite.Require().NoError(err) - } + packetFees := []types.PacketFee{packetFee, packetFee, packetFee} + suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, types.NewPacketFees(packetFees)) req = &types.QueryIncentivizedPacketRequest{ PacketId: packetID, @@ -272,11 +269,8 @@ func (suite *KeeperTestSuite) TestQueryTotalRecvFees() { fee := types.NewFee(defaultReceiveFee, defaultAckFee, defaultTimeoutFee) packetFee := types.NewPacketFee(fee, suite.chainA.SenderAccount.GetAddress().String(), []string(nil)) - for i := 0; i < 3; i++ { - // escrow three packet fees for the same packet - err := suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee) - suite.Require().NoError(err) - } + packetFees := []types.PacketFee{packetFee, packetFee, packetFee} + suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, types.NewPacketFees(packetFees)) req = &types.QueryTotalRecvFeesRequest{ PacketId: packetID, @@ -336,11 +330,8 @@ func (suite *KeeperTestSuite) TestQueryTotalAckFees() { fee := types.NewFee(defaultReceiveFee, defaultAckFee, defaultTimeoutFee) packetFee := types.NewPacketFee(fee, suite.chainA.SenderAccount.GetAddress().String(), []string(nil)) - for i := 0; i < 3; i++ { - // escrow three packet fees for the same packet - err := suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee) - suite.Require().NoError(err) - } + packetFees := []types.PacketFee{packetFee, packetFee, packetFee} + suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, types.NewPacketFees(packetFees)) req = &types.QueryTotalAckFeesRequest{ PacketId: packetID, @@ -400,11 +391,8 @@ func (suite *KeeperTestSuite) TestQueryTotalTimeoutFees() { fee := types.NewFee(defaultReceiveFee, defaultAckFee, defaultTimeoutFee) packetFee := types.NewPacketFee(fee, suite.chainA.SenderAccount.GetAddress().String(), []string(nil)) - for i := 0; i < 3; i++ { - // escrow three packet fees for the same packet - err := suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee) - suite.Require().NoError(err) - } + packetFees := []types.PacketFee{packetFee, packetFee, packetFee} + suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, types.NewPacketFees(packetFees)) req = &types.QueryTotalTimeoutFeesRequest{ PacketId: packetID, diff --git a/modules/apps/29-fee/keeper/keeper_test.go b/modules/apps/29-fee/keeper/keeper_test.go index 5c44e45c332..95699ff1b2a 100644 --- a/modules/apps/29-fee/keeper/keeper_test.go +++ b/modules/apps/29-fee/keeper/keeper_test.go @@ -102,11 +102,14 @@ func (suite *KeeperTestSuite) TestFeesInEscrow() { packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, 1) fee := types.NewFee(defaultReceiveFee, defaultAckFee, defaultTimeoutFee) + var packetFees []types.PacketFee for i := 1; i < 6; i++ { packetFee := types.NewPacketFee(fee, suite.chainA.SenderAccount.GetAddress().String(), nil) - suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee) + packetFees = append(packetFees, packetFee) } + suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, types.NewPacketFees(packetFees)) + // retrieve the fees in escrow and assert the length of PacketFees feesInEscrow, found := suite.chainA.GetSimApp().IBCFeeKeeper.GetFeesInEscrow(suite.chainA.GetContext(), packetID) suite.Require().True(found) diff --git a/modules/apps/29-fee/keeper/msg_server.go b/modules/apps/29-fee/keeper/msg_server.go index 40a7fb4df9c..c267d10d0ff 100644 --- a/modules/apps/29-fee/keeper/msg_server.go +++ b/modules/apps/29-fee/keeper/msg_server.go @@ -48,7 +48,7 @@ func (k Keeper) PayPacketFee(goCtx context.Context, msg *types.MsgPayPacketFee) ) packetFee := types.NewPacketFee(msg.Fee, msg.Signer, msg.Relayers) - if err := k.EscrowPacketFee(ctx, packetID, packetFee); err != nil { + if err := k.escrowPacketFee(ctx, packetID, packetFee); err != nil { return nil, err } @@ -65,7 +65,7 @@ func (k Keeper) PayPacketFeeAsync(goCtx context.Context, msg *types.MsgPayPacket return nil, types.ErrFeeModuleLocked } - if err := k.EscrowPacketFee(ctx, msg.PacketId, msg.PacketFee); err != nil { + if err := k.escrowPacketFee(ctx, msg.PacketId, msg.PacketFee); err != nil { return nil, err } From b427f23264a961cf70ac9a0f3fabb2d497d01b48 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Wed, 13 Apr 2022 14:48:44 +0200 Subject: [PATCH 2/6] refactor msg server tests to accomoodate escrowPacketFee --- modules/apps/29-fee/keeper/escrow.go | 7 +- modules/apps/29-fee/keeper/escrow_test.go | 112 --------- modules/apps/29-fee/keeper/msg_server.go | 18 +- modules/apps/29-fee/keeper/msg_server_test.go | 212 ++++++++++++++---- 4 files changed, 177 insertions(+), 172 deletions(-) diff --git a/modules/apps/29-fee/keeper/escrow.go b/modules/apps/29-fee/keeper/escrow.go index 55c15b1566d..f898d579d11 100644 --- a/modules/apps/29-fee/keeper/escrow.go +++ b/modules/apps/29-fee/keeper/escrow.go @@ -11,13 +11,8 @@ import ( channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types" ) -// EscrowPacketFee sends the packet fee to the 29-fee module account to hold in escrow +// escrowPacketFee sends the packet fee to the 29-fee module account to hold in escrow func (k Keeper) escrowPacketFee(ctx sdk.Context, packetID channeltypes.PacketId, packetFee types.PacketFee) error { - if !k.IsFeeEnabled(ctx, packetID.PortId, packetID.ChannelId) { - // users may not escrow fees on this channel. Must send packets without a fee message - return sdkerrors.Wrap(types.ErrFeeNotEnabled, "cannot escrow fee for packet") - } - // check if the refund address is valid refundAddr, err := sdk.AccAddressFromBech32(packetFee.RefundAddress) if err != nil { diff --git a/modules/apps/29-fee/keeper/escrow_test.go b/modules/apps/29-fee/keeper/escrow_test.go index 8b90b2ce224..d309e1ed41b 100644 --- a/modules/apps/29-fee/keeper/escrow_test.go +++ b/modules/apps/29-fee/keeper/escrow_test.go @@ -10,118 +10,6 @@ import ( ibctesting "github.com/cosmos/ibc-go/v3/testing" ) -// TODO: Move these tests to msg_server_test.go -// -// func (suite *KeeperTestSuite) TestEscrowPacketFee() { -// var ( -// err error -// refundAcc sdk.AccAddress -// ackFee sdk.Coins -// receiveFee sdk.Coins -// timeoutFee sdk.Coins -// packetID channeltypes.PacketId -// ) - -// testCases := []struct { -// name string -// malleate func() -// expPass bool -// }{ -// { -// "success", func() {}, true, -// }, -// { -// "success with existing packet fee", func() { -// fee := types.Fee{ -// RecvFee: receiveFee, -// AckFee: ackFee, -// TimeoutFee: timeoutFee, -// } - -// packetFee := types.NewPacketFee(fee, refundAcc.String(), []string{}) -// feesInEscrow := types.NewPacketFees([]types.PacketFee{packetFee}) - -// suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, feesInEscrow) -// }, true, -// }, -// { -// "fee not enabled on this channel", func() { -// packetID.ChannelId = "disabled_channel" -// }, false, -// }, -// { -// "refundAcc does not exist", func() { -// // this acc does not exist on chainA -// refundAcc = suite.chainB.SenderAccount.GetAddress() -// }, false, -// }, -// { -// "ackFee balance not found", func() { -// ackFee = invalidCoins -// }, false, -// }, -// { -// "receive balance not found", func() { -// receiveFee = invalidCoins -// }, false, -// }, -// { -// "timeout balance not found", func() { -// timeoutFee = invalidCoins -// }, false, -// }, -// } - -// for _, tc := range testCases { -// tc := tc - -// suite.Run(tc.name, func() { -// suite.SetupTest() // reset -// suite.coordinator.Setup(suite.path) // setup channel - -// // setup -// refundAcc = suite.chainA.SenderAccount.GetAddress() -// receiveFee = defaultReceiveFee -// ackFee = defaultAckFee -// timeoutFee = defaultTimeoutFee -// packetID = channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, uint64(1)) - -// tc.malleate() -// fee := types.Fee{ -// RecvFee: receiveFee, -// AckFee: ackFee, -// TimeoutFee: timeoutFee, -// } -// packetFee := types.NewPacketFee(fee, refundAcc.String(), []string{}) - -// // refundAcc balance before escrow -// originalBal := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom) - -// // escrow the packet fee -// err = suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), packetID, packetFee) - -// if tc.expPass { -// feesInEscrow, found := suite.chainA.GetSimApp().IBCFeeKeeper.GetFeesInEscrow(suite.chainA.GetContext(), packetID) -// suite.Require().True(found) -// // check if the escrowed fee is set in state -// suite.Require().True(feesInEscrow.PacketFees[0].Fee.AckFee.IsEqual(fee.AckFee)) -// suite.Require().True(feesInEscrow.PacketFees[0].Fee.RecvFee.IsEqual(fee.RecvFee)) -// suite.Require().True(feesInEscrow.PacketFees[0].Fee.TimeoutFee.IsEqual(fee.TimeoutFee)) -// // check if the fee is escrowed correctly -// hasBalance := suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress(), sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: sdk.NewInt(600)}) -// suite.Require().True(hasBalance) -// expectedBal := originalBal.Amount.Sub(sdk.NewInt(600)) -// // check if the refund acc has sent the fee -// hasBalance = suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), refundAcc, sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: expectedBal}) -// suite.Require().True(hasBalance) -// suite.Require().NoError(err) -// } else { -// suite.Require().Error(err) -// } -// }) -// } -// } - func (suite *KeeperTestSuite) TestDistributeFee() { var ( forwardRelayer string diff --git a/modules/apps/29-fee/keeper/msg_server.go b/modules/apps/29-fee/keeper/msg_server.go index c267d10d0ff..69f6520c759 100644 --- a/modules/apps/29-fee/keeper/msg_server.go +++ b/modules/apps/29-fee/keeper/msg_server.go @@ -31,6 +31,11 @@ func (k Keeper) RegisterCounterpartyAddress(goCtx context.Context, msg *types.Ms func (k Keeper) PayPacketFee(goCtx context.Context, msg *types.MsgPayPacketFee) (*types.MsgPayPacketFeeResponse, error) { ctx := sdk.UnwrapSDKContext(goCtx) + if !k.IsFeeEnabled(ctx, msg.SourcePortId, msg.SourceChannelId) { + // users may not escrow fees on this channel. Must send packets without a fee message + return nil, types.ErrFeeNotEnabled + } + if k.IsLocked(ctx) { return nil, types.ErrFeeModuleLocked } @@ -41,13 +46,9 @@ func (k Keeper) PayPacketFee(goCtx context.Context, msg *types.MsgPayPacketFee) return nil, channeltypes.ErrSequenceSendNotFound } - packetID := channeltypes.NewPacketId( - msg.SourcePortId, - msg.SourceChannelId, - sequence, - ) - + packetID := channeltypes.NewPacketId(msg.SourcePortId, msg.SourceChannelId, sequence) packetFee := types.NewPacketFee(msg.Fee, msg.Signer, msg.Relayers) + if err := k.escrowPacketFee(ctx, packetID, packetFee); err != nil { return nil, err } @@ -61,6 +62,11 @@ func (k Keeper) PayPacketFee(goCtx context.Context, msg *types.MsgPayPacketFee) func (k Keeper) PayPacketFeeAsync(goCtx context.Context, msg *types.MsgPayPacketFeeAsync) (*types.MsgPayPacketFeeAsyncResponse, error) { ctx := sdk.UnwrapSDKContext(goCtx) + if !k.IsFeeEnabled(ctx, msg.PacketId.PortId, msg.PacketId.ChannelId) { + // users may not escrow fees on this channel. Must send packets without a fee message + return nil, types.ErrFeeNotEnabled + } + if k.IsLocked(ctx) { return nil, types.ErrFeeModuleLocked } diff --git a/modules/apps/29-fee/keeper/msg_server_test.go b/modules/apps/29-fee/keeper/msg_server_test.go index 74c5342d674..1d22d7efffb 100644 --- a/modules/apps/29-fee/keeper/msg_server_test.go +++ b/modules/apps/29-fee/keeper/msg_server_test.go @@ -54,100 +54,216 @@ func (suite *KeeperTestSuite) TestRegisterCounterpartyAddress() { } func (suite *KeeperTestSuite) TestPayPacketFee() { + var ( + msg *types.MsgPayPacketFee + ) + testCases := []struct { name string - expPass bool malleate func() + expPass bool }{ { "success", - true, func() {}, + true, + }, + { + "success with existing packet fees in escrow", + func() { + fee := types.NewFee(defaultReceiveFee, defaultAckFee, defaultTimeoutFee) + + packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, 1) + packetFee := types.NewPacketFee(fee, suite.chainA.SenderAccount.GetAddress().String(), []string{}) + feesInEscrow := types.NewPacketFees([]types.PacketFee{packetFee}) + + suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, feesInEscrow) + }, + true, }, { "fee module is locked", - false, func() { lockFeeModule(suite.chainA) }, + false, + }, + { + "fee module disabled on channel", + func() { + msg.SourcePortId = "invalid-port" + msg.SourceChannelId = "invalid-channel" + }, + false, + }, + { + "invalid refund address", + func() { + msg.Signer = "invalid-address" + }, + false, + }, + { + "refund account does not exist", + func() { + msg.Signer = suite.chainB.SenderAccount.GetAddress().String() + }, + false, + }, + { + "acknowledgement fee balance not found", + func() { + msg.Fee.AckFee = invalidCoins + }, + false, + }, + { + "receive fee balance not found", + func() { + msg.Fee.RecvFee = invalidCoins + }, + false, + }, + { + "timeout fee balance not found", + func() { + msg.Fee.TimeoutFee = invalidCoins + }, + false, }, } for _, tc := range testCases { - suite.SetupTest() - suite.coordinator.Setup(suite.path) // setup channel - - refundAcc := suite.chainA.SenderAccount.GetAddress() - channelID := suite.path.EndpointA.ChannelID - fee := types.Fee{ - RecvFee: defaultReceiveFee, - AckFee: defaultAckFee, - TimeoutFee: defaultTimeoutFee, - } - msg := types.NewMsgPayPacketFee(fee, suite.path.EndpointA.ChannelConfig.PortID, channelID, refundAcc.String(), []string{}) + tc := tc - tc.malleate() + suite.Run(tc.name, func() { + suite.SetupTest() + suite.coordinator.Setup(suite.path) // setup channel - _, err := suite.chainA.GetSimApp().IBCFeeKeeper.PayPacketFee(sdk.WrapSDKContext(suite.chainA.GetContext()), msg) + fee := types.NewFee(defaultReceiveFee, defaultAckFee, defaultTimeoutFee) + msg = types.NewMsgPayPacketFee( + fee, + suite.path.EndpointA.ChannelConfig.PortID, + suite.path.EndpointA.ChannelID, + suite.chainA.SenderAccount.GetAddress().String(), + []string{}, + ) - if tc.expPass { - suite.Require().NoError(err) // message committed - } else { - suite.Require().Error(err) - } + tc.malleate() + + _, err := suite.chainA.GetSimApp().IBCFeeKeeper.PayPacketFee(sdk.WrapSDKContext(suite.chainA.GetContext()), msg) + + if tc.expPass { + suite.Require().NoError(err) // message committed + } else { + suite.Require().Error(err) + } + }) } } func (suite *KeeperTestSuite) TestPayPacketFeeAsync() { + var ( + msg *types.MsgPayPacketFeeAsync + ) + testCases := []struct { name string - expPass bool malleate func() + expPass bool }{ { "success", - true, func() {}, + true, + }, + { + "success with existing packet fees in escrow", + func() { + fee := types.NewFee(defaultReceiveFee, defaultAckFee, defaultTimeoutFee) + + packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, 1) + packetFee := types.NewPacketFee(fee, suite.chainA.SenderAccount.GetAddress().String(), []string{}) + feesInEscrow := types.NewPacketFees([]types.PacketFee{packetFee}) + + suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, feesInEscrow) + }, + true, }, { "fee module is locked", - false, func() { lockFeeModule(suite.chainA) }, + false, + }, + { + "fee module disabled on channel", + func() { + msg.PacketId.PortId = "invalid-port" + msg.PacketId.ChannelId = "invalid-channel" + }, + false, + }, + { + "invalid refund address", + func() { + msg.PacketFee.RefundAddress = "invalid-address" + }, + false, + }, + { + "refund account does not exist", + func() { + msg.PacketFee.RefundAddress = suite.chainB.SenderAccount.GetAddress().String() + }, + false, + }, + { + "acknowledgement fee balance not found", + func() { + msg.PacketFee.Fee.AckFee = invalidCoins + }, + false, + }, + { + "receive fee balance not found", + func() { + msg.PacketFee.Fee.RecvFee = invalidCoins + }, + false, + }, + { + "timeout fee balance not found", + func() { + msg.PacketFee.Fee.TimeoutFee = invalidCoins + }, + false, }, } for _, tc := range testCases { - suite.SetupTest() - suite.coordinator.Setup(suite.path) // setup channel + tc := tc - ctxA := suite.chainA.GetContext() + suite.Run(tc.name, func() { + suite.SetupTest() + suite.coordinator.Setup(suite.path) // setup channel - refundAcc := suite.chainA.SenderAccount.GetAddress() + packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, 1) + fee := types.NewFee(defaultReceiveFee, defaultAckFee, defaultTimeoutFee) + packetFee := types.NewPacketFee(fee, suite.chainA.SenderAccount.GetAddress().String(), nil) - // build packetID - channelID := suite.path.EndpointA.ChannelID - fee := types.Fee{ - RecvFee: defaultReceiveFee, - AckFee: defaultAckFee, - TimeoutFee: defaultTimeoutFee, - } - seq, _ := suite.chainA.App.GetIBCKeeper().ChannelKeeper.GetNextSequenceSend(ctxA, suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID) + msg = types.NewMsgPayPacketFeeAsync(packetID, packetFee) - // build fee - packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, channelID, seq) - packetFee := types.NewPacketFee(fee, refundAcc.String(), nil) + tc.malleate() - tc.malleate() + _, err := suite.chainA.GetSimApp().IBCFeeKeeper.PayPacketFeeAsync(sdk.WrapSDKContext(suite.chainA.GetContext()), msg) - msg := types.NewMsgPayPacketFeeAsync(packetID, packetFee) - _, err := suite.chainA.GetSimApp().IBCFeeKeeper.PayPacketFeeAsync(sdk.WrapSDKContext(suite.chainA.GetContext()), msg) - - if tc.expPass { - suite.Require().NoError(err) // message committed - } else { - suite.Require().Error(err) - } + if tc.expPass { + suite.Require().NoError(err) // message committed + } else { + suite.Require().Error(err) + } + }) } } From 0a14a719da16dd1a8b5d6700c3eac719063d6c79 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Thu, 14 Apr 2022 10:35:01 +0200 Subject: [PATCH 3/6] adding assertions on escrow account balance --- modules/apps/29-fee/keeper/msg_server_test.go | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/modules/apps/29-fee/keeper/msg_server_test.go b/modules/apps/29-fee/keeper/msg_server_test.go index 1d22d7efffb..3c2aeefa609 100644 --- a/modules/apps/29-fee/keeper/msg_server_test.go +++ b/modules/apps/29-fee/keeper/msg_server_test.go @@ -164,7 +164,8 @@ func (suite *KeeperTestSuite) TestPayPacketFee() { func (suite *KeeperTestSuite) TestPayPacketFeeAsync() { var ( - msg *types.MsgPayPacketFeeAsync + expEscrowBalance sdk.Coins + msg *types.MsgPayPacketFeeAsync ) testCases := []struct { @@ -187,6 +188,9 @@ func (suite *KeeperTestSuite) TestPayPacketFeeAsync() { feesInEscrow := types.NewPacketFees([]types.PacketFee{packetFee}) suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, feesInEscrow) + suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress(), types.ModuleName, fee.Total()) + + expEscrowBalance = expEscrowBalance.Add(fee.Total()...) }, true, }, @@ -253,6 +257,7 @@ func (suite *KeeperTestSuite) TestPayPacketFeeAsync() { fee := types.NewFee(defaultReceiveFee, defaultAckFee, defaultTimeoutFee) packetFee := types.NewPacketFee(fee, suite.chainA.SenderAccount.GetAddress().String(), nil) + expEscrowBalance = fee.Total() msg = types.NewMsgPayPacketFeeAsync(packetID, packetFee) tc.malleate() @@ -261,8 +266,14 @@ func (suite *KeeperTestSuite) TestPayPacketFeeAsync() { if tc.expPass { suite.Require().NoError(err) // message committed + + escrowBalance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress(), sdk.DefaultBondDenom) + suite.Require().Equal(expEscrowBalance.AmountOf(sdk.DefaultBondDenom), escrowBalance.Amount) } else { suite.Require().Error(err) + + escrowBalance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress(), sdk.DefaultBondDenom) + suite.Require().Equal(sdk.NewInt(0), escrowBalance.Amount) } }) } From 044795cd4b06b49a5051a095f5620df1741a32e7 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Thu, 14 Apr 2022 10:52:40 +0200 Subject: [PATCH 4/6] adding error checks on bank sends in tests, omitting loop as per review --- modules/apps/29-fee/ibc_module_test.go | 18 +++++++++----- modules/apps/29-fee/keeper/escrow_test.go | 24 ++++++++++++------- modules/apps/29-fee/keeper/keeper_test.go | 7 ++---- modules/apps/29-fee/keeper/msg_server_test.go | 3 ++- 4 files changed, 32 insertions(+), 20 deletions(-) diff --git a/modules/apps/29-fee/ibc_module_test.go b/modules/apps/29-fee/ibc_module_test.go index 336f88ae415..cb6a89d13b7 100644 --- a/modules/apps/29-fee/ibc_module_test.go +++ b/modules/apps/29-fee/ibc_module_test.go @@ -321,7 +321,8 @@ func (suite *FeeTestSuite) TestOnChanCloseInit() { packetFees := types.NewPacketFees([]types.PacketFee{types.NewPacketFee(fee, "invalid refund address", nil)}) suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, packetFees) - suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, fee.Total()) + err := suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, fee.Total()) + suite.Require().NoError(err) }, false, }, @@ -344,7 +345,8 @@ func (suite *FeeTestSuite) TestOnChanCloseInit() { packetFee := types.NewPacketFee(fee, refundAcc.String(), []string{}) suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, types.NewPacketFees([]types.PacketFee{packetFee})) - suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, fee.Total()) + err := suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, fee.Total()) + suite.Require().NoError(err) tc.malleate() @@ -396,7 +398,8 @@ func (suite *FeeTestSuite) TestOnChanCloseConfirm() { packetFees := types.NewPacketFees([]types.PacketFee{types.NewPacketFee(fee, "invalid refund address", nil)}) suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, packetFees) - suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, fee.Total()) + err := suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, fee.Total()) + suite.Require().NoError(err) }, false, }, @@ -420,7 +423,8 @@ func (suite *FeeTestSuite) TestOnChanCloseConfirm() { packetFee := types.NewPacketFee(fee, refundAcc.String(), []string{}) suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, types.NewPacketFees([]types.PacketFee{packetFee})) - suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, fee.Total()) + err := suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, fee.Total()) + suite.Require().NoError(err) tc.malleate() @@ -661,7 +665,8 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() { ) suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, types.NewPacketFees([]types.PacketFee{packetFee})) - suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress(), types.ModuleName, packetFee.Fee.Total()) + err = suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress(), types.ModuleName, packetFee.Fee.Total()) + suite.Require().NoError(err) relayerAddr := suite.chainB.SenderAccount.GetAddress() @@ -793,7 +798,8 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() { ) suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, types.NewPacketFees([]types.PacketFee{packetFee})) - suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress(), types.ModuleName, packetFee.Fee.Total()) + err = suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress(), types.ModuleName, packetFee.Fee.Total()) + suite.Require().NoError(err) // log original sender balance // NOTE: balance is logged after escrowing tokens diff --git a/modules/apps/29-fee/keeper/escrow_test.go b/modules/apps/29-fee/keeper/escrow_test.go index a080cdd7824..19bf7cec7ba 100644 --- a/modules/apps/29-fee/keeper/escrow_test.go +++ b/modules/apps/29-fee/keeper/escrow_test.go @@ -122,7 +122,8 @@ func (suite *KeeperTestSuite) TestDistributeFee() { packetFees := []types.PacketFee{packetFee, packetFee} suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, types.NewPacketFees(packetFees)) - suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, packetFee.Fee.Total().Add(packetFee.Fee.Total()...)) + err := suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, packetFee.Fee.Total().Add(packetFee.Fee.Total()...)) + suite.Require().NoError(err) tc.malleate() @@ -163,7 +164,8 @@ func (suite *KeeperTestSuite) TestDistributeTimeoutFee() { packetFees := []types.PacketFee{packetFee, packetFee} suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, types.NewPacketFees(packetFees)) - suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, packetFee.Fee.Total().Add(packetFee.Fee.Total()...)) + err := suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, packetFee.Fee.Total().Add(packetFee.Fee.Total()...)) + suite.Require().NoError(err) // refundAcc balance after escrow refundAccBal := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom) @@ -210,7 +212,8 @@ func (suite *KeeperTestSuite) TestRefundFeesOnChannelClosure() { suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, packetFees) - suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, fee.Total()) + err := suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, fee.Total()) + suite.Require().NoError(err) expIdentifiedPacketFees = append(expIdentifiedPacketFees, identifiedPacketFees) } @@ -226,7 +229,8 @@ func (suite *KeeperTestSuite) TestRefundFeesOnChannelClosure() { suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, packetFees) - suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, fee.Total()) + err := suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, fee.Total()) + suite.Require().NoError(err) expIdentifiedPacketFees = append(expIdentifiedPacketFees, identifiedPacketFees) } @@ -237,7 +241,8 @@ func (suite *KeeperTestSuite) TestRefundFeesOnChannelClosure() { suite.chainA.GetSimApp().IBCFeeKeeper.SetFeeEnabled(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, "channel-1") suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, packetFees) - suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, fee.Total()) + err := suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, fee.Total()) + suite.Require().NoError(err) expEscrowBal = fee.Total() expRefundBal = expRefundBal.Sub(fee.Total()) @@ -273,7 +278,8 @@ func (suite *KeeperTestSuite) TestRefundFeesOnChannelClosure() { suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID2, packetFees) // update escrow account balance for 1 fee - suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, fee.Total()) + err := suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, fee.Total()) + suite.Require().NoError(err) expIdentifiedPacketFees = []types.IdentifiedPacketFees{identifiedPacketFee1, identifiedPacketFee2} }, true, @@ -287,7 +293,8 @@ func (suite *KeeperTestSuite) TestRefundFeesOnChannelClosure() { suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, packetFees) - suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, fee.Total()) + err := suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, fee.Total()) + suite.Require().NoError(err) expIdentifiedPacketFees = []types.IdentifiedPacketFees{identifiedPacketFees} }, false, @@ -303,7 +310,8 @@ func (suite *KeeperTestSuite) TestRefundFeesOnChannelClosure() { suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, packetFees) - suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, fee.Total()) + err := suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), refundAcc, types.ModuleName, fee.Total()) + suite.Require().NoError(err) expIdentifiedPacketFees = []types.IdentifiedPacketFees{identifiedPacketFees} diff --git a/modules/apps/29-fee/keeper/keeper_test.go b/modules/apps/29-fee/keeper/keeper_test.go index 12550451dbf..a54dce1a47b 100644 --- a/modules/apps/29-fee/keeper/keeper_test.go +++ b/modules/apps/29-fee/keeper/keeper_test.go @@ -98,11 +98,8 @@ func (suite *KeeperTestSuite) TestFeesInEscrow() { packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, 1) fee := types.NewFee(defaultReceiveFee, defaultAckFee, defaultTimeoutFee) - var packetFees []types.PacketFee - for i := 1; i < 6; i++ { - packetFee := types.NewPacketFee(fee, suite.chainA.SenderAccount.GetAddress().String(), nil) - packetFees = append(packetFees, packetFee) - } + packetFee := types.NewPacketFee(fee, suite.chainA.SenderAccount.GetAddress().String(), nil) + packetFees := []types.PacketFee{packetFee, packetFee, packetFee, packetFee, packetFee} suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, types.NewPacketFees(packetFees)) diff --git a/modules/apps/29-fee/keeper/msg_server_test.go b/modules/apps/29-fee/keeper/msg_server_test.go index 3c2aeefa609..3d0fb9809ff 100644 --- a/modules/apps/29-fee/keeper/msg_server_test.go +++ b/modules/apps/29-fee/keeper/msg_server_test.go @@ -188,7 +188,8 @@ func (suite *KeeperTestSuite) TestPayPacketFeeAsync() { feesInEscrow := types.NewPacketFees([]types.PacketFee{packetFee}) suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, feesInEscrow) - suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress(), types.ModuleName, fee.Total()) + err := suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress(), types.ModuleName, fee.Total()) + suite.Require().NoError(err) expEscrowBalance = expEscrowBalance.Add(fee.Total()...) }, From eb2120bd6ad8f026b66ad0c1d7a788e00b5ec6d0 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Thu, 14 Apr 2022 11:47:54 +0200 Subject: [PATCH 5/6] adding escrow balance checks to TestPayPacketFee --- modules/apps/29-fee/keeper/msg_server_test.go | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/modules/apps/29-fee/keeper/msg_server_test.go b/modules/apps/29-fee/keeper/msg_server_test.go index 3d0fb9809ff..e8512bf6021 100644 --- a/modules/apps/29-fee/keeper/msg_server_test.go +++ b/modules/apps/29-fee/keeper/msg_server_test.go @@ -55,7 +55,8 @@ func (suite *KeeperTestSuite) TestRegisterCounterpartyAddress() { func (suite *KeeperTestSuite) TestPayPacketFee() { var ( - msg *types.MsgPayPacketFee + expEscrowBalance sdk.Coins + msg *types.MsgPayPacketFee ) testCases := []struct { @@ -78,6 +79,10 @@ func (suite *KeeperTestSuite) TestPayPacketFee() { feesInEscrow := types.NewPacketFees([]types.PacketFee{packetFee}) suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, feesInEscrow) + err := suite.chainA.GetSimApp().BankKeeper.SendCoinsFromAccountToModule(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress(), types.ModuleName, fee.Total()) + suite.Require().NoError(err) + + expEscrowBalance = expEscrowBalance.Add(fee.Total()...) }, true, }, @@ -149,14 +154,22 @@ func (suite *KeeperTestSuite) TestPayPacketFee() { []string{}, ) + expEscrowBalance = fee.Total() + tc.malleate() _, err := suite.chainA.GetSimApp().IBCFeeKeeper.PayPacketFee(sdk.WrapSDKContext(suite.chainA.GetContext()), msg) if tc.expPass { suite.Require().NoError(err) // message committed + + escrowBalance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress(), sdk.DefaultBondDenom) + suite.Require().Equal(expEscrowBalance.AmountOf(sdk.DefaultBondDenom), escrowBalance.Amount) } else { suite.Require().Error(err) + + escrowBalance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress(), sdk.DefaultBondDenom) + suite.Require().Equal(sdk.NewInt(0), escrowBalance.Amount) } }) } From 1ae8bdb220dea7cde8587de1b3bfbef05f3383d5 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Thu, 14 Apr 2022 16:37:04 +0200 Subject: [PATCH 6/6] adding assertions on expected fees in state --- modules/apps/29-fee/keeper/msg_server_test.go | 22 ++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/modules/apps/29-fee/keeper/msg_server_test.go b/modules/apps/29-fee/keeper/msg_server_test.go index 0a9295f4eb3..8a927559a51 100644 --- a/modules/apps/29-fee/keeper/msg_server_test.go +++ b/modules/apps/29-fee/keeper/msg_server_test.go @@ -56,6 +56,7 @@ func (suite *KeeperTestSuite) TestRegisterCounterpartyAddress() { func (suite *KeeperTestSuite) TestPayPacketFee() { var ( expEscrowBalance sdk.Coins + expFeesInEscrow []types.PacketFee msg *types.MsgPayPacketFee ) @@ -75,7 +76,7 @@ func (suite *KeeperTestSuite) TestPayPacketFee() { fee := types.NewFee(defaultRecvFee, defaultAckFee, defaultTimeoutFee) packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, 1) - packetFee := types.NewPacketFee(fee, suite.chainA.SenderAccount.GetAddress().String(), []string{}) + packetFee := types.NewPacketFee(fee, suite.chainA.SenderAccount.GetAddress().String(), nil) feesInEscrow := types.NewPacketFees([]types.PacketFee{packetFee}) suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, feesInEscrow) @@ -83,6 +84,7 @@ func (suite *KeeperTestSuite) TestPayPacketFee() { suite.Require().NoError(err) expEscrowBalance = expEscrowBalance.Add(fee.Total()...) + expFeesInEscrow = append(expFeesInEscrow, packetFee) }, true, }, @@ -151,10 +153,12 @@ func (suite *KeeperTestSuite) TestPayPacketFee() { suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, suite.chainA.SenderAccount.GetAddress().String(), - []string{}, + nil, ) expEscrowBalance = fee.Total() + expPacketFee := types.NewPacketFee(fee, suite.chainA.SenderAccount.GetAddress().String(), nil) + expFeesInEscrow = []types.PacketFee{expPacketFee} tc.malleate() @@ -163,6 +167,11 @@ func (suite *KeeperTestSuite) TestPayPacketFee() { if tc.expPass { suite.Require().NoError(err) // message committed + packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, 1) + feesInEscrow, found := suite.chainA.GetSimApp().IBCFeeKeeper.GetFeesInEscrow(suite.chainA.GetContext(), packetID) + suite.Require().True(found) + suite.Require().Equal(expFeesInEscrow, feesInEscrow.PacketFees) + escrowBalance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress(), sdk.DefaultBondDenom) suite.Require().Equal(expEscrowBalance.AmountOf(sdk.DefaultBondDenom), escrowBalance.Amount) } else { @@ -178,6 +187,7 @@ func (suite *KeeperTestSuite) TestPayPacketFee() { func (suite *KeeperTestSuite) TestPayPacketFeeAsync() { var ( expEscrowBalance sdk.Coins + expFeesInEscrow []types.PacketFee msg *types.MsgPayPacketFeeAsync ) @@ -197,7 +207,7 @@ func (suite *KeeperTestSuite) TestPayPacketFeeAsync() { fee := types.NewFee(defaultRecvFee, defaultAckFee, defaultTimeoutFee) packetID := channeltypes.NewPacketId(suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID, 1) - packetFee := types.NewPacketFee(fee, suite.chainA.SenderAccount.GetAddress().String(), []string{}) + packetFee := types.NewPacketFee(fee, suite.chainA.SenderAccount.GetAddress().String(), nil) feesInEscrow := types.NewPacketFees([]types.PacketFee{packetFee}) suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, feesInEscrow) @@ -205,6 +215,7 @@ func (suite *KeeperTestSuite) TestPayPacketFeeAsync() { suite.Require().NoError(err) expEscrowBalance = expEscrowBalance.Add(fee.Total()...) + expFeesInEscrow = append(expFeesInEscrow, packetFee) }, true, }, @@ -272,6 +283,7 @@ func (suite *KeeperTestSuite) TestPayPacketFeeAsync() { packetFee := types.NewPacketFee(fee, suite.chainA.SenderAccount.GetAddress().String(), nil) expEscrowBalance = fee.Total() + expFeesInEscrow = []types.PacketFee{packetFee} msg = types.NewMsgPayPacketFeeAsync(packetID, packetFee) tc.malleate() @@ -281,6 +293,10 @@ func (suite *KeeperTestSuite) TestPayPacketFeeAsync() { if tc.expPass { suite.Require().NoError(err) // message committed + feesInEscrow, found := suite.chainA.GetSimApp().IBCFeeKeeper.GetFeesInEscrow(suite.chainA.GetContext(), packetID) + suite.Require().True(found) + suite.Require().Equal(expFeesInEscrow, feesInEscrow.PacketFees) + escrowBalance := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress(), sdk.DefaultBondDenom) suite.Require().Equal(expEscrowBalance.AmountOf(sdk.DefaultBondDenom), escrowBalance.Amount) } else {