feat(transfer): move async decision and handling to the ibc module onrecv callback

modules/apps/transfer/ibc_module.go

@@ -178,39 +178,40 @@ func (IBCModule) OnChanCloseConfirm(
 // OnRecvPacket implements the IBCModule interface. A successful acknowledgement
 // is returned if the packet data is successfully decoded and the receive application
 // logic returns without error.
+// Return nil to signal that the acknowledgement will be written asynchronously.
 func (im IBCModule) OnRecvPacket(
 	ctx sdk.Context,
 	packet channeltypes.Packet,
 	relayer sdk.AccAddress,
 ) ibcexported.Acknowledgement {
 	ack := channeltypes.NewResultAcknowledgement([]byte{byte(1)})
+	var data types.FungibleTokenPacketDataV2
+	var ackErr error
 
-	data, ackErr := im.getICS20PacketData(ctx, packet.GetData(), packet.GetDestPort(), packet.GetDestChannel())
+	defer func() {
+		events.EmitOnRecvPacketEvent(ctx, data, ack, ackErr)
+	}()
+
+	data, ackErr = im.getICS20PacketData(ctx, packet.GetData(), packet.GetDestPort(), packet.GetDestChannel())
 	if ackErr != nil {
 		ackErr = errorsmod.Wrapf(ibcerrors.ErrInvalidType, ackErr.Error())
-		im.keeper.Logger(ctx).Error(fmt.Sprintf("%s sequence %d", ackErr.Error(), packet.Sequence))
 		ack = channeltypes.NewErrorAcknowledgement(ackErr)
+		im.keeper.Logger(ctx).Error(fmt.Sprintf("%s sequence %d", ackErr.Error(), packet.Sequence))
+		return ack
 	}
 
-	// only attempt the application logic if the packet data
-	// was successfully decoded
-	if ack.Success() {
-		async, err := im.keeper.OnRecvPacket(ctx, packet, data)
-		if err != nil {
-			ack = channeltypes.NewErrorAcknowledgement(err)
-			ackErr = err
-			im.keeper.Logger(ctx).Error(fmt.Sprintf("%s sequence %d", ackErr.Error(), packet.Sequence))
-		} else {
-			im.keeper.Logger(ctx).Info("successfully handled ICS-20 packet", "sequence", packet.Sequence)
-		}
-
-		if async {
-			// NOTE: acknowledgement will be written asynchronously
-			return nil
-		}
+	if ackErr = im.keeper.OnRecvPacket(ctx, packet, data); ackErr != nil {
+		ack = channeltypes.NewErrorAcknowledgement(ackErr)
+		im.keeper.Logger(ctx).Error(fmt.Sprintf("%s sequence %d", ackErr.Error(), packet.Sequence))
+		return ack
 	}
 
-	events.EmitOnRecvPacketEvent(ctx, data, ack, ackErr)
+	im.keeper.Logger(ctx).Info("successfully handled ICS-20 packet", "sequence", packet.Sequence)
+
+	if data.Forwarding != nil {
+		// NOTE: acknowledgement will be written asynchronously
+		return nil
+	}
 
 	// NOTE: acknowledgement will be written synchronously during IBC handler execution.
 	return ack

modules/apps/transfer/ibc_module_test.go

@@ -1,8 +1,10 @@
 package transfer_test
 
 import (
+	"encoding/json"
 	"errors"
 	"math"
+	"strconv"
 
 	sdkmath "cosmossdk.io/math"
 
@@ -12,6 +14,7 @@ import (
 	capabilitytypes "github.com/cosmos/ibc-go/modules/capability/types"
 	"github.com/cosmos/ibc-go/v8/modules/apps/transfer"
 	"github.com/cosmos/ibc-go/v8/modules/apps/transfer/types"
+	clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types"
 	connectiontypes "github.com/cosmos/ibc-go/v8/modules/core/03-connection/types"
 	channeltypes "github.com/cosmos/ibc-go/v8/modules/core/04-channel/types"
 	porttypes "github.com/cosmos/ibc-go/v8/modules/core/05-port/types"
@@ -273,6 +276,157 @@ func (suite *TransferTestSuite) TestOnChanOpenAck() {
 	}
 }
 
+func (suite *TransferTestSuite) TestOnRecvPacket() {
+	// This test suite mostly covers the top-level logic of the ibc module OnRecvPacket function
+	// The core logic is covered in keeper OnRecvPacket
+	var packet channeltypes.Packet
+	testCases := []struct {
+		name          string
+		malleate      func()
+		expAckSuccess bool
+		expAsyncAck   bool
+		eventErrorMsg string
+	}{
+		{
+			"success", func() {}, true, false, "",
+		},
+		{
+			"success: async aknowledgment with forwarding path",
+			func() {
+				packetData := types.NewFungibleTokenPacketDataV2(
+					[]types.Token{
+						{
+							Denom:  types.NewDenom(sdk.DefaultBondDenom),
+							Amount: sdkmath.NewInt(100).String(),
+						},
+					},
+					suite.chainA.SenderAccount.GetAddress().String(),
+					suite.chainB.SenderAccount.GetAddress().String(),
+					"",
+					types.NewForwarding("", types.Hop{PortId: "transfer", ChannelId: "channel-0"}),
+				)
+				packet.Data = packetData.GetBytes()
+			},
+			true,
+			true,
+			"",
+		},
+		{
+			"failure: invalid packet data bytes",
+			func() {
+				packet.Data = []byte("invalid data")
+			},
+			false,
+			false,
+			"cannot unmarshal ICS20-V2 transfer packet data: invalid character 'i' looking for beginning of value: invalid type: invalid type",
+		},
+		{
+			"failure: invalid token packet",
+			func() {
+				invalidPacket := types.FungibleTokenPacketDataV2{}
+				packet.Data = invalidPacket.GetBytes()
+			},
+			false,
+			false,
+			"sender address cannot be blank: invalid address: invalid type",
+		},
+	}
+
+	for _, tc := range testCases {
+		suite.Run(tc.name, func() {
+			suite.SetupTest() // reset
+
+			path := ibctesting.NewTransferPath(suite.chainA, suite.chainB)
+			path.Setup()
+
+			packetData := types.NewFungibleTokenPacketDataV2(
+				[]types.Token{
+					{
+						Denom:  types.NewDenom(sdk.DefaultBondDenom),
+						Amount: sdkmath.NewInt(100).String(),
+					},
+				},
+				suite.chainA.SenderAccount.GetAddress().String(),
+				suite.chainB.SenderAccount.GetAddress().String(),
+				"",
+				nil,
+			)
+
+			seq := uint64(1)
+			packet = channeltypes.NewPacket(packetData.GetBytes(), seq, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, clienttypes.NewHeight(1, 100), 0)
+
+			module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), ibctesting.TransferPort)
+			suite.Require().NoError(err)
+
+			cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(module)
+			suite.Require().True(ok)
+
+			tc.malleate() // change fields in packet
+
+			ctx := suite.chainA.GetContext()
+			ack := cbs.OnRecvPacket(ctx, packet, suite.chainA.SenderAccount.GetAddress())
+
+			if tc.expAckSuccess {
+				if tc.expAsyncAck {
+					suite.Require().Nil(ack)
+				} else {
+					suite.Require().True(ack.Success())
+
+					expectedAck := channeltypes.NewResultAcknowledgement([]byte{byte(1)})
+					suite.Require().Equal(expectedAck, ack)
+				}
+
+				tokensBz, err := json.Marshal(packetData.Tokens)
+				suite.Require().NoError(err)
+
+				// In 6585 this needs to change to account for forwarded packets
+				expectedAttributes := []sdk.Attribute{
+					sdk.NewAttribute(types.AttributeKeySender, packetData.Sender),
+					sdk.NewAttribute(types.AttributeKeyReceiver, packetData.Receiver),
+					sdk.NewAttribute(types.AttributeKeyTokens, string(tokensBz)),
+					sdk.NewAttribute(types.AttributeKeyMemo, packetData.Memo),
+					sdk.NewAttribute(types.AttributeKeyAckSuccess, strconv.FormatBool(tc.expAckSuccess)),
+				}
+
+				expectedEvents := sdk.Events{
+					sdk.NewEvent(
+						types.EventTypePacket,
+						expectedAttributes...,
+					),
+				}.ToABCIEvents()
+
+				expectedEvents = sdk.MarkEventsToIndex(expectedEvents, map[string]struct{}{})
+				ibctesting.AssertEvents(&suite.Suite, expectedEvents, ctx.EventManager().Events().ToABCIEvents())
+			} else {
+				if tc.expAsyncAck {
+					suite.Require().Nil(ack)
+				} else {
+					suite.Require().False(ack.Success())
+				}
+
+				expectedAttributes := []sdk.Attribute{
+					sdk.NewAttribute(types.AttributeKeySender, ""),
+					sdk.NewAttribute(types.AttributeKeyReceiver, ""),
+					sdk.NewAttribute(types.AttributeKeyTokens, "null"),
+					sdk.NewAttribute(types.AttributeKeyMemo, ""),
+					sdk.NewAttribute(types.AttributeKeyAckSuccess, strconv.FormatBool(tc.expAckSuccess)),
+					sdk.NewAttribute(types.AttributeKeyAckError, tc.eventErrorMsg),
+				}
+
+				expectedEvents := sdk.Events{
+					sdk.NewEvent(
+						types.EventTypePacket,
+						expectedAttributes...,
+					),
+				}.ToABCIEvents()
+
+				expectedEvents = sdk.MarkEventsToIndex(expectedEvents, map[string]struct{}{})
+				ibctesting.AssertEvents(&suite.Suite, expectedEvents, ctx.EventManager().Events().ToABCIEvents())
+			}
+		})
+	}
+}
+
 func (suite *TransferTestSuite) TestOnTimeoutPacket() {
 	var path *ibctesting.Path
 	var packet channeltypes.Packet

modules/apps/transfer/internal/events/events.go

@@ -31,7 +31,7 @@ func EmitTransferEvent(ctx sdk.Context, sender, receiver string, tokens types.To
 
 // EmitOnRecvPacketEvent emits a fungible token packet event in the OnRecvPacket callback
 func EmitOnRecvPacketEvent(ctx sdk.Context, packetData types.FungibleTokenPacketDataV2, ack channeltypes.Acknowledgement, ackErr error) {
-	jsonTokens := mustMarshalType[types.Tokens](types.Tokens(packetData.Tokens))
+	jsonTokens := mustMarshalType[types.Tokens](packetData.Tokens)
 
 	eventAttributes := []sdk.Attribute{
 		sdk.NewAttribute(types.AttributeKeySender, packetData.Sender),

modules/apps/transfer/keeper/mbt_relay_test.go

@@ -365,9 +365,7 @@ func (suite *KeeperTestSuite) TestModelBasedRelay() {
 
 					}
 				case "OnRecvPacket":
-					var async bool
-					async, err = suite.chainB.GetSimApp().TransferKeeper.OnRecvPacket(suite.chainB.GetContext(), packet, tc.packet.Data)
-					suite.Require().False(async)
+					err = suite.chainB.GetSimApp().TransferKeeper.OnRecvPacket(suite.chainB.GetContext(), packet, tc.packet.Data)
 
 				case "OnTimeoutPacket":
 					registerDenomFn()

modules/apps/transfer/keeper/relay.go

@@ -159,19 +159,19 @@ func (k Keeper) sendTransfer(
 // and sent to the receiving address. Otherwise if the sender chain is sending
 // back tokens this chain originally transferred to it, the tokens are
 // unescrowed and sent to the receiving address.
-func (k Keeper) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet, data types.FungibleTokenPacketDataV2) (bool, error) {
+func (k Keeper) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet, data types.FungibleTokenPacketDataV2) error {
 	// validate packet data upon receiving
 	if err := data.ValidateBasic(); err != nil {
-		return false, errorsmod.Wrapf(err, "error validating ICS-20 transfer packet data")
+		return errorsmod.Wrapf(err, "error validating ICS-20 transfer packet data")
 	}
 
 	if !k.GetParams(ctx).ReceiveEnabled {
-		return false, types.ErrReceiveDisabled
+		return types.ErrReceiveDisabled
 	}
 
 	receiver, err := getReceiverFromPacketData(data, packet.DestinationPort, packet.DestinationChannel)
 	if err != nil {
-		return false, err
+		return err
 	}
 
 	var receivedCoins sdk.Coins
@@ -184,7 +184,7 @@ func (k Keeper) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet, data t
 		// parse the transfer amount
 		transferAmount, ok := sdkmath.NewIntFromString(token.Amount)
 		if !ok {
-			return false, errorsmod.Wrapf(types.ErrInvalidAmount, "unable to parse transfer amount: %s", token.Amount)
+			return errorsmod.Wrapf(types.ErrInvalidAmount, "unable to parse transfer amount: %s", token.Amount)
 		}
 
 		// This is the prefix that would have been prefixed to the denomination
@@ -203,12 +203,12 @@ func (k Keeper) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet, data t
 			coin := sdk.NewCoin(token.Denom.IBCDenom(), transferAmount)
 
 			if k.bankKeeper.BlockedAddr(receiver) {
-				return false, errorsmod.Wrapf(ibcerrors.ErrUnauthorized, "%s is not allowed to receive funds", receiver)
+				return errorsmod.Wrapf(ibcerrors.ErrUnauthorized, "%s is not allowed to receive funds", receiver)
 			}
 
 			escrowAddress := types.GetEscrowAddress(packet.GetDestPort(), packet.GetDestChannel())
 			if err := k.unescrowCoin(ctx, escrowAddress, receiver, coin); err != nil {
-				return false, err
+				return err
 			}
 
 			denomPath := token.Denom.Path()
@@ -245,14 +245,14 @@ func (k Keeper) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet, data t
 		if err := k.bankKeeper.MintCoins(
 			ctx, types.ModuleName, sdk.NewCoins(voucher),
 		); err != nil {
-			return false, errorsmod.Wrap(err, "failed to mint IBC tokens")
+			return errorsmod.Wrap(err, "failed to mint IBC tokens")
 		}
 
 		// send to receiver
 		if err := k.bankKeeper.SendCoinsFromModuleToAccount(
 			ctx, types.ModuleName, receiver, sdk.NewCoins(voucher),
 		); err != nil {
-			return false, errorsmod.Wrapf(err, "failed to send coins to receiver %s", receiver.String())
+			return errorsmod.Wrapf(err, "failed to send coins to receiver %s", receiver.String())
 		}
 
 		denomPath := token.Denom.Path()
@@ -265,13 +265,12 @@ func (k Keeper) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet, data t
 	if data.ShouldBeForwarded() {
 		// we are now sending from the forward escrow address to the final receiver address.
 		if err := k.forwardPacket(ctx, data, packet, receivedCoins); err != nil {
-			return false, err
+			return err
 		}
-		return true, nil
 	}
 
 	// The ibc_module.go module will return the proper ack.
-	return false, nil
+	return nil
 }
 
 // OnAcknowledgementPacket either reverts the state changes executed in receive

modules/apps/transfer/keeper/relay_forwarding_test.go

@@ -212,10 +212,8 @@ func (suite *KeeperTestSuite) TestHappyPathForwarding() {
 		}, sender.GetAddress().String(), receiver.GetAddress().String(), "", &forwarding)
 	packetRecv := channeltypes.NewPacket(data.GetBytes(), 2, path1.EndpointA.ChannelConfig.PortID, path1.EndpointA.ChannelID, path1.EndpointB.ChannelConfig.PortID, path1.EndpointB.ChannelID, clienttypes.NewHeight(1, 100), 0)
 
-	var async bool
-	async, err = suite.chainB.GetSimApp().TransferKeeper.OnRecvPacket(suite.chainB.GetContext(), packetRecv, data)
+	err = suite.chainB.GetSimApp().TransferKeeper.OnRecvPacket(suite.chainB.GetContext(), packetRecv, data)
 	// If forwarding has been triggered then the async must be true.
-	suite.Require().True(async)
 	suite.Require().Nil(err)
 
 	// denomTrace path: transfer/channel-0
@@ -248,8 +246,7 @@ func (suite *KeeperTestSuite) TestHappyPathForwarding() {
 	packetRecv = channeltypes.NewPacket(data.GetBytes(), 3, path2.EndpointB.ChannelConfig.PortID, path2.EndpointB.ChannelID, path2.EndpointA.ChannelConfig.PortID, path2.EndpointA.ChannelID, clienttypes.NewHeight(1, 100), 0)
 
 	// execute onRecvPacket, when chaninA receives the tokens the escrow amount on B should increase to amount
-	async, err = suite.chainA.GetSimApp().TransferKeeper.OnRecvPacket(suite.chainA.GetContext(), packetRecv, data)
-	suite.Require().False(async)
+	err = suite.chainA.GetSimApp().TransferKeeper.OnRecvPacket(suite.chainA.GetContext(), packetRecv, data)
 	suite.Require().NoError(err)
 
 	// Check that the final receiver has received the expected tokens.

modules/apps/transfer/keeper/relay_test.go

@@ -384,9 +384,8 @@ func (suite *KeeperTestSuite) TestOnRecvPacket_ReceiverIsNotSource() {
 				}, suite.chainA.SenderAccount.GetAddress().String(), receiver, memo, nil)
 			packet := channeltypes.NewPacket(data.GetBytes(), seq, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, clienttypes.NewHeight(1, 100), 0)
 
-			var async bool
-			async, err = suite.chainB.GetSimApp().TransferKeeper.OnRecvPacket(suite.chainB.GetContext(), packet, data)
-			suite.Require().False(async)
+			err = suite.chainB.GetSimApp().TransferKeeper.OnRecvPacket(suite.chainB.GetContext(), packet, data)
+
 			// check total amount in escrow of received token denom on receiving chain
 			totalEscrow := suite.chainB.GetSimApp().TransferKeeper.GetTotalEscrowForDenom(suite.chainB.GetContext(), sdk.DefaultBondDenom)
 			suite.Require().Equal(expEscrowAmount, totalEscrow.Amount)
@@ -536,7 +535,7 @@ func (suite *KeeperTestSuite) TestOnRecvPacket_ReceiverIsSource() {
 				}, suite.chainA.SenderAccount.GetAddress().String(), receiver, memo, nil)
 			packet = channeltypes.NewPacket(data.GetBytes(), seq, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, clienttypes.NewHeight(1, 100), 0)
 
-			_, err = suite.chainB.GetSimApp().TransferKeeper.OnRecvPacket(suite.chainB.GetContext(), packet, data)
+			err = suite.chainB.GetSimApp().TransferKeeper.OnRecvPacket(suite.chainB.GetContext(), packet, data)
 
 			// check total amount in escrow of received token denom on receiving chain
 			totalEscrow := suite.chainB.GetSimApp().TransferKeeper.GetTotalEscrowForDenom(suite.chainB.GetContext(), sdk.DefaultBondDenom)
@@ -645,9 +644,7 @@ func (suite *KeeperTestSuite) TestOnRecvPacketSetsTotalEscrowAmountForSourceIBCT
 	suite.Require().Equal(sdkmath.NewInt(100), totalEscrowChainB.Amount)
 
 	// execute onRecvPacket, when chaninB receives the source token the escrow amount should decrease
-	var async bool
-	async, err := suite.chainB.GetSimApp().TransferKeeper.OnRecvPacket(suite.chainB.GetContext(), packet, data)
-	suite.Require().False(async)
+	err := suite.chainB.GetSimApp().TransferKeeper.OnRecvPacket(suite.chainB.GetContext(), packet, data)
 	suite.Require().NoError(err)
 
 	// check total amount in escrow of sent token on receiving chain