-
Notifications
You must be signed in to change notification settings - Fork 586
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
Changes from all commits
bf93c0e
8c16e8d
3500d06
89f608c
b77cfe6
1db4626
32b74ec
f8298da
f826ddd
ca33576
6bf400e
523f40b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 | ||
|
@@ -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 { | ||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we keep as |
||
if !ok { | ||
return "" | ||
} | ||
|
||
return callbackAddr | ||
colin-axner marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
// 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)) | ||
|
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 ( | ||||||
|
@@ -23,6 +25,8 @@ var ( | |||||
DefaultRelativePacketTimeoutTimestamp = uint64((time.Duration(10) * time.Minute).Nanoseconds()) | ||||||
) | ||||||
|
||||||
var _ exported.CallbackPacketData = (*FungibleTokenPacketData)(nil) | ||||||
|
||||||
// NewFungibleTokenPacketData contructs a new FungibleTokenPacketData instance | ||||||
func NewFungibleTokenPacketData( | ||||||
denom string, amount string, | ||||||
|
@@ -62,3 +66,109 @@ func (ftpd FungibleTokenPacketData) ValidateBasic() error { | |||||
func (ftpd FungibleTokenPacketData) GetBytes() []byte { | ||||||
return sdk.MustSortJSON(mustProtoMarshalJSON(&ftpd)) | ||||||
} | ||||||
|
||||||
/* | ||||||
|
||||||
ADR-8 CallbackPacketData implementation | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 CallbackPacketData interface. This will allow middlewares targeting specific VMs | ||||||
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": {contractAddrOnSourceChain}, | ||||||
"dest_callback_address": {contractAddrOnDestChain}, | ||||||
|
||||||
// optional fields | ||||||
"src_callback_msg": {jsonObjectForSourceChainCallback}, | ||||||
"dest_callback_msg": {jsonObjectForDestChainCallback}, | ||||||
} | ||||||
} | ||||||
``` | ||||||
|
||||||
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. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? |
||||||
|
||||||
*/ | ||||||
|
||||||
// GetSourceCallbackAddress returns the sender address if it is also specified in | ||||||
// the packet data memo. 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 called automatically. If no callback address is | ||||||
// specified, an empty string is returned. | ||||||
// | ||||||
// The memo is expected to contain the source 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 (ftpd FungibleTokenPacketData) GetSourceCallbackAddress() string { | ||||||
if len(ftpd.Memo) == 0 { | ||||||
return "" | ||||||
} | ||||||
|
||||||
jsonObject := make(map[string]interface{}) | ||||||
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 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. opened #3351 to track this discussion |
||||||
} | ||||||
|
||||||
return "" | ||||||
} | ||||||
|
||||||
// GetDestCallbackAddress returns the receiving address if it is also specified in | ||||||
// the packet data memo. 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 called automatically. If no callback address is | ||||||
// specified, an empty string is returned. | ||||||
// | ||||||
// The memo is expected to contain the destination callback address in the following format: | ||||||
// { "callbacks": { "dest_callback_address": {contractAddrOnDestChain}} | ||||||
// | ||||||
// ADR-8 middleware should callback on the returned address if it is a PacketActor | ||||||
// (i.e. smart contract that accepts IBC callbacks). | ||||||
func (ftpd FungibleTokenPacketData) GetDestCallbackAddress() string { | ||||||
if len(ftpd.Memo) == 0 { | ||||||
return "" | ||||||
} | ||||||
|
||||||
jsonObject := make(map[string]interface{}) | ||||||
err := json.Unmarshal([]byte(ftpd.Memo), &jsonObject) | ||||||
if err != nil { | ||||||
return "" | ||||||
} | ||||||
|
||||||
callbackData, ok := jsonObject["callbacks"].(map[string]interface{}) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can probably do
Suggested change
since non-strings would still fail as they can't be compared to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 "" | ||||||
} | ||||||
|
||||||
// UserDefinedGasLimit returns 0 (no-op). The gas limit of the executing | ||||||
// transaction will be used. | ||||||
func (ftpd FungibleTokenPacketData) UserDefinedGasLimit() uint64 { | ||||||
return 0 | ||||||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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