Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

imp: Use json for marshalling/unmarshalling transfer packet data. #5778

Merged
merged 5 commits into from
Feb 1, 2024
Merged

imp: Use json for marshalling/unmarshalling transfer packet data. #5778

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e4e4bed
Use json for marshalling/unmarshalling transfer packet data.
DimitrisJim Jan 31, 2024
9d4da5e
test: add test case for checking that memo is included if populated.
DimitrisJim Jan 31, 2024
1930eda
Merge branch 'main' into jim/3915-use-json-for-transfer-packet-data
DimitrisJim Jan 31, 2024
c700f6e
Merge branch 'main' into jim/3915-use-json-for-transfer-packet-data
DimitrisJim Jan 31, 2024
b8b38a2
Merge branch 'main' into jim/3915-use-json-for-transfer-packet-data
DimitrisJim Feb 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion modules/apps/callbacks/ibc_middleware_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package ibccallbacks_test

import (
"encoding/json"
"fmt"

errorsmod "cosmossdk.io/errors"
Expand Down Expand Up @@ -477,7 +478,7 @@ func (s *CallbacksTestSuite) TestOnTimeoutPacket() {
s.Require().NoError(err)
s.Require().NotNil(packet)

err = transfertypes.ModuleCdc.UnmarshalJSON(packet.Data, &packetData)
err = json.Unmarshal(packet.Data, &packetData)
s.Require().NoError(err)

ctx = s.chainA.GetContext()
Expand Down
9 changes: 5 additions & 4 deletions modules/apps/transfer/ibc_module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package transfer

import (
"encoding/json"
"fmt"
"math"
"strings"
Expand Down Expand Up @@ -182,7 +183,7 @@ func (im IBCModule) OnRecvPacket(

var data types.FungibleTokenPacketData
var ackErr error
if err := types.ModuleCdc.UnmarshalJSON(packet.GetData(), &data); err != nil {
if err := json.Unmarshal(packet.GetData(), &data); err != nil {
ackErr = errorsmod.Wrapf(ibcerrors.ErrInvalidType, "cannot unmarshal ICS-20 transfer packet data")
logger.Error(fmt.Sprintf("%s sequence %d", ackErr.Error(), packet.Sequence))
ack = channeltypes.NewErrorAcknowledgement(ackErr)
Expand Down Expand Up @@ -238,7 +239,7 @@ func (im IBCModule) OnAcknowledgementPacket(
return errorsmod.Wrapf(ibcerrors.ErrUnknownRequest, "cannot unmarshal ICS-20 transfer packet acknowledgement: %v", err)
}
var data types.FungibleTokenPacketData
if err := types.ModuleCdc.UnmarshalJSON(packet.GetData(), &data); err != nil {
if err := json.Unmarshal(packet.GetData(), &data); err != nil {
return errorsmod.Wrapf(ibcerrors.ErrUnknownRequest, "cannot unmarshal ICS-20 transfer packet data: %s", err.Error())
}

Expand Down Expand Up @@ -286,7 +287,7 @@ func (im IBCModule) OnTimeoutPacket(
relayer sdk.AccAddress,
) error {
var data types.FungibleTokenPacketData
if err := types.ModuleCdc.UnmarshalJSON(packet.GetData(), &data); err != nil {
if err := json.Unmarshal(packet.GetData(), &data); err != nil {
return errorsmod.Wrapf(ibcerrors.ErrUnknownRequest, "cannot unmarshal ICS-20 transfer packet data: %s", err.Error())
}
// refund tokens
Expand Down Expand Up @@ -352,7 +353,7 @@ func (IBCModule) OnChanUpgradeOpen(ctx sdk.Context, portID, channelID string, pr
// PacketDataUnmarshaler interface required for ADR 008 support.
func (IBCModule) UnmarshalPacketData(bz []byte) (interface{}, error) {
var packetData types.FungibleTokenPacketData
if err := types.ModuleCdc.UnmarshalJSON(bz, &packetData); err != nil {
if err := json.Unmarshal(bz, &packetData); err != nil {
return nil, err
}

Expand Down
34 changes: 0 additions & 34 deletions modules/apps/transfer/types/codec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,6 @@
package types

import (
"bytes"

"github.com/cosmos/gogoproto/jsonpb"
"github.com/cosmos/gogoproto/proto"

"github.com/cosmos/cosmos-sdk/codec"
"github.com/cosmos/cosmos-sdk/codec/legacy"
codectypes "github.com/cosmos/cosmos-sdk/codec/types"
Expand Down Expand Up @@ -39,32 +34,3 @@ func RegisterInterfaces(registry codectypes.InterfaceRegistry) {
// The actual codec used for serialization should be provided to x/ibc transfer and
// defined at the application level.
var ModuleCdc = codec.NewProtoCodec(codectypes.NewInterfaceRegistry())

// 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()
}
14 changes: 10 additions & 4 deletions modules/apps/transfer/types/packet.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,13 @@ package types

import (
"encoding/json"
"errors"
"strings"
"time"

errorsmod "cosmossdk.io/errors"
sdkmath "cosmossdk.io/math"

sdk "github.com/cosmos/cosmos-sdk/types"

ibcerrors "github.com/cosmos/ibc-go/v8/modules/core/errors"
ibcexported "github.com/cosmos/ibc-go/v8/modules/core/exported"
)
Expand Down Expand Up @@ -67,9 +66,16 @@ func (ftpd FungibleTokenPacketData) ValidateBasic() error {
return ValidatePrefixedDenom(ftpd.Denom)
}

// GetBytes is a helper for serialising
// GetBytes is a helper for serialising the packet to bytes.
// The memo field of FungibleTokenPacketData is marked with the JSON omitempty tag
// ensuring that the memo field is not included in the marshalled bytes if one is not specified.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

though one thing I did notice is that Unmarshal seems to work just fine using an old type w/o memo defined even if marshalled bytes contain the memo, i.e:

// Check that we can unmarshal into another type that does not have memo defined
type PacketData struct {
	Denom    string `json:"denom"`
	Amount   string `json:"amount"`
	Sender   string `json:"sender"`
	Receiver string `json:"receiver"`
}

ftpdWithMemo = types.FungibleTokenPacketData{
	Denom:    denom,
	Amount:   amount,
	Sender:   sender,
	Receiver: receiver,
	Memo:     "{memo: \"test\"}",
}

bz, err = json.Marshal(ftpdWithMemo)
suite.Require().NoError(err)

var packetData2 PacketData
err = json.Unmarshal(bz, &packetData2)

suite.Require().NoError(err)

no clue if this is the case with jsonpb tho

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

must be an additional check done when marshaling protoJSON

func (ftpd FungibleTokenPacketData) GetBytes() []byte {
return sdk.MustSortJSON(mustProtoMarshalJSON(&ftpd))
bz, err := json.Marshal(ftpd)
if err != nil {
panic(errors.New("cannot marshal FungibleTokenPacketData into bytes"))
}

return bz
}

// GetPacketSender returns the sender address embedded in the packet data.
Expand Down
25 changes: 25 additions & 0 deletions modules/apps/transfer/types/packet_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package types_test

import (
"encoding/json"
"fmt"
"testing"

Expand Down Expand Up @@ -134,3 +135,27 @@ func (suite *TypesTestSuite) TestPacketDataProvider() {
suite.Require().Equal(tc.expCustomData, customData)
}
}

func (suite *TypesTestSuite) TestFungibleTokenPacketDataOmitEmpty() {
// check that omitempty is present for the memo field
packetData := types.FungibleTokenPacketData{
Denom: denom,
Amount: amount,
Sender: sender,
Receiver: receiver,
// Default value for non-specified memo field is empty string
}

bz, err := json.Marshal(packetData)
suite.Require().NoError(err)

// check that the memo field is not present in the marshalled bytes
suite.Require().NotContains(string(bz), "memo")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just to double check, if you did have the memo set to "abc", this would fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea! just added an additional commit to do that check.


packetData.Memo = "abc"
bz, err = json.Marshal(packetData)
suite.Require().NoError(err)

// check that the memo field is present in the marshalled bytes
suite.Require().Contains(string(bz), "memo")
}
Loading