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

refactor(ica): packet data unmarshaling logic refactored #4232

Merged
merged 13 commits into from
Aug 21, 2023
Merged

refactor(ica): packet data unmarshaling logic refactored #4232

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
068f0da
refactor(ica): refactored packet data's unmarshal logic
srdtrk Aug 2, 2023
8f0f35e
fix(ica_test): made tests pass
srdtrk Aug 2, 2023
ab9a619
Merge branch 'main' into serdar/issue#4224-ica-refactor-unmarshalpack…
srdtrk Aug 8, 2023
d3b2775
Merge branch 'main' into serdar/issue#4224-ica-refactor-unmarshalpack…
srdtrk Aug 9, 2023
87e6549
Merge branch 'main' into serdar/issue#4224-ica-refactor-unmarshalpack…
srdtrk Aug 14, 2023
e58aa8d
merge: branch 'main' into serdar/issue#4224-ica-refactor-unmarshalpac…
srdtrk Aug 14, 2023
ae95265
imp(ica): changed to UnmarshalJSON api
srdtrk Aug 14, 2023
329be08
merge: branch 'main' into serdar/issue#4224-ica-refactor-unmarshalpac…
srdtrk Aug 14, 2023
01619ac
Merge branch 'main' into serdar/issue#4224-ica-refactor-unmarshalpack…
srdtrk Aug 15, 2023
2847751
Merge branch 'main' into serdar/issue#4224-ica-refactor-unmarshalpack…
srdtrk Aug 16, 2023
6bc3d9e
Merge branch 'main' into serdar/issue#4224-ica-refactor-unmarshalpack…
srdtrk Aug 17, 2023
bc5e40d
merge: branch 'main' into serdar/issue#4224-ica-refactor-unmarshalpac…
srdtrk Aug 21, 2023
dc40ae1
docs(ica): added godocs to iapd's 'UnmarshalJSON' method
srdtrk Aug 21, 2023
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
8 changes: 4 additions & 4 deletions modules/apps/27-interchain-accounts/controller/ibc_middleware.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,10 +261,10 @@ func (im IBCMiddleware) GetAppVersion(ctx sdk.Context, portID, channelID string)
// into an InterchainAccountPacketData. This function implements the optional
// PacketDataUnmarshaler interface required for ADR 008 support.
func (IBCMiddleware) UnmarshalPacketData(bz []byte) (interface{}, error) {
var packetData icatypes.InterchainAccountPacketData
if err := icatypes.ModuleCdc.UnmarshalJSON(bz, &packetData); err != nil {
var data icatypes.InterchainAccountPacketData
err := data.UnmarshalJSON(bz)
if err != nil {
return nil, err
}

return packetData, nil
return data, nil
}
4 changes: 2 additions & 2 deletions modules/apps/27-interchain-accounts/host/keeper/relay.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@ import (
// If the transaction is successfully executed, the transaction response bytes will be returned.
func (k Keeper) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet) ([]byte, error) {
var data icatypes.InterchainAccountPacketData

if err := icatypes.ModuleCdc.UnmarshalJSON(packet.GetData(), &data); err != nil {
err := data.UnmarshalJSON(packet.GetData())
if err != nil {
// UnmarshalJSON errors are indeterminate and therefore are not wrapped and included in failed acks
return nil, errorsmod.Wrapf(icatypes.ErrUnknownDataType, "cannot unmarshal ICS-27 interchain account packet data")
}
Expand Down
5 changes: 5 additions & 0 deletions modules/apps/27-interchain-accounts/types/packet.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,11 @@ func (iapd InterchainAccountPacketData) GetBytes() []byte {
return sdk.MustSortJSON(ModuleCdc.MustMarshalJSON(&iapd))
}

// UnmarshalJSON unmarshals raw JSON bytes into an InterchainAccountPacketData.
func (iapd *InterchainAccountPacketData) UnmarshalJSON(bz []byte) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am fine with this approach, but maybe an alternative could be to have instead and constructor function if that's possible? Like this:

func NewInterchainAccountPacketData(bz []byte) (InterchainAccountPacketData, error) {
  var packetData icatypes.InterchainAccountPacketData
  if err := ModuleCdc.UnmarshalJSON(bz, &packetData); err != nil {
    return nil, err
  }
  return packetData, nil
}

It just feels more natural to me that declaring the value and then mutating it. That way we don't mix value and pointer receiver functions in the type, which I think it's another go proverb? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

This is exactly what I did originally, but @damiannolan and @DimitrisJim were slightly favouring the current approach. I'm curious what others think. Regardless, I apparently can't merge this until ica code owners approve it

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, funny, haha! :) Well, if the majority prefers the current approach, I am ok with it.

Copy link
Member

Choose a reason for hiding this comment

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

Could we add a godoc? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

done

return ModuleCdc.UnmarshalJSON(bz, iapd)
}

// GetBytes returns the JSON marshalled interchain account CosmosTx.
func (ct CosmosTx) GetBytes() []byte {
return sdk.MustSortJSON(ModuleCdc.MustMarshalJSON(&ct))
Expand Down
21 changes: 21 additions & 0 deletions modules/apps/27-interchain-accounts/types/packet_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,3 +180,24 @@ func (suite *TypesTestSuite) TestPacketDataProvider() {
suite.Require().Equal(tc.expCustomData, customData)
}
}

func (suite *TypesTestSuite) TestPacketDataUnmarshalerInterface() {
expPacketData := types.InterchainAccountPacketData{
Type: types.EXECUTE_TX,
Data: []byte("data"),
Memo: "some memo",
}

var packetData types.InterchainAccountPacketData
err := packetData.UnmarshalJSON(expPacketData.GetBytes())
suite.Require().NoError(err)
suite.Require().Equal(expPacketData, packetData)

// test invalid packet data
invalidPacketDataBytes := []byte("invalid packet data")

var invalidPacketData types.InterchainAccountPacketData
err = packetData.UnmarshalJSON(invalidPacketDataBytes)
suite.Require().Error(err)
suite.Require().Equal(types.InterchainAccountPacketData{}, invalidPacketData)
}