From 4a3a3554438a4f8d98f8f6518b2dabfb274e573d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 2 Nov 2022 11:36:17 +0100 Subject: [PATCH 1/5] refactor: introduce forked function for ics20 packet data marshals Copied function from https://github.com/cosmos/cosmos-sdk/blob/971c542453e0972ef1dfc5a80159ad5049c7211c/codec/json.go Modified contents to set EmitDefaults to false, minimized code changes to avoid introducing unnecessary bugs --- go.mod | 1 + go.sum | 2 ++ modules/apps/transfer/types/codec.go | 33 +++++++++++++++++++++++++++ modules/apps/transfer/types/packet.go | 2 +- 4 files changed, 37 insertions(+), 1 deletion(-) diff --git a/go.mod b/go.mod index be0769050e4..2dd6052fbf9 100644 --- a/go.mod +++ b/go.mod @@ -9,6 +9,7 @@ require ( github.com/armon/go-metrics v0.4.1 github.com/confio/ics23/go v0.7.0 github.com/cosmos/cosmos-sdk v0.46.2 + github.com/cosmos/gogoproto v1.4.2 github.com/gogo/protobuf v1.3.3 github.com/golang/protobuf v1.5.2 github.com/gorilla/mux v1.8.0 diff --git a/go.sum b/go.sum index ca4224ce7b9..56690ffe414 100644 --- a/go.sum +++ b/go.sum @@ -239,6 +239,8 @@ github.com/cosmos/cosmos-sdk v0.46.2/go.mod h1:0aUPGPU6PWaDEaHNjtgrpNhgxo9bAUrQ7 github.com/cosmos/go-bip39 v0.0.0-20180819234021-555e2067c45d/go.mod h1:tSxLoYXyBmiFeKpvmq4dzayMdCjCnu8uqmCysIGBT2Y= github.com/cosmos/go-bip39 v1.0.0 h1:pcomnQdrdH22njcAatO0yWojsUnCO3y2tNoV1cb6hHY= github.com/cosmos/go-bip39 v1.0.0/go.mod h1:RNJv0H/pOIVgxw6KS7QeX2a0Uo0aKUlfhZ4xuwvCdJw= +github.com/cosmos/gogoproto v1.4.2 h1:UeGRcmFW41l0G0MiefWhkPEVEwvu78SZsHBvI78dAYw= +github.com/cosmos/gogoproto v1.4.2/go.mod h1:cLxOsn1ljAHSV527CHOtaIP91kK6cCrZETRBrkzItWU= github.com/cosmos/gorocksdb v1.2.0 h1:d0l3jJG8M4hBouIZq0mDUHZ+zjOx044J3nGRskwTb4Y= github.com/cosmos/gorocksdb v1.2.0/go.mod h1:aaKvKItm514hKfNJpUJXnnOWeBnk2GL4+Qw9NHizILw= github.com/cosmos/iavl v0.19.2-0.20220916140702-9b6be3095313 h1:R7CnaI/0OLwOusy7n9750n8fqQ3yCQ8OJQI2L3ws9RA= diff --git a/modules/apps/transfer/types/codec.go b/modules/apps/transfer/types/codec.go index 24ad7e5a902..b3cf0de3201 100644 --- a/modules/apps/transfer/types/codec.go +++ b/modules/apps/transfer/types/codec.go @@ -1,10 +1,14 @@ package types import ( + "bytes" + "github.com/cosmos/cosmos-sdk/codec" codectypes "github.com/cosmos/cosmos-sdk/codec/types" sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/types/msgservice" + "github.com/gogo/protobuf/jsonpb" + "github.com/gogo/protobuf/proto" ) // RegisterLegacyAminoCodec registers the necessary x/ibc transfer interfaces and concrete types @@ -39,3 +43,32 @@ func init() { RegisterLegacyAminoCodec(amino) amino.Seal() } + +// mustProtoMarshalJSON provides an auxiliary function to return Proto3 JSON encoded +// bytes of a message. +// NOTE: Copied from https://github.com/cosmos/cosmos-sdk/blob/971c542453e0972ef1dfc5a80159ad5049c7211c/codec/json.go +// and modified in order to allow `EmitDefaults` to be set to false for ics20 packet marshaling. +// This allows for the introduction of the memo field to be backwards compatible. +func mustProtoMarshalJSON(msg proto.Message) []byte { + anyResolver := codectypes.NewInterfaceRegistry() + + // EmitDefaults is set to false to prevent marshaling of unpopulated fields (memo) + // OrigName and the anyResovler match the fields the original SDK function would expect + // in order to minimize changes. + + // OrigName is true since there is no particular reason to use camel case + // The any resolver is empty, but provided anyways. + jm := &jsonpb.Marshaler{OrigName: true, EmitDefaults: false, AnyResolver: anyResolver} + + err := codectypes.UnpackInterfaces(msg, codectypes.ProtoJSONPacker{JSONPBMarshaler: jm}) + if err != nil { + panic(err) + } + + buf := new(bytes.Buffer) + if err := jm.Marshal(buf, msg); err != nil { + panic(err) + } + + return buf.Bytes() +} diff --git a/modules/apps/transfer/types/packet.go b/modules/apps/transfer/types/packet.go index 4708990310b..aed80c6043b 100644 --- a/modules/apps/transfer/types/packet.go +++ b/modules/apps/transfer/types/packet.go @@ -58,5 +58,5 @@ func (ftpd FungibleTokenPacketData) ValidateBasic() error { // GetBytes is a helper for serialising func (ftpd FungibleTokenPacketData) GetBytes() []byte { - return sdk.MustSortJSON(ModuleCdc.MustMarshalJSON(&ftpd)) + return sdk.MustSortJSON(mustProtoMarshalJSON(&ftpd)) } From 74b4be0136327235fbcfdf1f7dcf8eef75c44988 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 2 Nov 2022 11:49:57 +0100 Subject: [PATCH 2/5] add changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a8028adc51c..b9732b0c0eb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -70,6 +70,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### State Machine Breaking +* (apps/transfer) [\#2651](https://github.com/cosmos/ibc-go/pull/2651) Introduce `mustProtoMarshalJSON` for ics20 packet data marshaling which will skip emission (marshaling) of the memo field if unpopulated (empty). * (27-interchain-accounts) [\#2580](https://github.com/cosmos/ibc-go/issues/2580) Removing port prefix requirement from the ICA host channel handshake * (transfer) [\#2377](https://github.com/cosmos/ibc-go/pull/2377) Adding `sequence` to `MsgTransferResponse`. * (light-clients/07-tendermint) [\#2554](https://github.com/cosmos/ibc-go/pull/2554) Forbid negative values for `TrustingPeriod`, `UnbondingPeriod` and `MaxClockDrift` (as specified in ICS-07). From 527748d871327c469fd315951100d561066eafe7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 2 Nov 2022 11:51:02 +0100 Subject: [PATCH 3/5] go mod tidy --- go.mod | 1 - go.sum | 2 -- 2 files changed, 3 deletions(-) diff --git a/go.mod b/go.mod index 2dd6052fbf9..be0769050e4 100644 --- a/go.mod +++ b/go.mod @@ -9,7 +9,6 @@ require ( github.com/armon/go-metrics v0.4.1 github.com/confio/ics23/go v0.7.0 github.com/cosmos/cosmos-sdk v0.46.2 - github.com/cosmos/gogoproto v1.4.2 github.com/gogo/protobuf v1.3.3 github.com/golang/protobuf v1.5.2 github.com/gorilla/mux v1.8.0 diff --git a/go.sum b/go.sum index 56690ffe414..ca4224ce7b9 100644 --- a/go.sum +++ b/go.sum @@ -239,8 +239,6 @@ github.com/cosmos/cosmos-sdk v0.46.2/go.mod h1:0aUPGPU6PWaDEaHNjtgrpNhgxo9bAUrQ7 github.com/cosmos/go-bip39 v0.0.0-20180819234021-555e2067c45d/go.mod h1:tSxLoYXyBmiFeKpvmq4dzayMdCjCnu8uqmCysIGBT2Y= github.com/cosmos/go-bip39 v1.0.0 h1:pcomnQdrdH22njcAatO0yWojsUnCO3y2tNoV1cb6hHY= github.com/cosmos/go-bip39 v1.0.0/go.mod h1:RNJv0H/pOIVgxw6KS7QeX2a0Uo0aKUlfhZ4xuwvCdJw= -github.com/cosmos/gogoproto v1.4.2 h1:UeGRcmFW41l0G0MiefWhkPEVEwvu78SZsHBvI78dAYw= -github.com/cosmos/gogoproto v1.4.2/go.mod h1:cLxOsn1ljAHSV527CHOtaIP91kK6cCrZETRBrkzItWU= github.com/cosmos/gorocksdb v1.2.0 h1:d0l3jJG8M4hBouIZq0mDUHZ+zjOx044J3nGRskwTb4Y= github.com/cosmos/gorocksdb v1.2.0/go.mod h1:aaKvKItm514hKfNJpUJXnnOWeBnk2GL4+Qw9NHizILw= github.com/cosmos/iavl v0.19.2-0.20220916140702-9b6be3095313 h1:R7CnaI/0OLwOusy7n9750n8fqQ3yCQ8OJQI2L3ws9RA= From 08679a4d997527dcb7462338f716cc07959dfbc9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 2 Nov 2022 11:52:27 +0100 Subject: [PATCH 4/5] nit: fix spelling --- CHANGELOG.md | 2 +- modules/apps/transfer/types/codec.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b9732b0c0eb..8ea0f084d09 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -70,7 +70,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### State Machine Breaking -* (apps/transfer) [\#2651](https://github.com/cosmos/ibc-go/pull/2651) Introduce `mustProtoMarshalJSON` for ics20 packet data marshaling which will skip emission (marshaling) of the memo field if unpopulated (empty). +* (apps/transfer) [\#2651](https://github.com/cosmos/ibc-go/pull/2651) Introduce `mustProtoMarshalJSON` for ics20 packet data marshalling which will skip emission (marshalling) of the memo field if unpopulated (empty). * (27-interchain-accounts) [\#2580](https://github.com/cosmos/ibc-go/issues/2580) Removing port prefix requirement from the ICA host channel handshake * (transfer) [\#2377](https://github.com/cosmos/ibc-go/pull/2377) Adding `sequence` to `MsgTransferResponse`. * (light-clients/07-tendermint) [\#2554](https://github.com/cosmos/ibc-go/pull/2554) Forbid negative values for `TrustingPeriod`, `UnbondingPeriod` and `MaxClockDrift` (as specified in ICS-07). diff --git a/modules/apps/transfer/types/codec.go b/modules/apps/transfer/types/codec.go index b3cf0de3201..3067cf8dc9d 100644 --- a/modules/apps/transfer/types/codec.go +++ b/modules/apps/transfer/types/codec.go @@ -47,12 +47,12 @@ func init() { // mustProtoMarshalJSON provides an auxiliary function to return Proto3 JSON encoded // bytes of a message. // NOTE: Copied from https://github.com/cosmos/cosmos-sdk/blob/971c542453e0972ef1dfc5a80159ad5049c7211c/codec/json.go -// and modified in order to allow `EmitDefaults` to be set to false for ics20 packet marshaling. +// and modified in order to allow `EmitDefaults` to be set to false for ics20 packet marshalling. // This allows for the introduction of the memo field to be backwards compatible. func mustProtoMarshalJSON(msg proto.Message) []byte { anyResolver := codectypes.NewInterfaceRegistry() - // EmitDefaults is set to false to prevent marshaling of unpopulated fields (memo) + // EmitDefaults is set to false to prevent marshalling of unpopulated fields (memo) // OrigName and the anyResovler match the fields the original SDK function would expect // in order to minimize changes. From bd9921f4997e855c99ec018f8cbd83b3942e4c1b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 2 Nov 2022 13:37:36 +0100 Subject: [PATCH 5/5] test: add test for emitted and non-emitted memo field --- modules/apps/transfer/types/codec_test.go | 25 +++++++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 modules/apps/transfer/types/codec_test.go diff --git a/modules/apps/transfer/types/codec_test.go b/modules/apps/transfer/types/codec_test.go new file mode 100644 index 00000000000..b5e9c2515ed --- /dev/null +++ b/modules/apps/transfer/types/codec_test.go @@ -0,0 +1,25 @@ +package types_test + +import ( + "strings" + + sdk "github.com/cosmos/cosmos-sdk/types" + + "github.com/cosmos/ibc-go/v6/modules/apps/transfer/types" +) + +// TestMustMarshalProtoJSON tests that the memo field is only emitted (marshalled) if it is populated +func (suite *TypesTestSuite) TestMustMarshalProtoJSON() { + memo := "memo" + packetData := types.NewFungibleTokenPacketData(sdk.DefaultBondDenom, "1", suite.chainA.SenderAccount.GetAddress().String(), suite.chainB.SenderAccount.GetAddress().String(), memo) + + bz := packetData.GetBytes() + exists := strings.Contains(string(bz), memo) + suite.Require().True(exists) + + packetData.Memo = "" + + bz = packetData.GetBytes() + exists = strings.Contains(string(bz), memo) + suite.Require().False(exists) +}