docs: Add docs to Acknowledgement.Success #3515
Conversation
webmaster128
requested review from
colin-axner,
AdityaSripal and
damiannolan
as code owners
Co-authored-by: Alexander Peters <alpe@users.noreply.github.com>
There was a problem hiding this comment.
Thanks @webmaster128 🙏
Small suggestion to be slightly more explicit :)
Perhaps we could be even more explicit:
Note: IBC application callback events are always persisted so long as
RecvPacketsucceeds without error.
Co-authored-by: Damian Nolan <damiannolan@gmail.com>
|
Thank you for the review and suggestion, @damiannolan! I applied your expanded note and structured the text a bit better. I'm not familiar with Go doc comment conventions (seems like no markdown here), so feel free to further polish that if needed. |
modules/core/exported/channel.go
Outdated
| // | ||
| // Note 1: IBC application callback events are always persisted so long as `RecvPacket` succeeds without error. | ||
| // | ||
| // Note 2: This method is independent of application level success/error which is encoded in the acknowledgement bytes in a protocol specific way. |
There was a problem hiding this comment.
I'm not sure what this note means? Could you elaborate?
There was a problem hiding this comment.
You have a public type called "Acknowledgement" which is not an IBC acknowledgement. And acknowledgement.Success has nothing to do with success acknowledgements. This is a source of confusion for users that I tried to resolve.
There was a problem hiding this comment.
Ah, by "This method is independent of application level success/error" this is referencing result and error acknowledgements, not the success/error of a transaction at the application layer?
And acknowledgement.Success has nothing to do with success acknowledgements
Hmm, I'm not sure I agree with this point. If by "success acknowledgement" you mean "result acknowledgement" then the only reason it is referred to "success" is because it returns true when fulfilling the acknowledgement.Success() interface function. The response/error acknowledgements are not required, they are sane default acknowledgements IBC apps may use if they wish not to define their own custom types
After reading this again, are you referring to this?
There was a problem hiding this comment.
by "This method is independent of application level success/error" this is referencing result and error acknowledgements, not the success/error of a transaction at the application layer?
Right. I used "success" as the opposite to an error acknowledgement.
I think my primary source of confusion is that the "Acknowledgement" interface is not the acknowledgement but contains the acknowledgement.
There was a problem hiding this comment.
Okay I see now. Thanks for the followup comments. I see how this can be confusing, especially given that an acknowledgement can wrap other acknowledgements. The idea of the interface is to convey information about the acknowledgements so that core IBC can do some useful logic without having reference to the actual type. That's why we need the Success() function and the Acknowledgement() function. So that core IBC can do some useful handling of state during receive packets and also set the marshalled bytes
Maybe we could make some more adjustments to clear up the confusion (I also see we need to add docs about wrapping acknowledgements). I will add some suggestions
|
Thank you @webmaster128! This is a great addition, we definitely should have added this earlier! |
Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
Co-authored-by: Jim Fasarakis-Hilliard <d.f.hilliard@gmail.com>
There was a problem hiding this comment.
Thanks again for you patience @webmaster128. I added some additional suggestions that should hopefully clear up any confusion for readers. Let me know what you think!
modules/core/exported/channel.go
Outdated
| // | ||
| // Note 1: IBC application callback events are always persisted so long as `RecvPacket` succeeds without error. | ||
| // | ||
| // Note 2: This method is independent of application level success/error which is encoded in the acknowledgement bytes in a protocol specific way. |
There was a problem hiding this comment.
Okay I see now. Thanks for the followup comments. I see how this can be confusing, especially given that an acknowledgement can wrap other acknowledgements. The idea of the interface is to convey information about the acknowledgements so that core IBC can do some useful logic without having reference to the actual type. That's why we need the Success() function and the Acknowledgement() function. So that core IBC can do some useful handling of state during receive packets and also set the marshalled bytes
Maybe we could make some more adjustments to clear up the confusion (I also see we need to add docs about wrapping acknowledgements). I will add some suggestions
Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
|
Merci |
Closes #3434