ADR 8 Interface and Packet Data Implementations #3287

AdityaSripal · 2023-03-13T18:09:32Z

Description

This PR introduces an optional interface that IBC applications can implement for their packet data. When implemented, middleware can target this interface in order to get the desired callback addresses on the source and dest chains.

There is also an additional method that allows the packet sender to define a UserDefinedGasLimit for how much the callback is allowed to consume. This came from conversations with @ValarDragon . However, for the moment the apps in ibc-go return 0, which is effectively a no-op

closes: #XXXX

Commit Message / Changelog Entry

type: commit message

see the guidelines for commit messages. (view raw markdown for examples)

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md).
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards and Go style guide.
Wrote unit and integration tests.
Updated relevant documentation (docs/) or specification (x/<module>/spec/).
Added relevant godoc comments.
Provide a commit message to be used for the changelog entry in the PR description for review.
Re-reviewed Files changed in the Github PR explorer.
Review Codecov Report in the comment section below once CI passes.

AdityaSripal · 2023-03-13T18:33:26Z

modules/apps/27-interchain-accounts/types/packet.go

@@ -47,6 +51,83 @@ func (iapd InterchainAccountPacketData) GetBytes() []byte {
 	return sdk.MustSortJSON(ModuleCdc.MustMarshalJSON(&iapd))
 }

+/**
+
+ADR-8 CallbackPacketData implementation


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

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

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?

I like having the large godoc in the code tbh!

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

AdityaSripal · 2023-03-13T18:33:35Z

modules/apps/transfer/types/packet.go

+
+/**
+
+ADR-8 CallbackPacketData implementation


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.

AdityaSripal · 2023-03-13T18:36:03Z

modules/apps/transfer/types/packet.go

+// 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() (addr string) {


Do we need to reiterate in the memo here?

I think it makes sense on dest chain so we don't have automatic triggering of undefined smart contract logic. Plenty of bugs in Ethereum because of this behaviour.

But why wouldn't we want the automatic callback on ack and timeout if the sender itself is a smart contract. I think it makes sense to remove this logic and just return the sender directly here.

The middleware is responsible for determining if the address is a smart contract before executing

I don't have a preference. Both seem like valid options to me. I guess I just wouldn't want to change the behaviour in the future (once contracts are written to expect certain behaviour, it shouldn't be adjusted)

AdityaSripal · 2023-03-16T11:37:36Z

modules/apps/27-interchain-accounts/types/packet.go

+
+// 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() (addr string) {


I have the feeling this should just be disabled honestly. If you want custom logic to run on DEST, why not have it be a message encoded in the ICA tx itself?

I think for ICA, only sender callbacks make sense and for that I don't think we need a double confirmation in the memo

I think it makes sense to disable this. I'm can't remember if ICA allows for multiple messages in one ICA tx but even if it doesn't, a callback would just extend this to two messages instead of multiple messages (which would be more useful)

It does allow multiple messages

nicolaslara · 2023-03-16T13:21:29Z

modules/apps/transfer/types/packet.go

+```
+
+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.


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?

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?

nicolaslara · 2023-03-16T13:27:02Z

modules/apps/transfer/types/packet.go

+	}
+
+	if callbackData["src_callback_address"] == ftpd.Sender {
+		return ftpd.Sender


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.

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

We could change it

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)

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)

opened #3351 to track this discussion

nicolaslara · 2023-03-16T13:31:14Z

modules/apps/transfer/types/packet.go

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


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

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?

codecov-commenter · 2023-03-18T22:12:02Z

Codecov Report

Merging #3287 (32b74ec) into main (afdc278) will decrease coverage by 0.06%.
The diff coverage is 40.84%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3287      +/-   ##
==========================================
- Coverage   78.54%   78.48%   -0.06%     
==========================================
  Files         181      181              
  Lines       12580    12573       -7     
==========================================
- Hits         9881     9868      -13     
- Misses       2262     2275      +13     
+ Partials      437      430       -7

Impacted Files	Coverage Δ
modules/apps/transfer/types/packet.go	`39.68% <0.00%> (-60.32%)`	⬇️
...odules/apps/27-interchain-accounts/types/packet.go	`82.22% <82.60%> (+0.40%)`	⬆️
modules/apps/27-interchain-accounts/types/codec.go	`81.63% <100.00%> (+2.08%)`	⬆️
modules/apps/transfer/types/codec.go	`89.18% <100.00%> (+1.68%)`	⬆️

... and 19 files with indirect coverage changes

crodriguezvega

Thank you @AdityaSripal!

modules/apps/27-interchain-accounts/types/packet.go

crodriguezvega · 2023-03-19T21:40:36Z

modules/core/exported/callbacks.go

+	GetSrcCallbackAddress() string
+
+	// may return the empty string
+	GetDestCallbackAddress() string


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.

crodriguezvega · 2023-03-20T08:33:55Z

modules/apps/27-interchain-accounts/types/packet.go

@@ -47,6 +51,83 @@ func (iapd InterchainAccountPacketData) GetBytes() []byte {
 	return sdk.MustSortJSON(ModuleCdc.MustMarshalJSON(&iapd))
 }

+/**
+
+ADR-8 CallbackPacketData implementation


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?

crodriguezvega · 2023-03-20T08:36:00Z

modules/apps/27-interchain-accounts/types/packet.go

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


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

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

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.

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

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.

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

colin-axner

More fabulous work from the protocol mage 🧙‍♂️ 🪄 :)

Conceptually looks great to me. I left many nits.

The main component missing is tests. Each function added should have a unit test (helps us detect regressions in logic change, thinking of accidental changes in merge conflict fixing). In addition, it might be nice to add tests which mirror the expected ADR 8 flow, this might give implementers some concrete compiled code to look at as well. It can be very basic ( unmarshaling packet data into the interface and getting the src/dest addresses)

modules/apps/27-interchain-accounts/types/packet.go

modules/core/exported/callbacks.go

colin-axner · 2023-03-20T16:15:50Z

modules/apps/27-interchain-accounts/types/packet.go

@@ -47,6 +51,83 @@ func (iapd InterchainAccountPacketData) GetBytes() []byte {
 	return sdk.MustSortJSON(ModuleCdc.MustMarshalJSON(&iapd))
 }

+/**
+
+ADR-8 CallbackPacketData implementation


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

modules/apps/transfer/types/packet.go

colin-axner · 2023-03-20T16:41:07Z

modules/apps/transfer/types/packet.go

+	}
+
+	if callbackData["src_callback_address"] == ftpd.Sender {
+		return ftpd.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.

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)

modules/apps/transfer/types/packet.go

modules/core/exported/codec.go

modules/core/exported/callbacks.go

0xekez · 2023-03-20T19:32:37Z

Hi folks, thanks for the work here. It would be great if we could use this work with DAO DAO.

From my read of the PR, in the current callback, a smart contract would get the packet sent as data on message completion. I'm not sure this is sufficient to build good UX around, as IMO the primary reason for callbacks is to know if an error happened while executing the message. It would be great if this callback system would give the ACK that occured as the callback, instead of the initial packet (which is my read of this document for current behavior).

This current system also seems limited in that there is no way for the host chain to return data to the controller chain for the callback, which limits how interoperable this is. I have been working on an ICA replacement, and there I allow the host chain to return data to the controller by setting the data field of the transaction. This integration test demonstrates this functionality.

Without this, the ability of the receiver of the callback to interact with the host chain on success is pretty limited as the only data it has to work with is the initial packet (the contents of which it should already know).

Finally, it would be nice to have:

The initiator of the callback
Data sent from the initiator

In the callback. This allows for flows like:

transfer tokens via ICS-20
on callback, wake up the ICA module and do something with those tokens

For reference, here is the callback system I have been working on myself.

AdityaSripal · 2023-03-21T13:48:52Z

Hi @0xekez , yes the OnAcknowledgementCallback will include the acknowledgement itself so that the contract or intitiator can read whether the packet was successful or not.

This is specified in the ADR. I noticed the transfer template you linked did not have that which is an oversight of mine, apologies.

here is no way for the host chain to return data to the controller chain for the callback

The host chain can return data in the ack.

I don't believe an incompatible fork of ICA is necessary to accomplish your goals. We can discuss what it is you need and how to get there.

0xekez · 2023-03-21T20:26:38Z

@AdityaSripal: The host chain can return data in the ack.

Thanks for the response and clarification. It seems like the module will return the response information from the message executed in the ACK success variant which seems reasonable. Thanks!

…adr-8-interface-impl

crodriguezvega · 2023-03-27T07:01:48Z

modules/core/exported/callbacks.go

@@ -0,0 +1,19 @@
+package exported
+
+type CallbackPacketDataI interface {


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?

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.

* remove query client (#3227) * remove query client * merge main * go mod tidy * Rename ``IsBound`` to ``HasCapability`` (#3253) ## Description closes: #828 ### Commit Message / Changelog Entry ```bash imp(api!): rename `IsBound` to `HasCapability` for IBC application modules ``` see the [guidelines](https://github.com/cosmos/ibc-go/blob/main/docs/dev/pull-requests.md#commit-messages) for commit messages. (view raw markdown for examples) --- Before we can merge this PR, please make sure that all the following items have been checked off. If any of the checklist items are not applicable, please leave them but write a little note why. - [ ] Targeted PR against correct branch (see [CONTRIBUTING.md](https://github.com/cosmos/ibc-go/blob/main/docs/dev/pull-requests.md#pull-request-targeting)). - [x] Linked to Github issue with discussion and accepted design OR link to spec that describes this work. - [x] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/main/docs/docs/building-modules/10-structure.md) and [Go style guide](../docs/dev/go-style-guide.md). - [ ] Wrote unit and integration [tests](https://github.com/cosmos/ibc-go/blob/main/testing/README.md#ibc-testing-package). - [ ] Updated relevant documentation (`docs/`) or specification (`x/<module>/spec/`). - [ ] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code). - [ ] Provide a [commit message](https://github.com/cosmos/ibc-go/blob/main/docs/dev/pull-requests.md#commit-messages) to be used for the changelog entry in the PR description for review. - [ ] Re-reviewed `Files changed` in the Github PR explorer. - [ ] Review `Codecov Report` in the comment section below once CI passes. * chore: add support for tendermint debug log level (#3279) * build(deps): bump cosmossdk.io/math from 1.0.0-beta.6.0.20230216172121-959ce49135e4 to 1.0.0-rc.0 (#3285) Bumps [cosmossdk.io/math](https://github.com/cosmos/cosmos-sdk) from 1.0.0-beta.6.0.20230216172121-959ce49135e4 to 1.0.0-rc.0. <details> <summary>Commits</summary> <ul> <li>See full diff in <a href="https://github.com/cosmos/cosmos-sdk/commits/math/v1.0.0-rc.0">compare view</a></li> </ul> </details> <br /> [![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=cosmossdk.io/math&package-manager=go_modules&previous-version=1.0.0-beta.6.0.20230216172121-959ce49135e4&new-version=1.0.0-rc.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> * Write docker inspect output to diagnostics (#3291) * chore: fix dead links (#3293) ## Description closes: #XXXX ### Commit Message / Changelog Entry ```bash type: commit message ``` see the [guidelines](https://github.com/cosmos/ibc-go/blob/main/docs/dev/pull-requests.md#commit-messages) for commit messages. (view raw markdown for examples) --- Before we can merge this PR, please make sure that all the following items have been checked off. If any of the checklist items are not applicable, please leave them but write a little note why. - [x] Targeted PR against correct branch (see [CONTRIBUTING.md](https://github.com/cosmos/ibc-go/blob/main/docs/dev/pull-requests.md#pull-request-targeting)). - [ ] Linked to Github issue with discussion and accepted design OR link to spec that describes this work. - [ ] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/main/docs/docs/building-modules/10-structure.md) and [Go style guide](../docs/dev/go-style-guide.md). - [ ] Wrote unit and integration [tests](https://github.com/cosmos/ibc-go/blob/main/testing/README.md#ibc-testing-package). - [ ] Updated relevant documentation (`docs/`) or specification (`x/<module>/spec/`). - [ ] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code). - [ ] Provide a [commit message](https://github.com/cosmos/ibc-go/blob/main/docs/dev/pull-requests.md#commit-messages) to be used for the changelog entry in the PR description for review. - [x] Re-reviewed `Files changed` in the Github PR explorer. - [ ] Review `Codecov Report` in the comment section below once CI passes. * build(deps): bump google.golang.org/protobuf from 1.29.0 to 1.29.1 (#3292) * deps: bump SDK v0.47 (#3295) Co-authored-by: Damian Nolan <damiannolan@gmail.com> Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> * remove unnecessary import from doc * chore: remove support for v3 (#3294) * build(deps): bump actions/setup-go from 3 to 4 (#3307) * imp: remove unnecessary defer func statements (#3304) * Remove gogoproto yaml tags from proto files (#3290) ## Description Refer from original issue, I removed all `yaml` tags in proto files. closes: #3145 ### Commit Message / Changelog Entry ```bash type: commit message ``` see the [guidelines](https://github.com/cosmos/ibc-go/blob/main/docs/dev/pull-requests.md#commit-messages) for commit messages. (view raw markdown for examples) --- Before we can merge this PR, please make sure that all the following items have been checked off. If any of the checklist items are not applicable, please leave them but write a little note why. - [ ] Targeted PR against correct branch (see [CONTRIBUTING.md](https://github.com/cosmos/ibc-go/blob/main/docs/dev/pull-requests.md#pull-request-targeting)). - [ ] Linked to Github issue with discussion and accepted design OR link to spec that describes this work. - [ ] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/main/docs/docs/building-modules/10-structure.md) and [Go style guide](../docs/dev/go-style-guide.md). - [ ] Wrote unit and integration [tests](https://github.com/cosmos/ibc-go/blob/main/testing/README.md#ibc-testing-package). - [ ] Updated relevant documentation (`docs/`) or specification (`x/<module>/spec/`). - [ ] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code). - [ ] Provide a [commit message](https://github.com/cosmos/ibc-go/blob/main/docs/dev/pull-requests.md#commit-messages) to be used for the changelog entry in the PR description for review. - [ ] Re-reviewed `Files changed` in the Github PR explorer. - [ ] Review `Codecov Report` in the comment section below once CI passes. * add reasoning for migration to enable localhost * Support configuration file for e2e tests (#3260) * E2E fzf Test Selection Autocompletion (#3313) * post v7 release chores (#3310) * chore: fix linter warnings (#3311) * ADR 008: IBC Actor Callbacks (#1976) * context and decision * complete adr * Apply suggestions from code review Co-authored-by: Carlos Rodriguez <carlos@interchain.io> * change from caller to generalized actor * Apply suggestions from code review Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> * create folder and scaffolded middleware * add error handling and generify packetdata interface * complete renaming * add user defined gas limit and clarify pseudocode * Clarify CallbackPacketData interface imp: Add ADR 008: IBC Actor Callbacks --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io> Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> * lint: fix spelling * chore: apply self review concerns * chore: rename CallbackPacketDataI to CallbackPacketData * chore: finish applying remaining review suggestions * test: add remaining unit tests for transfer and ica * test: add unmarshaling test * imp: address ADR 8 review suggestions (#3319) --------- Co-authored-by: Damian Nolan <damiannolan@gmail.com> * Bump interchain test (#3314) * fix: remove codec registration * fix: build + linting * Only run e2e on R4R (#3330) * fix fork workflows (#3328) * Revert "Merge branch 'main' of github.com:cosmos/ibc-go into colin/callback-packet-data-impl" This reverts commit 1c6164b, reversing changes made to 6f25b8e. * chore: add optional interface godoc * Apply suggestions from code review Co-authored-by: Carlos Rodriguez <carlos@interchain.io> * chore: use backticks instead of escape characters in testing --------- Co-authored-by: Lặc <67097720+expertdicer@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Cian Hatton <cian@interchain.io> Co-authored-by: Carlos Rodriguez <carlos@interchain.io> Co-authored-by: Damian Nolan <damiannolan@gmail.com> Co-authored-by: GNaD13 <89174180+GNaD13@users.noreply.github.com> Co-authored-by: Hieu Vu <72878483+hieuvubk@users.noreply.github.com> Co-authored-by: Aditya <adityasripal@gmail.com>

colin-axner · 2023-03-27T16:15:27Z

We will be merging these changes into this feature branch. Changes will be backported to this testing branch (based off v7.1.x). This will allow us to start testing these changes with the osmosis adr8 implementation. Once the proposed changes have been tested and shown to work and/or additional changes have been applied, the feature branch will be merged to main and backported to a v7 minor release

I have opened up the following issues for points raised in review of this pr:

Add a UnmarshalPacketData optional interface function to support ADR 8 callbacks #3349
Add documentation for the interfaces required by the callbacks module #3350
Consider allowing alternative sender/receiver address in ICS 20 ADR 8 callbacks #3351

colin-axner

ACK. Looking forward to seeing these changes in action!

Would appreciate 2 approvals from folks who didn't work on these changes

damiannolan

ACK changes! Nice work

damiannolan · 2023-03-28T09:44:19Z

modules/core/exported/packet.go

+// CallbackPacketData defines the interface used by ADR 8 implementations
+// to obtain callback addresses associated with a specific packet data type.
+// This is an optional interface which indicates support for ADR 8 implementations.


nit: maybe we can add a link to ADR 8 in this godoc

added 523f40b, I used a link to main. I think that's fine since I don't expect our docs/architecture layout to change for a very long time

colin-axner · 2023-03-28T10:05:03Z

modules/apps/27-interchain-accounts/types/packet.go

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


should we keep as src_callback_address? Or change to source_callback_address?

chatton

Nice job LGTM, thanks for all the work you've put into this 🙏

chatton · 2023-03-29T08:52:31Z

modules/apps/27-interchain-accounts/types/packet.go

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


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

modules/apps/27-interchain-accounts/types/packet.go

colin-axner · 2023-03-29T09:54:40Z

merging into the feature branch so work can continue on this. Please feel free to review/open issues/ask questions/suggest improvements :)

* adr 8 with 20/27 implementation * change interface name and register codecs * documentation * add comma before new line * fix return arg and dest for ica * add ica tests * adr 8 callback packet data impl followups (#3325) * remove query client (#3227) * remove query client * merge main * go mod tidy * Rename ``IsBound`` to ``HasCapability`` (#3253) closes: #828 ```bash imp(api!): rename `IsBound` to `HasCapability` for IBC application modules ``` see the [guidelines](https://github.com/cosmos/ibc-go/blob/main/docs/dev/pull-requests.md#commit-messages) for commit messages. (view raw markdown for examples) --- Before we can merge this PR, please make sure that all the following items have been checked off. If any of the checklist items are not applicable, please leave them but write a little note why. - [ ] Targeted PR against correct branch (see [CONTRIBUTING.md](https://github.com/cosmos/ibc-go/blob/main/docs/dev/pull-requests.md#pull-request-targeting)). - [x] Linked to Github issue with discussion and accepted design OR link to spec that describes this work. - [x] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/main/docs/docs/building-modules/10-structure.md) and [Go style guide](../docs/dev/go-style-guide.md). - [ ] Wrote unit and integration [tests](https://github.com/cosmos/ibc-go/blob/main/testing/README.md#ibc-testing-package). - [ ] Updated relevant documentation (`docs/`) or specification (`x/<module>/spec/`). - [ ] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code). - [ ] Provide a [commit message](https://github.com/cosmos/ibc-go/blob/main/docs/dev/pull-requests.md#commit-messages) to be used for the changelog entry in the PR description for review. - [ ] Re-reviewed `Files changed` in the Github PR explorer. - [ ] Review `Codecov Report` in the comment section below once CI passes. * chore: add support for tendermint debug log level (#3279) * build(deps): bump cosmossdk.io/math from 1.0.0-beta.6.0.20230216172121-959ce49135e4 to 1.0.0-rc.0 (#3285) Bumps [cosmossdk.io/math](https://github.com/cosmos/cosmos-sdk) from 1.0.0-beta.6.0.20230216172121-959ce49135e4 to 1.0.0-rc.0. <details> <summary>Commits</summary> <ul> <li>See full diff in <a href="https://github.com/cosmos/cosmos-sdk/commits/math/v1.0.0-rc.0">compare view</a></li> </ul> </details> <br /> [![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=cosmossdk.io/math&package-manager=go_modules&previous-version=1.0.0-beta.6.0.20230216172121-959ce49135e4&new-version=1.0.0-rc.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> * Write docker inspect output to diagnostics (#3291) * chore: fix dead links (#3293) closes: #XXXX ```bash type: commit message ``` see the [guidelines](https://github.com/cosmos/ibc-go/blob/main/docs/dev/pull-requests.md#commit-messages) for commit messages. (view raw markdown for examples) --- Before we can merge this PR, please make sure that all the following items have been checked off. If any of the checklist items are not applicable, please leave them but write a little note why. - [x] Targeted PR against correct branch (see [CONTRIBUTING.md](https://github.com/cosmos/ibc-go/blob/main/docs/dev/pull-requests.md#pull-request-targeting)). - [ ] Linked to Github issue with discussion and accepted design OR link to spec that describes this work. - [ ] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/main/docs/docs/building-modules/10-structure.md) and [Go style guide](../docs/dev/go-style-guide.md). - [ ] Wrote unit and integration [tests](https://github.com/cosmos/ibc-go/blob/main/testing/README.md#ibc-testing-package). - [ ] Updated relevant documentation (`docs/`) or specification (`x/<module>/spec/`). - [ ] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code). - [ ] Provide a [commit message](https://github.com/cosmos/ibc-go/blob/main/docs/dev/pull-requests.md#commit-messages) to be used for the changelog entry in the PR description for review. - [x] Re-reviewed `Files changed` in the Github PR explorer. - [ ] Review `Codecov Report` in the comment section below once CI passes. * build(deps): bump google.golang.org/protobuf from 1.29.0 to 1.29.1 (#3292) * deps: bump SDK v0.47 (#3295) Co-authored-by: Damian Nolan <damiannolan@gmail.com> Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> * remove unnecessary import from doc * chore: remove support for v3 (#3294) * build(deps): bump actions/setup-go from 3 to 4 (#3307) * imp: remove unnecessary defer func statements (#3304) * Remove gogoproto yaml tags from proto files (#3290) Refer from original issue, I removed all `yaml` tags in proto files. closes: #3145 ```bash type: commit message ``` see the [guidelines](https://github.com/cosmos/ibc-go/blob/main/docs/dev/pull-requests.md#commit-messages) for commit messages. (view raw markdown for examples) --- Before we can merge this PR, please make sure that all the following items have been checked off. If any of the checklist items are not applicable, please leave them but write a little note why. - [ ] Targeted PR against correct branch (see [CONTRIBUTING.md](https://github.com/cosmos/ibc-go/blob/main/docs/dev/pull-requests.md#pull-request-targeting)). - [ ] Linked to Github issue with discussion and accepted design OR link to spec that describes this work. - [ ] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/main/docs/docs/building-modules/10-structure.md) and [Go style guide](../docs/dev/go-style-guide.md). - [ ] Wrote unit and integration [tests](https://github.com/cosmos/ibc-go/blob/main/testing/README.md#ibc-testing-package). - [ ] Updated relevant documentation (`docs/`) or specification (`x/<module>/spec/`). - [ ] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code). - [ ] Provide a [commit message](https://github.com/cosmos/ibc-go/blob/main/docs/dev/pull-requests.md#commit-messages) to be used for the changelog entry in the PR description for review. - [ ] Re-reviewed `Files changed` in the Github PR explorer. - [ ] Review `Codecov Report` in the comment section below once CI passes. * add reasoning for migration to enable localhost * Support configuration file for e2e tests (#3260) * E2E fzf Test Selection Autocompletion (#3313) * post v7 release chores (#3310) * chore: fix linter warnings (#3311) * ADR 008: IBC Actor Callbacks (#1976) * context and decision * complete adr * Apply suggestions from code review Co-authored-by: Carlos Rodriguez <carlos@interchain.io> * change from caller to generalized actor * Apply suggestions from code review Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> * create folder and scaffolded middleware * add error handling and generify packetdata interface * complete renaming * add user defined gas limit and clarify pseudocode * Clarify CallbackPacketData interface imp: Add ADR 008: IBC Actor Callbacks --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io> Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> * lint: fix spelling * chore: apply self review concerns * chore: rename CallbackPacketDataI to CallbackPacketData * chore: finish applying remaining review suggestions * test: add remaining unit tests for transfer and ica * test: add unmarshaling test * imp: address ADR 8 review suggestions (#3319) --------- Co-authored-by: Damian Nolan <damiannolan@gmail.com> * Bump interchain test (#3314) * fix: remove codec registration * fix: build + linting * Only run e2e on R4R (#3330) * fix fork workflows (#3328) * Revert "Merge branch 'main' of github.com:cosmos/ibc-go into colin/callback-packet-data-impl" This reverts commit 1c6164b, reversing changes made to 6f25b8e. * chore: add optional interface godoc * Apply suggestions from code review Co-authored-by: Carlos Rodriguez <carlos@interchain.io> * chore: use backticks instead of escape characters in testing --------- Co-authored-by: Lặc <67097720+expertdicer@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Cian Hatton <cian@interchain.io> Co-authored-by: Carlos Rodriguez <carlos@interchain.io> Co-authored-by: Damian Nolan <damiannolan@gmail.com> Co-authored-by: GNaD13 <89174180+GNaD13@users.noreply.github.com> Co-authored-by: Hieu Vu <72878483+hieuvubk@users.noreply.github.com> Co-authored-by: Aditya <adityasripal@gmail.com> * fix: merge conflicts * chore: nits from self review * chore: add link to ADR 008 in godoc --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io> Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> Co-authored-by: Lặc <67097720+expertdicer@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Cian Hatton <cian@interchain.io> Co-authored-by: Damian Nolan <damiannolan@gmail.com> Co-authored-by: GNaD13 <89174180+GNaD13@users.noreply.github.com> Co-authored-by: Hieu Vu <72878483+hieuvubk@users.noreply.github.com>

fedekunze · 2023-03-29T19:02:48Z

modules/core/exported/packet.go

+	// GetSourceCallbackAddress should return the callback address of a packet data on the source chain.
+	// This may or may not be the sender of the packet. If no source callback address exists for the packet,
+	// an empty string may be returned.
+	GetSourceCallbackAddress() string
+
+	// GetDestCallbackAddress should return the callback address of a packet data on the destination chain.
+	// This may or may not be the receiver of the packet. If no dest callback address exists for the packet,
+	// an empty string may be returned.
+	GetDestCallbackAddress() string


curious why these are string as opposed to addresses

We didn't want to assert that these were SDK addresses.

Conflicts:

AdityaSripal added 2 commits March 13, 2023 18:58

adr 8 with 20/27 implementation

bf93c0e

change interface name and register codecs

8c16e8d

AdityaSripal requested review from colin-axner and damiannolan as code owners March 13, 2023 18:09

AdityaSripal marked this pull request as draft March 13, 2023 18:09

documentation

3500d06

AdityaSripal commented Mar 13, 2023

View reviewed changes

AdityaSripal marked this pull request as ready for review March 13, 2023 18:40

AdityaSripal commented Mar 16, 2023

View reviewed changes

nicolaslara reviewed Mar 16, 2023

View reviewed changes

add comma before new line

89f608c

crodriguezvega reviewed Mar 20, 2023

View reviewed changes

crodriguezvega modified the milestones: v7.2.0, v7.1.0 Mar 20, 2023

colin-axner reviewed Mar 20, 2023

View reviewed changes

AdityaSripal mentioned this pull request Mar 20, 2023

ADR 008: IBC Actor Callbacks #1976

Merged

9 tasks

AdityaSripal added 3 commits March 22, 2023 12:15

fix return arg and dest for ica

b77cfe6

add ica tests

1db4626

Merge branch 'adr-8-interface-impl' of github.com:cosmos/ibc-go into …

32b74ec

…adr-8-interface-impl

colin-axner self-assigned this Mar 22, 2023

colin-axner mentioned this pull request Mar 22, 2023

adr 8 callback packet data impl followups #3325

Merged

9 tasks

crodriguezvega added the backport-to-v7.1.x label Mar 23, 2023

crodriguezvega reviewed Mar 27, 2023

View reviewed changes

colin-axner and others added 2 commits March 27, 2023 17:42

Merge branch 'main' into adr-8-interface-impl

f826ddd

colin-axner changed the base branch from main to adr-8-callbacks-interface-impl March 27, 2023 15:44

This was referenced Mar 27, 2023

Add documentation for the interfaces required by the callbacks module #3350

Closed

Consider allowing alternative sender/receiver address in ICS 20 ADR 8 callbacks #3351

Open

colin-axner added 2 commits March 27, 2023 17:57

fix: merge conflicts

ca33576

chore: nits from self review

6bf400e

colin-axner removed the backport-to-v7.1.x label Mar 27, 2023

colin-axner approved these changes Mar 27, 2023

View reviewed changes

colin-axner removed their assignment Mar 27, 2023

damiannolan approved these changes Mar 28, 2023

View reviewed changes

colin-axner reviewed Mar 28, 2023

View reviewed changes

chore: add link to ADR 008 in godoc

523f40b

alpe mentioned this pull request Mar 28, 2023

[Do not merge] Spike adr-8 #3359

Closed

chatton approved these changes Mar 29, 2023

View reviewed changes

colin-axner merged commit 69b8971 into adr-8-callbacks-interface-impl Mar 29, 2023

colin-axner deleted the adr-8-interface-impl branch March 29, 2023 09:54

fedekunze reviewed Mar 29, 2023

View reviewed changes

crodriguezvega added backport-to-v7.1.x and removed backport-to-v7.1.x labels Apr 2, 2023

colin-axner pushed a commit that referenced this pull request Apr 5, 2023

ADR 8 Interface and Packet Data Implementations (#3287)

df18b12

Conflicts:

crodriguezvega mentioned this pull request Jun 12, 2023

v7.3.0 tracking issue #3815

Closed

20 tasks

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

		@@ -0,0 +1,19 @@
		package exported

		type CallbackPacketDataI interface {

ADR 8 Interface and Packet Data Implementations #3287

ADR 8 Interface and Packet Data Implementations #3287

Conversation

AdityaSripal commented Mar 13, 2023 • edited Loading

Description

Commit Message / Changelog Entry

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 18, 2023 • edited Loading

Codecov Report

crodriguezvega left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-axner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xekez commented Mar 20, 2023 • edited Loading

AdityaSripal commented Mar 21, 2023

0xekez commented Mar 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-axner commented Mar 27, 2023

colin-axner left a comment

Choose a reason for hiding this comment

damiannolan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-axner Mar 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chatton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-axner commented Mar 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdityaSripal commented Mar 13, 2023 •

edited

Loading

codecov-commenter commented Mar 18, 2023 •

edited

Loading

0xekez commented Mar 20, 2023 •

edited

Loading

colin-axner Mar 28, 2023 •

edited

Loading