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

ADR 8 Interface and Packet Data Implementations #3287

Merged
merged 12 commits into from
Mar 29, 2023
6 changes: 6 additions & 0 deletions modules/apps/27-interchain-accounts/types/codec.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
sdk "github.com/cosmos/cosmos-sdk/types"
authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
"github.com/cosmos/gogoproto/proto"
"github.com/cosmos/ibc-go/v7/modules/core/exported"
)

// ModuleCdc references the global interchain accounts module codec. Note, the codec
Expand All @@ -21,6 +22,11 @@ var ModuleCdc = codec.NewProtoCodec(codectypes.NewInterfaceRegistry())
func RegisterInterfaces(registry codectypes.InterfaceRegistry) {
registry.RegisterImplementations((*authtypes.AccountI)(nil), &InterchainAccount{})
registry.RegisterImplementations((*authtypes.GenesisAccount)(nil), &InterchainAccount{})

registry.RegisterImplementations(
(*exported.CallbackPacketDataI)(nil),
&InterchainAccountPacketData{},
)
}

// SerializeCosmosTx serializes a slice of sdk.Msg's using the CosmosTx type. The sdk.Msg's are
Expand Down
69 changes: 69 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.CallbackPacketDataI = (*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,71 @@ func (iapd InterchainAccountPacketData) GetBytes() []byte {
return sdk.MustSortJSON(ModuleCdc.MustMarshalJSON(&iapd))
}

/*

ADR-8 CallbackPacketData implementation
Copy link
Member Author

Choose a reason for hiding this comment

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

Is this the right place for this documentation? Should I instead have it in a README in ICS27 directory?

Copy link
Contributor

Choose a reason for hiding this comment

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

it might make sense to have it in the ICS27 directory as well, but I think it would be nice to have here as well so there is an example of how to make use of this ADR-8

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we don't really make use of README files in our different modules, so maybe it's better to add this to our regular docs site?

Copy link
Member

Choose a reason for hiding this comment

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

I like having the large godoc in the code tbh!

Copy link
Contributor

Choose a reason for hiding this comment

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

I think in-code and external facing documentation (information under docs/) are great. The in-code documentation should help explain the hard-coded values while the external facing documentation should explain how to interact with it


InterchainAccountPacketData implements CallbackPacketDataI interface. This will allow middlewares targetting specific VMs
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
to retrieve the desired callback addresses for the ICA packet on the source and destination chains.

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": {contractAddrOnSrcChain},
"dest_callback_address": {contractAddrOnDestChain},
"src_callback_msg": {jsonObjectForSrcChainCallback}, // optional field
"dest_callback_msg": {jsonObjectForDestChainCallback}, // optional field
}

}
```

*/

// GetSrcCallbackAddress returns the callback address on the source chain.
// ADR-8 middleware should callback on the sender address on the source chain
// if the sender address is an IBC Actor (i.e. smart contract that accepts IBC callbacks)
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
func (iapd InterchainAccountPacketData) GetSrcCallbackAddress() string {
if len(iapd.Memo) == 0 {
return ""
}

jsonObject := make(map[string]interface{})
// the jsonObject must be a valid JSON object
err := json.Unmarshal([]byte(iapd.Memo), &jsonObject)
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of parsing the JSON ourselves, couldn't we use a JSON parser library like gjson or jsonparser?

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer not adding a dependency on a 3rd party module. Would there be any benefits? stdlib seems fine to me

Copy link
Contributor

Choose a reason for hiding this comment

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

The benefit would be that extracting the desired value could be just one line of code, something like addr := gjson.Get(iapd.Memo, "callbacks.src_callback_address") with gjson.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could also add a helper function? I agree with @damiannolan on not relying on third party modules

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 using standard library, but might be worth considering using a third party library if we see ourselves doing more and more JSON parsing on the memo field. I found JSON parsing pretty error prone and it's such a common activity that sounds logical to rely on a well tested third party library to take care of that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would also lean towards not pulling in a third party library for json processing.

if err != nil {
return ""
}

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

callbackAddr, ok := callbackData["src_callback_address"].(string)
Copy link
Contributor

Choose a reason for hiding this comment

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

should we keep as src_callback_address? Or change to source_callback_address?

if !ok {
return ""
}
return callbackAddr
}

// GetDestCallbackAddress returns the callback address on the destination chain.
// ADR-8 middleware should callback on the receiver address on the destination chain
// if the receiver address is an IBC Actor (i.e. smart contract that accepts IBC callbacks)
func (iapd InterchainAccountPacketData) GetDestCallbackAddress() string {
return ""
}

// UserDefinedGasLimit no-ops on this method to use relayer passed in gas
func (fptd 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
68 changes: 68 additions & 0 deletions modules/apps/27-interchain-accounts/types/packet_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -82,3 +82,71 @@ func (suite *TypesTestSuite) TestValidateBasic() {
})
}
}

func (suite *TypesTestSuite) TestGetCallbackAddresses() {
testCases := []struct {
name string
packetData types.InterchainAccountPacketData
expSrcCallbackAddr string
}{
{
"memo is empty",
types.InterchainAccountPacketData{
Type: types.EXECUTE_TX,
Data: []byte("data"),
Memo: "",
},
"",
},
{
"memo is not json string",
types.InterchainAccountPacketData{
Type: types.EXECUTE_TX,
Data: []byte("data"),
Memo: "memo",
},
"",
},
{
"memo is does not have callbacks in json struct",
types.InterchainAccountPacketData{
Type: types.EXECUTE_TX,
Data: []byte("data"),
Memo: "{\"Key\": 10}",
},
"",
},
{
"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}}",
},
"",
},
{
"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}}",
},
"",
},
{
"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\": \"testAddress\"}}",
},
"testAddress",
},
}

for _, tc := range testCases {
srcCbAddr := tc.packetData.GetSrcCallbackAddress()
suite.Require().Equal(tc.expSrcCallbackAddr, srcCbAddr, "%s testcase does not have expected src_callback_address", tc.name)
}
}
6 changes: 6 additions & 0 deletions modules/apps/transfer/types/codec.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"github.com/cosmos/cosmos-sdk/x/authz"
"github.com/cosmos/gogoproto/jsonpb"
"github.com/cosmos/gogoproto/proto"
"github.com/cosmos/ibc-go/v7/modules/core/exported"

"github.com/cosmos/cosmos-sdk/codec"
codectypes "github.com/cosmos/cosmos-sdk/codec/types"
Expand All @@ -29,6 +30,11 @@ func RegisterInterfaces(registry codectypes.InterfaceRegistry) {
&TransferAuthorization{},
)

registry.RegisterImplementations(
(*exported.CallbackPacketDataI)(nil),
&FungibleTokenPacketData{},
)

msgservice.RegisterMsgServiceDesc(registry, &_Msg_serviceDesc)
}

Expand Down
96 changes: 96 additions & 0 deletions modules/apps/transfer/types/packet.go
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
package types

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

errorsmod "cosmossdk.io/errors"
sdk "github.com/cosmos/cosmos-sdk/types"

ibcerrors "github.com/cosmos/ibc-go/v7/internal/errors"
"github.com/cosmos/ibc-go/v7/modules/core/exported"
)

var (
Expand All @@ -23,6 +25,8 @@ var (
DefaultRelativePacketTimeoutTimestamp = uint64((time.Duration(10) * time.Minute).Nanoseconds())
)

var _ exported.CallbackPacketDataI = (*FungibleTokenPacketData)(nil)

// NewFungibleTokenPacketData contructs a new FungibleTokenPacketData instance
func NewFungibleTokenPacketData(
denom string, amount string,
Expand Down Expand Up @@ -62,3 +66,95 @@ func (ftpd FungibleTokenPacketData) ValidateBasic() error {
func (ftpd FungibleTokenPacketData) GetBytes() []byte {
return sdk.MustSortJSON(mustProtoMarshalJSON(&ftpd))
}

/**

ADR-8 CallbackPacketData implementation
Copy link
Member Author

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it makes sense to have this documentation here as long as it's referenced from the ICS27 readme as two specific examples of how to implement the callbacks.


FungibleTokenPacketData implements CallbackPacketDataI interface. This will allow middlewares targetting specific VMs
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
to retrieve the desired callback addresses for the ICS20 packet on the source and destination chains.

The Memo is used to ensure that the callback is desired by the user. This allows a user to send an ICS20 packet
to a contract with ADR-8 enabled without automatically triggering the callback logic which may lead to unexpected
behaviour.

The Memo format is defined like so:

```json
{
// ... other memo fields we don't care about
"callbacks": {
"src_callback_address": {contractAddrOnSrcChain},
"dest_callback_address": {contractAddrOnDestChain},
"src_callback_msg": {jsonObjectForSrcChainCallback},
"dest_callback_msg": {jsonObjectForDestChainCallback},
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
}

}
```

For transfer, we will enforce that the src_callback_address is the same as sender and dest_callback_address is the same as receiver.
However, we may remove this restriction at a later date if it proves useful.
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to make that restriction optional? Maybe have an optional field allow_non_receiving_callback (or a better name) that defaults to false if non-existing?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm ok with an additional field, but it'd be nice to know this is a feature that would be requested fairly soon. Is there a use case we have in mind?


**/

// ADR-8 middleware should callback on the sender address on the source chain
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
// if the sender address is an IBC Actor (i.e. smart contract that accepts IBC callbacks)
// The desired callback address must be confirmed in the memo under the "callbacks" key.
// This ensures that the callback is explicitly desired by the user and not just called
// automatically.
func (ftpd FungibleTokenPacketData) GetSrcCallbackAddress() string {
if len(ftpd.Memo) == 0 {
return ""
}

jsonObject := make(map[string]interface{})
// the jsonObject must be a valid JSON object
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
err := json.Unmarshal([]byte(ftpd.Memo), &jsonObject)
if err != nil {
return ""
}

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

if callbackData["src_callback_address"] == ftpd.Sender {
return ftpd.Sender
Copy link
Contributor

Choose a reason for hiding this comment

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

This is prevents us from specifying a callback as anything other than the sender. I think there's value in calling a different address (as long as it's explicitly specified by the sender).

As an example use case, a FE may want to build packets that trigger a contract on Ack but without having to write/deploy a contract themselves and have all transactions be sent through that contract.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, i just put this as an initial behaviour. Isn't this the behaviour of your hooks at the moment?

We could change it

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, that's what we're currently doing (mostly to prevent user error). Just thought since callbacks were a generalization that they could allow for more flexibility here (see my other comment about making it optional based on a key in the memo)

Copy link
Contributor

Choose a reason for hiding this comment

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

As an example use case, a FE may want to build packets that trigger a contract on Ack but without having to write/deploy a contract themselves and have all transactions be sent through that contract.

This sounds like a pretty good example. I guess the idea is being able to specify on the source side a contract call to continue execution on the packet flow that does not relate to the original sender (external user, or smart contract with a separate purpose)

Copy link
Contributor

Choose a reason for hiding this comment

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

opened #3351 to track this discussion

}
return ""
}

// ADR-8 middleware should callback on the receiver address on the destination chain
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
// if the receiver address is an IBC Actor (i.e. smart contract that accepts IBC callbacks)
// The desired callback address must be confirmed in the memo under the "callbacks" key.
// This ensures that the callback is explicitly desired by the user and not just called
// automatically.
func (ftpd FungibleTokenPacketData) GetDestCallbackAddress() string {
if len(ftpd.Memo) == 0 {
return ""
}

jsonObject := make(map[string]interface{})
// the jsonObject must be a valid JSON object
err := json.Unmarshal([]byte(ftpd.Memo), &jsonObject)
if err != nil {
return ""
}

callbackData, ok := jsonObject["callbacks"].(map[string]interface{})
Copy link
Contributor

Choose a reason for hiding this comment

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

We can probably do

Suggested change
callbackData, ok := jsonObject["callbacks"].(map[string]interface{})
callbackData, ok := jsonObject["callbacks"].(map[string]string)

since non-strings would still fail as they can't be compared to ftpd.Receiver

Copy link
Contributor

Choose a reason for hiding this comment

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

since non-strings would still fail as they can't be compared to ftpd.Receiver

Is it possible to have additional fields in the callbacks section that will not be strings? Maybe something like:

{
    "callbacks": {
        "src_callback_address": "contractAddrOnSrcChain",
        "dest_callback_address": "contractAddrOnDestChain",
        "delegation_amount": 100, 
    }
}

I guess the callbacks section isn't intended to hold contract specific arguments? Do we have it specified somewhere what fields are okay for the callbacks section?

if !ok {
return ""
}

if callbackData["dest_callback_address"] == ftpd.Receiver {
return ftpd.Receiver
}
return ""
}

// no-op on this method to use relayer passed in gas
func (fptd FungibleTokenPacketData) UserDefinedGasLimit() uint64 {
return 0
}
19 changes: 19 additions & 0 deletions modules/core/exported/callbacks.go
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package exported

type CallbackPacketDataI interface {
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

There was a user on Discord asking about the flow of token transfer + ICA call (i.e. on acknowledgement of a token transfer, execute ICA call to send a transaction to the interchain account). It seems like the user was not using a smart contract, so I suggested to use the message router to send a MsgSendTx to the interchain account sub-controller. After discussing with @womensrights, for such flow we will probably need also an ADR-8 interface so that when processing the token transfer acknowledgement, the necessary information to send a TX to an interchain account is provided (e.g connection ID, owner address, maybe interchain account packet data?). If a new interface is required (maybe is good to have separate interfaces for different use cases), should we rename this interface to something like ContractCallbackPacketDataI to make it specific to the use case of smart contracts?

Copy link
Member Author

Choose a reason for hiding this comment

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

They should still be able to use the interfaces defined here. They would have to make their own custom middleware to do the go logic though.

// may return the empty string
GetSrcCallbackAddress() string

// may return the empty string
GetDestCallbackAddress() string
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the code for these two functions is almost identical in both implementations (except for checking if the src callback address is equal to the sender and the dest callback address is the same as the receiver), so would it be possible to move the logic for extracting the values from the JSON string into a separate utility function and call that one in the implementations? You can still do the additional check for transfers and return empty string if not equal.

colin-axner marked this conversation as resolved.
Show resolved Hide resolved

// UserDefinedGasLimit allows the sender of the packet to define inside the packet data
// a gas limit for how much the ADR-8 callbacks can consume. If defined, this will be passed
// in as the gas limit so that the callback is guaranteed to complete within a specific limit.
// On recvPacket, a gas-overflow will just fail the transaction allowing it to timeout on the sender side.
// On ackPacket and timeoutPacket, a gas-overflow will reject state changes made during callback but still
// commit the transaction. This ensures the packet lifecycle can always complete.
// If the packet data returns 0, the remaining gas limit will be passed in (modulo any chain-defined limit)
// Otherwise, we will set the gas limit passed into the callback to the `min(ctx.GasLimit, UserDefinedGasLimit())`
UserDefinedGasLimit() uint64
}
12 changes: 12 additions & 0 deletions modules/core/exported/codec.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package exported

import (
codectypes "github.com/cosmos/cosmos-sdk/codec/types"
)

func RegisterInterfaces(registry codectypes.InterfaceRegistry) {
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
registry.RegisterInterface(
"ibc.core.exported.v1.CallbackPacketDataI",
(*CallbackPacketDataI)(nil),
)
}
2 changes: 2 additions & 0 deletions modules/core/types/codec.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
connectiontypes "github.com/cosmos/ibc-go/v7/modules/core/03-connection/types"
channeltypes "github.com/cosmos/ibc-go/v7/modules/core/04-channel/types"
commitmenttypes "github.com/cosmos/ibc-go/v7/modules/core/23-commitment/types"
"github.com/cosmos/ibc-go/v7/modules/core/exported"
localhost "github.com/cosmos/ibc-go/v7/modules/light-clients/09-localhost"
)

Expand All @@ -18,4 +19,5 @@ func RegisterInterfaces(registry codectypes.InterfaceRegistry) {
channeltypes.RegisterInterfaces(registry)
commitmenttypes.RegisterInterfaces(registry)
localhost.RegisterInterfaces(registry)
exported.RegisterInterfaces(registry)
}