OnRecv changes to revert state on failed acknowledgement

[colin-axner]

Summary

Currently if partial application logic occurs in the OnRecvPacket callback and a failed acknowledgement is returned, these partial state changes persist. See #68

There are two potential solutions:

• add a 4th return argument to the callback to indicate if the application state should be reverted
• revert all application logic callback if an error is returned

Adding a 4th argument allows the most flexibility since the application developer can revert application state on a failed acknowledgement and still decide to revert the entire transaction in a unique situation.

Reverting all application logic on an error returned makes the most sense if there exists no use case to revert the entire transaction. Note, a panic in the callback will result in a reverted transaction. Returning an error which results in a successful SDK transaction is semantically different than the other callbacks. Returning an error in all the other callbacks results in the entire transaction failing

The 1st case might look like this at the application level

    revert = true
    return result, failedAck, revert, nil // failed ack, revert app state

or

    revert = false
    return result, successAck, revert, nil // successful ack, write app state

or

    return result, nil, revert, err // failed transaction, revert all state changes (including in core IBC)

where true indicates to revert the application state and the error indicates if the transaction should be reverted.

The 2nd case might look like this at the application level

    return result, failedAck, err // failed ack, revert app state

or

    return result, successAck, nil // successful ack, write app state

NOTE: using the following is a non-atomic failed callback, this could be potentially dangerous if the developer is not aware of these concerns.

    return result, failedAck, nil // failed ack, write app state

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged/assigned

[ethanfrey]

I agree with the general design and am more in line with the 2nd case. One thing I am grappling with is that in Go, if a function returns err != nil, then generally you do not consider any of the other arguments (usually null or garbage). I think the approach should be made to make it as easy as possible for app developers to do the right thing, even if you limit their choices a bit (and ideally some way to opt-in to the complex case).

This choice has been made over and over again in the SDK. For example, defining a standard set of AnteHandlers, which run before the contract and the rollback logic in base app. This is far less flexible and powerful than the approach I had suggested, but is much more obvious for external devs just to use it.

[AdityaSripal]

I also am in favor of the second approach. I don't believe there's a reason for an error returned in OnRecvPacket callback to revert the entire tx. I also think having a revert boolean and an error is just confusing imo with limited added benefits.

I think limiting developer choice here is worthwhile for simplicity unless there's a very strong reason that we might want to support this case 1 possibility:
nil, false, error -> revert full tx

[colin-axner]

One thing I realized about the second approach that is slightly odd is that the error returned to core IBC is never logged (it is likely logged in the failed ack by the app developer)

Any thoughts about just replacing the error with the boolean?

    return result, failedAck, revert

One nice side benefit of removing the error is that the return signature loses conventional golang notions of what an error means. It is a little more explicit to the app developer in indicating that something different happens in this callback

[ethanfrey]

Any thoughts about just replacing the error with the boolean?

I would actually take another approach, by having the error actually carry the failed acknowledgement packet.

So you get:

return result, successAck, nil

or

err := failingAckError(failedAck)
return nil, nil, err

Passing thing info via the error requires a special interface like:

type AckFailure interface {
  error
  Acknowledgement() []byte
}

I would even go father (making this dev-friendly) that if the error doesn't implement AckFailure to just use the default 4-channel ack packet JSON encoded (like ics20). Something like:

var ack []byte
if ackErr, ok := err.(AckFailure); ok {
  ack = ackErr.Acknowledgement()
} else {
  resp := channeltypes.Acknowledgement{Error: err.String()}
  // FIXME: handle error here
  ack, _ = json.Marshal(resp) 
}

[colin-axner]

I like the proposal in general, but two things strike me as odd.

1. We are making the assumption an acknowledgement is either successful or failed. It seems less complex to construct an interface for an acknowledgement to indicate this:

type Acknowledgement interface {
    Success() bool
    Acknowledgement() []byte
}

2. I like the devUX of constructing the standard ack using the error, but developers need to know how to decode the acknowledgement in OnAckPacket callback which makes this step feel not very useful

If we used an acknowledgement interface to specify success or failure, we could end up with just returning the acknowledgement itself (The result return value is currently discarded and should be removed

    var ack channel.Acknowledgement
    if err != nil {
        ack = MyFailedAck{err}
    } else {
        ack = MySuccessAck{}
    }
   
    return ack

in core ibc

    if ack.Success() {
        // write state changes
        writeFn()
    }
    
    WriteAcknowledgement(ack.Acknowledgement())

[ethanfrey]

I like this idea. And since you can no longer return []byte, it forces devs to explicitly mark it as a success or failure