Use global success value in acknowledgment

modules/apps/transfer/ibc_module.go

@@ -230,7 +230,8 @@ func (im IBCModule) OnAcknowledgementPacket(
 		return err
 	}
 
-	events.EmitOnAcknowledgementPacketEvent(ctx, data, ack)
+	recvSuccess := ack.Success()
+	events.EmitOnAcknowledgementPacketEvent(ctx, data, recvSuccess, ack)
 
 	return nil
 }

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

@@ -66,7 +66,7 @@ func EmitOnRecvPacketEvent(ctx context.Context, packetData types.FungibleTokenPa
 }
 
 // EmitOnAcknowledgementPacketEvent emits a fungible token packet event in the OnAcknowledgementPacket callback
-func EmitOnAcknowledgementPacketEvent(ctx context.Context, packetData types.FungibleTokenPacketDataV2, ack channeltypes.Acknowledgement) {
+func EmitOnAcknowledgementPacketEvent(ctx context.Context, packetData types.FungibleTokenPacketDataV2, recvSuccess bool, ack channeltypes.Acknowledgement) {
 	tokensStr := mustMarshalJSON(packetData.Tokens)
 	forwardingHopsStr := mustMarshalJSON(packetData.Forwarding.Hops)
 	sdkCtx := sdk.UnwrapSDKContext(ctx) // TODO: https://github.com/cosmos/ibc-go/issues/5917
@@ -78,6 +78,7 @@ func EmitOnAcknowledgementPacketEvent(ctx context.Context, packetData types.Fung
 			sdk.NewAttribute(types.AttributeKeyTokens, tokensStr),
 			sdk.NewAttribute(types.AttributeKeyMemo, packetData.Memo),
 			sdk.NewAttribute(types.AttributeKeyForwardingHops, forwardingHopsStr),
+			sdk.NewAttribute(types.AttributeKeyRecvSuccess, strconv.FormatBool(recvSuccess)),
 			sdk.NewAttribute(types.AttributeKeyAck, ack.String()),
 		),
 		sdk.NewEvent(

modules/apps/transfer/types/events.go

@@ -16,6 +16,7 @@ const (
 	AttributeKeyRefundReceiver = "refund_receiver"
 	AttributeKeyRefundTokens   = "refund_tokens"
 	AttributeKeyAckSuccess     = "success"
+	AttributeKeyRecvSuccess    = "recv_success"
 	AttributeKeyAck            = "acknowledgement"
 	AttributeKeyAckError       = "error"
 	AttributeKeyMemo           = "memo"

modules/apps/transfer/v2/ibc_module.go

@@ -113,7 +113,7 @@ func (im *IBCModule) OnTimeoutPacket(ctx context.Context, sourceChannel string,
 	return nil
 }
 
-func (im *IBCModule) OnAcknowledgementPacket(ctx context.Context, sourceChannel string, destinationChannel string, sequence uint64, acknowledgement []byte, payload types.Payload, relayer sdk.AccAddress) error {
+func (im *IBCModule) OnAcknowledgementPacket(ctx context.Context, sourceChannel string, destinationChannel string, sequence uint64, recvSuccess bool, acknowledgement []byte, payload types.Payload, relayer sdk.AccAddress) error {
 	var ack channeltypes.Acknowledgement
 	if err := transfertypes.ModuleCdc.UnmarshalJSON(acknowledgement, &ack); err != nil {
 		return errorsmod.Wrapf(ibcerrors.ErrUnknownRequest, "cannot unmarshal ICS-20 transfer packet acknowledgement: %v", err)
@@ -124,10 +124,10 @@ func (im *IBCModule) OnAcknowledgementPacket(ctx context.Context, sourceChannel
 		return err
 	}
 
-	if err := im.keeper.OnAcknowledgementPacket(ctx, payload.SourcePort, sourceChannel, data, ack); err != nil {
+	if err := im.keeper.OnAcknowledgementPacket(ctx, payload.SourcePort, sourceChannel, data, recvSuccess); err != nil {
 		return err
 	}
 
-	events.EmitOnAcknowledgementPacketEvent(ctx, data, ack)
+	events.EmitOnAcknowledgementPacketEvent(ctx, data, recvSuccess, ack)
 	return nil
 }

modules/apps/transfer/v2/keeper/keeper.go

@@ -12,7 +12,6 @@ import (
 	"github.com/cosmos/ibc-go/v9/modules/apps/transfer/internal/events"
 	transferkeeper "github.com/cosmos/ibc-go/v9/modules/apps/transfer/keeper"
 	"github.com/cosmos/ibc-go/v9/modules/apps/transfer/types"
-	channeltypes "github.com/cosmos/ibc-go/v9/modules/core/04-channel/types"
 	channelkeeperv2 "github.com/cosmos/ibc-go/v9/modules/core/04-channel/v2/keeper"
 	channeltypesv2 "github.com/cosmos/ibc-go/v9/modules/core/04-channel/v2/types"
 	ibcerrors "github.com/cosmos/ibc-go/v9/modules/core/errors"
@@ -186,18 +185,12 @@ func (k *Keeper) OnRecvPacket(ctx context.Context, sourceChannel, destChannel st
 	return nil
 }
 
-func (k *Keeper) OnAcknowledgementPacket(ctx context.Context, sourcePort, sourceChannel string, data types.FungibleTokenPacketDataV2, ack channeltypes.Acknowledgement) error {
-	switch ack.Response.(type) {
-	case *channeltypes.Acknowledgement_Result:
-		// the acknowledgement succeeded on the receiving chain so nothing
-		// needs to be executed and no error needs to be returned
-		return nil
-	case *channeltypes.Acknowledgement_Error:
-		// We refund the tokens from the escrow address to the sender
+func (k *Keeper) OnAcknowledgementPacket(ctx context.Context, sourcePort, sourceChannel string, data types.FungibleTokenPacketDataV2, recvSuccess bool) error {
+	if !recvSuccess {
+		// refund the tokens back to the sender
 		return k.refundPacketTokens(ctx, sourcePort, sourceChannel, data)
-	default:
-		return errorsmod.Wrapf(ibcerrors.ErrInvalidType, "expected one of [%T, %T], got %T", channeltypes.Acknowledgement_Result{}, channeltypes.Acknowledgement_Error{}, ack.Response)
 	}
+	return nil
 }
 
 func (k *Keeper) OnTimeoutPacket(ctx context.Context, sourcePort, sourceChannel string, data types.FungibleTokenPacketDataV2) error {

modules/apps/transfer/v2/keeper/msg_server_test.go

@@ -190,6 +190,7 @@ func (suite *KeeperTestSuite) TestMsgRecvPacketTransfer() {
 			"failure: receive is disabled",
 			func() {},
 			func() {
+				expectedAck.RecvSuccess = false
 				expectedAck.AppAcknowledgements[0] = channeltypes.NewErrorAcknowledgement(transfertypes.ErrReceiveDisabled).Acknowledgement()
 				suite.chainB.GetSimApp().TransferKeeperV2.SetParams(suite.chainB.GetContext(),
 					transfertypes.Params{
@@ -232,7 +233,7 @@ func (suite *KeeperTestSuite) TestMsgRecvPacketTransfer() {
 
 			// by default, we assume a successful acknowledgement will be written.
 			ackBytes := channeltypes.NewResultAcknowledgement([]byte{byte(1)}).Acknowledgement()
-			expectedAck = channeltypesv2.Acknowledgement{AppAcknowledgements: [][]byte{ackBytes}}
+			expectedAck = channeltypesv2.Acknowledgement{RecvSuccess: true, AppAcknowledgements: [][]byte{ackBytes}}
 			tc.malleate()
 
 			err = path.EndpointB.MsgRecvPacket(packet)
@@ -296,6 +297,7 @@ func (suite *KeeperTestSuite) TestMsgAckPacketTransfer() {
 		{
 			"failure: proof verification failure",
 			func() {
+				expectedAck.RecvSuccess = false
 				expectedAck.AppAcknowledgements[0] = mockv2.MockFailRecvPacketResult.Acknowledgement
 			},
 			commitmenttypes.ErrInvalidProof,
@@ -304,6 +306,7 @@ func (suite *KeeperTestSuite) TestMsgAckPacketTransfer() {
 		{
 			"failure: escrowed tokens are refunded",
 			func() {
+				expectedAck.RecvSuccess = false
 				expectedAck.AppAcknowledgements[0] = channeltypes.NewErrorAcknowledgement(transfertypes.ErrReceiveDisabled).Acknowledgement()
 			},
 			nil,
@@ -353,7 +356,7 @@ func (suite *KeeperTestSuite) TestMsgAckPacketTransfer() {
 			suite.Require().NoError(err)
 
 			ackBytes := channeltypes.NewResultAcknowledgement([]byte{byte(1)}).Acknowledgement()
-			expectedAck = channeltypesv2.Acknowledgement{AppAcknowledgements: [][]byte{ackBytes}}
+			expectedAck = channeltypesv2.Acknowledgement{RecvSuccess: true, AppAcknowledgements: [][]byte{ackBytes}}
 			tc.malleate()
 
 			err = path.EndpointA.MsgAcknowledgePacket(packet, expectedAck)
@@ -362,7 +365,7 @@ func (suite *KeeperTestSuite) TestMsgAckPacketTransfer() {
 			if expPass {
 				suite.Require().NoError(err)
 
-				if bytes.Equal(expectedAck.AppAcknowledgements[0], ackBytes) {
+				if expectedAck.RecvSuccess {
 					// tokens remain escrowed
 					for _, t := range tokens {
 						escrowedAmount := suite.chainA.GetSimApp().TransferKeeperV2.GetTotalEscrowForDenom(suite.chainA.GetContext(), t.Denom.IBCDenom())
@@ -505,7 +508,7 @@ func (suite *KeeperTestSuite) TestV2RetainsFungibility() {
 	}
 
 	ackBytes := channeltypes.NewResultAcknowledgement([]byte{byte(1)}).Acknowledgement()
-	successfulAck := channeltypesv2.Acknowledgement{AppAcknowledgements: [][]byte{ackBytes}}
+	successfulAck := channeltypesv2.Acknowledgement{RecvSuccess: true, AppAcknowledgements: [][]byte{ackBytes}}
 
 	originalAmount, ok := sdkmath.NewIntFromString(ibctesting.DefaultGenesisAccBalance)
 	suite.Require().True(ok)

modules/core/04-channel/v2/keeper/msg_server.go

@@ -131,20 +131,20 @@ func (k *Keeper) RecvPacket(ctx context.Context, msg *types.MsgRecvPacket) (*typ
 
 	// build up the recv results for each application callback.
 	ack := types.Acknowledgement{
+		RecvSuccess:         true,
 		AppAcknowledgements: [][]byte{},
 	}
 
 	var isAsync bool
+	cacheCtx, writeFn = sdkCtx.CacheContext()
 	for _, pd := range msg.Packet.Payloads {
 		// Cache context so that we may discard state changes from callback if the acknowledgement is unsuccessful.
-		cacheCtx, writeFn = sdkCtx.CacheContext()
 		cb := k.Router.Route(pd.DestinationPort)
 		res := cb.OnRecvPacket(cacheCtx, msg.Packet.SourceChannel, msg.Packet.DestinationChannel, msg.Packet.Sequence, pd, signer)
 
-		if res.Status != types.PacketStatus_Failure {
-			// write application state changes for asynchronous and successful acknowledgements
-			writeFn()
-		} else {
+		if res.Status == types.PacketStatus_Failure {
+			// Set RecvSuccess to false
+			ack.RecvSuccess = false
 			// Modify events in cached context to reflect unsuccessful acknowledgement
 			sdkCtx.EventManager().EmitEvents(internalerrors.ConvertToErrorEvents(cacheCtx.EventManager().Events()))
 		}
@@ -157,19 +157,27 @@ func (k *Keeper) RecvPacket(ctx context.Context, msg *types.MsgRecvPacket) (*typ
 			if len(msg.Packet.Payloads) > 1 {
 				return nil, errorsmod.Wrapf(types.ErrInvalidPacket, "packet with multiple payloads cannot have async acknowledgement")
 			}
+			// break out of the loop if the acknowledgement is async
+			// to prevent the addition of nil to the acknowledgement
+			break
 		}
 
 		// append app acknowledgement to the overall acknowledgement
 		ack.AppAcknowledgements = append(ack.AppAcknowledgements, res.Acknowledgement)
 	}
 
-	// note this should never happen as the payload would have had to be empty.
-	if len(ack.AppAcknowledgements) == 0 {
-		sdkCtx.Logger().Error("receive packet failed", "source-channel", msg.Packet.SourceChannel, "error", errorsmod.Wrap(err, "invalid acknowledgement results"))
-		return &types.MsgRecvPacketResponse{Result: types.FAILURE}, errorsmod.Wrapf(err, "receive packet failed source-channel %s invalid acknowledgement results", msg.Packet.SourceChannel)
+	if ack.RecvSuccess {
+		// write application state changes for successful acknowledgements
+		writeFn()
 	}
 
 	if !isAsync {
+		// note this should never happen as the payload would have had to be empty.
+		if len(ack.AppAcknowledgements) == 0 {
+			sdkCtx.Logger().Error("receive packet failed", "source-channel", msg.Packet.SourceChannel, "error", errorsmod.Wrap(err, "invalid acknowledgement results"))
+			return &types.MsgRecvPacketResponse{Result: types.FAILURE}, errorsmod.Wrapf(err, "receive packet failed source-channel %s invalid acknowledgement results", msg.Packet.SourceChannel)
+		}
+
 		// Set packet acknowledgement only if the acknowledgement is not async.
 		// NOTE: IBC applications modules may call the WriteAcknowledgement asynchronously if the
 		// acknowledgement is async.
@@ -212,7 +220,7 @@ func (k *Keeper) Acknowledgement(ctx context.Context, msg *types.MsgAcknowledgem
 	for i, pd := range msg.Packet.Payloads {
 		cbs := k.Router.Route(pd.SourcePort)
 		ack := msg.Acknowledgement.AppAcknowledgements[i]
-		err := cbs.OnAcknowledgementPacket(ctx, msg.Packet.SourceChannel, msg.Packet.DestinationChannel, msg.Packet.Sequence, ack, pd, relayer)
+		err := cbs.OnAcknowledgementPacket(ctx, msg.Packet.SourceChannel, msg.Packet.DestinationChannel, msg.Packet.Sequence, msg.Acknowledgement.RecvSuccess, ack, pd, relayer)
 		if err != nil {
 			return nil, errorsmod.Wrapf(err, "failed OnAcknowledgementPacket for source port %s, source channel %s, destination channel %s", pd.SourcePort, msg.Packet.SourceChannel, msg.Packet.DestinationChannel)
 		}

modules/core/04-channel/v2/keeper/msg_server_test.go

@@ -313,7 +313,10 @@ func (suite *KeeperTestSuite) TestMsgRecvPacket() {
 			tc.malleate()
 
 			// expectedAck is derived from the expected recv result.
-			expectedAck := types.Acknowledgement{AppAcknowledgements: [][]byte{expRecvRes.Acknowledgement}}
+			expectedAck := types.Acknowledgement{
+				RecvSuccess:         expRecvRes.Status == types.PacketStatus_Success,
+				AppAcknowledgements: [][]byte{expRecvRes.Acknowledgement},
+			}
 
 			// modify the callback to return the expected recv result.
 			path.EndpointB.Chain.GetSimApp().MockModuleV2B.IBCApp.OnRecvPacket = func(ctx context.Context, sourceChannel string, destinationChannel string, sequence uint64, data types.Payload, relayer sdk.AccAddress) types.RecvPacketResult {
@@ -377,15 +380,15 @@ func (suite *KeeperTestSuite) TestMsgAcknowledgement() {
 
 				// Modify the callback to return an error.
 				// This way, we can verify that the callback is not executed in a No-op case.
-				path.EndpointA.Chain.GetSimApp().MockModuleV2A.IBCApp.OnAcknowledgementPacket = func(context.Context, string, string, uint64, types.Payload, []byte, sdk.AccAddress) error {
+				path.EndpointA.Chain.GetSimApp().MockModuleV2A.IBCApp.OnAcknowledgementPacket = func(context.Context, string, string, uint64, types.Payload, bool, []byte, sdk.AccAddress) error {
 					return mock.MockApplicationCallbackError
 				}
 			},
 		},
 		{
 			name: "failure: callback fails",
 			malleate: func() {
-				path.EndpointA.Chain.GetSimApp().MockModuleV2A.IBCApp.OnAcknowledgementPacket = func(context.Context, string, string, uint64, types.Payload, []byte, sdk.AccAddress) error {
+				path.EndpointA.Chain.GetSimApp().MockModuleV2A.IBCApp.OnAcknowledgementPacket = func(context.Context, string, string, uint64, types.Payload, bool, []byte, sdk.AccAddress) error {
 					return mock.MockApplicationCallbackError
 				}
 			},
@@ -432,7 +435,7 @@ func (suite *KeeperTestSuite) TestMsgAcknowledgement() {
 			suite.Require().NoError(err)
 
 			// Construct expected acknowledgement
-			ack = types.Acknowledgement{AppAcknowledgements: [][]byte{mockv2.MockRecvPacketResult.Acknowledgement}}
+			ack = types.Acknowledgement{RecvSuccess: true, AppAcknowledgements: [][]byte{mockv2.MockRecvPacketResult.Acknowledgement}}
 
 			tc.malleate()
 

modules/core/04-channel/v2/keeper/packet_test.go

@@ -307,6 +307,7 @@ func (suite *KeeperTestSuite) TestWriteAcknowledgement() {
 
 			// create standard ack that can be malleated
 			ack = types.Acknowledgement{
+				RecvSuccess:         true,
 				AppAcknowledgements: [][]byte{mockv2.MockRecvPacketResult.Acknowledgement},
 			}
 
@@ -335,6 +336,7 @@ func (suite *KeeperTestSuite) TestAcknowledgePacket() {
 		packet types.Packet
 		err    error
 		ack    = types.Acknowledgement{
+			RecvSuccess:         true,
 			AppAcknowledgements: [][]byte{mockv2.MockRecvPacketResult.Acknowledgement},
 		}
 		freezeClient bool

modules/core/04-channel/v2/types/commitment.go

@@ -59,6 +59,11 @@ func CommitAcknowledgement(acknowledgement Acknowledgement) []byte {
 		buf = append(buf, hash[:]...)
 	}
 
+	if acknowledgement.RecvSuccess {
+		buf = append([]byte{byte(1)}, buf...)
+	} else {
+		buf = append([]byte{byte(0)}, buf...)
+	}
 	buf = append([]byte{byte(2)}, buf...)
 
 	hash := sha256.Sum256(buf)

modules/core/04-channel/v2/types/commitment_test.go

@@ -84,11 +84,16 @@ func TestCommitPacket(t *testing.T) {
 // same commitment output. But it is also useful to catch any changes in the commitment.
 func TestCommitAcknowledgement(t *testing.T) {
 	ack := types.Acknowledgement{
+		RecvSuccess: true,
 		AppAcknowledgements: [][]byte{
 			[]byte("some bytes"),
 		},
 	}
 
 	commitment := types.CommitAcknowledgement(ack)
-	require.Equal(t, "f03b4667413e56aaf086663267913e525c442b56fa1af4fa3f3dab9f37044c5b", hex.EncodeToString(commitment))
+	require.Equal(t, "fc02a4453c297c9b65189ec354f4fc7f0c1327b72f6044a20d4dd1fac8fda9f7", hex.EncodeToString(commitment))
+
+	ack.RecvSuccess = false
+	commitment = types.CommitAcknowledgement(ack)
+	require.Equal(t, "47a3b131712a356465258d5a9f50340f990a37b14e665b49ea5afa170f5e7aac", hex.EncodeToString(commitment))
 }

modules/core/04-channel/v2/types/msgs.go

@@ -174,7 +174,11 @@ func (msg *MsgAcknowledgement) ValidateBasic() error {
 		return errorsmod.Wrapf(ibcerrors.ErrInvalidAddress, "string could not be parsed as address: %v", err)
 	}
 
-	return msg.Packet.ValidateBasic()
+	if err := msg.Packet.ValidateBasic(); err != nil {
+		return err
+	}
+
+	return msg.Acknowledgement.ValidateBasic()
 }
 
 // NewMsgTimeout creates a new MsgTimeout instance

modules/core/04-channel/v2/types/msgs_test.go

@@ -322,12 +322,19 @@ func (s *TypesTestSuite) TestMsgAcknowledge_ValidateBasic() {
 			},
 			expError: types.ErrInvalidPacket,
 		},
+		{
+			name: "failure: invalid acknowledgement",
+			malleate: func() {
+				msg.Acknowledgement = types.Acknowledgement{}
+			},
+			expError: types.ErrInvalidAcknowledgement,
+		},
 	}
 	for _, tc := range testCases {
 		s.Run(tc.name, func() {
 			msg = types.NewMsgAcknowledgement(
 				types.NewPacket(1, ibctesting.FirstChannelID, ibctesting.SecondChannelID, s.chainA.GetTimeoutTimestamp(), mockv2.NewMockPayload(mockv2.ModuleNameA, mockv2.ModuleNameB)),
-				types.Acknowledgement{},
+				types.Acknowledgement{RecvSuccess: true, AppAcknowledgements: [][]byte{[]byte("ack")}},
 				testProof,
 				clienttypes.ZeroHeight(),
 				s.chainA.SenderAccount.GetAddress().String(),

modules/core/04-channel/v2/types/packet.go

@@ -84,3 +84,24 @@ func (p Payload) ValidateBasic() error {
 func TimeoutTimestampToNanos(seconds uint64) uint64 {
 	return uint64(time.Unix(int64(seconds), 0).UnixNano())
 }
+
+// NewAcknowledgement creates a new Acknowledgement instance
+func NewAcknowledgement(recvSuccess bool, appAcknowledgements [][]byte) Acknowledgement {
+	return Acknowledgement{
+		RecvSuccess:         recvSuccess,
+		AppAcknowledgements: appAcknowledgements,
+	}
+}
+
+// ValidateBasic validates the acknowledgment
+func (a Acknowledgement) ValidateBasic() error {
+	if len(a.GetAppAcknowledgements()) == 0 {
+		return errorsmod.Wrap(ErrInvalidAcknowledgement, "acknowledgement cannot be empty")
+	}
+	for _, ack := range a.GetAppAcknowledgements() {
+		if len(ack) == 0 {
+			return errorsmod.Wrap(ErrInvalidAcknowledgement, "app acknowledgement cannot be empty")
+		}
+	}
+	return nil
+}