ack validation

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

@@ -157,6 +157,9 @@ 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
@@ -168,13 +171,13 @@ func (k *Keeper) RecvPacket(ctx context.Context, msg *types.MsgRecvPacket) (*typ
 		writeFn()
 	}
 
-	// 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 {
+		// 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.

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
+}

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

@@ -125,3 +125,77 @@ func TestValidateBasic(t *testing.T) {
 		})
 	}
 }
+
+func TestValidateBasicAcknowledgment(t *testing.T) {
+	var ack types.Acknowledgement
+	testCases := []struct {
+		name     string
+		malleate func()
+		expErr   error
+	}{
+		{
+			"success",
+			func() {
+				ack = types.Acknowledgement{
+					RecvSuccess: true,
+					AppAcknowledgements: [][]byte{
+						[]byte("some bytes"),
+						[]byte("some bytes 2"),
+					},
+				}
+			},
+			nil,
+		},
+		{
+			"success with failed receive",
+			func() {
+				ack = types.Acknowledgement{
+					RecvSuccess: false,
+					AppAcknowledgements: [][]byte{
+						[]byte("some bytes"),
+						[]byte("some bytes 2"),
+					},
+				}
+			},
+			nil,
+		},
+		{
+			"failure: empty ack",
+			func() {
+				ack = types.Acknowledgement{}
+			},
+			types.ErrInvalidAcknowledgement,
+		},
+		{
+			"failure: empty success ack",
+			func() {
+				ack = types.Acknowledgement{RecvSuccess: true}
+			},
+			types.ErrInvalidAcknowledgement,
+		},
+		{
+			"failure: empty app ack",
+			func() {
+				ack = types.Acknowledgement{
+					RecvSuccess: false,
+					AppAcknowledgements: [][]byte{
+						[]byte(""), // empty app ack
+					},
+				}
+			},
+			types.ErrInvalidAcknowledgement,
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			tc.malleate()
+
+			err := ack.ValidateBasic()
+			if tc.expErr == nil {
+				require.NoError(t, err)
+			} else {
+				require.ErrorIs(t, err, tc.expErr)
+			}
+		})
+	}
+}