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

Jump to bottom

imp: add UnmarshalPacketData interface function #3353

Merged
merged 27 commits into from
Mar 29, 2023
Merged

imp: add UnmarshalPacketData interface function #3353

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
bf93c0e
adr 8 with 20/27 implementation
AdityaSripal Mar 13, 2023
8c16e8d
change interface name and register codecs
AdityaSripal Mar 13, 2023
3500d06
documentation
AdityaSripal Mar 13, 2023
89f608c
add comma before new line
crodriguezvega Mar 18, 2023
b77cfe6
fix return arg and dest for ica
AdityaSripal Mar 22, 2023
1db4626
add ica tests
AdityaSripal Mar 22, 2023
32b74ec
Merge branch 'adr-8-interface-impl' of github.com:cosmos/ibc-go into …
AdityaSripal Mar 22, 2023
f8298da
adr 8 callback packet data impl followups (#3325)
colin-axner Mar 27, 2023
f826ddd
Merge branch 'main' into adr-8-interface-impl
colin-axner Mar 27, 2023
ca33576
fix: merge conflicts
colin-axner Mar 27, 2023
6bf400e
chore: nits from self review
colin-axner Mar 27, 2023
19ab301
imp: add UnmarshalPacketData interface function
colin-axner Mar 27, 2023
1a1a082
test: add tests for ica
colin-axner Mar 28, 2023
07cc36a
test: add remaining tests
colin-axner Mar 28, 2023
9d9e33a
adr 8 with 20/27 implementation
AdityaSripal Mar 13, 2023
882dc95
change interface name and register codecs
AdityaSripal Mar 13, 2023
bfb359d
documentation
AdityaSripal Mar 13, 2023
27f2447
fix return arg and dest for ica
AdityaSripal Mar 22, 2023
d80eb6f
add ica tests
AdityaSripal Mar 22, 2023
16a8e84
adr 8 callback packet data impl followups (#3325)
colin-axner Mar 27, 2023
c4987af
fix: merge conflicts
colin-axner Mar 27, 2023
d4bae04
chore: nits from self review
colin-axner Mar 27, 2023
9919cb1
imp: add UnmarshalPacketData interface function
colin-axner Mar 27, 2023
06916c5
test: add tests for ica
colin-axner Mar 28, 2023
bec5a03
test: add remaining tests
colin-axner Mar 28, 2023
6781f20
Merge branch 'adr-8-callbacks-interface-impl' of github.com:cosmos/ib…
colin-axner Mar 29, 2023
1f04b6d
Merge branch 'colin/unmarshal-packet-data-interface' of github.com:co…
colin-axner Mar 29, 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
17 changes: 16 additions & 1 deletion 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 @@ -16,7 +16,10 @@ import (
ibcexported "github.com/cosmos/ibc-go/v7/modules/core/exported"
)

var _ porttypes.Middleware = &IBCMiddleware{}
var (
_ porttypes.Middleware = &IBCMiddleware{}
_ porttypes.PacketDataUnmarshaler = &IBCMiddleware{}
)

// IBCMiddleware implements the ICS26 callbacks for the fee middleware given the
// ICA controller keeper and the underlying application.
Expand Down Expand Up @@ -252,3 +255,15 @@ func (im IBCMiddleware) WriteAcknowledgement(
func (im IBCMiddleware) GetAppVersion(ctx sdk.Context, portID, channelID string) (string, bool) {
return im.keeper.GetAppVersion(ctx, portID, channelID)
}

// UnmarshalPacketData attempts to unmarshal the provided packet data bytes
// into an InterchainAccountPacketData. This function implements the optional
// PacketDataUnmarshaler interface required for ADR 008 support.
func (im 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
}
17 changes: 17 additions & 0 deletions 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 @@ -917,3 +917,20 @@ func (suite *InterchainAccountsTestSuite) TestClosedChannelReopensWithMsgServer(
err = path.EndpointB.ChanOpenConfirm()
suite.Require().NoError(err)
}

func (suite *InterchainAccountsTestSuite) TestUnmarshalPacketData() {
expPacketData := icatypes.InterchainAccountPacketData{
Type: icatypes.EXECUTE_TX,
Data: []byte("data"),
Memo: `{"callbacks": {"src_callback_address": "testAddr"}}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

}

packetData, err := controller.IBCMiddleware{}.UnmarshalPacketData(expPacketData.GetBytes())
suite.Require().NoError(err)
suite.Require().Equal(expPacketData, packetData)

invalidPacketData := []byte("invalid packet data")
packetData, err = controller.IBCMiddleware{}.UnmarshalPacketData(invalidPacketData)
suite.Require().Error(err)
suite.Require().Nil(packetData)
}
18 changes: 18 additions & 0 deletions modules/apps/27-interchain-accounts/host/ibc_module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,15 @@ import (
"github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/host/types"
icatypes "github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/types"
channeltypes "github.com/cosmos/ibc-go/v7/modules/core/04-channel/types"
porttypes "github.com/cosmos/ibc-go/v7/modules/core/05-port/types"
ibcexported "github.com/cosmos/ibc-go/v7/modules/core/exported"
)

var (
_ porttypes.IBCModule = &IBCModule{}
_ porttypes.PacketDataUnmarshaler = &IBCModule{}
)

// IBCModule implements the ICS26 interface for interchain accounts host chains
type IBCModule struct {
keeper keeper.Keeper
Expand Down Expand Up @@ -148,3 +154,15 @@ func (im IBCModule) OnTimeoutPacket(
) error {
return errorsmod.Wrap(icatypes.ErrInvalidChannelFlow, "cannot cause a packet timeout on a host channel end, a host chain does not send a packet over the channel")
}

// UnmarshalPacketData attempts to unmarshal the provided packet data bytes
// into an InterchainAccountPacketData. This function implements the optional
// PacketDataUnmarshaler interface required for ADR 008 support.
func (im IBCModule) UnmarshalPacketData(bz []byte) (interface{}, error) {
var packetData icatypes.InterchainAccountPacketData
if err := icatypes.ModuleCdc.UnmarshalJSON(bz, &packetData); err != nil {
return nil, err
}

return packetData, nil
}
18 changes: 18 additions & 0 deletions modules/apps/27-interchain-accounts/host/ibc_module_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"github.com/cosmos/gogoproto/proto"
"github.com/stretchr/testify/suite"

icahost "github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/host"
"github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/host/types"
icatypes "github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/types"
feetypes "github.com/cosmos/ibc-go/v7/modules/apps/29-fee/types"
Expand Down Expand Up @@ -705,6 +706,23 @@ func (suite *InterchainAccountsTestSuite) TestControlAccountAfterChannelClose()
suite.assertBalance(icaAddr, expBalAfterSecondSend)
}

func (suite *InterchainAccountsTestSuite) TestUnmarshalPacketData() {
expPacketData := icatypes.InterchainAccountPacketData{
Type: icatypes.EXECUTE_TX,
Data: []byte("data"),
Memo: `{"callbacks": {"src_callback_address": "testAddr"}}`,
}

packetData, err := icahost.IBCModule{}.UnmarshalPacketData(expPacketData.GetBytes())
suite.Require().NoError(err)
suite.Require().Equal(expPacketData, packetData)

invalidPacketData := []byte("invalid packet data")
packetData, err = icahost.IBCModule{}.UnmarshalPacketData(invalidPacketData)
suite.Require().Error(err)
suite.Require().Nil(packetData)
}

// assertBalance asserts that the provided address has exactly the expected balance.
// CONTRACT: the expected balance must only contain one coin denom.
func (suite *InterchainAccountsTestSuite) assertBalance(addr sdk.AccAddress, expBalance sdk.Coins) {
Expand Down
4 changes: 0 additions & 4 deletions modules/apps/27-interchain-accounts/module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,21 +19,17 @@ import (
controllerkeeper "github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/controller/keeper"
controllertypes "github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/controller/types"
genesistypes "github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/genesis/types"
"github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/host"
hostkeeper "github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/host/keeper"
hosttypes "github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/host/types"
"github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/simulation"
"github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/types"
porttypes "github.com/cosmos/ibc-go/v7/modules/core/05-port/types"
ibchost "github.com/cosmos/ibc-go/v7/modules/core/24-host"
)

var (
_ module.AppModule = AppModule{}
_ module.AppModuleBasic = AppModuleBasic{}
_ module.AppModuleSimulation = AppModule{}

_ porttypes.IBCModule = host.IBCModule{}
)

// AppModuleBasic is the IBC interchain accounts AppModuleBasic
Expand Down
17 changes: 16 additions & 1 deletion modules/apps/29-fee/ibc_middleware.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,10 @@ import (
"github.com/cosmos/ibc-go/v7/modules/core/exported"
)

var _ porttypes.Middleware = &IBCMiddleware{}
var (
_ porttypes.Middleware = &IBCMiddleware{}
_ porttypes.PacketDataUnmarshaler = &IBCMiddleware{}
)

// IBCMiddleware implements the ICS26 callbacks for the fee middleware given the
// fee keeper and the underlying application.
Expand Down Expand Up @@ -347,3 +350,15 @@ func (im IBCMiddleware) WriteAcknowledgement(
func (im IBCMiddleware) GetAppVersion(ctx sdk.Context, portID, channelID string) (string, bool) {
return im.keeper.GetAppVersion(ctx, portID, channelID)
}

// UnmarshalPacketData attempts to use the underlying app to unmarshal the packet data.
// If the underlying app does not support the PacketDataUnmarshaler interface, an error is returned.
// This function implements the optional PacketDataUnmarshaler interface required for ADR 008 support.
func (im IBCMiddleware) UnmarshalPacketData(bz []byte) (interface{}, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is only necessary if the ADR 8 implementation is higher than ICS 29, which should not be the case. I added the function anyways so we could discuss here whether to keep it or not

Copy link
Member

Choose a reason for hiding this comment

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

hmm, I guess its best to keep it?

Copy link
Member

Choose a reason for hiding this comment

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

Agree that it should be kept. Let's remove ordering requirements from our stack whenever possible

unmarshaler, ok := im.app.(porttypes.PacketDataUnmarshaler)
if !ok {
return nil, errorsmod.Wrapf(types.ErrUnsupportedAction, "underlying app does not implement PacketDataUnmarshaler")
}

return unmarshaler.UnmarshalPacketData(bz)
}
14 changes: 14 additions & 0 deletions modules/apps/29-fee/ibc_middleware_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1078,3 +1078,17 @@ func (suite *FeeTestSuite) TestGetAppVersion() {
})
}
}

func (suite *FeeTestSuite) TestUnmarshalPacketData() {
module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), ibctesting.MockFeePort)
suite.Require().NoError(err)

cbs, ok := suite.chainA.App.GetIBCKeeper().Router.GetRoute(module)
suite.Require().True(ok)

feeModule := cbs.(fee.IBCMiddleware)

packetData, err := feeModule.UnmarshalPacketData(ibcmock.MockPacketData)
suite.Require().NoError(err)
suite.Require().Equal(ibcmock.MockPacketData, packetData)
}
1 change: 1 addition & 0 deletions modules/apps/29-fee/types/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,5 @@ var (
ErrFeeNotEnabled = errorsmod.Register(ModuleName, 9, "fee module is not enabled for this channel. If this error occurs after channel setup, fee module may not be enabled")
ErrRelayerNotFoundForAsyncAck = errorsmod.Register(ModuleName, 10, "relayer address must be stored for async WriteAcknowledgement")
ErrFeeModuleLocked = errorsmod.Register(ModuleName, 11, "the fee module is currently locked, a severe bug has been detected")
ErrUnsupportedAction = errorsmod.Register(ModuleName, 12, "unsupported action")
Copy link
Member

Choose a reason for hiding this comment

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

nit: maybe this go into internal/errors pkg? Seems pretty generic

feel free to ignore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided to ignore, only because in a world where ics29 becomes it's own go.mod, this change might become reverted. I think it makes sense to keep it in ics29 unless we added it to the ibc errors which are exported outside of ibc-go

)
17 changes: 17 additions & 0 deletions modules/apps/transfer/ibc_module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@ import (
ibcexported "github.com/cosmos/ibc-go/v7/modules/core/exported"
)

var (
_ porttypes.IBCModule = IBCModule{}
_ porttypes.PacketDataUnmarshaler = IBCModule{}
)

// IBCModule implements the ICS26 interface for transfer given the transfer keeper.
type IBCModule struct {
keeper keeper.Keeper
Expand Down Expand Up @@ -300,3 +305,15 @@ func (im IBCModule) OnTimeoutPacket(

return nil
}

// UnmarshalPacketData attempts to unmarshal the provided packet data bytes
// into a FungibleTokenPacketData. This function implements the optional
// PacketDataUnmarshaler interface required for ADR 008 support.
func (im IBCModule) UnmarshalPacketData(bz []byte) (interface{}, error) {
Copy link
Member

Choose a reason for hiding this comment

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

do we have a PacketData interface, and would it be helpful or have any benefit?

e.g. something like

type PacketData interface {
    GetBytes() []byte
    ValidateBasic() error
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could do this, but it wouldn't provide any functional benefits, would just clean up the API slightly. It would be an API breaking change to add which is why we have avoided it for now. I believe the interface would likely become unnecessary in the existence of a flat map packet data

var packetData types.FungibleTokenPacketData
if err := types.ModuleCdc.UnmarshalJSON(bz, &packetData); err != nil {
return nil, err
}

return packetData, nil
}
35 changes: 35 additions & 0 deletions modules/apps/transfer/ibc_module_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,18 @@
package transfer_test

import (
"fmt"
"math"

"github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1"
sdk "github.com/cosmos/cosmos-sdk/types"
capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types"

"github.com/cosmos/ibc-go/v7/modules/apps/transfer"
"github.com/cosmos/ibc-go/v7/modules/apps/transfer/types"
channeltypes "github.com/cosmos/ibc-go/v7/modules/core/04-channel/types"
host "github.com/cosmos/ibc-go/v7/modules/core/24-host"
ibcexported "github.com/cosmos/ibc-go/v7/modules/core/exported"
ibctesting "github.com/cosmos/ibc-go/v7/testing"
)

Expand Down Expand Up @@ -239,3 +243,34 @@ func (suite *TransferTestSuite) TestOnChanOpenAck() {
})
}
}

func (suite *TransferTestSuite) TestUnmarshalPacketData() {
var (
sender = sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()).String()
receiver = sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()).String()
denom = "transfer/channel-0/atom"
amount = "100"
)

expPacketData := types.FungibleTokenPacketData{
Denom: denom,
Amount: amount,
Sender: sender,
Receiver: receiver,
Memo: fmt.Sprintf(`{"callbacks": {"src_callback_address": "%s", "dest_callback_address": "%s"}}`, sender, receiver),
}

packetData, err := transfer.IBCModule{}.UnmarshalPacketData(expPacketData.GetBytes())
suite.Require().NoError(err)
suite.Require().Equal(expPacketData, packetData)

callbackPacketData, ok := packetData.(ibcexported.CallbackPacketData)
suite.Require().True(ok)
suite.Require().Equal(sender, callbackPacketData.GetSourceCallbackAddress(), "incorrect source callback address")
suite.Require().Equal(receiver, callbackPacketData.GetDestCallbackAddress(), "incorrect destination callback address")

invalidPacketData := []byte("invalid packet data")
packetData, err = transfer.IBCModule{}.UnmarshalPacketData(invalidPacketData)
suite.Require().Error(err)
suite.Require().Nil(packetData)
}
2 changes: 0 additions & 2 deletions modules/apps/transfer/module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,11 @@ import (
"github.com/cosmos/ibc-go/v7/modules/apps/transfer/keeper"
"github.com/cosmos/ibc-go/v7/modules/apps/transfer/simulation"
"github.com/cosmos/ibc-go/v7/modules/apps/transfer/types"
porttypes "github.com/cosmos/ibc-go/v7/modules/core/05-port/types"
)

var (
_ module.AppModule = AppModule{}
_ module.AppModuleBasic = AppModuleBasic{}
_ porttypes.IBCModule = IBCModule{}
)

// AppModuleBasic is the IBC Transfer AppModuleBasic
Expand Down
6 changes: 6 additions & 0 deletions modules/core/05-port/types/module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,3 +138,9 @@ type Middleware interface {
IBCModule
ICS4Wrapper
}

// PacketDataUnmarshaler defines an optional interface which allows a middleware to
// request the packet data to be unmarshaled by the base application.
type PacketDataUnmarshaler interface {
UnmarshalPacketData([]byte) (interface{}, error)
}
Comment on lines +144 to +146
Copy link
Contributor

Choose a reason for hiding this comment

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

I looked very briefly into what this could look like with generics as I think it would be nice to be able to specify the data type that will be unmarshalled, but things get a little tricky with middleware!

Copy link
Member

Choose a reason for hiding this comment

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

@alpe I think wrote up a quick POC on generics here: #3359

Planning to compare the two solutions this week

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the solution with generics that could potentially work here is to change the interface function from UnmarshalPacketData to GetPacketDataType() T which returns the packet data type to unmarshal into. The only benefit is that the apps wouldn't have to call cdc.Unmarshal, but I'm not super convinced this is a significant benefit, especially given that this interface may eventually be removed with the existence of a flat map packet data

Happy to review a follow pr if someone wanted to explore that solution

6 changes: 6 additions & 0 deletions testing/mock/ibc_module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,12 @@ func (im IBCModule) OnTimeoutPacket(ctx sdk.Context, packet channeltypes.Packet,
return nil
}

// UnmarshalPacketData returns the MockPacketData. This function implements the optional
// PacketDataUnmarshaler interface required for ADR 008 support.
func (im IBCModule) UnmarshalPacketData(bz []byte) (interface{}, error) {
return MockPacketData, nil
}

// GetMockRecvCanaryCapabilityName generates a capability name for testing OnRecvPacket functionality.
func GetMockRecvCanaryCapabilityName(packet channeltypes.Packet) string {
return fmt.Sprintf("%s%s%s%s", MockRecvCanaryCapabilityName, packet.GetDestPort(), packet.GetDestChannel(), strconv.Itoa(int(packet.GetSequence())))
Expand Down