-
Notifications
You must be signed in to change notification settings - Fork 373
feat: Add IBCv2 timeout entrypoint #2454
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
Conversation
4f9b672
to
2ca6b99
Compare
448753a
to
cb4fc7f
Compare
cb4fc7f
to
cae413d
Compare
pub const REQUIRED_IBC2_EXPORT: &[Entrypoint] = | ||
&[Entrypoint::Ibc2PacketReceive, Entrypoint::Ibc2PacketTimeout]; |
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.
If I remember correctly we decided that the Ibc2PacketReceive
is the only entry point required to determine that the contract supports the Ibc2. @hashedone do we have to check other ep too?
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.
Currently the HasIBC2EntryPoint flag is used only during instantiation and migration procedure to register the contract to the ibc-go router. If IBC team will implement the prefix-based routing this flag won't be used anymore. We can consider calling AnalyzeCode
each time an IBCv2 message comes before calling the entry point. Though we might end up with increasing the time spent on double-checking a contract, which is already part of calling the entrypoint in CosmWasm.
So we have to decide if we want to keep this flag in the contract analysis report at all
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.
IMHO, we can get rid of the flag, since we don't need to find out if the contract needs an IBCv2 port. We can just do the call in wasmvm and if it doesn't have the entrypoint, the call errors.
That also makes for much nicer usage because I don't have to implement the receive entrypoint if I only send.
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.
How is the ack/timeout dispatched? Is it using port from which message was sent, or there is some other way (like registering for the response for a particular packet)?
The answer here is relevant:
- If the ack/timeout is dispatched based on the original packet sender port we need to export ibc2 capabilities for the contract that is exporting ack/timeout as well, otherwise the contract will not be able to handle acks/callbacks
- If ack/timeout dispatching is not port-based, I don't see any reason to enable ibc2 capabilities for contract without recv entry point.
By enabling ibc2 capabilities I basically making sure the contract is capable of receiving ibc2 messages - having a port allocated, and dispatching for this port set up. However every single contract, regardless of the fact if it is supposed to receive any messages, but also regardless of implementing ack/timeout has to be able to send ibc2 messages (like ibc "fire and forget"). So if timeout and ack are dispatched basing on sender port, then every single contract should have the port allocated (unless we can skip the port info if we don't expect ack/timeout - I did not dig into ibc-go codebase).
Finally - similar question/issue would probably be there when implementing the ibc2_send
entry point. If I understand correctly, the idea of send handler is that every message send from the blockchain is triggering this handler, not only those send from particular port. That would make it possible to use this entry point to basically observe messages traffic and react to them in certain way. If that is true, then I suppose ibc2_send
would not need to enable ibc2 capabilities - if the entry point would be defined, we could just register it in the handler.
The big question about this entry point however is that this kind of handling might lead to lot of costly executions - basically all the issues of cron/begin blocker. Because of that, we would have to provide additional protection that would need to be addressed by integrator. My suggestion is - we implement the send entry point, but we never register it to the handler. Instead, we will provide go API, to add the particular contract address as a ibc2 observer (with the requirement that the contract is implementing this entry point), and not it's the chain integrator job to define how to handle gas about this, maybe to just make it gov-voting which contracts are allowed for that. Obv this is not related to this issue, but it just hit me today when I was thinking of that, so copy this comment somewhere in proper issue so its kept 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.
To clear up some of those points:
If the ack/timeout is dispatched based on the original packet sender port we need to export ibc2 capabilities for the contract that is exporting ack/timeout as well, otherwise the contract will not be able to handle acks/callbacks
Yes, ack and timeout are routed using the source port in ibc-go and we would then extract the contract address from that and call the corresponding contract.
Why do we need to export capabilities for that? We can just try to call the entrypoint and if it doesn't exist, we error.
ibc2_send
is only called for those payloads in the packet that are sent from the module's (in our case contract's) port. As can be seen here: https://github.com/cosmos/ibc-go/blob/bd79edf6749398e36fb367730b7e20da5c563806/modules/core/04-channel/v2/keeper/msg_server.go#L35C2-L42C1
Again, this entrypoint is absolutely needed and every contract that needs any kind of logic on the sending side (e.g. locking funds) needs to have it.
Co-authored-by: Jan Woźniak <[email protected]>
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.
Code looks good. Just some minor comments about the docs
85d58b0
to
0ff5104
Compare
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.
LGTM
No description provided.