Add a generic function to construct error acks with abci codes & string const

[damiannolan]

Summary

Code is duplicated in ics27 and ics20 in package types to provide deterministic error acknowledgements that include the ABCI error code and a string constant defined at the local package level.
Application developers may want to define their own string constants to be included, however a generic approach would also be useful.

See: #777 & #794

Proposal

• Add a generic function to 04-channel/types which extracts the ABCI code from a given error and returns a deterministic error acknowledgement

---

For Admin Use

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

[colin-axner]

After giving this more thought. I'm very much in favor of adjusting the current channeltypes.NewErrorAcknowledgement to perform this behaviour. I'd rather require developers to explicitly work around this function (if they really want to avoid the error message extraction)

[damiannolan]

Is this something we should try to get included for v3? If so, I can pick this up later this week.

[colin-axner]

I think not (that boat has sailed), but lets get it in for a minor release (it doesn't need to be API breaking)

[crodriguezvega]

So is it the idea to refactor the existing channeltypes.NewErrorAcknowledgement, so that it looks like this?

func NewErrorAcknowledgement(err error, errString string) Acknowledgement {
	// the ABCI code is included in the abcitypes.ResponseDeliverTx hash
	// constructed in Tendermint and is therefore deterministic
	_, code, _ := sdkerrors.ABCIInfo(err, false) // discard non-determinstic codespace and log values

	errorString := fmt.Sprintf("ABCI code: %d: %s", code, errString)

	return Acknowledgement{
		Response: &Acknowledgement_Error{
			Error: errorString,
		},
	}
}

The errString passed as input parameter can be "error handling packet on host chain: see events for details" for interchain accounts, "error handling packet on destination chain: see events for details" for transfers, or any other message that is currently used in the other locations where we use channeltypes.NewErrorAcknowledgement?

[colin-axner]

I actually don't think we should take in an error string as an argument. The intention behind this function is that we remove the possibility of accidental state changes via modification of the error string. I think it is very likely this function would be misused if we allowed the developers to pass in their own error string. I'd prefer to use our own constant "error handling packet: see events for details". If developers want a custom error string, they can construct their own error acknowledgement (as currently requried)

[crodriguezvega]

Ok, cool. Then we have to remove the NewErrorAcknowledgement for transfers and the NewErrorAcknowledgement for ICA and we refactor channeltypes.NewErrorAcknowledgement so that it becomes

const (
	// ackErrorString defines a string constant included in error acknowledgements
	// NOTE: Changing this const is state machine breaking as acknowledgements are written into state
	ackErrorString = "error handling packet: see events for details"
)

// NewErrorAcknowledgement returns a deterministic error string which may be used in
// the packet acknowledgement.
func NewErrorAcknowledgement(err error) channeltypes.Acknowledgement {
	// the ABCI code is included in the abcitypes.ResponseDeliverTx hash
	// constructed in Tendermint and is therefore deterministic
	_, code, _ := sdkerrors.ABCIInfo(err, false) // discard non-determinstic codespace and log values

	errorString := fmt.Sprintf("ABCI code: %d: %s", code, ackErrorString)

	return Acknowledgement{
		Response: &Acknowledgement_Error{
			Error: errorString,
		},
	}
}

And we use this one everywhere.