-
Notifications
You must be signed in to change notification settings - Fork 19
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
Properly test callbacks to contracts #14
Conversation
0b22872
to
ffda656
Compare
This is completed. Proper full stack tests with the callback contract receiving and storing the result properly. |
c11bed0
to
ef330cf
Compare
@webmaster128 or @JakeHartnell I'd like a review here so I can merge it in. Maybe comments on how to improve the implementation as well |
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.
Nice!
I guess the example would be better to understand if the callback capturer had an identity of its own and not only pass requests one step further. But this is just some feeling imagening a newcomer trying to understand all this wiring.
I think we should find a better name for "IBC query" because IBC queries are something different that do not require the data source chain to perform any writes. Maybe something like "foreign query" (i.e. a query executed on the other chain, not only reading the other chain). But I don't think I have a great solution here.
msg: to_binary(&ReceiveIbcResponseMsg { id, msg })?, | ||
funds: vec![], | ||
}); | ||
.add_message(ReceiveIbcResponseMsg { id, msg }.into_cosmos_msg(sender)?); |
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 this be a sub-message with gas limit? Because simple-ica-controller has no control over what the callback capturer executes. I guess we don't want the relayer to be responsible for paying all the gas of something heavy.
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.
That is a very good design question. So far this is a "sample", but it would be good to raise such an issue as to be useful in production, we would need some limit
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.
For me the motivation would be to tell the reader: Hey, look, this is something you need to think about!
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.
Can add a comment. Something like:
.add_message(ReceiveIbcResponseMsg { id, msg }.into_cosmos_msg(sender)?); | |
// In production, you may want to think about gas limits for this callback. | |
.add_message(ReceiveIbcResponseMsg { id, msg }.into_cosmos_msg(sender)?); |
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.
Leaving this unresolved, as this is something important to revisit.
Made issue #17 for it
e9542d3
to
0aa6ef2
Compare
Extends #13
Main goal is to add a contract we can use to ensure callbacks work properly.
We also open up the IbcQuery to use
QueryRequest
not justWasmQuery
.We add a new callback-capturer contract that can call the simple_ica_controller with a callback and then stores it locally. This is not so interesting for production, but a great way to test and query on a full-stack integration test.
Add some TS tests that have the contract call the controller with callbacks enabled and store the result upon ack callback. It works properly now and found some issues in the existing Rust code interactions.
(When reviewing, you can skip all
callback-capturer
first and look at changes to the other contracts. That shows changes to the core logic. This is a test contract and probably just needs a basic review, but other changes deserve more attention)