From e2fd8575997fd4380bb67937d58c4809c633f140 Mon Sep 17 00:00:00 2001 From: Nikolas De Giorgis Date: Tue, 19 Nov 2024 09:17:21 +0000 Subject: [PATCH 1/7] chore: use encoding fields for packet data unmarshaling --- modules/apps/transfer/ibc_module.go | 8 +-- modules/apps/transfer/types/packet.go | 68 +++++++++++++------ modules/apps/transfer/types/packet_test.go | 4 +- modules/apps/transfer/v2/ibc_module.go | 8 +-- .../transfer/v2/keeper/msg_server_test.go | 15 ++-- .../core/04-channel/v2/types/packet_test.go | 2 +- testing/mock/v2/mock.go | 2 +- 7 files changed, 66 insertions(+), 41 deletions(-) diff --git a/modules/apps/transfer/ibc_module.go b/modules/apps/transfer/ibc_module.go index c6445645c23..95d4681b9b9 100644 --- a/modules/apps/transfer/ibc_module.go +++ b/modules/apps/transfer/ibc_module.go @@ -184,7 +184,7 @@ func (im IBCModule) OnRecvPacket( events.EmitOnRecvPacketEvent(ctx, data, ack, ackErr) }() - data, ackErr = types.UnmarshalPacketData(packet.GetData(), channelVersion) + data, ackErr = types.UnmarshalPacketData(packet.GetData(), channelVersion, "") if ackErr != nil { ack = channeltypes.NewErrorAcknowledgement(ackErr) im.keeper.Logger(ctx).Error(fmt.Sprintf("%s sequence %d", ackErr.Error(), packet.Sequence)) @@ -221,7 +221,7 @@ func (im IBCModule) OnAcknowledgementPacket( return errorsmod.Wrapf(ibcerrors.ErrUnknownRequest, "cannot unmarshal ICS-20 transfer packet acknowledgement: %v", err) } - data, err := types.UnmarshalPacketData(packet.GetData(), channelVersion) + data, err := types.UnmarshalPacketData(packet.GetData(), channelVersion, "") if err != nil { return err } @@ -242,7 +242,7 @@ func (im IBCModule) OnTimeoutPacket( packet channeltypes.Packet, relayer sdk.AccAddress, ) error { - data, err := types.UnmarshalPacketData(packet.GetData(), channelVersion) + data, err := types.UnmarshalPacketData(packet.GetData(), channelVersion, "") if err != nil { return err } @@ -305,6 +305,6 @@ func (im IBCModule) UnmarshalPacketData(ctx context.Context, portID string, chan return types.FungibleTokenPacketDataV2{}, "", errorsmod.Wrapf(ibcerrors.ErrNotFound, "app version not found for port %s and channel %s", portID, channelID) } - ftpd, err := types.UnmarshalPacketData(bz, ics20Version) + ftpd, err := types.UnmarshalPacketData(bz, ics20Version, "") return ftpd, ics20Version, err } diff --git a/modules/apps/transfer/types/packet.go b/modules/apps/transfer/types/packet.go index 98525dac769..c094fb07b7b 100644 --- a/modules/apps/transfer/types/packet.go +++ b/modules/apps/transfer/types/packet.go @@ -5,13 +5,12 @@ import ( "errors" "strings" + "github.com/cosmos/cosmos-sdk/codec/unknownproto" "github.com/cosmos/gogoproto/proto" errorsmod "cosmossdk.io/errors" sdkmath "cosmossdk.io/math" - "github.com/cosmos/cosmos-sdk/codec/unknownproto" - ibcerrors "github.com/cosmos/ibc-go/v9/modules/core/errors" ibcexported "github.com/cosmos/ibc-go/v9/modules/core/exported" ) @@ -208,36 +207,65 @@ func (ftpd FungibleTokenPacketDataV2) HasForwarding() bool { } // UnmarshalPacketData attempts to unmarshal the provided packet data bytes into a FungibleTokenPacketDataV2. -// The version of ics20 should be provided and should be either ics20-1 or ics20-2. -func UnmarshalPacketData(bz []byte, ics20Version string) (FungibleTokenPacketDataV2, error) { - // TODO: in transfer ibc module V2, we need to respect he encoding value passed via the payload, some hard coded assumptions about - // encoding exist here based on the ics20 version passed in. +func UnmarshalPacketData(bz []byte, ics20Version string, encoding string) (FungibleTokenPacketDataV2, error) { + // TODO: encoding might be turned into an enum + var foo proto.Message switch ics20Version { case V1: - var datav1 FungibleTokenPacketData - if err := json.Unmarshal(bz, &datav1); err != nil { - return FungibleTokenPacketDataV2{}, errorsmod.Wrapf(ibcerrors.ErrInvalidType, "cannot unmarshal ICS20-V1 transfer packet data: %s", err.Error()) + if encoding == "" { + encoding = "json" } - - return PacketDataV1ToV2(datav1) + foo = &FungibleTokenPacketData{} case V2: - var datav2 FungibleTokenPacketDataV2 - if err := unknownproto.RejectUnknownFieldsStrict(bz, &datav2, unknownproto.DefaultAnyResolver{}); err != nil { - return FungibleTokenPacketDataV2{}, errorsmod.Wrapf(ibcerrors.ErrInvalidType, "cannot unmarshal ICS20-V2 transfer packet data: %s", err.Error()) + if encoding == "" { + encoding = "proto" } + foo = &FungibleTokenPacketDataV2{} + default: + return FungibleTokenPacketDataV2{}, errorsmod.Wrap(ErrInvalidVersion, ics20Version) + } + + errorMsgVersion := "ICS20-V2" + if ics20Version == V1 { + errorMsgVersion = "ICS20-V1" + } - if err := proto.Unmarshal(bz, &datav2); err != nil { - return FungibleTokenPacketDataV2{}, errorsmod.Wrapf(ibcerrors.ErrInvalidType, "cannot unmarshal ICS20-V2 transfer packet data: %s", err.Error()) + switch encoding { + case "json": + if err := json.Unmarshal(bz, &foo); err != nil { + return FungibleTokenPacketDataV2{}, errorsmod.Wrapf(ibcerrors.ErrInvalidType, "cannot unmarshal %s transfer packet data: %s", errorMsgVersion, err.Error()) + } + case "proto": + if err := unknownproto.RejectUnknownFieldsStrict(bz, foo, unknownproto.DefaultAnyResolver{}); err != nil { + return FungibleTokenPacketDataV2{}, errorsmod.Wrapf(ibcerrors.ErrInvalidType, "cannot unmarshal %s transfer packet data: %s", errorMsgVersion, err.Error()) } - if err := datav2.ValidateBasic(); err != nil { - return FungibleTokenPacketDataV2{}, err + if err := proto.Unmarshal(bz, foo); err != nil { + return FungibleTokenPacketDataV2{}, errorsmod.Wrapf(ibcerrors.ErrInvalidType, "cannot unmarshal %s transfer packet data: %s", errorMsgVersion, err.Error()) } - return datav2, nil default: - return FungibleTokenPacketDataV2{}, errorsmod.Wrap(ErrInvalidVersion, ics20Version) + return FungibleTokenPacketDataV2{}, errorsmod.Wrapf(ibcerrors.ErrInvalidType, "invalid encoding provided, must be either empty or one of `json`,`proto`, got %s", encoding) + } + + if ics20Version == V1 { + datav1, ok := foo.(*FungibleTokenPacketData) + if !ok { + // We should never get here, as we manually constructed the type at the beginning of the file + return FungibleTokenPacketDataV2{}, errorsmod.Wrapf(ibcerrors.ErrInvalidType, "cannot convert proto message into FungibleTokenPacketData") + } + return PacketDataV1ToV2(*datav1) + } + datav2, ok := foo.(*FungibleTokenPacketDataV2) + if !ok { + // We should never get here, as we manually constructed the type at the beginning of the file + return FungibleTokenPacketDataV2{}, errorsmod.Wrapf(ibcerrors.ErrInvalidType, "cannot convert proto message into FungibleTokenPacketDataV2") + } + + if err := datav2.ValidateBasic(); err != nil { + return FungibleTokenPacketDataV2{}, err } + return *datav2, nil } // PacketDataV1ToV2 converts a v1 packet data to a v2 packet data. The packet data is validated diff --git a/modules/apps/transfer/types/packet_test.go b/modules/apps/transfer/types/packet_test.go index 254cec03522..780c30a6a07 100644 --- a/modules/apps/transfer/types/packet_test.go +++ b/modules/apps/transfer/types/packet_test.go @@ -765,7 +765,7 @@ func TestUnmarshalPacketData(t *testing.T) { tc.malleate() - packetData, err := types.UnmarshalPacketData(packetDataBz, version) + packetData, err := types.UnmarshalPacketData(packetDataBz, version, "") expPass := tc.expError == nil if expPass { @@ -819,7 +819,7 @@ func TestV2ForwardsCompatibilityFails(t *testing.T) { tc.malleate() - packetData, err := types.UnmarshalPacketData(packetDataBz, types.V2) + packetData, err := types.UnmarshalPacketData(packetDataBz, types.V2, "proto") expPass := tc.expError == nil if expPass { diff --git a/modules/apps/transfer/v2/ibc_module.go b/modules/apps/transfer/v2/ibc_module.go index fcbedc6ef03..19aa3db5292 100644 --- a/modules/apps/transfer/v2/ibc_module.go +++ b/modules/apps/transfer/v2/ibc_module.go @@ -41,7 +41,7 @@ func (im *IBCModule) OnSendPacket(goCtx context.Context, sourceChannel string, d return errorsmod.Wrapf(ibcerrors.ErrUnauthorized, "%s is not allowed to send funds", signer) } - data, err := transfertypes.UnmarshalPacketData(payload.Value, payload.Version) + data, err := transfertypes.UnmarshalPacketData(payload.Value, payload.Version, payload.Encoding) if err != nil { return err } @@ -66,7 +66,7 @@ func (im *IBCModule) OnRecvPacket(ctx context.Context, sourceChannel string, des events.EmitOnRecvPacketEvent(ctx, data, ack, ackErr) }() - data, ackErr = transfertypes.UnmarshalPacketData(payload.Value, payload.Version) + data, ackErr = transfertypes.UnmarshalPacketData(payload.Value, payload.Version, payload.Encoding) if ackErr != nil { ack = channeltypes.NewErrorAcknowledgement(ackErr) im.keeper.Logger(ctx).Error(fmt.Sprintf("%s sequence %d", ackErr.Error(), sequence)) @@ -99,7 +99,7 @@ func (im *IBCModule) OnRecvPacket(ctx context.Context, sourceChannel string, des } func (im *IBCModule) OnTimeoutPacket(ctx context.Context, sourceChannel string, destinationChannel string, sequence uint64, payload types.Payload, relayer sdk.AccAddress) error { - data, err := transfertypes.UnmarshalPacketData(payload.Value, payload.Version) + data, err := transfertypes.UnmarshalPacketData(payload.Value, payload.Version, payload.Encoding) if err != nil { return err } @@ -119,7 +119,7 @@ func (im *IBCModule) OnAcknowledgementPacket(ctx context.Context, sourceChannel return errorsmod.Wrapf(ibcerrors.ErrUnknownRequest, "cannot unmarshal ICS-20 transfer packet acknowledgement: %v", err) } - data, err := transfertypes.UnmarshalPacketData(payload.Value, payload.Version) + data, err := transfertypes.UnmarshalPacketData(payload.Value, payload.Version, payload.Encoding) if err != nil { return err } diff --git a/modules/apps/transfer/v2/keeper/msg_server_test.go b/modules/apps/transfer/v2/keeper/msg_server_test.go index 9dcaba67e89..819d8a01866 100644 --- a/modules/apps/transfer/v2/keeper/msg_server_test.go +++ b/modules/apps/transfer/v2/keeper/msg_server_test.go @@ -68,10 +68,7 @@ func (suite *KeeperTestSuite) TestMsgSendPacketTransfer() { bz := suite.chainA.Codec.MustMarshal(&ftpd) timestamp := suite.chainA.GetTimeoutTimestampSecs() - // TODO: note, encoding field currently not respected in the implementation. encoding is determined by the version. - // ics20-v1 == json - // ics20-v2 == proto - payload = channeltypesv2.NewPayload(transfertypes.ModuleName, transfertypes.ModuleName, transfertypes.V2, "json", bz) + payload = channeltypesv2.NewPayload(transfertypes.ModuleName, transfertypes.ModuleName, transfertypes.V2, "proto", bz) tc.malleate() @@ -166,7 +163,7 @@ func (suite *KeeperTestSuite) TestMsgRecvPacketTransfer() { bz := suite.chainA.Codec.MustMarshal(&ftpd) timestamp := suite.chainA.GetTimeoutTimestampSecs() - payload := channeltypesv2.NewPayload(transfertypes.ModuleName, transfertypes.ModuleName, transfertypes.V2, "json", bz) + payload := channeltypesv2.NewPayload(transfertypes.ModuleName, transfertypes.ModuleName, transfertypes.V2, "proto", bz) var err error packet, err = path.EndpointA.MsgSendPacket(timestamp, payload) suite.Require().NoError(err) @@ -275,7 +272,7 @@ func (suite *KeeperTestSuite) TestMsgAckPacketTransfer() { bz := suite.chainA.Codec.MustMarshal(&ftpd) timestamp := suite.chainA.GetTimeoutTimestampSecs() - payload := channeltypesv2.NewPayload(transfertypes.ModuleName, transfertypes.ModuleName, transfertypes.V2, "json", bz) + payload := channeltypesv2.NewPayload(transfertypes.ModuleName, transfertypes.ModuleName, transfertypes.V2, "proto", bz) var err error packet, err = path.EndpointA.MsgSendPacket(timestamp, payload) @@ -376,7 +373,7 @@ func (suite *KeeperTestSuite) TestMsgTimeoutPacketTransfer() { bz := suite.chainA.Codec.MustMarshal(&ftpd) timeoutTimestamp = uint64(suite.chainA.GetContext().BlockTime().Unix()) + uint64(time.Hour.Seconds()) - payload := channeltypesv2.NewPayload(transfertypes.ModuleName, transfertypes.ModuleName, transfertypes.V2, "json", bz) + payload := channeltypesv2.NewPayload(transfertypes.ModuleName, transfertypes.ModuleName, transfertypes.V2, "proto", bz) var err error packet, err = path.EndpointA.MsgSendPacket(timeoutTimestamp, payload) @@ -497,7 +494,7 @@ func (suite *KeeperTestSuite) TestV2RetainsFungibility() { bz := suite.chainB.Codec.MustMarshal(&ftpd) timeoutTimestamp := uint64(suite.chainB.GetContext().BlockTime().Unix()) + uint64(time.Hour.Seconds()) - payload := channeltypesv2.NewPayload(transfertypes.ModuleName, transfertypes.ModuleName, transfertypes.V2, "json", bz) + payload := channeltypesv2.NewPayload(transfertypes.ModuleName, transfertypes.ModuleName, transfertypes.V2, "proto", bz) var err error packetV2, err = pathv2.EndpointA.MsgSendPacket(timeoutTimestamp, payload) @@ -536,7 +533,7 @@ func (suite *KeeperTestSuite) TestV2RetainsFungibility() { bz := suite.chainC.Codec.MustMarshal(&ftpd) timeoutTimestamp := uint64(suite.chainC.GetContext().BlockTime().Unix()) + uint64(time.Hour.Seconds()) - payload := channeltypesv2.NewPayload(transfertypes.ModuleName, transfertypes.ModuleName, transfertypes.V2, "json", bz) + payload := channeltypesv2.NewPayload(transfertypes.ModuleName, transfertypes.ModuleName, transfertypes.V2, "proto", bz) var err error packetV2, err = pathv2.EndpointB.MsgSendPacket(timeoutTimestamp, payload) diff --git a/modules/core/04-channel/v2/types/packet_test.go b/modules/core/04-channel/v2/types/packet_test.go index 0a8b079b0a5..8bc1d57df38 100644 --- a/modules/core/04-channel/v2/types/packet_test.go +++ b/modules/core/04-channel/v2/types/packet_test.go @@ -109,7 +109,7 @@ func TestValidateBasic(t *testing.T) { SourcePort: ibctesting.MockPort, DestinationPort: ibctesting.MockPort, Version: "ics20-v2", - Encoding: "json", + Encoding: "proto", Value: mock.MockPacketData, }) diff --git a/testing/mock/v2/mock.go b/testing/mock/v2/mock.go index 41582217070..9178eb6d794 100644 --- a/testing/mock/v2/mock.go +++ b/testing/mock/v2/mock.go @@ -24,7 +24,7 @@ func NewMockPayload(sourcePort, destPort string) channeltypesv2.Payload { return channeltypesv2.Payload{ SourcePort: sourcePort, DestinationPort: destPort, - Encoding: "json", + Encoding: "proto", Value: mockv1.MockPacketData, Version: mockv1.Version, } From d7f282ddaf64d87ffec168738b378621e28c3edb Mon Sep 17 00:00:00 2001 From: Nikolas De Giorgis Date: Tue, 19 Nov 2024 09:24:00 +0000 Subject: [PATCH 2/7] linter --- modules/apps/transfer/types/packet.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/modules/apps/transfer/types/packet.go b/modules/apps/transfer/types/packet.go index c094fb07b7b..b3d989a1146 100644 --- a/modules/apps/transfer/types/packet.go +++ b/modules/apps/transfer/types/packet.go @@ -5,12 +5,13 @@ import ( "errors" "strings" - "github.com/cosmos/cosmos-sdk/codec/unknownproto" "github.com/cosmos/gogoproto/proto" errorsmod "cosmossdk.io/errors" sdkmath "cosmossdk.io/math" + "github.com/cosmos/cosmos-sdk/codec/unknownproto" + ibcerrors "github.com/cosmos/ibc-go/v9/modules/core/errors" ibcexported "github.com/cosmos/ibc-go/v9/modules/core/exported" ) From 7248d0c6b5e70ecbd3ef7f963a039756f1b5d76c Mon Sep 17 00:00:00 2001 From: Nikolas De Giorgis Date: Tue, 19 Nov 2024 09:29:36 +0000 Subject: [PATCH 3/7] clean up function --- modules/apps/transfer/types/packet.go | 30 ++++++++++++++++----------- 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/modules/apps/transfer/types/packet.go b/modules/apps/transfer/types/packet.go index b3d989a1146..357bc03139c 100644 --- a/modules/apps/transfer/types/packet.go +++ b/modules/apps/transfer/types/packet.go @@ -209,19 +209,22 @@ func (ftpd FungibleTokenPacketDataV2) HasForwarding() bool { // UnmarshalPacketData attempts to unmarshal the provided packet data bytes into a FungibleTokenPacketDataV2. func UnmarshalPacketData(bz []byte, ics20Version string, encoding string) (FungibleTokenPacketDataV2, error) { - // TODO: encoding might be turned into an enum - var foo proto.Message + const failedUnmarshalingErrorMsg = "cannot unmarshal %s transfer packet data: %s" + + // Depending on the ics20 version, we use a different default encoding (json for V1, proto for V2) + // and we have a different type to unmarshal the data into.i + var data proto.Message switch ics20Version { case V1: if encoding == "" { encoding = "json" } - foo = &FungibleTokenPacketData{} + data = &FungibleTokenPacketData{} case V2: if encoding == "" { encoding = "proto" } - foo = &FungibleTokenPacketDataV2{} + data = &FungibleTokenPacketDataV2{} default: return FungibleTokenPacketDataV2{}, errorsmod.Wrap(ErrInvalidVersion, ics20Version) } @@ -233,31 +236,34 @@ func UnmarshalPacketData(bz []byte, ics20Version string, encoding string) (Fungi switch encoding { case "json": - if err := json.Unmarshal(bz, &foo); err != nil { - return FungibleTokenPacketDataV2{}, errorsmod.Wrapf(ibcerrors.ErrInvalidType, "cannot unmarshal %s transfer packet data: %s", errorMsgVersion, err.Error()) + if err := json.Unmarshal(bz, &data); err != nil { + return FungibleTokenPacketDataV2{}, errorsmod.Wrapf(ibcerrors.ErrInvalidType, failedUnmarshalingErrorMsg, errorMsgVersion, err.Error()) } case "proto": - if err := unknownproto.RejectUnknownFieldsStrict(bz, foo, unknownproto.DefaultAnyResolver{}); err != nil { - return FungibleTokenPacketDataV2{}, errorsmod.Wrapf(ibcerrors.ErrInvalidType, "cannot unmarshal %s transfer packet data: %s", errorMsgVersion, err.Error()) + if err := unknownproto.RejectUnknownFieldsStrict(bz, data, unknownproto.DefaultAnyResolver{}); err != nil { + return FungibleTokenPacketDataV2{}, errorsmod.Wrapf(ibcerrors.ErrInvalidType, failedUnmarshalingErrorMsg, errorMsgVersion, err.Error()) } - if err := proto.Unmarshal(bz, foo); err != nil { - return FungibleTokenPacketDataV2{}, errorsmod.Wrapf(ibcerrors.ErrInvalidType, "cannot unmarshal %s transfer packet data: %s", errorMsgVersion, err.Error()) + if err := proto.Unmarshal(bz, data); err != nil { + return FungibleTokenPacketDataV2{}, errorsmod.Wrapf(ibcerrors.ErrInvalidType, failedUnmarshalingErrorMsg, errorMsgVersion, err.Error()) } default: return FungibleTokenPacketDataV2{}, errorsmod.Wrapf(ibcerrors.ErrInvalidType, "invalid encoding provided, must be either empty or one of `json`,`proto`, got %s", encoding) } + // We need to branch again based on the version as we have to cast the interface to the correct type. if ics20Version == V1 { - datav1, ok := foo.(*FungibleTokenPacketData) + datav1, ok := data.(*FungibleTokenPacketData) if !ok { // We should never get here, as we manually constructed the type at the beginning of the file return FungibleTokenPacketDataV2{}, errorsmod.Wrapf(ibcerrors.ErrInvalidType, "cannot convert proto message into FungibleTokenPacketData") } + // The call to ValidateBasic for V1 is done inside PacketDataV1toV2. return PacketDataV1ToV2(*datav1) } - datav2, ok := foo.(*FungibleTokenPacketDataV2) + + datav2, ok := data.(*FungibleTokenPacketDataV2) if !ok { // We should never get here, as we manually constructed the type at the beginning of the file return FungibleTokenPacketDataV2{}, errorsmod.Wrapf(ibcerrors.ErrInvalidType, "cannot convert proto message into FungibleTokenPacketDataV2") From ebb60b20636931dd309bc39e580ca0f41394eebe Mon Sep 17 00:00:00 2001 From: Nikolas De Giorgis Date: Tue, 19 Nov 2024 11:06:55 +0000 Subject: [PATCH 4/7] more comments --- modules/apps/transfer/types/packet.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/modules/apps/transfer/types/packet.go b/modules/apps/transfer/types/packet.go index 357bc03139c..158f130c183 100644 --- a/modules/apps/transfer/types/packet.go +++ b/modules/apps/transfer/types/packet.go @@ -212,7 +212,7 @@ func UnmarshalPacketData(bz []byte, ics20Version string, encoding string) (Fungi const failedUnmarshalingErrorMsg = "cannot unmarshal %s transfer packet data: %s" // Depending on the ics20 version, we use a different default encoding (json for V1, proto for V2) - // and we have a different type to unmarshal the data into.i + // and we have a different type to unmarshal the data into. var data proto.Message switch ics20Version { case V1: @@ -234,6 +234,10 @@ func UnmarshalPacketData(bz []byte, ics20Version string, encoding string) (Fungi errorMsgVersion = "ICS20-V1" } + // Here we perform the unmarshaling based on the specified encoding. + // The functions act on the generic "data" variable which is of type proto.Message (an interface). + // The underlying type is either FungibleTokenPacketData or FungibleTokenPacketDataV2, based on the value + // of "ics20Version". switch encoding { case "json": if err := json.Unmarshal(bz, &data); err != nil { @@ -252,7 +256,9 @@ func UnmarshalPacketData(bz []byte, ics20Version string, encoding string) (Fungi return FungibleTokenPacketDataV2{}, errorsmod.Wrapf(ibcerrors.ErrInvalidType, "invalid encoding provided, must be either empty or one of `json`,`proto`, got %s", encoding) } - // We need to branch again based on the version as we have to cast the interface to the correct type. + // When the unmarshaling is done, we want to retrieve the underlying data type based on the value of ics20Version + // If it's v1, we convert the data to FungibleTokenPacketData and then call the conversion function to construct + // the v2 type. if ics20Version == V1 { datav1, ok := data.(*FungibleTokenPacketData) if !ok { @@ -263,6 +269,7 @@ func UnmarshalPacketData(bz []byte, ics20Version string, encoding string) (Fungi return PacketDataV1ToV2(*datav1) } + // If it's v2, we convert the data to FungibleTokenPacketDataV2, validate it and return it. datav2, ok := data.(*FungibleTokenPacketDataV2) if !ok { // We should never get here, as we manually constructed the type at the beginning of the file From 9e52766bf0773c3fc69bdbf12feac3996be5fc10 Mon Sep 17 00:00:00 2001 From: Nikolas De Giorgis Date: Wed, 20 Nov 2024 11:12:55 +0000 Subject: [PATCH 5/7] use MIME type and constants --- modules/apps/transfer/types/packet.go | 15 ++++++++++----- modules/apps/transfer/types/packet_test.go | 2 +- .../apps/transfer/v2/keeper/msg_server_test.go | 12 ++++++------ modules/core/04-channel/v2/types/msgs_test.go | 3 ++- modules/core/04-channel/v2/types/packet_test.go | 3 ++- testing/mock/v2/mock.go | 3 ++- 6 files changed, 23 insertions(+), 15 deletions(-) diff --git a/modules/apps/transfer/types/packet.go b/modules/apps/transfer/types/packet.go index 158f130c183..903962f7e4d 100644 --- a/modules/apps/transfer/types/packet.go +++ b/modules/apps/transfer/types/packet.go @@ -23,6 +23,11 @@ var ( _ ibcexported.PacketDataProvider = (*FungibleTokenPacketDataV2)(nil) ) +const ( + JsonEncoding = "application/json" + ProtoEncoding = "application/proto" +) + // NewFungibleTokenPacketData constructs a new FungibleTokenPacketData instance func NewFungibleTokenPacketData( denom string, amount string, @@ -217,12 +222,12 @@ func UnmarshalPacketData(bz []byte, ics20Version string, encoding string) (Fungi switch ics20Version { case V1: if encoding == "" { - encoding = "json" + encoding = JsonEncoding } data = &FungibleTokenPacketData{} case V2: if encoding == "" { - encoding = "proto" + encoding = ProtoEncoding } data = &FungibleTokenPacketDataV2{} default: @@ -239,11 +244,11 @@ func UnmarshalPacketData(bz []byte, ics20Version string, encoding string) (Fungi // The underlying type is either FungibleTokenPacketData or FungibleTokenPacketDataV2, based on the value // of "ics20Version". switch encoding { - case "json": + case JsonEncoding: if err := json.Unmarshal(bz, &data); err != nil { return FungibleTokenPacketDataV2{}, errorsmod.Wrapf(ibcerrors.ErrInvalidType, failedUnmarshalingErrorMsg, errorMsgVersion, err.Error()) } - case "proto": + case ProtoEncoding: if err := unknownproto.RejectUnknownFieldsStrict(bz, data, unknownproto.DefaultAnyResolver{}); err != nil { return FungibleTokenPacketDataV2{}, errorsmod.Wrapf(ibcerrors.ErrInvalidType, failedUnmarshalingErrorMsg, errorMsgVersion, err.Error()) } @@ -253,7 +258,7 @@ func UnmarshalPacketData(bz []byte, ics20Version string, encoding string) (Fungi } default: - return FungibleTokenPacketDataV2{}, errorsmod.Wrapf(ibcerrors.ErrInvalidType, "invalid encoding provided, must be either empty or one of `json`,`proto`, got %s", encoding) + return FungibleTokenPacketDataV2{}, errorsmod.Wrapf(ibcerrors.ErrInvalidType, "invalid encoding provided, must be either empty or one of [%q, %q], got %s", JsonEncoding, ProtoEncoding, encoding) } // When the unmarshaling is done, we want to retrieve the underlying data type based on the value of ics20Version diff --git a/modules/apps/transfer/types/packet_test.go b/modules/apps/transfer/types/packet_test.go index 780c30a6a07..7f71a3a049f 100644 --- a/modules/apps/transfer/types/packet_test.go +++ b/modules/apps/transfer/types/packet_test.go @@ -819,7 +819,7 @@ func TestV2ForwardsCompatibilityFails(t *testing.T) { tc.malleate() - packetData, err := types.UnmarshalPacketData(packetDataBz, types.V2, "proto") + packetData, err := types.UnmarshalPacketData(packetDataBz, types.V2, types.ProtoEncoding) expPass := tc.expError == nil if expPass { diff --git a/modules/apps/transfer/v2/keeper/msg_server_test.go b/modules/apps/transfer/v2/keeper/msg_server_test.go index 819d8a01866..7eb05929f7b 100644 --- a/modules/apps/transfer/v2/keeper/msg_server_test.go +++ b/modules/apps/transfer/v2/keeper/msg_server_test.go @@ -68,7 +68,7 @@ func (suite *KeeperTestSuite) TestMsgSendPacketTransfer() { bz := suite.chainA.Codec.MustMarshal(&ftpd) timestamp := suite.chainA.GetTimeoutTimestampSecs() - payload = channeltypesv2.NewPayload(transfertypes.ModuleName, transfertypes.ModuleName, transfertypes.V2, "proto", bz) + payload = channeltypesv2.NewPayload(transfertypes.ModuleName, transfertypes.ModuleName, transfertypes.V2, transfertypes.ProtoEncoding, bz) tc.malleate() @@ -163,7 +163,7 @@ func (suite *KeeperTestSuite) TestMsgRecvPacketTransfer() { bz := suite.chainA.Codec.MustMarshal(&ftpd) timestamp := suite.chainA.GetTimeoutTimestampSecs() - payload := channeltypesv2.NewPayload(transfertypes.ModuleName, transfertypes.ModuleName, transfertypes.V2, "proto", bz) + payload := channeltypesv2.NewPayload(transfertypes.ModuleName, transfertypes.ModuleName, transfertypes.V2, transfertypes.ProtoEncoding, bz) var err error packet, err = path.EndpointA.MsgSendPacket(timestamp, payload) suite.Require().NoError(err) @@ -272,7 +272,7 @@ func (suite *KeeperTestSuite) TestMsgAckPacketTransfer() { bz := suite.chainA.Codec.MustMarshal(&ftpd) timestamp := suite.chainA.GetTimeoutTimestampSecs() - payload := channeltypesv2.NewPayload(transfertypes.ModuleName, transfertypes.ModuleName, transfertypes.V2, "proto", bz) + payload := channeltypesv2.NewPayload(transfertypes.ModuleName, transfertypes.ModuleName, transfertypes.V2, transfertypes.ProtoEncoding, bz) var err error packet, err = path.EndpointA.MsgSendPacket(timestamp, payload) @@ -373,7 +373,7 @@ func (suite *KeeperTestSuite) TestMsgTimeoutPacketTransfer() { bz := suite.chainA.Codec.MustMarshal(&ftpd) timeoutTimestamp = uint64(suite.chainA.GetContext().BlockTime().Unix()) + uint64(time.Hour.Seconds()) - payload := channeltypesv2.NewPayload(transfertypes.ModuleName, transfertypes.ModuleName, transfertypes.V2, "proto", bz) + payload := channeltypesv2.NewPayload(transfertypes.ModuleName, transfertypes.ModuleName, transfertypes.V2, transfertypes.ProtoEncoding, bz) var err error packet, err = path.EndpointA.MsgSendPacket(timeoutTimestamp, payload) @@ -494,7 +494,7 @@ func (suite *KeeperTestSuite) TestV2RetainsFungibility() { bz := suite.chainB.Codec.MustMarshal(&ftpd) timeoutTimestamp := uint64(suite.chainB.GetContext().BlockTime().Unix()) + uint64(time.Hour.Seconds()) - payload := channeltypesv2.NewPayload(transfertypes.ModuleName, transfertypes.ModuleName, transfertypes.V2, "proto", bz) + payload := channeltypesv2.NewPayload(transfertypes.ModuleName, transfertypes.ModuleName, transfertypes.V2, transfertypes.ProtoEncoding, bz) var err error packetV2, err = pathv2.EndpointA.MsgSendPacket(timeoutTimestamp, payload) @@ -533,7 +533,7 @@ func (suite *KeeperTestSuite) TestV2RetainsFungibility() { bz := suite.chainC.Codec.MustMarshal(&ftpd) timeoutTimestamp := uint64(suite.chainC.GetContext().BlockTime().Unix()) + uint64(time.Hour.Seconds()) - payload := channeltypesv2.NewPayload(transfertypes.ModuleName, transfertypes.ModuleName, transfertypes.V2, "proto", bz) + payload := channeltypesv2.NewPayload(transfertypes.ModuleName, transfertypes.ModuleName, transfertypes.V2, transfertypes.ProtoEncoding, bz) var err error packetV2, err = pathv2.EndpointB.MsgSendPacket(timeoutTimestamp, payload) diff --git a/modules/core/04-channel/v2/types/msgs_test.go b/modules/core/04-channel/v2/types/msgs_test.go index fd6f5ceaa0a..92a868b90d1 100644 --- a/modules/core/04-channel/v2/types/msgs_test.go +++ b/modules/core/04-channel/v2/types/msgs_test.go @@ -6,6 +6,7 @@ import ( "github.com/stretchr/testify/suite" + transfertypes "github.com/cosmos/ibc-go/v9/modules/apps/transfer/types" clienttypes "github.com/cosmos/ibc-go/v9/modules/core/02-client/types" "github.com/cosmos/ibc-go/v9/modules/core/04-channel/v2/types" commitmenttypes "github.com/cosmos/ibc-go/v9/modules/core/23-commitment/types" @@ -205,7 +206,7 @@ func (s *TypesTestSuite) TestMsgSendPacketValidateBasic() { msg = types.NewMsgSendPacket( ibctesting.FirstChannelID, s.chainA.GetTimeoutTimestamp(), s.chainA.SenderAccount.GetAddress().String(), - types.Payload{SourcePort: ibctesting.MockPort, DestinationPort: ibctesting.MockPort, Version: "ics20-1", Encoding: "json", Value: ibctesting.MockPacketData}, + types.Payload{SourcePort: ibctesting.MockPort, DestinationPort: ibctesting.MockPort, Version: "ics20-1", Encoding: transfertypes.JsonEncoding, Value: ibctesting.MockPacketData}, ) tc.malleate() diff --git a/modules/core/04-channel/v2/types/packet_test.go b/modules/core/04-channel/v2/types/packet_test.go index 8bc1d57df38..15592589e08 100644 --- a/modules/core/04-channel/v2/types/packet_test.go +++ b/modules/core/04-channel/v2/types/packet_test.go @@ -6,6 +6,7 @@ import ( "github.com/stretchr/testify/require" + transfertypes "github.com/cosmos/ibc-go/v9/modules/apps/transfer/types" "github.com/cosmos/ibc-go/v9/modules/core/04-channel/v2/types" host "github.com/cosmos/ibc-go/v9/modules/core/24-host" ibctesting "github.com/cosmos/ibc-go/v9/testing" @@ -109,7 +110,7 @@ func TestValidateBasic(t *testing.T) { SourcePort: ibctesting.MockPort, DestinationPort: ibctesting.MockPort, Version: "ics20-v2", - Encoding: "proto", + Encoding: transfertypes.ProtoEncoding, Value: mock.MockPacketData, }) diff --git a/testing/mock/v2/mock.go b/testing/mock/v2/mock.go index 9178eb6d794..4b71fa76fd9 100644 --- a/testing/mock/v2/mock.go +++ b/testing/mock/v2/mock.go @@ -1,6 +1,7 @@ package mock import ( + transfertypes "github.com/cosmos/ibc-go/v9/modules/apps/transfer/types" channeltypesv2 "github.com/cosmos/ibc-go/v9/modules/core/04-channel/v2/types" mockv1 "github.com/cosmos/ibc-go/v9/testing/mock" ) @@ -24,7 +25,7 @@ func NewMockPayload(sourcePort, destPort string) channeltypesv2.Payload { return channeltypesv2.Payload{ SourcePort: sourcePort, DestinationPort: destPort, - Encoding: "proto", + Encoding: transfertypes.ProtoEncoding, Value: mockv1.MockPacketData, Version: mockv1.Version, } From cc85a3a5159a67a6656e29de4ee300c277ef04fe Mon Sep 17 00:00:00 2001 From: Nikolas De Giorgis Date: Wed, 20 Nov 2024 15:45:20 +0000 Subject: [PATCH 6/7] use application/x-protobuf as MIME type for proto encoding --- modules/apps/transfer/types/packet.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/apps/transfer/types/packet.go b/modules/apps/transfer/types/packet.go index 903962f7e4d..4d3ee832fab 100644 --- a/modules/apps/transfer/types/packet.go +++ b/modules/apps/transfer/types/packet.go @@ -25,7 +25,7 @@ var ( const ( JsonEncoding = "application/json" - ProtoEncoding = "application/proto" + ProtoEncoding = "application/x-protobuf" ) // NewFungibleTokenPacketData constructs a new FungibleTokenPacketData instance From 1ceab4c95e54a33a57e2d4171d2e32ed3d7dd7cf Mon Sep 17 00:00:00 2001 From: Nikolas De Giorgis Date: Wed, 20 Nov 2024 16:00:37 +0000 Subject: [PATCH 7/7] renaming and linter --- modules/apps/transfer/types/packet.go | 14 +++++++------- modules/apps/transfer/types/packet_test.go | 2 +- modules/apps/transfer/v2/keeper/msg_server_test.go | 12 ++++++------ modules/core/04-channel/v2/types/msgs_test.go | 2 +- modules/core/04-channel/v2/types/packet_test.go | 2 +- testing/mock/v2/mock.go | 2 +- 6 files changed, 17 insertions(+), 17 deletions(-) diff --git a/modules/apps/transfer/types/packet.go b/modules/apps/transfer/types/packet.go index 4d3ee832fab..968e996c08f 100644 --- a/modules/apps/transfer/types/packet.go +++ b/modules/apps/transfer/types/packet.go @@ -24,8 +24,8 @@ var ( ) const ( - JsonEncoding = "application/json" - ProtoEncoding = "application/x-protobuf" + EncodingJSON = "application/json" + EncodingProtobuf = "application/x-protobuf" ) // NewFungibleTokenPacketData constructs a new FungibleTokenPacketData instance @@ -222,12 +222,12 @@ func UnmarshalPacketData(bz []byte, ics20Version string, encoding string) (Fungi switch ics20Version { case V1: if encoding == "" { - encoding = JsonEncoding + encoding = EncodingJSON } data = &FungibleTokenPacketData{} case V2: if encoding == "" { - encoding = ProtoEncoding + encoding = EncodingProtobuf } data = &FungibleTokenPacketDataV2{} default: @@ -244,11 +244,11 @@ func UnmarshalPacketData(bz []byte, ics20Version string, encoding string) (Fungi // The underlying type is either FungibleTokenPacketData or FungibleTokenPacketDataV2, based on the value // of "ics20Version". switch encoding { - case JsonEncoding: + case EncodingJSON: if err := json.Unmarshal(bz, &data); err != nil { return FungibleTokenPacketDataV2{}, errorsmod.Wrapf(ibcerrors.ErrInvalidType, failedUnmarshalingErrorMsg, errorMsgVersion, err.Error()) } - case ProtoEncoding: + case EncodingProtobuf: if err := unknownproto.RejectUnknownFieldsStrict(bz, data, unknownproto.DefaultAnyResolver{}); err != nil { return FungibleTokenPacketDataV2{}, errorsmod.Wrapf(ibcerrors.ErrInvalidType, failedUnmarshalingErrorMsg, errorMsgVersion, err.Error()) } @@ -258,7 +258,7 @@ func UnmarshalPacketData(bz []byte, ics20Version string, encoding string) (Fungi } default: - return FungibleTokenPacketDataV2{}, errorsmod.Wrapf(ibcerrors.ErrInvalidType, "invalid encoding provided, must be either empty or one of [%q, %q], got %s", JsonEncoding, ProtoEncoding, encoding) + return FungibleTokenPacketDataV2{}, errorsmod.Wrapf(ibcerrors.ErrInvalidType, "invalid encoding provided, must be either empty or one of [%q, %q], got %s", EncodingJSON, EncodingProtobuf, encoding) } // When the unmarshaling is done, we want to retrieve the underlying data type based on the value of ics20Version diff --git a/modules/apps/transfer/types/packet_test.go b/modules/apps/transfer/types/packet_test.go index 7f71a3a049f..db166b2b8d8 100644 --- a/modules/apps/transfer/types/packet_test.go +++ b/modules/apps/transfer/types/packet_test.go @@ -819,7 +819,7 @@ func TestV2ForwardsCompatibilityFails(t *testing.T) { tc.malleate() - packetData, err := types.UnmarshalPacketData(packetDataBz, types.V2, types.ProtoEncoding) + packetData, err := types.UnmarshalPacketData(packetDataBz, types.V2, types.EncodingProtobuf) expPass := tc.expError == nil if expPass { diff --git a/modules/apps/transfer/v2/keeper/msg_server_test.go b/modules/apps/transfer/v2/keeper/msg_server_test.go index 7eb05929f7b..1e4c270c4ec 100644 --- a/modules/apps/transfer/v2/keeper/msg_server_test.go +++ b/modules/apps/transfer/v2/keeper/msg_server_test.go @@ -68,7 +68,7 @@ func (suite *KeeperTestSuite) TestMsgSendPacketTransfer() { bz := suite.chainA.Codec.MustMarshal(&ftpd) timestamp := suite.chainA.GetTimeoutTimestampSecs() - payload = channeltypesv2.NewPayload(transfertypes.ModuleName, transfertypes.ModuleName, transfertypes.V2, transfertypes.ProtoEncoding, bz) + payload = channeltypesv2.NewPayload(transfertypes.ModuleName, transfertypes.ModuleName, transfertypes.V2, transfertypes.EncodingProtobuf, bz) tc.malleate() @@ -163,7 +163,7 @@ func (suite *KeeperTestSuite) TestMsgRecvPacketTransfer() { bz := suite.chainA.Codec.MustMarshal(&ftpd) timestamp := suite.chainA.GetTimeoutTimestampSecs() - payload := channeltypesv2.NewPayload(transfertypes.ModuleName, transfertypes.ModuleName, transfertypes.V2, transfertypes.ProtoEncoding, bz) + payload := channeltypesv2.NewPayload(transfertypes.ModuleName, transfertypes.ModuleName, transfertypes.V2, transfertypes.EncodingProtobuf, bz) var err error packet, err = path.EndpointA.MsgSendPacket(timestamp, payload) suite.Require().NoError(err) @@ -272,7 +272,7 @@ func (suite *KeeperTestSuite) TestMsgAckPacketTransfer() { bz := suite.chainA.Codec.MustMarshal(&ftpd) timestamp := suite.chainA.GetTimeoutTimestampSecs() - payload := channeltypesv2.NewPayload(transfertypes.ModuleName, transfertypes.ModuleName, transfertypes.V2, transfertypes.ProtoEncoding, bz) + payload := channeltypesv2.NewPayload(transfertypes.ModuleName, transfertypes.ModuleName, transfertypes.V2, transfertypes.EncodingProtobuf, bz) var err error packet, err = path.EndpointA.MsgSendPacket(timestamp, payload) @@ -373,7 +373,7 @@ func (suite *KeeperTestSuite) TestMsgTimeoutPacketTransfer() { bz := suite.chainA.Codec.MustMarshal(&ftpd) timeoutTimestamp = uint64(suite.chainA.GetContext().BlockTime().Unix()) + uint64(time.Hour.Seconds()) - payload := channeltypesv2.NewPayload(transfertypes.ModuleName, transfertypes.ModuleName, transfertypes.V2, transfertypes.ProtoEncoding, bz) + payload := channeltypesv2.NewPayload(transfertypes.ModuleName, transfertypes.ModuleName, transfertypes.V2, transfertypes.EncodingProtobuf, bz) var err error packet, err = path.EndpointA.MsgSendPacket(timeoutTimestamp, payload) @@ -494,7 +494,7 @@ func (suite *KeeperTestSuite) TestV2RetainsFungibility() { bz := suite.chainB.Codec.MustMarshal(&ftpd) timeoutTimestamp := uint64(suite.chainB.GetContext().BlockTime().Unix()) + uint64(time.Hour.Seconds()) - payload := channeltypesv2.NewPayload(transfertypes.ModuleName, transfertypes.ModuleName, transfertypes.V2, transfertypes.ProtoEncoding, bz) + payload := channeltypesv2.NewPayload(transfertypes.ModuleName, transfertypes.ModuleName, transfertypes.V2, transfertypes.EncodingProtobuf, bz) var err error packetV2, err = pathv2.EndpointA.MsgSendPacket(timeoutTimestamp, payload) @@ -533,7 +533,7 @@ func (suite *KeeperTestSuite) TestV2RetainsFungibility() { bz := suite.chainC.Codec.MustMarshal(&ftpd) timeoutTimestamp := uint64(suite.chainC.GetContext().BlockTime().Unix()) + uint64(time.Hour.Seconds()) - payload := channeltypesv2.NewPayload(transfertypes.ModuleName, transfertypes.ModuleName, transfertypes.V2, transfertypes.ProtoEncoding, bz) + payload := channeltypesv2.NewPayload(transfertypes.ModuleName, transfertypes.ModuleName, transfertypes.V2, transfertypes.EncodingProtobuf, bz) var err error packetV2, err = pathv2.EndpointB.MsgSendPacket(timeoutTimestamp, payload) diff --git a/modules/core/04-channel/v2/types/msgs_test.go b/modules/core/04-channel/v2/types/msgs_test.go index 92a868b90d1..35165f0d72e 100644 --- a/modules/core/04-channel/v2/types/msgs_test.go +++ b/modules/core/04-channel/v2/types/msgs_test.go @@ -206,7 +206,7 @@ func (s *TypesTestSuite) TestMsgSendPacketValidateBasic() { msg = types.NewMsgSendPacket( ibctesting.FirstChannelID, s.chainA.GetTimeoutTimestamp(), s.chainA.SenderAccount.GetAddress().String(), - types.Payload{SourcePort: ibctesting.MockPort, DestinationPort: ibctesting.MockPort, Version: "ics20-1", Encoding: transfertypes.JsonEncoding, Value: ibctesting.MockPacketData}, + types.Payload{SourcePort: ibctesting.MockPort, DestinationPort: ibctesting.MockPort, Version: "ics20-1", Encoding: transfertypes.EncodingJSON, Value: ibctesting.MockPacketData}, ) tc.malleate() diff --git a/modules/core/04-channel/v2/types/packet_test.go b/modules/core/04-channel/v2/types/packet_test.go index 15592589e08..4952e03671d 100644 --- a/modules/core/04-channel/v2/types/packet_test.go +++ b/modules/core/04-channel/v2/types/packet_test.go @@ -110,7 +110,7 @@ func TestValidateBasic(t *testing.T) { SourcePort: ibctesting.MockPort, DestinationPort: ibctesting.MockPort, Version: "ics20-v2", - Encoding: transfertypes.ProtoEncoding, + Encoding: transfertypes.EncodingProtobuf, Value: mock.MockPacketData, }) diff --git a/testing/mock/v2/mock.go b/testing/mock/v2/mock.go index 4b71fa76fd9..3d2a0bd8201 100644 --- a/testing/mock/v2/mock.go +++ b/testing/mock/v2/mock.go @@ -25,7 +25,7 @@ func NewMockPayload(sourcePort, destPort string) channeltypesv2.Payload { return channeltypesv2.Payload{ SourcePort: sourcePort, DestinationPort: destPort, - Encoding: transfertypes.ProtoEncoding, + Encoding: transfertypes.EncodingProtobuf, Value: mockv1.MockPacketData, Version: mockv1.Version, }