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

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

@@ -19,7 +19,7 @@ const (
 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(types.AckErrorString)
+			ack = types.NewErrorAcknowledgement(err)
 		}
 	}
 

modules/apps/transfer/types/ack.go

@@ -1,7 +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
+	// 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"
+	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)
+
+}