-
Notifications
You must be signed in to change notification settings - Fork 359
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
Create Contract to send cw20 tokens over ics20 #238
Conversation
aaa5668
to
4480f4b
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.
No time left for a proper require but I peeked and ❤️ it
contracts/cw20-ics20/src/ibc.rs
Outdated
let res = IbcReceiveResponse { | ||
acknowledgement: ack_success()?, | ||
messages: vec![msg], | ||
// TODO: similar event messages like ibctransfer module |
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.
Following event is emitted:
types.EventTypePacket,
sdk.NewAttribute(sdk.AttributeKeyModule, types.ModuleName),
sdk.NewAttribute(types.AttributeKeyReceiver, data.Receiver),
sdk.NewAttribute(types.AttributeKeyDenom, data.Denom),
sdk.NewAttribute(types.AttributeKeyAmount, fmt.Sprintf("%d", data.Amount)),
sdk.NewAttribute(types.AttributeKeyAckSuccess, fmt.Sprintf("%t", err != nil)),
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.
ohh.... this is a very important and very major issue!
packet and acknowledgements are never stored in the iavl tree, but rather their hashes are there and you need to get the preimage from the events.
When we submit SendPacket
, this is handled in wasmd and should emit such a packet (need to check). Since the contract cannot create custom event types, we need to also auto-emit such an event in wasmd for acknowledgements. Otherwise we cannot relay such packets.
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.
Okay, investigated a bit more. The ChannelKeeper manages all the essential events.
On SendPacket
and WriteAcknowledgement
In wasmd
, we call SendPacket
on receiving the IbcMsg::SendPacket. However, I cannot find where WriteAcknowledgement
is called.
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.
Update: We return the acknowledgement from IbcModule.OnRecvPacket
in x/wasm
.
That is the same pattern that is used in ibctransfer
and when digging deeper, I see that the acknowledgement is written by the ibc msg server.
So, it looks like we don't need to do anything here, but add some app-specific attributes
cur.outstanding = (cur.outstanding - amount)?; | ||
Ok(cur) | ||
}, | ||
)?; |
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 fully understand this block so I may go this wrong. The "no funds" is an IBC application level error that is part of the payload and should not result in an error that reverts the TX in the module.
See https://github.com/cosmos/cosmos-sdk/blob/v0.41.4/proto/ibc/core/channel/v1/channel.proto#L145
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.
You are very correct here.
I made a separate function in 0844e34 so that I can catch the errors and turn them into "acknowledgement messages containing errors".
8289d53
to
704e2af
Compare
Rebased with I'd like a review, but I guess I merge if no blockers and test with the relayer. There will likely be a few more follow up PRs on this anyway, so happy for comments even after a merge |
Closes #226