ICS27 cleanup #639
ICS27 cleanup #639
Conversation
AdityaSripal
commented
Jan 17, 2022
AdityaSripal
commented
- Minor cleanups and fixes to keep consistent with implementation
| @@ -118,10 +118,11 @@ function AuthenticateTx(msgs []Any, portId string) error { | |||
| Executes each message sent by the owner account on the controller chain. | |||
|
|
|||
| ```typescript | |||
| function ExecuteTx(sourcePort: string, destPort: string, destChannel: string, msgs []Any) error { | |||
There was a problem hiding this comment.
These fields are not used
| return ack | ||
| switch data.Type { | ||
| case types.EXECUTE_TX: | ||
| msgs, err := types.DeserializeTx(data.Data) |
There was a problem hiding this comment.
Isn't the norm "throwing exceptions" in this ics spec "pseudo code".
It seems modelled heavily on TypeScript
There was a problem hiding this comment.
We don't want to throw an exception in this case and abort the transaction, we want to write an error acknowledgement and commit the state changes
There was a problem hiding this comment.
Understood. But the idiom msg, err := fn() is very go. The rest of the ibc specs are modelled on TypeScript.
I would say:
try {
const msg = deserializeTx(data.data)
// ... other stuff
} catch (err) {
return newErrorAcknowledgement(err)
}would look more in line with the other specs (but still needs some adjustment).
I believe the specs were intentionally not written in Go to keep a clear distinction between them and the implementation. Whether that was a good decision I will leave to you and @cwgoes to discuss. I just like consistency in the style.
| case types.EXECUTE_TX: | ||
| msgs, err := types.DeserializeTx(data.Data) | ||
| if err != nil { | ||
| return NewErrorAcknowledgement(err) |
There was a problem hiding this comment.
should the spec discuss the implication of returning the error in the acknowledgement? That is, including the error string in the acknowledgement makes error strings state machine breaking
There was a problem hiding this comment.
Maybe this should be optional, as we will not include the error in the ibc-go implementation
There was a problem hiding this comment.
should the spec discuss the implication of returning the error in the acknowledgement? That is, including the error string in the acknowledgement makes error strings state machine breaking
Very good point.
That has proved to be the case for CosmWasm with submessages and we did hit a case where one node included the stack trace and fell out of consensus.
I like to include error message, but this is a very good thing to think through (and check code paths)
| } | ||
|
|
||
| // return acknowledgement containing the transaction result after executing on host chain | ||
| return NewAcknowledgement(result) |
There was a problem hiding this comment.
ditto for implications of including the result
There was a problem hiding this comment.
The result of a transaction is currently hashed and included in the the Tendermint block header (DataHash I believe). The data field and the code (u32) are the only fields currently consensus breaking (not events). I think including Data is fully acceptable, where errors and events may need more consideration.
There was a problem hiding this comment.
Great insight! I did not know the data field is included in consensus. I agree, if the result is already included in consensus then it is fully acceptable to include in the acknowledgement
There was a problem hiding this comment.
Looks good.
Agree with @ethanfrey about the state of the pseudo-code in general. We should have a standard for this.
|
Superceded by #643 |
