From 393247bff61c225a29fab7aa002877e6653ab415 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:42:49 +0100 Subject: [PATCH] fix: skip emission of unpopulated memo field in ics20 (#2651) ## Description By setting `EmitDefaults` to false, we will not include an empty memo field in the marshaled json bytes closes: #2645 --- Before we can merge this PR, please make sure that all the following items have been checked off. If any of the checklist items are not applicable, please leave them but write a little note why. - [x] Targeted PR against correct branch (see [CONTRIBUTING.md](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] Linked to Github issue with discussion and accepted design OR link to spec that describes this work. - [ ] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/main/docs/docs/building-modules/10-structure.md). - [ ] Wrote unit and integration [tests](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#testing) - [ ] Updated relevant documentation (`docs/`) or specification (`x//spec/`) - [ ] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code). - [ ] Added a relevant changelog entry to the `Unreleased` section in `CHANGELOG.md` - [ ] Re-reviewed `Files changed` in the Github PR explorer - [ ] Review `Codecov Report` in the comment section below once CI passes --- CHANGELOG.md | 1 + modules/apps/transfer/types/codec.go | 33 +++++++++++++++++++++++ modules/apps/transfer/types/codec_test.go | 25 +++++++++++++++++ modules/apps/transfer/types/packet.go | 2 +- 4 files changed, 60 insertions(+), 1 deletion(-) create mode 100644 modules/apps/transfer/types/codec_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index a8028adc51c..8ea0f084d09 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 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 24ad7e5a902..3067cf8dc9d 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 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 marshalling 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/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) +} 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)) }