From a4a763a2851bbc4add35346d6ae9885476925e02 Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Tue, 2 Jul 2024 06:48:41 +0300 Subject: [PATCH 1/2] feat(transfer): use protobuf for ser/de. --- modules/apps/callbacks/ibc_middleware_test.go | 5 +- modules/apps/transfer/ibc_module_test.go | 4 +- modules/apps/transfer/internal/packet.go | 10 ++- modules/apps/transfer/internal/packet_test.go | 61 ++++++++++++++++++- .../transfer/keeper/relay_forwarding_test.go | 12 ++-- modules/apps/transfer/keeper/relay_test.go | 56 +---------------- modules/apps/transfer/types/packet.go | 7 ++- 7 files changed, 87 insertions(+), 68 deletions(-) diff --git a/modules/apps/callbacks/ibc_middleware_test.go b/modules/apps/callbacks/ibc_middleware_test.go index 889e852938f..bfbca027d87 100644 --- a/modules/apps/callbacks/ibc_middleware_test.go +++ b/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() diff --git a/modules/apps/transfer/ibc_module_test.go b/modules/apps/transfer/ibc_module_test.go index b3f4dfd08e2..78e82fcf064 100644 --- a/modules/apps/transfer/ibc_module_test.go +++ b/modules/apps/transfer/ibc_module_test.go @@ -332,11 +332,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", diff --git a/modules/apps/transfer/internal/packet.go b/modules/apps/transfer/internal/packet.go index cd6a2638954..cc5885523a8 100644 --- a/modules/apps/transfer/internal/packet.go +++ b/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()) } diff --git a/modules/apps/transfer/internal/packet_test.go b/modules/apps/transfer/internal/packet_test.go index 30202ed8218..b0f45c318c1 100644 --- a/modules/apps/transfer/internal/packet_test.go +++ b/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,57 @@ 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 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 = []byte("\n%\n\x1d\n\x04atom\x1a\x15\n\btransfer\x12\tchannel-0\x12\x041000\x12\x06sender\x1a\breceiver*\x002\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" diff --git a/modules/apps/transfer/keeper/relay_forwarding_test.go b/modules/apps/transfer/keeper/relay_forwarding_test.go index 2fb76a887cf..944df472306 100644 --- a/modules/apps/transfer/keeper/relay_forwarding_test.go +++ b/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) diff --git a/modules/apps/transfer/keeper/relay_test.go b/modules/apps/transfer/keeper/relay_test.go index fd1d7f69320..9e2dacd35ff 100644 --- a/modules/apps/transfer/keeper/relay_test.go +++ b/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() diff --git a/modules/apps/transfer/types/packet.go b/modules/apps/transfer/types/packet.go index 7a182b8598e..dadb354d28e 100644 --- a/modules/apps/transfer/types/packet.go +++ b/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")) } From c82e7191ac215c8b00b3f84f8d85078f7af73519 Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Thu, 4 Jul 2024 17:01:14 +0300 Subject: [PATCH 2/2] chore: append to raw marshalled bytes. --- modules/apps/transfer/internal/packet_test.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/modules/apps/transfer/internal/packet_test.go b/modules/apps/transfer/internal/packet_test.go index b0f45c318c1..f9ed72aab42 100644 --- a/modules/apps/transfer/internal/packet_test.go +++ b/modules/apps/transfer/internal/packet_test.go @@ -83,7 +83,11 @@ func TestUnmarshalPacketData(t *testing.T) { // FungibleTokenPacketDataV2 fail to unmarshal with previous versions. In essence, permit backwards compatibility // but restrict forward one. func TestV2ForwardsCompatibilityFails(t *testing.T) { - var packetDataBz []byte + var ( + packet types.FungibleTokenPacketDataV2 + packetDataBz []byte + ) + testCases := []struct { name string malleate func() @@ -98,14 +102,14 @@ func TestV2ForwardsCompatibilityFails(t *testing.T) { "failure: new field present in packet data", func() { // packet data containing extra field unknown to current proto file. - packetDataBz = []byte("\n%\n\x1d\n\x04atom\x1a\x15\n\btransfer\x12\tchannel-0\x12\x041000\x12\x06sender\x1a\breceiver*\x002\tnew_value") + packetDataBz = append(packet.GetBytes(), []byte("22\tnew_value")...) }, ibcerrors.ErrInvalidType, }, } for _, tc := range testCases { - packet := types.NewFungibleTokenPacketDataV2( + packet = types.NewFungibleTokenPacketDataV2( []types.Token{ { Denom: types.NewDenom("atom", types.NewHop("transfer", "channel-0")),