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 2 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
7 changes: 1 addition & 6 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,5 @@ 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 {
return nil, err
}

return packetData, nil
return icatypes.PacketDataFromBytes(bz)
Copy link
Member

@damiannolan damiannolan Aug 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We could also make UnmarshalJSON a method of InterchainAccountPacketData.

func (iapd InterchainAccountPacketData) UnmarshalJSON(bz []byte) error

Usage:

var packetData icatypes.InterchainAccountPacketData
if err := packetData.UnmarshalJSON(bz); err != nil { 
   // handle err
}

Happy to defer to you guys on what API you prefer

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is also a valid approach although the function signature likely should be different. I tried this first (and called it FromBytes) but I thought the one I added looked better in the end. I'm happy to keep this PR open until we get some other opinions

func (iapd *InterchainAccountPacketData) FromBytes(bz []byte) error

Copy link
Member

Choose a reason for hiding this comment

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

although the function signature likely should be different

Aha, yes, sorry for my typo! Indeed, you don't need to return (InterchainAccountPacketData, error) since you're acting on unmarshaling into the receiver. I'll edit my first comment! 😛

I think UnmarshalJSON naming is pretty common/standard in golang and in cosmos. Here's an example in x/staking and x/auth

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, sounds good. I'll do this

Copy link
Member Author

Choose a reason for hiding this comment

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

Note: I did this

}
2 changes: 1 addition & 1 deletion modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -939,5 +939,5 @@ func (suite *InterchainAccountsTestSuite) TestPacketDataUnmarshalerInterface() {
invalidPacketData := []byte("invalid packet data")
packetData, err = controller.IBCMiddleware{}.UnmarshalPacketData(invalidPacketData)
suite.Require().Error(err)
suite.Require().Nil(packetData)
suite.Require().Equal(icatypes.InterchainAccountPacketData{}, packetData)
}
5 changes: 2 additions & 3 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 @@ -17,9 +17,8 @@ import (
// OnRecvPacket handles a given interchain accounts packet on a destination host chain.
// 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 {
data, err := icatypes.PacketDataFromBytes(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
10 changes: 10 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,16 @@ func (iapd InterchainAccountPacketData) GetBytes() []byte {
return sdk.MustSortJSON(ModuleCdc.MustMarshalJSON(&iapd))
}

// PacketDataFromBytes returns the interchain account packet data from a JSON marshalled byte slice.
func PacketDataFromBytes(bz []byte) (InterchainAccountPacketData, error) {
var iapd InterchainAccountPacketData
err := ModuleCdc.UnmarshalJSON(bz, &iapd)
if err != nil {
return InterchainAccountPacketData{}, err
}
return iapd, nil
}

// GetBytes returns the JSON marshalled interchain account CosmosTx.
func (ct CosmosTx) GetBytes() []byte {
return sdk.MustSortJSON(ModuleCdc.MustMarshalJSON(&ct))
Expand Down
18 changes: 18 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,21 @@ 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",
}

packetData, err := types.PacketDataFromBytes(expPacketData.GetBytes())
suite.Require().NoError(err)
suite.Require().Equal(expPacketData, packetData)

// test invalid packet data
invalidPacketData := []byte("invalid packet data")
packetData, err = types.PacketDataFromBytes(invalidPacketData)
suite.Require().Error(err)
suite.Require().Equal(types.InterchainAccountPacketData{}, packetData)
}