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.

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: add UnmarshalPacketData interface function #3353

Merged
Merged
Show file tree
Hide file tree
Changes from 12 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
11 changes: 11 additions & 0 deletions modules/apps/27-interchain-accounts/controller/ibc_middleware.go
Original file line number Diff line number Diff line change
Expand Up @@ -252,3 +252,14 @@ 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 a InterchainAccountPacketData.
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
}
11 changes: 11 additions & 0 deletions modules/apps/27-interchain-accounts/host/ibc_module.go
Original file line number Diff line number Diff line change
Expand Up @@ -148,3 +148,14 @@ 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.
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
}
4 changes: 3 additions & 1 deletion modules/apps/27-interchain-accounts/module.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,9 @@ var (
_ module.AppModuleBasic = AppModuleBasic{}
_ module.AppModuleSimulation = AppModule{}

_ porttypes.IBCModule = host.IBCModule{}
_ porttypes.IBCModule = host.IBCModule{}
_ porttypes.PacketDataUnmarshaler = host.IBCModule{}
_ porttypes.PacketDataUnmarshaler = controller.IBCMiddleware{}
)

// AppModuleBasic is the IBC interchain accounts AppModuleBasic
Expand Down
75 changes: 75 additions & 0 deletions modules/apps/27-interchain-accounts/types/packet.go
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
package types

import (
"encoding/json"
"time"

errorsmod "cosmossdk.io/errors"
codectypes "github.com/cosmos/cosmos-sdk/codec/types"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/ibc-go/v7/modules/core/exported"
)

// MaxMemoCharLength defines the maximum length for the InterchainAccountPacketData memo field
Expand All @@ -24,6 +26,8 @@ var (
DefaultRelativePacketTimeoutTimestamp = uint64((time.Duration(10) * time.Minute).Nanoseconds())
)

var _ exported.CallbackPacketData = (*InterchainAccountPacketData)(nil)

// ValidateBasic performs basic validation of the interchain account packet data.
// The memo may be empty.
func (iapd InterchainAccountPacketData) ValidateBasic() error {
Expand All @@ -47,6 +51,77 @@ func (iapd InterchainAccountPacketData) GetBytes() []byte {
return sdk.MustSortJSON(ModuleCdc.MustMarshalJSON(&iapd))
}

/*

ADR-8 CallbackPacketData implementation

InterchainAccountPacketData implements CallbackPacketData interface. This will allow middlewares targeting specific VMs
to retrieve the desired callback address for the ICA packet on the source chain. Destination callback addresses are not
supported for ICS 27.

The Memo is used to set the desired callback addresses.

The Memo format is defined like so:

```json
{
// ... other memo fields we don't care about
"callbacks": {
"src_callback_address": {contractAddrOnSourceChain},

// optional fields
"src_callback_msg": {jsonObjectForSourceChainCallback},
}
}
```

*/

// GetSourceCallbackAddress returns the source callback address provided in the packet data memo.
// If no callback address is specified, an empty string is returned.
//
// The memo is expected to specify the callback address in the following format:
// { "callbacks": { "src_callback_address": {contractAddrOnSourceChain}}
//
// ADR-8 middleware should callback on the returned address if it is a PacketActor
// (i.e. smart contract that accepts IBC callbacks).
func (iapd InterchainAccountPacketData) GetSourceCallbackAddress() string {
if len(iapd.Memo) == 0 {
return ""
}

jsonObject := make(map[string]interface{})
err := json.Unmarshal([]byte(iapd.Memo), &jsonObject)
if err != nil {
return ""
}

callbackData, ok := jsonObject["callbacks"].(map[string]interface{})
if !ok {
return ""
}

callbackAddr, ok := callbackData["src_callback_address"].(string)
if !ok {
return ""
}

return callbackAddr
}

// GetDestCallbackAddress returns an empty string. Destination callback addresses
// are not supported for ICS 27. This feature is natively supported by
// interchain accounts host submodule transaction execution.
func (iapd InterchainAccountPacketData) GetDestCallbackAddress() string {
return ""
}

// UserDefinedGasLimit returns 0 (no-op). The gas limit of the executing
// transaction will be used.
func (iapd InterchainAccountPacketData) UserDefinedGasLimit() uint64 {
return 0
}

// GetBytes returns the JSON marshalled interchain account CosmosTx.
func (ct CosmosTx) GetBytes() []byte {
return sdk.MustSortJSON(ModuleCdc.MustMarshalJSON(&ct))
Expand Down
122 changes: 122 additions & 0 deletions modules/apps/27-interchain-accounts/types/packet_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package types_test

import (
"fmt"

"github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/types"
)

Expand Down Expand Up @@ -82,3 +84,123 @@ func (suite *TypesTestSuite) TestValidateBasic() {
})
}
}

func (suite *TypesTestSuite) TestGetSourceCallbackAddress() {
const expSrcCbAddr = "srcCbAddr"

testCases := []struct {
name string
packetData types.InterchainAccountPacketData
expPass bool
}{
{
"memo is empty",
types.InterchainAccountPacketData{
Type: types.EXECUTE_TX,
Data: []byte("data"),
Memo: "",
},
false,
},
{
"memo is not json string",
types.InterchainAccountPacketData{
Type: types.EXECUTE_TX,
Data: []byte("data"),
Memo: "memo",
},
false,
},
{
"memo does not have callbacks in json struct",
types.InterchainAccountPacketData{
Type: types.EXECUTE_TX,
Data: []byte("data"),
Memo: `{"Key": 10}`,
},
false,
},
{
"memo has callbacks in json struct but does not have src_callback_address key",
types.InterchainAccountPacketData{
Type: types.EXECUTE_TX,
Data: []byte("data"),
Memo: `{"callbacks": {"Key": 10}}`,
},
false,
},
{
"memo has callbacks in json struct but does not have string value for src_callback_address key",
types.InterchainAccountPacketData{
Type: types.EXECUTE_TX,
Data: []byte("data"),
Memo: `{"callbacks": {"src_callback_address": 10}}`,
},
false,
},
{
"memo has callbacks in json struct and properly formatted src_callback_address",
types.InterchainAccountPacketData{
Type: types.EXECUTE_TX,
Data: []byte("data"),
Memo: fmt.Sprintf(`{"callbacks": {"src_callback_address": "%s"}}`, expSrcCbAddr),
},
true,
},
}

for _, tc := range testCases {
tc := tc
suite.Run(tc.name, func() {
srcCbAddr := tc.packetData.GetSourceCallbackAddress()

if tc.expPass {
suite.Require().Equal(expSrcCbAddr, srcCbAddr)
} else {
suite.Require().Equal("", srcCbAddr)
}
})
}
}

func (suite *TypesTestSuite) TestGetDestCallbackAddress() {
testCases := []struct {
name string
packetData types.InterchainAccountPacketData
}{
{
"memo is empty",
types.InterchainAccountPacketData{
Type: types.EXECUTE_TX,
Data: []byte("data"),
Memo: "",
},
},
{
"memo has dest callback address specified in json struct",
types.InterchainAccountPacketData{
Type: types.EXECUTE_TX,
Data: []byte("data"),
Memo: `{"callbacks": {"dest_callback_address": "testAddress"}}`,
},
},
}

for _, tc := range testCases {
tc := tc
suite.Run(tc.name, func() {
destCbAddr := tc.packetData.GetDestCallbackAddress()
suite.Require().Equal("", destCbAddr)
})
}
}

func (suite *TypesTestSuite) TestUserDefinedGasLimit() {
packetData := types.InterchainAccountPacketData{
Type: types.EXECUTE_TX,
Data: []byte("data"),
Memo: `{"callbacks": {"user_defined_gas_limit": 100}}`,
}

suite.Require().Equal(uint64(0), packetData.UserDefinedGasLimit(), "user defined gas limit does not return 0")
}
16 changes: 15 additions & 1 deletion modules/apps/29-fee/ibc_middleware.go
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,14 @@ 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.
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.ErrUnsupported, "underlying app does not implement PacketDataUnmarshaler")
}

return unmarshaler.UnmarshalPacketData(bz)
}
1 change: 1 addition & 0 deletions modules/apps/29-fee/types/errors.go
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")
ErrUnsupported = errorsmod.Register(ModuleName, 12, "unsupported action")
)
11 changes: 11 additions & 0 deletions modules/apps/transfer/ibc_module.go
Original file line number Diff line number Diff line change
Expand Up @@ -300,3 +300,14 @@ func (im IBCModule) OnTimeoutPacket(

return nil
}

// UnmarshalPacketData attempts to unmarshal the provided packet data bytes
// into a FungibleTokenPacketData.
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
}
7 changes: 4 additions & 3 deletions modules/apps/transfer/module.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,10 @@ import (
)

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

// AppModuleBasic is the IBC Transfer AppModuleBasic
Expand Down
7 changes: 3 additions & 4 deletions modules/apps/transfer/types/codec.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,13 @@ package types
import (
"bytes"

"github.com/cosmos/cosmos-sdk/x/authz"
"github.com/cosmos/gogoproto/jsonpb"
"github.com/cosmos/gogoproto/proto"

"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/cosmos/cosmos-sdk/x/authz"
"github.com/cosmos/gogoproto/jsonpb"
"github.com/cosmos/gogoproto/proto"
)

// RegisterLegacyAminoCodec registers the necessary x/ibc transfer interfaces and concrete types
Expand Down
Loading