chore: add validation for Ack and plug it in.

Author: DimitrisJim

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

@@ -163,13 +163,21 @@ func (k *Keeper) RecvPacket(ctx context.Context, msg *types.MsgRecvPacket) (*typ
 		ack.AppAcknowledgements = append(ack.AppAcknowledgements, res.Acknowledgement)
 	}
 
+	if len(ack.AppAcknowledgements) != len(msg.Packet.Payloads) {
+		return nil, errorsmod.Wrapf(types.ErrInvalidAcknowledgement, "length of app acknowledgement %d does not match length of app payload %d", len(ack.AppAcknowledgements), len(msg.Packet.Payloads))
+	}
+
 	// 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 !isAsync {
+		// Validate ack before forwarding to WriteAcknowledgement.
+		if err := ack.Validate(); err != nil {
+			return nil, err
+		}
 		// Set packet acknowledgement only if the acknowledgement is not async.
 		// NOTE: IBC applications modules may call the WriteAcknowledgement asynchronously if the
 		// acknowledgement is async.

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

@@ -290,6 +290,16 @@ func (suite *KeeperTestSuite) TestMsgRecvPacket() {
 			},
 			expError: commitmenttypes.ErrInvalidProof,
 		},
+		{
+			name: "failure: invalid acknowledgement",
+			malleate: func() {
+				expRecvRes = types.RecvPacketResult{
+					Status:          types.PacketStatus_Success,
+					Acknowledgement: []byte(""),
+				}
+			},
+			expError: types.ErrInvalidAcknowledgement,
+		},
 	}
 
 	for _, tc := range testCases {

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

@@ -190,13 +190,6 @@ func (k Keeper) WriteAcknowledgement(
 		return errorsmod.Wrap(types.ErrInvalidPacket, "receipt not found for packet")
 	}
 
-	// TODO: Validate Acknowledgment more thoroughly here after Issue #7472: https://github.com/cosmos/ibc-go/issues/7472
-
-	// TODO: remove this check, maybe pull it up to the handler.
-	if len(ack.AppAcknowledgements) != len(packet.Payloads) {
-		return errorsmod.Wrapf(types.ErrInvalidAcknowledgement, "length of app acknowledgement %d does not match length of app payload %d", len(ack.AppAcknowledgements), len(packet.Payloads))
-	}
-
 	// set the acknowledgement so that it can be verified on the other side
 	k.SetPacketAcknowledgement(
 		ctx, packet.DestinationChannel, packet.Sequence,

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

@@ -271,15 +271,6 @@ func (suite *KeeperTestSuite) TestWriteAcknowledgement() {
 			},
 			types.ErrAcknowledgementExists,
 		},
-		{
-			"failure: empty ack",
-			func() {
-				ack = types.Acknowledgement{
-					AppAcknowledgements: [][]byte{},
-				}
-			},
-			types.ErrInvalidAcknowledgement,
-		},
 		{
 			"failure: receipt not found for packet",
 			func() {

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

@@ -0,0 +1,25 @@
+package types
+
+import (
+	errorsmod "cosmossdk.io/errors"
+)
+
+// NewAcknowledgement creates a new Acknowledgement containing the provided app acknowledgements.
+func NewAcknowledgement(appAcknowledgements ...[]byte) Acknowledgement {
+	return Acknowledgement{AppAcknowledgements: appAcknowledgements}
+}
+
+// Validate performs a basic validation of the acknowledgement
+func (ack Acknowledgement) Validate() error {
+	if len(ack.AppAcknowledgements) != 1 {
+		return errorsmod.Wrap(ErrInvalidAcknowledgement, "app acknowledgements must be of length one")
+	}
+
+	for _, ack := range ack.AppAcknowledgements {
+		if len(ack) == 0 {
+			return errorsmod.Wrap(ErrInvalidAcknowledgement, "app acknowledgement cannot be empty")
+		}
+	}
+
+	return nil
+}

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

@@ -0,0 +1,47 @@
+package types_test
+
+import (
+	"github.com/cosmos/ibc-go/v9/modules/core/04-channel/v2/types"
+)
+
+// Test_ValidateAcknowledgement tests the acknowledgements Validate method
+func (s *TypesTestSuite) Test_ValidateAcknowledgement() {
+	testCases := []struct {
+		name     string
+		ack      types.Acknowledgement
+		expError error
+	}{
+		{
+			"success: valid successful ack",
+			types.NewAcknowledgement([]byte("appAck1")),
+			nil,
+		},
+		{
+			"failure: more than one app acknowledgements",
+			types.NewAcknowledgement([]byte("appAck1"), []byte("appAck2")),
+			types.ErrInvalidAcknowledgement,
+		},
+		{
+			"failure: app acknowledgement is empty",
+			types.NewAcknowledgement([]byte("")),
+			types.ErrInvalidAcknowledgement,
+		},
+	}
+
+	for _, tc := range testCases {
+		tc := tc
+
+		s.Run(tc.name, func() {
+			s.SetupTest()
+
+			err := tc.ack.Validate()
+
+			expPass := tc.expError == nil
+			if expPass {
+				s.Require().NoError(err)
+			} else {
+				s.Require().ErrorIs(err, tc.expError)
+			}
+		})
+	}
+}

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

@@ -167,7 +167,9 @@ func (msg *MsgAcknowledgement) ValidateBasic() error {
 		return errorsmod.Wrap(commitmenttypesv1.ErrInvalidProof, "cannot submit an empty acknowledgement proof")
 	}
 
-	// TODO: Add validation for ack object https://github.com/cosmos/ibc-go/issues/7472
+	if err := msg.Acknowledgement.Validate(); err != nil {
+		return err
+	}
 
 	_, err := sdk.AccAddressFromBech32(msg.Signer)
 	if err != nil {

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.NewAcknowledgement([]byte(""))
+			},
+			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.NewAcknowledgement([]byte("appAck1")),
 				testProof,
 				clienttypes.ZeroHeight(),
 				s.chainA.SenderAccount.GetAddress().String(),