Skip to content
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

Add docs on Error handling for ibc_packet_receive #1646

Merged
merged 5 commits into from
Mar 11, 2024

Conversation

webmaster128
Copy link
Member

@webmaster128 webmaster128 commented Mar 23, 2023

Copy link
Member

@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.

docs/ERROR_HANDLING.md Show resolved Hide resolved
docs/ERROR_HANDLING.md Outdated Show resolved Hide resolved
@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

docs/ERROR_HANDLING.md Show resolved Hide resolved
@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
Collaborator

@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.

docs/ERROR_HANDLING.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@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
29 checks passed
@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.

5 participants