chore: replace error string in transfer acks with const (#818) (#831)

* fix: adding ack error string const for transfer

* updating godoc

* adding warning note to godoc in 04-channel

* updating to include abci error code, and copy tests from ica

* adding changelog entry

(cherry picked from commit ac46ac0)

Co-authored-by: Damian Nolan <damiannolan@gmail.com>

CHANGELOG.md

@@ -56,6 +56,8 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ### State Machine Breaking
 
+* (transfer) [\#818](https://github.com/cosmos/ibc-go/pull/818) Error acknowledgements returned from Transfer `OnRecvPacket` now include a deterministic ABCI code and error message.
+
 ### Improvements
 
 * (testing) [\#810](https://github.com/cosmos/ibc-go/pull/810) Additional testing function added to `Endpoint` type called `RecvPacketWithResult`. Performs the same functionality as the existing `RecvPacket` function but also returns the message result. `path.RelayPacket` no longer uses the provided acknowledgement argument and instead obtains the acknowledgement via MsgRecvPacket events.

modules/apps/27-interchain-accounts/host/types/ack.go

@@ -14,12 +14,12 @@ const (
 	ackErrorString = "error handling packet on host chain: see events for details"
 )
 
-// AcknowledgementErrorString returns a deterministic error string which may be used in
+// 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 determinstic
-	_, code, _ := sdkerrors.ABCIInfo(err, false) // discard non-determinstic codespace and log values
+	_, code, _ := sdkerrors.ABCIInfo(err, false) // discard non-deterministic codespace and log values
 
 	errorString := fmt.Sprintf("ABCI code: %d: %s", code, ackErrorString)
 

modules/apps/transfer/ibc_module.go

@@ -181,7 +181,7 @@ func (im IBCModule) OnRecvPacket(
 	if ack.Success() {
 		err := im.keeper.OnRecvPacket(ctx, packet, data)
 		if err != nil {
-			ack = channeltypes.NewErrorAcknowledgement(err.Error())
+			ack = types.NewErrorAcknowledgement(err)
 		}
 	}
 

modules/apps/transfer/types/ack.go

@@ -0,0 +1,27 @@
+package types
+
+import (
+	"fmt"
+
+	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
+
+	channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types"
+)
+
+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 on destination chain: 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 channeltypes.NewErrorAcknowledgement(errorString)
+}

modules/apps/transfer/types/ack_test.go

@@ -0,0 +1,101 @@
+package types_test
+
+import (
+	"testing"
+
+	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
+	"github.com/stretchr/testify/suite"
+	abcitypes "github.com/tendermint/tendermint/abci/types"
+	tmprotostate "github.com/tendermint/tendermint/proto/tendermint/state"
+	tmstate "github.com/tendermint/tendermint/state"
+
+	"github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/host/types"
+	ibctesting "github.com/cosmos/ibc-go/v3/testing"
+)
+
+const (
+	gasUsed   = uint64(100)
+	gasWanted = uint64(100)
+)
+
+type TypesTestSuite struct {
+	suite.Suite
+
+	coordinator *ibctesting.Coordinator
+
+	chainA *ibctesting.TestChain
+	chainB *ibctesting.TestChain
+}
+
+func (suite *TypesTestSuite) SetupTest() {
+	suite.coordinator = ibctesting.NewCoordinator(suite.T(), 2)
+
+	suite.chainA = suite.coordinator.GetChain(ibctesting.GetChainID(1))
+	suite.chainB = suite.coordinator.GetChain(ibctesting.GetChainID(2))
+}
+
+func TestTypesTestSuite(t *testing.T) {
+	suite.Run(t, new(TypesTestSuite))
+}
+
+// The safety of including ABCI error codes in the acknowledgement rests
+// on the inclusion of these ABCI error codes in the abcitypes.ResposneDeliverTx
+// hash. If the ABCI codes get removed from consensus they must no longer be used
+// in the packet acknowledgement.
+//
+// This test acts as an indicator that the ABCI error codes may no longer be deterministic.
+func (suite *TypesTestSuite) TestABCICodeDeterminism() {
+	// same ABCI error code used
+	err := sdkerrors.Wrap(sdkerrors.ErrOutOfGas, "error string 1")
+	errSameABCICode := sdkerrors.Wrap(sdkerrors.ErrOutOfGas, "error string 2")
+
+	// different ABCI error code used
+	errDifferentABCICode := sdkerrors.ErrNotFound
+
+	deliverTx := sdkerrors.ResponseDeliverTx(err, gasUsed, gasWanted, false)
+	responses := tmprotostate.ABCIResponses{
+		DeliverTxs: []*abcitypes.ResponseDeliverTx{
+			&deliverTx,
+		},
+	}
+
+	deliverTxSameABCICode := sdkerrors.ResponseDeliverTx(errSameABCICode, gasUsed, gasWanted, false)
+	responsesSameABCICode := tmprotostate.ABCIResponses{
+		DeliverTxs: []*abcitypes.ResponseDeliverTx{
+			&deliverTxSameABCICode,
+		},
+	}
+
+	deliverTxDifferentABCICode := sdkerrors.ResponseDeliverTx(errDifferentABCICode, gasUsed, gasWanted, false)
+	responsesDifferentABCICode := tmprotostate.ABCIResponses{
+		DeliverTxs: []*abcitypes.ResponseDeliverTx{
+			&deliverTxDifferentABCICode,
+		},
+	}
+
+	hash := tmstate.ABCIResponsesResultsHash(&responses)
+	hashSameABCICode := tmstate.ABCIResponsesResultsHash(&responsesSameABCICode)
+	hashDifferentABCICode := tmstate.ABCIResponsesResultsHash(&responsesDifferentABCICode)
+
+	suite.Require().Equal(hash, hashSameABCICode)
+	suite.Require().NotEqual(hash, hashDifferentABCICode)
+}
+
+// TestAcknowledgementError will verify that only a constant string and
+// ABCI error code are used in constructing the acknowledgement error string
+func (suite *TypesTestSuite) TestAcknowledgementError() {
+	// same ABCI error code used
+	err := sdkerrors.Wrap(sdkerrors.ErrOutOfGas, "error string 1")
+	errSameABCICode := sdkerrors.Wrap(sdkerrors.ErrOutOfGas, "error string 2")
+
+	// different ABCI error code used
+	errDifferentABCICode := sdkerrors.ErrNotFound
+
+	ack := types.NewErrorAcknowledgement(err)
+	ackSameABCICode := types.NewErrorAcknowledgement(errSameABCICode)
+	ackDifferentABCICode := types.NewErrorAcknowledgement(errDifferentABCICode)
+
+	suite.Require().Equal(ack, ackSameABCICode)
+	suite.Require().NotEqual(ack, ackDifferentABCICode)
+
+}

modules/core/04-channel/types/acknowledgement.go

@@ -20,6 +20,8 @@ func NewResultAcknowledgement(result []byte) Acknowledgement {
 
 // NewErrorAcknowledgement returns a new instance of Acknowledgement using an Acknowledgement_Error
 // type in the Response field.
+// NOTE: Acknowledgements are written into state and thus, changes made to error strings included in packet acknowledgements
+// risk an app hash divergence when nodes in a network are running different patch versions of software.
 func NewErrorAcknowledgement(err string) Acknowledgement {
 	return Acknowledgement{
 		Response: &Acknowledgement_Error{