diff --git a/modules/apps/29-fee/ibc_module.go b/modules/apps/29-fee/ibc_module.go index d9044200528..a45a9548426 100644 --- a/modules/apps/29-fee/ibc_module.go +++ b/modules/apps/29-fee/ibc_module.go @@ -213,28 +213,16 @@ func (im IBCModule) OnAcknowledgementPacket( } packetId := channeltypes.NewPacketId(packet.SourceChannel, packet.SourcePort, packet.Sequence) - identifiedPacketFee, found := im.keeper.GetFeeInEscrow(ctx, packetId) + identifiedPacketFee, found := im.keeper.GetFeeInEscrow(ctx, packetId) if !found { // return underlying callback if no fee found for given packetID return im.app.OnAcknowledgementPacket(ctx, packet, ack.Result, relayer) } - // cache context before trying to distribute the fee - cacheCtx, writeFn := ctx.CacheContext() - - forwardRelayer, _ := sdk.AccAddressFromBech32(ack.ForwardRelayerAddress) - refundAcc, _ := sdk.AccAddressFromBech32(identifiedPacketFee.RefundAddress) + im.keeper.DistributePacketFees(ctx, identifiedPacketFee.RefundAddress, ack.ForwardRelayerAddress, relayer, identifiedPacketFee) - err := im.keeper.DistributeFee(cacheCtx, refundAcc, forwardRelayer, relayer, packetId) - - if err == nil { - // write the cache and then call underlying callback - 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()) - } - // otherwise discard cache and call underlying callback + // call underlying callback return im.app.OnAcknowledgementPacket(ctx, packet, ack.Result, relayer) } @@ -252,25 +240,13 @@ func (im IBCModule) OnTimeoutPacket( packetId := channeltypes.NewPacketId(packet.SourceChannel, packet.SourcePort, packet.Sequence) identifiedPacketFee, found := im.keeper.GetFeeInEscrow(ctx, packetId) - if !found { // return underlying callback if fee not found for given packetID return im.app.OnTimeoutPacket(ctx, packet, relayer) } - // cache context before trying to distribute the fee - cacheCtx, writeFn := ctx.CacheContext() - - refundAcc, _ := sdk.AccAddressFromBech32(identifiedPacketFee.RefundAddress) - err := im.keeper.DistributeFeeTimeout(cacheCtx, refundAcc, relayer, packetId) - - if err == nil { - // write the cache and then call underlying callback - 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()) - } + im.keeper.DistributePacketFeesOnTimeout(ctx, identifiedPacketFee.RefundAddress, relayer, identifiedPacketFee) - // otherwise discard cache and call underlying callback + // call underlying callback return im.app.OnTimeoutPacket(ctx, packet, relayer) } diff --git a/modules/apps/29-fee/ibc_module_test.go b/modules/apps/29-fee/ibc_module_test.go index 08087dbc9cb..41234d7468a 100644 --- a/modules/apps/29-fee/ibc_module_test.go +++ b/modules/apps/29-fee/ibc_module_test.go @@ -508,7 +508,14 @@ func (suite *FeeTestSuite) TestOnRecvPacket() { // different channel than sending chain func (suite *FeeTestSuite) TestOnAcknowledgementPacket() { - var ack []byte + var ( + ack []byte + identifiedFee *types.IdentifiedPacketFee + originalBalance sdk.Coins + expectedBalance sdk.Coins + expectedRelayerBalance sdk.Coins + ) + testCases := []struct { name string malleate func() @@ -516,7 +523,9 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() { }{ { "success", - func() {}, + func() { + expectedRelayerBalance = identifiedFee.Fee.ReceiveFee.Add(identifiedFee.Fee.AckFee[0]) + }, true, }, { @@ -529,13 +538,17 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() { Result: channeltypes.NewResultAcknowledgement([]byte{1}).Acknowledgement(), ForwardRelayerAddress: suite.chainA.SenderAccount.GetAddress().String(), }.Acknowledgement() + + expectedBalance = originalBalance }, - false, + true, }, { "ack wrong format", func() { ack = []byte("unsupported acknowledgement format") + + expectedBalance = originalBalance }, false, }, @@ -544,20 +557,24 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() { func() { suite.chainA.GetSimApp().IBCFeeKeeper.DeleteFeeEnabled(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID) ack = channeltypes.NewResultAcknowledgement([]byte{1}).Acknowledgement() + + expectedBalance = originalBalance }, - false, + true, }, { - "error on distribute fee (blocked address)", + "fail on distribute receive fee (blocked address)", func() { - blockedAddr := suite.chainA.GetSimApp().AccountKeeper.GetModuleAccount(suite.chainA.GetContext(), types.ModuleName).GetAddress() + blockedAddr := suite.chainA.GetSimApp().AccountKeeper.GetModuleAccount(suite.chainA.GetContext(), transfertypes.ModuleName).GetAddress() ack = types.IncentivizedAcknowledgement{ Result: channeltypes.NewResultAcknowledgement([]byte{1}).Acknowledgement(), ForwardRelayerAddress: blockedAddr.String(), }.Acknowledgement() + + expectedRelayerBalance = identifiedFee.Fee.AckFee }, - false, + true, }, } @@ -565,6 +582,7 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() { tc := tc suite.Run(tc.name, func() { suite.SetupTest() + expectedRelayerBalance = sdk.Coins{} // reset // open incentivized channel suite.coordinator.Setup(suite.path) @@ -582,7 +600,7 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() { // escrow the packet fee packetId := channeltypes.NewPacketId(suite.path.EndpointA.ChannelID, suite.path.EndpointA.ChannelConfig.PortID, suite.chainA.SenderAccount.GetSequence()) - identifiedFee := types.NewIdentifiedPacketFee( + identifiedFee = types.NewIdentifiedPacketFee( packetId, types.Fee{ ReceiveFee: validCoins, @@ -595,50 +613,59 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() { err = suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), identifiedFee) suite.Require().NoError(err) + relayerAddr := suite.chainB.SenderAccount.GetAddress() + // must be changed explicitly ack = types.IncentivizedAcknowledgement{ Result: channeltypes.NewResultAcknowledgement([]byte{1}).Acknowledgement(), - ForwardRelayerAddress: suite.chainA.SenderAccount.GetAddress().String(), + ForwardRelayerAddress: relayerAddr.String(), }.Acknowledgement() + // log original sender balance + // NOTE: balance is logged after escrowing tokens + originalBalance = sdk.NewCoins(suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress(), ibctesting.TestCoin.Denom)) + + // default to success case + expectedBalance = originalBalance. + Add(identifiedFee.Fee.TimeoutFee[0]) + // malleate test case tc.malleate() - err = cbs.OnAcknowledgementPacket(suite.chainA.GetContext(), packet, ack, suite.chainA.SenderAccount.GetAddress()) + err = cbs.OnAcknowledgementPacket(suite.chainA.GetContext(), packet, ack, relayerAddr) if tc.expPass { - suite.Require().NoError(err, "unexpected error for case: %s", tc.name) - - expectedAmt, ok := sdk.NewIntFromString("10000000000000000000") - suite.Require().True(ok) - suite.Require().Equal( - sdk.Coin{ - Denom: ibctesting.TestCoin.Denom, - Amount: expectedAmt, - }, - suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress(), ibctesting.TestCoin.Denom)) + suite.Require().NoError(err) } else { - expectedAmt, ok := sdk.NewIntFromString("9999999999999999400") - suite.Require().True(ok) - suite.Require().Equal( - sdk.Coin{ - Denom: ibctesting.TestCoin.Denom, - Amount: expectedAmt, - }, - suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress(), ibctesting.TestCoin.Denom)) + suite.Require().Error(err) } + + suite.Require().Equal( + expectedBalance, + sdk.NewCoins(suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress(), ibctesting.TestCoin.Denom)), + ) + + relayerBalance := sdk.NewCoins(suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), relayerAddr, ibctesting.TestCoin.Denom)) + suite.Require().Equal( + expectedRelayerBalance, + relayerBalance, + ) + }) } } func (suite *FeeTestSuite) TestOnTimeoutPacket() { var ( - relayerAddr sdk.AccAddress + relayerAddr sdk.AccAddress + identifiedFee *types.IdentifiedPacketFee + originalBalance sdk.Coins + expectedBalance sdk.Coins ) testCases := []struct { - name string - malleate func() - expPass bool + name string + malleate func() + expFeeDistributed bool }{ { "success", @@ -646,25 +673,34 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() { true, }, { - "no op if identified packet fee doesn't exist", + "fee not enabled", func() { - // delete packet fee - packetId := channeltypes.NewPacketId(suite.path.EndpointA.ChannelID, suite.path.EndpointA.ChannelConfig.PortID, suite.chainA.SenderAccount.GetSequence()) - suite.chainA.GetSimApp().IBCFeeKeeper.DeleteFeeInEscrow(suite.chainA.GetContext(), packetId) + suite.chainA.GetSimApp().IBCFeeKeeper.DeleteFeeEnabled(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID) + + expectedBalance = originalBalance.Add(ibctesting.TestCoin) // timeout refund for ics20 transfer }, false, }, { - "error on distribute fee (blocked address)", + "no op if identified packet fee doesn't exist", func() { - relayerAddr = suite.chainA.GetSimApp().AccountKeeper.GetModuleAccount(suite.chainA.GetContext(), types.ModuleName).GetAddress() + // delete packet fee + packetId := channeltypes.NewPacketId(suite.path.EndpointA.ChannelID, suite.path.EndpointA.ChannelConfig.PortID, suite.chainA.SenderAccount.GetSequence()) + suite.chainA.GetSimApp().IBCFeeKeeper.DeleteFeeInEscrow(suite.chainA.GetContext(), packetId) + + expectedBalance = originalBalance.Add(ibctesting.TestCoin) // timeout refund for ics20 transfer }, false, }, { - "fee not enabled", + "distribute fee fails for timeout fee (blocked address)", func() { - suite.chainA.GetSimApp().IBCFeeKeeper.DeleteFeeEnabled(suite.chainA.GetContext(), suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID) + relayerAddr = suite.chainA.GetSimApp().AccountKeeper.GetModuleAccount(suite.chainA.GetContext(), transfertypes.ModuleName).GetAddress() + + expectedBalance = originalBalance. + Add(identifiedFee.Fee.ReceiveFee[0]). + Add(identifiedFee.Fee.AckFee[0]). + Add(ibctesting.TestCoin) // timeout refund for ics20 transfer }, false, }, @@ -696,9 +732,9 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() { packetId := channeltypes.NewPacketId(suite.path.EndpointA.ChannelID, suite.path.EndpointA.ChannelConfig.PortID, suite.chainA.SenderAccount.GetSequence()) // must be explicitly changed - relayerAddr = suite.chainA.SenderAccount.GetAddress() + relayerAddr = suite.chainB.SenderAccount.GetAddress() - identifiedFee := types.NewIdentifiedPacketFee( + identifiedFee = types.NewIdentifiedPacketFee( packetId, types.Fee{ ReceiveFee: validCoins, @@ -712,31 +748,35 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() { err = suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), identifiedFee) suite.Require().NoError(err) + // log original sender balance + // NOTE: balance is logged after escrowing tokens + originalBalance = sdk.NewCoins(suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress(), ibctesting.TestCoin.Denom)) + + // default to success case + expectedBalance = originalBalance. + Add(identifiedFee.Fee.ReceiveFee[0]). + Add(identifiedFee.Fee.AckFee[0]). + Add(coin) // timeout refund from ics20 transfer + // malleate test case tc.malleate() err = cbs.OnTimeoutPacket(suite.chainA.GetContext(), packet, relayerAddr) + suite.Require().NoError(err) - if tc.expPass { - suite.Require().NoError(err, "unexpected error for case: %s", tc.name) + suite.Require().Equal( + expectedBalance, + sdk.NewCoins(suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress(), ibctesting.TestCoin.Denom)), + ) - expectedAmt, ok := sdk.NewIntFromString("10000000000000000100") - suite.Require().True(ok) + relayerBalance := sdk.NewCoins(suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), relayerAddr, ibctesting.TestCoin.Denom)) + if tc.expFeeDistributed { suite.Require().Equal( - sdk.Coin{ - Denom: ibctesting.TestCoin.Denom, - Amount: expectedAmt, - }, - suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress(), ibctesting.TestCoin.Denom)) + identifiedFee.Fee.TimeoutFee, + relayerBalance, + ) } else { - expectedAmt, ok := sdk.NewIntFromString("9999999999999999500") - suite.Require().True(ok) - suite.Require().Equal( - sdk.Coin{ - Denom: ibctesting.TestCoin.Denom, - Amount: expectedAmt, - }, - suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress(), ibctesting.TestCoin.Denom)) + suite.Require().Empty(relayerBalance) } }) } diff --git a/modules/apps/29-fee/keeper/escrow.go b/modules/apps/29-fee/keeper/escrow.go index 0dae91cea93..45ffc2f72d3 100644 --- a/modules/apps/29-fee/keeper/escrow.go +++ b/modules/apps/29-fee/keeper/escrow.go @@ -7,7 +7,6 @@ import ( sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" "github.com/cosmos/ibc-go/v3/modules/apps/29-fee/types" - 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 @@ -43,68 +42,66 @@ func (k Keeper) EscrowPacketFee(ctx sdk.Context, identifiedFee *types.Identified return nil } -// DistributeFee 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) DistributeFee(ctx sdk.Context, refundAcc, forwardRelayer, reverseRelayer sdk.AccAddress, packetID *channeltypes.PacketId) error { - // check if there is a Fee in escrow for the given packetId - feeInEscrow, found := k.GetFeeInEscrow(ctx, packetID) - if !found { - return sdkerrors.Wrapf(types.ErrFeeNotFound, "with channelID %s, sequence %d", packetID.ChannelId, packetID.Sequence) +// 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, refundAcc, forwardRelayer string, reverseRelayer sdk.AccAddress, feeInEscrow types.IdentifiedPacketFee) { + // distribute fee for forward relaying + forward, err := sdk.AccAddressFromBech32(forwardRelayer) + if err == nil { + k.distributeFee(ctx, forward, feeInEscrow.Fee.ReceiveFee) } - // send receive fee to forward relayer - err := k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, forwardRelayer, feeInEscrow.Fee.ReceiveFee) - if err != nil { - return sdkerrors.Wrap(err, "failed to send fee to forward relayer") - } + // distribute fee for reverse relaying + k.distributeFee(ctx, reverseRelayer, feeInEscrow.Fee.AckFee) - // send ack fee to reverse relayer - err = k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, reverseRelayer, feeInEscrow.Fee.AckFee) + // refund timeout fee refund, + refundAddr, err := sdk.AccAddressFromBech32(refundAcc) if err != nil { - return sdkerrors.Wrap(err, "error sending fee to reverse relayer") + panic(fmt.Sprintf("could not parse refundAcc %s to sdk.AccAddress", refundAcc)) } - // refund timeout fee to refundAddr - err = k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, refundAcc, feeInEscrow.Fee.TimeoutFee) - if err != nil { - return sdkerrors.Wrap(err, "error refunding timeout fee") - } + // refund timeout fee for unused timeout + k.distributeFee(ctx, refundAddr, feeInEscrow.Fee.TimeoutFee) // removes the fee from the store as fee is now paid - k.DeleteFeeInEscrow(ctx, packetID) - - return nil + k.DeleteFeeInEscrow(ctx, feeInEscrow.PacketId) } -// DistributeFeeTimeout 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) DistributeFeeTimeout(ctx sdk.Context, refundAcc, timeoutRelayer sdk.AccAddress, packetID *channeltypes.PacketId) error { - // check if there is a Fee in escrow for the given packetId - feeInEscrow, found := k.GetFeeInEscrow(ctx, packetID) - if !found { - return sdkerrors.Wrapf(types.ErrFeeNotFound, "for packetID %s", packetID) - } - - // refund the receive fee - err := k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, refundAcc, feeInEscrow.Fee.ReceiveFee) +// 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, refundAcc string, timeoutRelayer sdk.AccAddress, feeInEscrow types.IdentifiedPacketFee) { + // check if refundAcc address works + refundAddr, err := sdk.AccAddressFromBech32(refundAcc) if err != nil { - return sdkerrors.Wrap(err, "error refunding receive fee") + panic(fmt.Sprintf("could not parse refundAcc %s to sdk.AccAddress", refundAcc)) } - // refund the ack fee - err = k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, refundAcc, feeInEscrow.Fee.AckFee) - if err != nil { - return sdkerrors.Wrap(err, "error refunding ack fee") - } + // refund receive fee for unused forward relaying + k.distributeFee(ctx, refundAddr, feeInEscrow.Fee.ReceiveFee) - // pay the timeout fee to the timeout relayer - err = k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, timeoutRelayer, feeInEscrow.Fee.TimeoutFee) - if err != nil { - return sdkerrors.Wrap(err, "error sending fee to timeout relayer") - } + // refund ack fee for unused reverse relaying + k.distributeFee(ctx, refundAddr, feeInEscrow.Fee.AckFee) + + // distribute fee for timeout relaying + k.distributeFee(ctx, timeoutRelayer, feeInEscrow.Fee.TimeoutFee) // removes the fee from the store as fee is now paid - k.DeleteFeeInEscrow(ctx, packetID) + k.DeleteFeeInEscrow(ctx, feeInEscrow.PacketId) +} - return nil +// 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) { + // cache context before trying to send to reverse relayer + 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()) + } } func (k Keeper) RefundFeesOnChannel(ctx sdk.Context, portID, channelID string) error { diff --git a/modules/apps/29-fee/keeper/escrow_test.go b/modules/apps/29-fee/keeper/escrow_test.go index fb71461913d..3b25f03d101 100644 --- a/modules/apps/29-fee/keeper/escrow_test.go +++ b/modules/apps/29-fee/keeper/escrow_test.go @@ -64,13 +64,17 @@ func (suite *KeeperTestSuite) TestEscrowPacketFee() { // setup refundAcc = suite.chainA.SenderAccount.GetAddress() - ackFee = validCoins - receiveFee = validCoins2 - timeoutFee = validCoins3 + receiveFee = defaultReceiveFee + ackFee = defaultAckFee + timeoutFee = defaultTimeoutFee packetId = &channeltypes.PacketId{ChannelId: suite.path.EndpointA.ChannelID, PortId: transfertypes.PortID, Sequence: uint64(1)} tc.malleate() - fee := types.Fee{ackFee, receiveFee, timeoutFee} + fee := types.Fee{ + ReceiveFee: receiveFee, + AckFee: ackFee, + TimeoutFee: timeoutFee, + } identifiedPacketFee := &types.IdentifiedPacketFee{PacketId: packetId, Fee: fee, RefundAddress: refundAcc.String(), Relayers: []string{}} // refundAcc balance before escrow @@ -102,13 +106,8 @@ func (suite *KeeperTestSuite) TestEscrowPacketFee() { func (suite *KeeperTestSuite) TestDistributeFee() { var ( - err error - ackFee sdk.Coins - receiveFee sdk.Coins - timeoutFee sdk.Coins - packetId *channeltypes.PacketId reverseRelayer sdk.AccAddress - forwardRelayer sdk.AccAddress + forwardRelayer string refundAcc sdk.AccAddress ) @@ -123,9 +122,8 @@ func (suite *KeeperTestSuite) TestDistributeFee() { "success", func() {}, true, }, { - "fee not found for packet", func() { - // setting packetId with an invalid sequence of 2 - packetId = &channeltypes.PacketId{ChannelId: suite.path.EndpointA.ChannelID, PortId: transfertypes.PortID, Sequence: uint64(2)} + "invalid forward address", func() { + forwardRelayer = "invalid address" }, false, }, } @@ -140,18 +138,19 @@ func (suite *KeeperTestSuite) TestDistributeFee() { // setup refundAcc = suite.chainA.SenderAccount.GetAddress() reverseRelayer = sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()) - forwardRelayer = sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()) + forwardRelayer = sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()).String() - ackFee = validCoins - receiveFee = validCoins2 - timeoutFee = validCoins3 - packetId = &channeltypes.PacketId{ChannelId: suite.path.EndpointA.ChannelID, PortId: transfertypes.PortID, Sequence: validSeq} - fee := types.Fee{receiveFee, ackFee, timeoutFee} + packetId := &channeltypes.PacketId{ChannelId: suite.path.EndpointA.ChannelID, PortId: transfertypes.PortID, Sequence: validSeq} + fee := types.Fee{ + ReceiveFee: defaultReceiveFee, + AckFee: defaultAckFee, + TimeoutFee: defaultTimeoutFee, + } // escrow the packet fee & store the fee in state - identifiedPacketFee := types.IdentifiedPacketFee{PacketId: packetId, Fee: fee, RefundAddress: refundAcc.String(), Relayers: []string{}} + identifiedPacketFee := types.NewIdentifiedPacketFee(packetId, fee, refundAcc.String(), []string{}) - err = suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), &identifiedPacketFee) + err := suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), identifiedPacketFee) suite.Require().NoError(err) tc.malleate() @@ -159,123 +158,92 @@ func (suite *KeeperTestSuite) TestDistributeFee() { // refundAcc balance after escrow refundAccBal := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom) - err = suite.chainA.GetSimApp().IBCFeeKeeper.DistributeFee(suite.chainA.GetContext(), refundAcc, forwardRelayer, reverseRelayer, packetId) + suite.chainA.GetSimApp().IBCFeeKeeper.DistributePacketFees(suite.chainA.GetContext(), refundAcc.String(), forwardRelayer, reverseRelayer, *identifiedPacketFee) if tc.expPass { - suite.Require().NoError(err) - hasFeeInEscrow := suite.chainA.GetSimApp().IBCFeeKeeper.HasFeeInEscrow(suite.chainA.GetContext(), packetId) // there should no longer be a fee in escrow for this packet - suite.Require().False(hasFeeInEscrow) + found := suite.chainA.GetSimApp().IBCFeeKeeper.HasFeeInEscrow(suite.chainA.GetContext(), packetId) + suite.Require().False(found) + // check if the reverse relayer is paid - hasBalance := suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), reverseRelayer, ackFee[0]) + hasBalance := suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), reverseRelayer, fee.AckFee[0]) suite.Require().True(hasBalance) + // check if the forward relayer is paid - hasBalance = suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), forwardRelayer, receiveFee[0]) + forward, err := sdk.AccAddressFromBech32(forwardRelayer) + suite.Require().NoError(err) + hasBalance = suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), forward, fee.ReceiveFee[0]) suite.Require().True(hasBalance) + // check if the refund acc has been refunded the timeoutFee - expectedRefundAccBal := refundAccBal.Add(timeoutFee[0]) + expectedRefundAccBal := refundAccBal.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) - - suite.Require().NoError(err) - } else { - suite.Require().Error(err) - invalidPacketID := &channeltypes.PacketId{PortId: transfertypes.PortID, ChannelId: suite.path.EndpointA.ChannelID, Sequence: 1} - hasFeeInEscrow := suite.chainA.GetSimApp().IBCFeeKeeper.HasFeeInEscrow(suite.chainA.GetContext(), invalidPacketID) - // there should still be a fee in escrow for this packet - suite.Require().True(hasFeeInEscrow) + // check the module acc wallet still has forward relaying balance + hasBalance := suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCFeeKeeper.GetFeeModuleAddress(), fee.ReceiveFee[0]) + suite.Require().True(hasBalance) } }) } } func (suite *KeeperTestSuite) TestDistributeTimeoutFee() { - var ( - err error - ackFee sdk.Coins - receiveFee sdk.Coins - timeoutFee sdk.Coins - packetId *channeltypes.PacketId - refundAcc sdk.AccAddress - ) + suite.coordinator.Setup(suite.path) // setup channel - validSeq := uint64(1) + // setup + refundAcc := suite.chainA.SenderAccount.GetAddress() + timeoutRelayer := sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()) - testCases := []struct { - name string - malleate func() - expPass bool - }{ - { - "success", func() {}, true, - }, - { - "fee not found for packet", func() { - // setting packetId with an invalid sequence of 2 - packetId = &channeltypes.PacketId{ChannelId: suite.path.EndpointA.ChannelID, PortId: transfertypes.PortID, Sequence: uint64(2)} - }, false, - }, + packetId := &channeltypes.PacketId{ + ChannelId: suite.path.EndpointA.ChannelID, + PortId: transfertypes.PortID, + Sequence: 1, } - for _, tc := range testCases { - tc := tc - - suite.Run(tc.name, func() { - suite.SetupTest() // reset - suite.coordinator.Setup(suite.path) // setup channel + fee := types.Fee{ + ReceiveFee: defaultReceiveFee, + AckFee: defaultAckFee, + TimeoutFee: defaultTimeoutFee, + } - // setup - refundAcc = suite.chainA.SenderAccount.GetAddress() - timeoutRelayer := sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()) + // escrow the packet fee & store the fee in state + identifiedPacketFee := types.IdentifiedPacketFee{ + PacketId: packetId, + Fee: fee, + RefundAddress: refundAcc.String(), + Relayers: []string{}, + } - ackFee = validCoins - receiveFee = validCoins2 - timeoutFee = validCoins3 - packetId = &channeltypes.PacketId{ChannelId: suite.path.EndpointA.ChannelID, PortId: transfertypes.PortID, Sequence: validSeq} - fee := types.Fee{receiveFee, ackFee, timeoutFee} + err := suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), &identifiedPacketFee) + suite.Require().NoError(err) - // escrow the packet fee & store the fee in state - identifiedPacketFee := types.IdentifiedPacketFee{PacketId: packetId, Fee: fee, RefundAddress: refundAcc.String(), Relayers: []string{}} - err = suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), &identifiedPacketFee) - suite.Require().NoError(err) + // refundAcc balance after escrow + refundAccBal := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom) - tc.malleate() + suite.chainA.GetSimApp().IBCFeeKeeper.DistributePacketFeesOnTimeout(suite.chainA.GetContext(), refundAcc.String(), timeoutRelayer, identifiedPacketFee) - // refundAcc balance after escrow - refundAccBal := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom) + // there should no longer be a fee in escrow for this packet + found := suite.chainA.GetSimApp().IBCFeeKeeper.HasFeeInEscrow(suite.chainA.GetContext(), packetId) + suite.Require().False(found) - err = suite.chainA.GetSimApp().IBCFeeKeeper.DistributeFeeTimeout(suite.chainA.GetContext(), refundAcc, timeoutRelayer, packetId) + // check if the timeoutRelayer has been paid + hasBalance := suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), timeoutRelayer, fee.TimeoutFee[0]) + suite.Require().True(hasBalance) - if tc.expPass { - suite.Require().NoError(err) - hasFeeInEscrow := suite.chainA.GetSimApp().IBCFeeKeeper.HasFeeInEscrow(suite.chainA.GetContext(), packetId) - // there should no longer be a fee in escrow for this packet - suite.Require().False(hasFeeInEscrow) - // check if the timeoutRelayer has been paid - hasBalance := suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), timeoutRelayer, timeoutFee[0]) - suite.Require().True(hasBalance) - // check if the refund acc has been refunded the recv & ack fees - expectedRefundAccBal := refundAccBal.Add(ackFee[0]) - expectedRefundAccBal = refundAccBal.Add(receiveFee[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) + // check if the refund acc has been refunded the recv & ack fees + expectedRefundAccBal := refundAccBal.Add(fee.AckFee[0]) + expectedRefundAccBal = refundAccBal.Add(fee.ReceiveFee[0]) + hasBalance = suite.chainA.GetSimApp().BankKeeper.HasBalance(suite.chainA.GetContext(), refundAcc, expectedRefundAccBal) + suite.Require().True(hasBalance) - } else { - suite.Require().Error(err) - invalidPacketID := &channeltypes.PacketId{PortId: transfertypes.PortID, ChannelId: suite.path.EndpointA.ChannelID, Sequence: 1} - hasFeeInEscrow := suite.chainA.GetSimApp().IBCFeeKeeper.HasFeeInEscrow(suite.chainA.GetContext(), invalidPacketID) - // there should still be a fee in escrow for this packet - suite.Require().True(hasFeeInEscrow) - } - }) - } + // 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) } func (suite *KeeperTestSuite) TestRefundFeesOnChannel() { @@ -285,13 +253,13 @@ func (suite *KeeperTestSuite) TestRefundFeesOnChannel() { // refundAcc balance before escrow prevBal := suite.chainA.GetSimApp().BankKeeper.GetAllBalances(suite.chainA.GetContext(), refundAcc) - ackFee := validCoins - receiveFee := validCoins2 - timeoutFee := validCoins3 - for i := 0; i < 5; i++ { packetId := &channeltypes.PacketId{ChannelId: "channel-0", PortId: transfertypes.PortID, Sequence: uint64(i)} - fee := types.Fee{receiveFee, ackFee, timeoutFee} + fee := types.Fee{ + ReceiveFee: defaultReceiveFee, + AckFee: defaultAckFee, + TimeoutFee: defaultTimeoutFee, + } identifiedPacketFee := types.IdentifiedPacketFee{PacketId: packetId, Fee: fee, RefundAddress: refundAcc.String(), Relayers: []string{}} suite.chainA.GetSimApp().IBCFeeKeeper.SetFeeEnabled(suite.chainA.GetContext(), transfertypes.PortID, "channel-0") @@ -301,7 +269,11 @@ func (suite *KeeperTestSuite) TestRefundFeesOnChannel() { // send a packet over a different channel to ensure this fee is not refunded packetId := &channeltypes.PacketId{ChannelId: "channel-1", PortId: transfertypes.PortID, Sequence: 1} - fee := types.Fee{receiveFee, ackFee, timeoutFee} + fee := types.Fee{ + ReceiveFee: defaultReceiveFee, + AckFee: defaultAckFee, + TimeoutFee: defaultTimeoutFee, + } identifiedPacketFee := types.IdentifiedPacketFee{PacketId: packetId, Fee: fee, RefundAddress: refundAcc.String(), Relayers: []string{}} suite.chainA.GetSimApp().IBCFeeKeeper.SetFeeEnabled(suite.chainA.GetContext(), transfertypes.PortID, "channel-1") @@ -314,17 +286,16 @@ func (suite *KeeperTestSuite) TestRefundFeesOnChannel() { // add fee sent to channel-1 to after balance to recover original balance afterBal := suite.chainA.GetSimApp().BankKeeper.GetAllBalances(suite.chainA.GetContext(), refundAcc) - suite.Require().Equal(prevBal, afterBal.Add(validCoins...).Add(validCoins2...).Add(validCoins3...), "refund account not back to original balance after refunding all tokens") + suite.Require().Equal(prevBal, afterBal.Add(fee.ReceiveFee...).Add(fee.AckFee...).Add(fee.TimeoutFee...), "refund account not back to original balance after refunding all tokens") // create escrow and then change module account balance to cause error on refund packetId = &channeltypes.PacketId{ChannelId: "channel-0", PortId: transfertypes.PortID, Sequence: uint64(6)} - fee = types.Fee{receiveFee, ackFee, timeoutFee} identifiedPacketFee = types.IdentifiedPacketFee{PacketId: packetId, Fee: fee, RefundAddress: refundAcc.String(), Relayers: []string{}} err = suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), &identifiedPacketFee) suite.Require().NoError(err) - suite.chainA.GetSimApp().BankKeeper.SendCoinsFromModuleToAccount(suite.chainA.GetContext(), types.ModuleName, refundAcc, validCoins3) + suite.chainA.GetSimApp().BankKeeper.SendCoinsFromModuleToAccount(suite.chainA.GetContext(), types.ModuleName, refundAcc, fee.TimeoutFee) err = suite.chainA.GetSimApp().IBCFeeKeeper.RefundFeesOnChannel(suite.chainA.GetContext(), transfertypes.PortID, "channel-0") suite.Require().Error(err, "refund fees returned no error with insufficient balance on module account") diff --git a/modules/apps/29-fee/keeper/genesis_test.go b/modules/apps/29-fee/keeper/genesis_test.go index 6833ed72816..45198e4b0a5 100644 --- a/modules/apps/29-fee/keeper/genesis_test.go +++ b/modules/apps/29-fee/keeper/genesis_test.go @@ -18,9 +18,9 @@ func (suite *KeeperTestSuite) TestInitGenesis() { uint64(1), ) fee := types.Fee{ - validCoins, - validCoins2, - validCoins3, + defaultReceiveFee, + defaultAckFee, + defaultTimeoutFee, } // relayer addresses @@ -80,9 +80,9 @@ func (suite *KeeperTestSuite) TestExportGenesis() { uint64(1), ) fee := types.Fee{ - validCoins, - validCoins2, - validCoins3, + defaultReceiveFee, + defaultAckFee, + defaultTimeoutFee, } identifiedPacketFee := &types.IdentifiedPacketFee{PacketId: packetId, Fee: fee, RefundAddress: refundAcc.String(), Relayers: []string{}} err := suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), identifiedPacketFee) diff --git a/modules/apps/29-fee/keeper/keeper_test.go b/modules/apps/29-fee/keeper/keeper_test.go index 40eeaed8d40..267190fa080 100644 --- a/modules/apps/29-fee/keeper/keeper_test.go +++ b/modules/apps/29-fee/keeper/keeper_test.go @@ -15,10 +15,10 @@ import ( ) var ( - validCoins = sdk.Coins{sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: sdk.NewInt(100)}} - validCoins2 = sdk.Coins{sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: sdk.NewInt(200)}} - validCoins3 = sdk.Coins{sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: sdk.NewInt(300)}} - invalidCoins = sdk.Coins{sdk.Coin{Denom: "invalidDenom", Amount: sdk.NewInt(100)}} + defaultReceiveFee = 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)}} ) type KeeperTestSuite struct { @@ -57,10 +57,7 @@ func TestKeeperTestSuite(t *testing.T) { } func (suite *KeeperTestSuite) TestFeeInEscrow() { - ackFee := validCoins - receiveFee := validCoins2 - timeoutFee := validCoins3 - fee := types.Fee{ReceiveFee: receiveFee, AckFee: ackFee, TimeoutFee: timeoutFee} + fee := types.Fee{ReceiveFee: defaultReceiveFee, AckFee: defaultAckFee, TimeoutFee: defaultTimeoutFee} // set some fees for i := 1; i < 6; i++ { @@ -104,11 +101,8 @@ func (suite *KeeperTestSuite) TestGetAllIdentifiedPacketFees() { // escrow a fee refundAcc := suite.chainA.SenderAccount.GetAddress() - ackFee := validCoins - receiveFee := validCoins2 - timeoutFee := validCoins3 packetId := &channeltypes.PacketId{ChannelId: suite.path.EndpointA.ChannelID, PortId: transfertypes.PortID, Sequence: uint64(1)} - fee := types.Fee{ackFee, receiveFee, timeoutFee} + fee := types.Fee{defaultAckFee, defaultReceiveFee, defaultTimeoutFee} identifiedPacketFee := &types.IdentifiedPacketFee{PacketId: packetId, Fee: fee, RefundAddress: refundAcc.String(), Relayers: []string{}} // escrow the packet fee diff --git a/modules/apps/29-fee/keeper/msg_server_test.go b/modules/apps/29-fee/keeper/msg_server_test.go index 9b6e0596b94..0664b4e7fd9 100644 --- a/modules/apps/29-fee/keeper/msg_server_test.go +++ b/modules/apps/29-fee/keeper/msg_server_test.go @@ -70,9 +70,9 @@ func (suite *KeeperTestSuite) TestPayPacketFee() { refundAcc := suite.chainA.SenderAccount.GetAddress() channelID := suite.path.EndpointA.ChannelID fee := types.Fee{ - ReceiveFee: validCoins, - AckFee: validCoins, - TimeoutFee: validCoins, + ReceiveFee: defaultReceiveFee, + AckFee: defaultAckFee, + TimeoutFee: defaultTimeoutFee, } msg := types.NewMsgPayPacketFee(fee, suite.path.EndpointA.ChannelConfig.PortID, channelID, refundAcc.String(), []string{}) @@ -111,9 +111,9 @@ func (suite *KeeperTestSuite) TestPayPacketFeeAsync() { // build packetId channelID := suite.path.EndpointA.ChannelID fee := types.Fee{ - ReceiveFee: validCoins, - AckFee: validCoins, - TimeoutFee: validCoins, + ReceiveFee: defaultReceiveFee, + AckFee: defaultAckFee, + TimeoutFee: defaultTimeoutFee, } seq, _ := suite.chainA.App.GetIBCKeeper().ChannelKeeper.GetNextSequenceSend(ctxA, suite.path.EndpointA.ChannelConfig.PortID, suite.path.EndpointA.ChannelID)