diff --git a/modules/apps/29-fee/ibc_middleware_test.go b/modules/apps/29-fee/ibc_middleware_test.go index 0463a656624..061415cca29 100644 --- a/modules/apps/29-fee/ibc_middleware_test.go +++ b/modules/apps/29-fee/ibc_middleware_test.go @@ -564,12 +564,14 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() { originalBalance sdk.Coins expectedBalance sdk.Coins expectedRelayerBalance sdk.Coins + expLocked bool ) testCases := []struct { - name string - malleate func() - expPass bool + name string + malleate func() + expFeesDistributed bool + expPass bool }{ { "success", @@ -577,6 +579,7 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() { expectedRelayerBalance = packetFee.Fee.RecvFee.Add(packetFee.Fee.AckFee[0]) }, true, + true, }, { "no op success without a packet fee", @@ -592,6 +595,7 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() { expectedBalance = originalBalance }, true, + true, }, { "ack wrong format", @@ -601,6 +605,7 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() { expectedBalance = originalBalance }, false, + false, }, { "channel is not fee not enabled, success", @@ -610,15 +615,18 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() { expectedBalance = originalBalance }, + false, true, }, { "success: fee module is disabled, skip fee logic", func() { lockFeeModule(suite.chainA) + expLocked = true expectedBalance = originalBalance }, + false, true, }, { @@ -634,6 +642,7 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() { expectedRelayerBalance = packetFee.Fee.AckFee expectedBalance = expectedBalance.Add(packetFee.Fee.RecvFee...) }, + false, true, }, { @@ -648,7 +657,10 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() { expectedRelayerBalance = sdk.NewCoins() suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, types.NewPacketFees([]types.PacketFee{packetFee})) + + expLocked = true }, + false, true, }, } @@ -657,6 +669,8 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() { tc := tc suite.Run(tc.name, func() { suite.SetupTest() + expLocked = false + suite.coordinator.Setup(suite.path) packet := suite.CreateMockPacket() @@ -711,12 +725,20 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() { suite.Require().Error(err) } + suite.Require().Equal(expLocked, suite.chainA.GetSimApp().IBCFeeKeeper.IsLocked(suite.chainA.GetContext())) + suite.Require().Equal( expectedBalance, sdk.NewCoins(suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress(), ibctesting.TestCoin.Denom)), ) relayerBalance := sdk.NewCoins(suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), relayerAddr, ibctesting.TestCoin.Denom)) + if tc.expFeesDistributed { + // there should no longer be a fee in escrow for this packet + found := suite.chainA.GetSimApp().IBCFeeKeeper.HasFeesInEscrow(suite.chainA.GetContext(), packetID) + suite.Require().False(found) + } + suite.Require().Equal( expectedRelayerBalance, relayerBalance, @@ -731,18 +753,17 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() { packetFee types.PacketFee originalBalance sdk.Coins expectedBalance sdk.Coins + expLocked bool ) testCases := []struct { name string malleate func() expFeeDistributed bool - expLocked bool }{ { "success", func() {}, true, - true, }, { "fee not enabled", @@ -752,17 +773,16 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() { expectedBalance = originalBalance }, false, - true, }, { "fee module is disabled, skip fee logic", func() { lockFeeModule(suite.chainA) + expLocked = true expectedBalance = originalBalance }, false, - false, }, { "no op if identified packet fee doesn't exist", @@ -774,7 +794,6 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() { expectedBalance = originalBalance }, false, - true, }, { "distribute fee fails for timeout fee (blocked address)", @@ -787,7 +806,6 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() { Add(packetFee.Fee.TimeoutFee...) }, false, - true, }, { "fail on no distribution by escrow account out of balance", @@ -800,9 +818,10 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() { expectedBalance = originalBalance.Add(smallAmount...) suite.chainA.GetSimApp().IBCFeeKeeper.SetFeesInEscrow(suite.chainA.GetContext(), packetID, types.NewPacketFees([]types.PacketFee{packetFee})) + + expLocked = true }, false, - false, }, } @@ -810,6 +829,8 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() { tc := tc suite.Run(tc.name, func() { suite.SetupTest() + expLocked = false + suite.coordinator.Setup(suite.path) packet := suite.CreateMockPacket() @@ -852,11 +873,9 @@ func (suite *FeeTestSuite) TestOnTimeoutPacket() { tc.malleate() err = cbs.OnTimeoutPacket(suite.chainA.GetContext(), packet, relayerAddr) - if tc.expLocked { - suite.Require().False(suite.chainA.GetSimApp().IBCFeeKeeper.IsLocked(suite.chainA.GetContext())) - } else { - suite.Require().True(suite.chainA.GetSimApp().IBCFeeKeeper.IsLocked(suite.chainA.GetContext())) - } + suite.Require().NoError(err) + + suite.Require().Equal(expLocked, suite.chainA.GetSimApp().IBCFeeKeeper.IsLocked(suite.chainA.GetContext())) suite.Require().Equal( expectedBalance, diff --git a/modules/apps/29-fee/keeper/escrow.go b/modules/apps/29-fee/keeper/escrow.go index e609c734f5e..e4d5af69fa8 100644 --- a/modules/apps/29-fee/keeper/escrow.go +++ b/modules/apps/29-fee/keeper/escrow.go @@ -73,15 +73,14 @@ func (k Keeper) DistributePacketFeesOnAcknowledgement(ctx sdk.Context, forwardRe k.distributePacketFeeOnAcknowledgement(cacheCtx, refundAddr, forwardAddr, reverseRelayer, packetFee) } + // NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context. + ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events()) + // write the cache writeFn() // removes the fees from the store as fees are now paid k.DeleteFeesInEscrow(ctx, packetID) - - // NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context. - ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events()) - } // distributePacketFeeOnAcknowledgement pays the receive fee for a given packetID while refunding the timeout fee to the refund account associated with the Fee. @@ -105,7 +104,7 @@ func (k Keeper) distributePacketFeeOnAcknowledgement(ctx sdk.Context, refundAddr } // DistributePacketsFeesOnTimeout pays all the timeout fees for a given packetID while refunding the acknowledgement & receive fees to the refund account. -func (k Keeper) DistributePacketFeesOnTimeout(ctx sdk.Context, timeoutRelayer sdk.AccAddress, packetFees []types.PacketFee, packetID channeltypes.PacketId) error { +func (k Keeper) DistributePacketFeesOnTimeout(ctx sdk.Context, timeoutRelayer sdk.AccAddress, packetFees []types.PacketFee, packetID channeltypes.PacketId) { // cache context before trying to distribute fees // if the escrow account has insufficient balance then we want to avoid partially distributing fees cacheCtx, writeFn := ctx.CacheContext() @@ -119,8 +118,7 @@ func (k Keeper) DistributePacketFeesOnTimeout(ctx sdk.Context, timeoutRelayer sd // NOTE: we use the uncached context to lock the fee module so that the state changes from // locking the fee module are persisted k.lockFeeModule(ctx) - - return nil + return } // check if refundAcc address works @@ -132,16 +130,14 @@ func (k Keeper) DistributePacketFeesOnTimeout(ctx sdk.Context, timeoutRelayer sd k.distributePacketFeeOnTimeout(cacheCtx, refundAddr, timeoutRelayer, packetFee) } - // removing the fee from the store as the fee is now paid - k.DeleteFeesInEscrow(ctx, packetID) + // NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context. + ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events()) // write the cache writeFn() - // NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context. - ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events()) - - return nil + // removing the fee from the store as the fee is now paid + k.DeleteFeesInEscrow(ctx, packetID) } // distributePacketFeeOnTimeout pays the timeout fee to the timeout relayer and refunds the acknowledgement & receive fee.