Skip to content

Conversation

@webmaster128
Copy link
Member

@webmaster128 webmaster128 commented Mar 23, 2023

Copy link
Contributor

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice article. Minor comments about how much detail we want.

Fine if you ignore the second comment as TMI, but wanted to bring it up.

@webmaster128 webmaster128 mentioned this pull request Mar 23, 2023
@0xekez
Copy link
Contributor

0xekez commented Mar 23, 2023

this is excellent documentation. thank you!

@0xekez
Copy link
Contributor

0xekez commented Mar 23, 2023

some additional questions i have thinking about this a little:

  1. will the SDK ever override a success ACK?
  2. what exactly are the rules for what i can send in a packet? for example, empty string values are not allowed.

these may be more general SDK questions, so let me know if they're out of scope.


If you are using the acknowledgement types shipped with cosmwasm-std
([#1512](https://github.com/CosmWasm/cosmwasm/issues/1512)), your protocol's
acknowledgement is compatible with that.
Copy link
Contributor

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 how the StdAck is handled downstream. It is important to consider the state commit/rollback for "errors" scenario 2 and 3

Scenario 3:

If the "error" is returned to wasmd as IBCReceiveResult.Err, then the state is rolled back and the error text redacted (currently) and serialized into the json format above.

Scenario 2:

If the "error" is returned to wasmd as IBCReceiveResult.Ok.Acknowledgement then the data is returned as it is with the state committed. I assume StdAck is doing this but I may got it wrong

@alpe
Copy link
Contributor

alpe commented Mar 24, 2023

Good questions:

  1. will the SDK ever override a success ACK?

The ACK data is not overwritten but if any panic happens downstream in ibc-go or SDK (due to out of gas for example) the TX would roll back and no state would be persisted.
More likely to cause trouble are submessages returned with the IBCReceiveResponse. They would be processed before the ACK is passed downstream to ibc-go. For example, if a bank transfer submsg fails due to out of funds then the redacted error would be returned as ErrorAck instead of the contract's success Ack.

  1. what exactly are the rules for what i can send in a packet? for example, empty string values are not allowed.

An IBC packet is a raw bytes slice. You can put any data in there that makes sense for you. Can json, proto or any custom format. The total length of this packet data must be >0 or it will be rejected in ibc-go packet validation.
For the error returned by the contract, we do not have constraints. An empty string would be possible but does not make much sense.
I will add a note to CosmWasm/wasmd#1289

@alpe
Copy link
Contributor

alpe commented Mar 27, 2023

It would be good to document behaviour for contract/ sub-message events for the success/error Ack scenario as well.
I have noticed that we were relying on downstream event handling which is not intuitive. See CosmWasm/wasmd#1288 for context and discussion

Copy link
Contributor

@chipshort chipshort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very important info. Would be great to merge this.
I have added a suggestion for the changes we made in 2.0.

@webmaster128 webmaster128 force-pushed the ibc_packet_receive-erros branch from b0cfb73 to 0508f1c Compare March 9, 2024 12:26
Copy link
Contributor

@chipshort chipshort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@webmaster128
Copy link
Member Author

Thank you, Chris!

@webmaster128 webmaster128 merged commit fb08588 into main Mar 11, 2024
@webmaster128 webmaster128 deleted the ibc_packet_receive-erros branch March 11, 2024 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants