Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
ADR 8 Interface and Packet Data Implementations #3287
Changes from 3 commits
bf93c0e
8c16e8d
3500d06
89f608c
b77cfe6
1db4626
32b74ec
f8298da
f826ddd
ca33576
6bf400e
523f40b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 itThere 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.
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 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
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.
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.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.
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 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.
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 would also lean towards not pulling in a third party library for json processing.
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 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
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 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)
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 does allow multiple messages
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.
ditto
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 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.
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.
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?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'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?
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.
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
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 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)
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.
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 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
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.
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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
opened #3351 to track this discussion
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.
We can probably do
since non-strings would still fail as they can't be compared to
ftpd.Receiver
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 it possible to have additional fields in the callbacks section that will not be strings? Maybe something like:
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?
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.
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 likeContractCallbackPacketDataI
to make it specific to the use case of smart contracts?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.
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.
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 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.