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
Ack callbacks for IBC transfers #3589
Ack callbacks for IBC transfers #3589
Changes from 22 commits
2f48bf0
a119e98
a99bee2
eff6127
696bc72
ce4ef41
d44c9f7
a03d6bb
f289823
5bdeb31
981dad6
4987812
bccc9a4
0bfa4a3
37c80f4
05fe3ae
7d3566c
f83a3e2
ade5cdd
aadb41f
ba8bf7b
0a9ac6b
6aeef7b
3857b43
7001f1e
ff8ea2f
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.
Just realized, contract authors have to handle someone setting a callback the contract wasn't expecting. (So they need to do some validation)
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.
Should we add a version field on
ibc_callback
or probably not needed? (e.g.ibc_callback_v1
)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.
Senders don't specify the callback format though. Just who will receive it. So as long as the format doesn't change on the chain, contracts should be able to trust the interface.
I like the idea of versioning when we need it. But since we don't have plans for v2 yet, I think the easiest thing here would be to leave it as is and then add
ibc_callback_v2
if we make a change.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.
Would it be at all valuable to include a MsgTransfer without a memo, to ensure packet is still properly relayed?
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.
added a test for this. I'm still using NewMsgTransfer but with "" as 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.
Sweet! Do you feel like using a message that has the field completely missing would potentially act differently than a blank string?
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 think it will, as
getMemo()
would still return the default""
if the memo isn't there.