fix: event caching for fee distribution

modules/apps/29-fee/ibc_module.go

@@ -219,21 +219,9 @@ func (im IBCModule) OnAcknowledgementPacket(
 		return im.app.OnAcknowledgementPacket(ctx, packet, ack.Result, relayer)
 	}
 
-	// cache context before trying to distribute the fee
-	cacheCtx, writeFn := ctx.CacheContext()
+	im.keeper.DistributeFee(ctx, identifiedPacketFee.RefundAddress, ack.ForwardRelayerAddress, relayer.String(), packetId)
 
-	forwardRelayer, _ := sdk.AccAddressFromBech32(ack.ForwardRelayerAddress)
-	refundAcc, _ := sdk.AccAddressFromBech32(identifiedPacketFee.RefundAddress)
-
-	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)
 }
 
@@ -257,19 +245,8 @@ func (im IBCModule) OnTimeoutPacket(
 		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.DistributeFeeTimeout(ctx, identifiedPacketFee.RefundAddress, relayer.String(), packetId)
 
-	// otherwise discard cache and call underlying callback
+	// call underlying callback
 	return im.app.OnTimeoutPacket(ctx, packet, relayer)
 }

modules/apps/29-fee/ibc_module_test.go

@@ -653,6 +653,7 @@ func (suite *FeeTestSuite) TestOnAcknowledgementPacket() {
 			tc.malleate()
 
 			err = cbs.OnAcknowledgementPacket(suite.chainA.GetContext(), packet, ack, suite.chainA.SenderAccount.GetAddress())
+			fmt.Println(err)
 
 			if tc.expPass {
 				suite.Require().NoError(err, "unexpected error for case: %s", tc.name)

modules/apps/29-fee/keeper/escrow.go

@@ -44,66 +44,143 @@ func (k Keeper) EscrowPacketFee(ctx sdk.Context, identifiedFee *types.Identified
 }
 
 // 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 {
+func (k Keeper) DistributeFee(ctx sdk.Context, refundAcc, forwardRelayer, reverseRelayer string, packetID *channeltypes.PacketId) error {
+	var packetIdErr error
+	var distributeFwdErr error
+	var distributeReverseErr error
+	var refundAccErr 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)
+		packetIdErr = sdkerrors.Wrapf(types.ErrFeeNotFound, "with channelID %s, sequence %d", packetID.ChannelId, packetID.Sequence)
 	}
 
-	// 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")
+	// cache context before trying to send to forward relayer
+	cacheCtx, writeFn := ctx.CacheContext()
+
+	fwd, err := sdk.AccAddressFromBech32(forwardRelayer)
+	if err == nil {
+		err = k.bankKeeper.SendCoinsFromModuleToAccount(cacheCtx, types.ModuleName, fwd, feeInEscrow.Fee.ReceiveFee)
+		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())
+		}
+	} else {
+		distributeFwdErr = sdkerrors.Wrap(err, "failed to send fee to forward relayer")
 	}
 
-	// send ack fee to reverse relayer
-	err = k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, reverseRelayer, feeInEscrow.Fee.AckFee)
-	if err != nil {
-		return sdkerrors.Wrap(err, "error sending fee to reverse relayer")
+	// cache context before trying to send to reverse relayer
+	cacheCtx, writeFn = ctx.CacheContext()
+
+	reverse, err := sdk.AccAddressFromBech32(reverseRelayer)
+	if err == nil {
+		err = k.bankKeeper.SendCoinsFromModuleToAccount(cacheCtx, types.ModuleName, reverse, feeInEscrow.Fee.AckFee)
+		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())
+		}
+	} else {
+		distributeReverseErr = sdkerrors.Wrap(err, "failed to send fee to reverse relayer")
 	}
 
-	// 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")
+	// cache context before trying to send timeout fee to refund address
+	cacheCtx, writeFn = ctx.CacheContext()
+
+	refund, err := sdk.AccAddressFromBech32(refundAcc)
+	if err == nil {
+		err = k.bankKeeper.SendCoinsFromModuleToAccount(cacheCtx, types.ModuleName, refund, feeInEscrow.Fee.TimeoutFee)
+		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())
+		}
+	} else {
+		refundAccErr = sdkerrors.Wrap(err, "refunding timeout fee")
 	}
 
 	// removes the fee from the store as fee is now paid
 	k.DeleteFeeInEscrow(ctx, packetID)
 
+	// check if there are any errors, otherwise return nil
+	if packetIdErr != nil || distributeFwdErr != nil || distributeReverseErr != nil || refundAccErr != nil {
+		return sdkerrors.Wrapf(types.ErrFeeDistribution, "error in distributing fee: %s %s %s %s", packetIdErr, distributeFwdErr, distributeReverseErr, refundAccErr)
+	}
 	return nil
 }
 
 // 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 {
+func (k Keeper) DistributeFeeTimeout(ctx sdk.Context, refundAcc, timeoutRelayer string, packetID *channeltypes.PacketId) error {
+	var packetIdErr error
+	var refundAccErr error
+	var distributeTimeoutErr 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)
+		packetIdErr = sdkerrors.Wrapf(types.ErrFeeNotFound, "for packetID %s", packetID)
 	}
 
-	// refund the receive fee
-	err := k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, refundAcc, feeInEscrow.Fee.ReceiveFee)
-	if err != nil {
-		return sdkerrors.Wrap(err, "error refunding receive fee")
-	}
+	// cache context before trying to refund the receive fee
+	cacheCtx, writeFn := ctx.CacheContext()
+
+	// check if refundAcc address works
+	refund, err := sdk.AccAddressFromBech32(refundAcc)
+	if err == nil {
+		// first try to refund receive fee
+		err = k.bankKeeper.SendCoinsFromModuleToAccount(cacheCtx, types.ModuleName, refund, feeInEscrow.Fee.ReceiveFee)
+		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())
+		} else {
+			// set refundAccErr to error resulting from failed refund
+			refundAccErr = sdkerrors.Wrap(err, "error refunding receive fee")
+		}
 
-	// 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")
+		// then try to refund ack fee
+		err = k.bankKeeper.SendCoinsFromModuleToAccount(cacheCtx, types.ModuleName, refund, feeInEscrow.Fee.AckFee)
+		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())
+		} else {
+			// set refundAccErr to error resulting from failed refund
+			refundAccErr = sdkerrors.Wrap(err, "error refunding ack fee")
+		}
+	} else {
+		refundAccErr = sdkerrors.Wrap(err, "failed to parse refund account address")
 	}
 
-	// pay the timeout fee to the timeout relayer
-	err = k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, timeoutRelayer, feeInEscrow.Fee.TimeoutFee)
+	// parse timeout relayer address
+	cacheCtx, writeFn = ctx.CacheContext()
+	timeout, err := sdk.AccAddressFromBech32(timeoutRelayer)
 	if err != nil {
-		return sdkerrors.Wrap(err, "error sending fee to timeout relayer")
+		err = k.bankKeeper.SendCoinsFromModuleToAccount(cacheCtx, types.ModuleName, timeout, feeInEscrow.Fee.TimeoutFee)
+		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())
+		}
+	} else {
+		distributeTimeoutErr = sdkerrors.Wrap(err, "error sending fee to timeout relayer")
 	}
 
 	// removes the fee from the store as fee is now paid
 	k.DeleteFeeInEscrow(ctx, packetID)
 
+	// check if there are any errors, otherwise return nil
+	if packetIdErr != nil || refundAccErr != nil || distributeTimeoutErr != nil {
+		return sdkerrors.Wrapf(types.ErrFeeDistribution, "error in distributing fee: %s %s %s", packetIdErr, refundAccErr, distributeTimeoutErr)
+	}
 	return nil
 }
 

modules/apps/29-fee/keeper/escrow_test.go

@@ -70,7 +70,11 @@ func (suite *KeeperTestSuite) TestEscrowPacketFee() {
 			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
@@ -146,7 +150,11 @@ func (suite *KeeperTestSuite) TestDistributeFee() {
 			receiveFee = validCoins2
 			timeoutFee = validCoins3
 			packetId = &channeltypes.PacketId{ChannelId: suite.path.EndpointA.ChannelID, PortId: transfertypes.PortID, Sequence: validSeq}
-			fee := types.Fee{receiveFee, ackFee, timeoutFee}
+			fee := types.Fee{
+				ReceiveFee: receiveFee,
+				AckFee:     ackFee,
+				TimeoutFee: timeoutFee,
+			}
 
 			// escrow the packet fee & store the fee in state
 			identifiedPacketFee := types.IdentifiedPacketFee{PacketId: packetId, Fee: fee, RefundAddress: refundAcc.String(), Relayers: []string{}}
@@ -159,7 +167,7 @@ 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)
+			err = suite.chainA.GetSimApp().IBCFeeKeeper.DistributeFee(suite.chainA.GetContext(), refundAcc.String(), forwardRelayer.String(), reverseRelayer.String(), packetId)
 
 			if tc.expPass {
 				suite.Require().NoError(err)
@@ -236,7 +244,11 @@ func (suite *KeeperTestSuite) TestDistributeTimeoutFee() {
 			receiveFee = validCoins2
 			timeoutFee = validCoins3
 			packetId = &channeltypes.PacketId{ChannelId: suite.path.EndpointA.ChannelID, PortId: transfertypes.PortID, Sequence: validSeq}
-			fee := types.Fee{receiveFee, ackFee, timeoutFee}
+			fee := types.Fee{
+				ReceiveFee: receiveFee,
+				AckFee:     ackFee,
+				TimeoutFee: timeoutFee,
+			}
 
 			// escrow the packet fee & store the fee in state
 			identifiedPacketFee := types.IdentifiedPacketFee{PacketId: packetId, Fee: fee, RefundAddress: refundAcc.String(), Relayers: []string{}}
@@ -248,7 +260,7 @@ func (suite *KeeperTestSuite) TestDistributeTimeoutFee() {
 			// refundAcc balance after escrow
 			refundAccBal := suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), refundAcc, sdk.DefaultBondDenom)
 
-			err = suite.chainA.GetSimApp().IBCFeeKeeper.DistributeFeeTimeout(suite.chainA.GetContext(), refundAcc, timeoutRelayer, packetId)
+			err = suite.chainA.GetSimApp().IBCFeeKeeper.DistributeFeeTimeout(suite.chainA.GetContext(), refundAcc.String(), timeoutRelayer.String(), packetId)
 
 			if tc.expPass {
 				suite.Require().NoError(err)
@@ -291,7 +303,11 @@ func (suite *KeeperTestSuite) TestRefundFeesOnChannel() {
 
 	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: receiveFee,
+			AckFee:     ackFee,
+			TimeoutFee: timeoutFee,
+		}
 
 		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 +317,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: receiveFee,
+		AckFee:     ackFee,
+		TimeoutFee: timeoutFee,
+	}
 
 	identifiedPacketFee := types.IdentifiedPacketFee{PacketId: packetId, Fee: fee, RefundAddress: refundAcc.String(), Relayers: []string{}}
 	suite.chainA.GetSimApp().IBCFeeKeeper.SetFeeEnabled(suite.chainA.GetContext(), transfertypes.PortID, "channel-1")
@@ -318,7 +338,11 @@ func (suite *KeeperTestSuite) TestRefundFeesOnChannel() {
 
 	// 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}
+	fee = types.Fee{
+		ReceiveFee: receiveFee,
+		AckFee:     ackFee,
+		TimeoutFee: timeoutFee,
+	}
 
 	identifiedPacketFee = types.IdentifiedPacketFee{PacketId: packetId, Fee: fee, RefundAddress: refundAcc.String(), Relayers: []string{}}
 	err = suite.chainA.GetSimApp().IBCFeeKeeper.EscrowPacketFee(suite.chainA.GetContext(), &identifiedPacketFee)

modules/apps/29-fee/types/errors.go

@@ -13,4 +13,5 @@ var (
 	ErrRelayersNotNil           = sdkerrors.Register(ModuleName, 6, "relayers must be nil. This feature is not supported")
 	ErrCounterpartyAddressEmpty = sdkerrors.Register(ModuleName, 7, "counterparty address must not be empty")
 	ErrFeeNotEnabled            = sdkerrors.Register(ModuleName, 8, "fee module is not enabled for this channel. If this error occurs after channel setup, fee module may not be enabled")
+	ErrFeeDistribution          = sdkerrors.Register(ModuleName, 9, "error on distributing packet fee")
 )