feat(transfer): use protobuf for packet data v2 serialisation (#6734)

* feat(transfer): use protobuf for ser/de.

* chore: append to raw marshalled bytes.

modules/apps/callbacks/ibc_middleware_test.go

@@ -1,9 +1,10 @@
 package ibccallbacks_test
 
 import (
-	"encoding/json"
 	"fmt"
 
+	"github.com/cosmos/gogoproto/proto"
+
 	errorsmod "cosmossdk.io/errors"
 	storetypes "cosmossdk.io/store/types"
 
@@ -507,7 +508,7 @@ func (s *CallbacksTestSuite) TestOnTimeoutPacket() {
 			s.Require().NoError(err)
 			s.Require().NotNil(packet)
 
-			err = json.Unmarshal(packet.Data, &packetData)
+			err = proto.Unmarshal(packet.Data, &packetData)
 			s.Require().NoError(err)
 
 			ctx = s.chainA.GetContext()

modules/apps/transfer/ibc_module_test.go

@@ -334,11 +334,11 @@ func (suite *TransferTestSuite) TestOnRecvPacket() {
 					sdk.NewAttribute(types.AttributeKeyMemo, ""),
 					sdk.NewAttribute(types.AttributeKeyForwardingHops, "null"),
 					sdk.NewAttribute(types.AttributeKeyAckSuccess, "false"),
-					sdk.NewAttribute(types.AttributeKeyAckError, "cannot unmarshal ICS20-V2 transfer packet data: invalid character 'i' looking for beginning of value: invalid type: invalid type"),
+					sdk.NewAttribute(types.AttributeKeyAckError, "cannot unmarshal ICS20-V2 transfer packet data: errUnknownField \"*types.FungibleTokenPacketDataV2\": {TagNum: 13, WireType:\"fixed64\"}: invalid type: invalid type"),
 				}
 			},
 			channeltypes.NewErrorAcknowledgement(ibcerrors.ErrInvalidType),
-			"cannot unmarshal ICS20-V2 transfer packet data: invalid character 'i' looking for beginning of value: invalid type: invalid type",
+			"cannot unmarshal ICS20-V2 transfer packet data: unexpected EOF: invalid type: invalid type",
 		},
 		{
 			"failure: receive disabled",

modules/apps/transfer/internal/packet.go

@@ -3,8 +3,12 @@ package internal
 import (
 	"encoding/json"
 
+	"github.com/cosmos/gogoproto/proto"
+
 	errorsmod "cosmossdk.io/errors"
 
+	"github.com/cosmos/cosmos-sdk/codec/unknownproto"
+
 	"github.com/cosmos/ibc-go/v8/modules/apps/transfer/types"
 	ibcerrors "github.com/cosmos/ibc-go/v8/modules/core/errors"
 )
@@ -22,7 +26,11 @@ func UnmarshalPacketData(bz []byte, ics20Version string) (types.FungibleTokenPac
 		return packetDataV1ToV2(datav1)
 	case types.V2:
 		var datav2 types.FungibleTokenPacketDataV2
-		if err := json.Unmarshal(bz, &datav2); err != nil {
+		if err := unknownproto.RejectUnknownFieldsStrict(bz, &datav2, unknownproto.DefaultAnyResolver{}); err != nil {
+			return types.FungibleTokenPacketDataV2{}, errorsmod.Wrapf(ibcerrors.ErrInvalidType, "cannot unmarshal ICS20-V2 transfer packet data: %s", err.Error())
+		}
+
+		if err := proto.Unmarshal(bz, &datav2); err != nil {
 			return types.FungibleTokenPacketDataV2{}, errorsmod.Wrapf(ibcerrors.ErrInvalidType, "cannot unmarshal ICS20-V2 transfer packet data: %s", err.Error())
 		}
 

modules/apps/transfer/internal/packet_test.go

@@ -8,6 +8,12 @@ import (
 	errorsmod "cosmossdk.io/errors"
 
 	"github.com/cosmos/ibc-go/v8/modules/apps/transfer/types"
+	ibcerrors "github.com/cosmos/ibc-go/v8/modules/core/errors"
+)
+
+const (
+	sender   = "sender"
+	receiver = "receiver"
 )
 
 var emptyForwardingPacketData = types.ForwardingPacketData{}
@@ -37,7 +43,7 @@ func TestUnmarshalPacketData(t *testing.T) {
 							Denom:  types.NewDenom("atom", types.NewHop("transfer", "channel-0")),
 							Amount: "1000",
 						},
-					}, "sender", "receiver", "", emptyForwardingPacketData)
+					}, sender, receiver, "", emptyForwardingPacketData)
 
 				packetDataBz = packetData.GetBytes()
 				version = types.V2
@@ -55,7 +61,7 @@ func TestUnmarshalPacketData(t *testing.T) {
 
 	for _, tc := range testCases {
 
-		packetDataV1 := types.NewFungibleTokenPacketData("transfer/channel-0/atom", "1000", "sender", "receiver", "")
+		packetDataV1 := types.NewFungibleTokenPacketData("transfer/channel-0/atom", "1000", sender, receiver, "")
 
 		packetDataBz = packetDataV1.GetBytes()
 		version = types.V1
@@ -73,6 +79,61 @@ func TestUnmarshalPacketData(t *testing.T) {
 	}
 }
 
+// TestV2ForwardsCompatibilityFails asserts that new fields being added to a future proto definition of
+// FungibleTokenPacketDataV2 fail to unmarshal with previous versions. In essence, permit backwards compatibility
+// but restrict forward one.
+func TestV2ForwardsCompatibilityFails(t *testing.T) {
+	var (
+		packet       types.FungibleTokenPacketDataV2
+		packetDataBz []byte
+	)
+
+	testCases := []struct {
+		name     string
+		malleate func()
+		expError error
+	}{
+		{
+			"success",
+			func() {},
+			nil,
+		},
+		{
+			"failure: new field present in packet data",
+			func() {
+				// packet data containing extra field unknown to current proto file.
+				packetDataBz = append(packet.GetBytes(), []byte("22\tnew_value")...)
+			},
+			ibcerrors.ErrInvalidType,
+		},
+	}
+
+	for _, tc := range testCases {
+		packet = types.NewFungibleTokenPacketDataV2(
+			[]types.Token{
+				{
+					Denom:  types.NewDenom("atom", types.NewHop("transfer", "channel-0")),
+					Amount: "1000",
+				},
+			}, "sender", "receiver", "", emptyForwardingPacketData,
+		)
+
+		packetDataBz = packet.GetBytes()
+
+		tc.malleate()
+
+		packetData, err := UnmarshalPacketData(packetDataBz, types.V2)
+
+		expPass := tc.expError == nil
+		if expPass {
+			require.NoError(t, err)
+			require.NotEqual(t, types.FungibleTokenPacketDataV2{}, packetData)
+		} else {
+			require.ErrorIs(t, err, tc.expError)
+		}
+	}
+}
+
 func TestPacketV1ToPacketV2(t *testing.T) {
 	const (
 		sender   = "sender"

modules/apps/transfer/keeper/relay_forwarding_test.go

@@ -4,6 +4,8 @@ import (
 	"fmt"
 	"time"
 
+	"github.com/cosmos/gogoproto/proto"
+
 	sdkmath "cosmossdk.io/math"
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
@@ -277,7 +279,7 @@ func (suite *KeeperTestSuite) TestSuccessfulForwardWithMemo() {
 
 	// Check that the memo is stored correctly in the packet sent from A
 	var tokenPacketOnA types.FungibleTokenPacketDataV2
-	err = suite.chainA.Codec.UnmarshalJSON(packetFromAtoB.Data, &tokenPacketOnA)
+	err = proto.Unmarshal(packetFromAtoB.Data, &tokenPacketOnA)
 	suite.Require().NoError(err)
 	suite.Require().Equal("", tokenPacketOnA.Memo)
 	suite.Require().Equal(testMemo, tokenPacketOnA.Forwarding.DestinationMemo)
@@ -299,7 +301,7 @@ func (suite *KeeperTestSuite) TestSuccessfulForwardWithMemo() {
 
 	// Check that the memo is stored correctly in the packet sent from B
 	var tokenPacketOnB types.FungibleTokenPacketDataV2
-	err = suite.chainB.Codec.UnmarshalJSON(packetFromBtoC.Data, &tokenPacketOnB)
+	err = proto.Unmarshal(packetFromBtoC.Data, &tokenPacketOnB)
 	suite.Require().NoError(err)
 	suite.Require().Equal(testMemo, tokenPacketOnB.Memo)
 	suite.Require().Equal("", tokenPacketOnB.Forwarding.DestinationMemo)
@@ -324,7 +326,7 @@ func (suite *KeeperTestSuite) TestSuccessfulForwardWithMemo() {
 
 	// Check that the memo is stored directly in the memo field on C
 	var tokenPacketOnC types.FungibleTokenPacketDataV2
-	err = suite.chainC.Codec.UnmarshalJSON(packetOnC.Data, &tokenPacketOnC)
+	err = proto.Unmarshal(packetOnC.Data, &tokenPacketOnC)
 	suite.Require().NoError(err)
 	suite.Require().Equal("", tokenPacketOnC.Forwarding.DestinationMemo)
 	suite.Require().Equal(testMemo, tokenPacketOnC.Memo)
@@ -412,7 +414,7 @@ func (suite *KeeperTestSuite) TestSuccessfulForwardWithNonCosmosAccAddress() {
 
 	// Check that the token sent from A has final receiver intact
 	var tokenPacketOnA types.FungibleTokenPacketDataV2
-	err = suite.chainA.Codec.UnmarshalJSON(packetFromAtoB.Data, &tokenPacketOnA)
+	err = proto.Unmarshal(packetFromAtoB.Data, &tokenPacketOnA)
 	suite.Require().NoError(err)
 	suite.Require().Equal(nonCosmosReceiver, tokenPacketOnA.Receiver)
 
@@ -432,7 +434,7 @@ func (suite *KeeperTestSuite) TestSuccessfulForwardWithNonCosmosAccAddress() {
 
 	// Check that the token sent from B has final receiver intact
 	var tokenPacketOnB types.FungibleTokenPacketDataV2
-	err = suite.chainB.Codec.UnmarshalJSON(packetFromBtoC.Data, &tokenPacketOnB)
+	err = proto.Unmarshal(packetFromBtoC.Data, &tokenPacketOnB)
 	suite.Require().NoError(err)
 	suite.Require().Equal(nonCosmosReceiver, tokenPacketOnB.Receiver)
 

modules/apps/transfer/keeper/relay_test.go

@@ -1154,55 +1154,9 @@ func (suite *KeeperTestSuite) TestPacketForwardsCompatibility() {
 		expError    error
 		expAckError error
 	}{
-		{
-			"success: no new field with memo v2",
-			func() {
-				jsonString := fmt.Sprintf(`{"tokens":[{"denom": {"base": "atom", "trace": []},"amount":"100"}],"sender":"%s","receiver":"%s"}`, suite.chainB.SenderAccount.GetAddress().String(), suite.chainA.SenderAccount.GetAddress().String())
-				packetData = []byte(jsonString)
-			},
-			nil,
-			nil,
-		},
-		{
-			"success: no new field without memo",
-			func() {
-				jsonString := fmt.Sprintf(`{"tokens":[{"denom": {"base": "atom", "trace": []},"amount":"100"}],"sender":"%s","receiver":"%s"}`, suite.chainB.SenderAccount.GetAddress().String(), suite.chainA.SenderAccount.GetAddress().String())
-				packetData = []byte(jsonString)
-			},
-			nil,
-			nil,
-		},
-		{
-			"failure: invalid packet data v2",
-			func() {
-				packetData = []byte("invalid packet data")
-			},
-			ibcerrors.ErrInvalidType,
-			ibcerrors.ErrInvalidType,
-		},
-		{
-			"failure: new field v2",
-			func() {
-				jsonString := fmt.Sprintf(`{"tokens":[{"denom": {"base": "atom", "trace": []},"amount":"100"}],"sender":"%s","receiver":"%s", "new_field":"value"}`, suite.chainB.SenderAccount.GetAddress().String(), suite.chainA.SenderAccount.GetAddress().String())
-				packetData = []byte(jsonString)
-			},
-			ibcerrors.ErrInvalidType,
-			ibcerrors.ErrInvalidType,
-		},
-		{
-			"failure: missing field v2",
-			func() {
-				jsonString := fmt.Sprintf(`{"tokens":[{"denom": {"trace": []},"amount":"100"}],"sender":"%s","receiver":"%s"}`, suite.chainB.SenderAccount.GetAddress().String(), suite.chainA.SenderAccount.GetAddress().String())
-				packetData = []byte(jsonString)
-			},
-			types.ErrInvalidDenomForTransfer,
-			ibcerrors.ErrInvalidType,
-		},
 		{
 			"success: no new field with memo",
 			func() {
-				path.EndpointA.ChannelConfig.Version = types.V1
-				path.EndpointB.ChannelConfig.Version = types.V1
 				jsonString := fmt.Sprintf(`{"denom":"denom","amount":"100","sender":"%s","receiver":"%s","memo":"memo"}`, suite.chainB.SenderAccount.GetAddress().String(), suite.chainA.SenderAccount.GetAddress().String())
 				packetData = []byte(jsonString)
 			},
@@ -1212,8 +1166,6 @@ func (suite *KeeperTestSuite) TestPacketForwardsCompatibility() {
 		{
 			"success: no new field without memo",
 			func() {
-				path.EndpointA.ChannelConfig.Version = types.V1
-				path.EndpointB.ChannelConfig.Version = types.V1
 				jsonString := fmt.Sprintf(`{"denom":"denom","amount":"100","sender":"%s","receiver":"%s"}`, suite.chainB.SenderAccount.GetAddress().String(), suite.chainA.SenderAccount.GetAddress().String())
 				packetData = []byte(jsonString)
 			},
@@ -1223,8 +1175,6 @@ func (suite *KeeperTestSuite) TestPacketForwardsCompatibility() {
 		{
 			"failure: invalid packet data",
 			func() {
-				path.EndpointA.ChannelConfig.Version = types.V1
-				path.EndpointB.ChannelConfig.Version = types.V1
 				packetData = []byte("invalid packet data")
 			},
 			ibcerrors.ErrInvalidType,
@@ -1233,8 +1183,6 @@ func (suite *KeeperTestSuite) TestPacketForwardsCompatibility() {
 		{
 			"failure: new field",
 			func() {
-				path.EndpointA.ChannelConfig.Version = types.V1
-				path.EndpointB.ChannelConfig.Version = types.V1
 				jsonString := fmt.Sprintf(`{"denom":"denom","amount":"100","sender":"%s","receiver":"%s","memo":"memo","new_field":"value"}`, suite.chainB.SenderAccount.GetAddress().String(), suite.chainA.SenderAccount.GetAddress().String())
 				packetData = []byte(jsonString)
 			},
@@ -1244,8 +1192,6 @@ func (suite *KeeperTestSuite) TestPacketForwardsCompatibility() {
 		{
 			"failure: missing field",
 			func() {
-				path.EndpointA.ChannelConfig.Version = types.V1
-				path.EndpointB.ChannelConfig.Version = types.V1
 				jsonString := fmt.Sprintf(`{"amount":"100","sender":%s","receiver":"%s"}`, suite.chainB.SenderAccount.GetAddress().String(), suite.chainA.SenderAccount.GetAddress().String())
 				packetData = []byte(jsonString)
 			},
@@ -1261,6 +1207,8 @@ func (suite *KeeperTestSuite) TestPacketForwardsCompatibility() {
 			packetData = nil
 
 			path = ibctesting.NewTransferPath(suite.chainA, suite.chainB)
+			path.EndpointA.ChannelConfig.Version = types.V1
+			path.EndpointB.ChannelConfig.Version = types.V1
 
 			tc.malleate()
 

modules/apps/transfer/types/packet.go

@@ -5,6 +5,8 @@ import (
 	"errors"
 	"strings"
 
+	"github.com/cosmos/gogoproto/proto"
+
 	errorsmod "cosmossdk.io/errors"
 	sdkmath "cosmossdk.io/math"
 
@@ -152,9 +154,10 @@ func (ftpd FungibleTokenPacketDataV2) ValidateBasic() error {
 	return nil
 }
 
-// GetBytes is a helper for serialising
+// GetBytes is a helper for serialising a FungibleTokenPacketDataV2. It uses protobuf to serialise
+// the packet data and panics on failure.
 func (ftpd FungibleTokenPacketDataV2) GetBytes() []byte {
-	bz, err := json.Marshal(&ftpd)
+	bz, err := proto.Marshal(&ftpd)
 	if err != nil {
 		panic(errors.New("cannot marshal FungibleTokenPacketDataV2 into bytes"))
 	}