Consolidate usage of NewErrorAcknowledgement

CHANGELOG.md

@@ -47,6 +47,9 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * (modules/29-fee)[\#1338](https://github.com/cosmos/ibc-go/pull/1338) Renaming `Result` field in `IncentivizedAcknowledgement` to `AppAcknowledgement`.
 * (modules/29-fee)[\#1343](https://github.com/cosmos/ibc-go/pull/1343) Renaming `KeyForwardRelayerAddress` to `KeyRelayerAddressForAsyncAck`, and `ParseKeyForwardRelayerAddress` to `ParseKeyRelayerAddressForAsyncAck`.
 * (apps/27-interchain-accounts)[\#1432](https://github.com/cosmos/ibc-go/pull/1432) Updating `RegisterInterchainAccount` to include an additional `version` argument, supporting ICS29 fee middleware functionality in ICS27 interchain accounts.
+* (apps/27-interchain-accounts)[\#1565](https://github.com/cosmos/ibc-go/pull/1565) Removing `NewErrorAcknowledgement` in favour of `channeltypes.NewErrorAcknowledgement`.
+* (transfer)[\#1565](https://github.com/cosmos/ibc-go/pull/1565) Removing `NewErrorAcknowledgement` in favour of `channeltypes.NewErrorAcknowledgement`.
+* (channel)[\#1565](https://github.com/cosmos/ibc-go/pull/1565) Updating `NewErrorAcknowledgement` to accept an error instead of a string and removing the possibility of accidental state changes.
 
 ### State Machine Breaking
 

modules/apps/27-interchain-accounts/controller/ibc_middleware.go

@@ -1,6 +1,8 @@
 package controller
 
 import (
+	"fmt"
+
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
 	capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types"
@@ -137,7 +139,10 @@ func (im IBCMiddleware) OnRecvPacket(
 	packet channeltypes.Packet,
 	_ sdk.AccAddress,
 ) ibcexported.Acknowledgement {
-	return channeltypes.NewErrorAcknowledgement("cannot receive packet on controller chain")
+	err := fmt.Errorf("cannot receive packet on controller chain")
+	ack := channeltypes.NewErrorAcknowledgement(err)
+	keeper.EmitAcknowledgementEvent(ctx, packet, ack, err)
+	return channeltypes.NewErrorAcknowledgement(err)
 }
 
 // OnAcknowledgementPacket implements the IBCMiddleware interface

modules/apps/27-interchain-accounts/controller/keeper/events.go

@@ -0,0 +1,27 @@
+package keeper
+
+import (
+	"fmt"
+	sdk "github.com/cosmos/cosmos-sdk/types"
+	icatypes "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/types"
+	"github.com/cosmos/ibc-go/v3/modules/core/exported"
+)
+
+// EmitAcknowledgementEvent emits an event signalling a successful or failed acknowledgement and including the error
+// details if any.
+func EmitAcknowledgementEvent(ctx sdk.Context, packet exported.PacketI, ack exported.Acknowledgement, err error) {
+	var errorMsg string
+	if err != nil {
+		errorMsg = err.Error()
+	}
+
+	ctx.EventManager().EmitEvent(
+		sdk.NewEvent(
+			icatypes.EventTypePacket,
+			sdk.NewAttribute(sdk.AttributeKeyModule, icatypes.ModuleName),
+			sdk.NewAttribute(icatypes.AttributeKeyAckError, errorMsg),
+			sdk.NewAttribute(icatypes.AttributeKeyHostChannelID, packet.GetDestChannel()),
+			sdk.NewAttribute(icatypes.AttributeKeyAckSuccess, fmt.Sprintf("%t", ack.Success())),
+		),
+	)
+}

modules/apps/27-interchain-accounts/host/ibc_module.go

@@ -106,13 +106,13 @@ func (im IBCModule) OnRecvPacket(
 	_ sdk.AccAddress,
 ) ibcexported.Acknowledgement {
 	if !im.keeper.IsHostEnabled(ctx) {
-		return types.NewErrorAcknowledgement(types.ErrHostSubModuleDisabled)
+		return channeltypes.NewErrorAcknowledgement(types.ErrHostSubModuleDisabled)
 	}
 
 	txResponse, err := im.keeper.OnRecvPacket(ctx, packet)
 	ack := channeltypes.NewResultAcknowledgement(txResponse)
 	if err != nil {
-		ack = types.NewErrorAcknowledgement(err)
+		ack = channeltypes.NewErrorAcknowledgement(err)
 	}
 
 	// Emit an event indicating a successful or failed acknowledgement.

modules/apps/27-interchain-accounts/host/ibc_module_test.go

@@ -403,7 +403,7 @@ func (suite *InterchainAccountsTestSuite) TestOnRecvPacket() {
 				suite.chainB.GetSimApp().ICAAuthModule.IBCApp.OnRecvPacket = func(
 					ctx sdk.Context, packet channeltypes.Packet, relayer sdk.AccAddress,
 				) exported.Acknowledgement {
-					return channeltypes.NewErrorAcknowledgement("failed OnRecvPacket mock callback")
+					return channeltypes.NewErrorAcknowledgement(fmt.Errorf("failed OnRecvPacket mock callback"))
 				}
 			}, true,
 		},

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

@@ -9,7 +9,6 @@ import (
 	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"
 )
 
@@ -80,21 +79,3 @@ func (suite *TypesTestSuite) TestABCICodeDeterminism() {
 	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/apps/transfer/ibc_module.go

@@ -178,16 +178,19 @@ func (im IBCModule) OnRecvPacket(
 	ack := channeltypes.NewResultAcknowledgement([]byte{byte(1)})
 
 	var data types.FungibleTokenPacketData
+	var ackErrorMsg string
 	if err := types.ModuleCdc.UnmarshalJSON(packet.GetData(), &data); err != nil {
-		ack = channeltypes.NewErrorAcknowledgement("cannot unmarshal ICS-20 transfer packet data")
+		ackError := fmt.Errorf("cannot unmarshal ICS-20 transfer packet data")
+		ackErrorMsg = ackError.Error()
+		ack = channeltypes.NewErrorAcknowledgement(ackError)
 	}
 
 	// only attempt the application logic if the packet data
 	// was successfully decoded
 	if ack.Success() {
 		err := im.keeper.OnRecvPacket(ctx, packet, data)
 		if err != nil {
-			ack = types.NewErrorAcknowledgement(err)
+			ack = channeltypes.NewErrorAcknowledgement(err)
 		}
 	}
 
@@ -197,6 +200,7 @@ func (im IBCModule) OnRecvPacket(
 			sdk.NewAttribute(sdk.AttributeKeyModule, types.ModuleName),
 			sdk.NewAttribute(sdk.AttributeKeySender, data.Sender),
 			sdk.NewAttribute(types.AttributeKeyReceiver, data.Receiver),
+			sdk.NewAttribute(types.AttributeKeyAckError, ackErrorMsg),
 			sdk.NewAttribute(types.AttributeKeyDenom, data.Denom),
 			sdk.NewAttribute(types.AttributeKeyAmount, data.Amount),
 			sdk.NewAttribute(types.AttributeKeyAckSuccess, fmt.Sprintf("%t", ack.Success())),

modules/apps/transfer/keeper/mbt_relay_test.go

@@ -360,7 +360,7 @@ func (suite *KeeperTestSuite) TestModelBasedRelay() {
 					registerDenom()
 					err = suite.chainB.GetSimApp().TransferKeeper.OnAcknowledgementPacket(
 						suite.chainB.GetContext(), packet, tc.packet.Data,
-						channeltypes.NewErrorAcknowledgement("MBT Error Acknowledgement"))
+						channeltypes.NewErrorAcknowledgement(fmt.Errorf("MBT Error Acknowledgement")))
 				default:
 					err = fmt.Errorf("Unknown handler:  %s", tc.handler)
 				}

modules/apps/transfer/keeper/relay_test.go

@@ -255,7 +255,7 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() {
 func (suite *KeeperTestSuite) TestOnAcknowledgementPacket() {
 	var (
 		successAck = channeltypes.NewResultAcknowledgement([]byte{byte(1)})
-		failedAck  = channeltypes.NewErrorAcknowledgement("failed packet transfer")
+		failedAck  = channeltypes.NewErrorAcknowledgement(fmt.Errorf("failed packet transfer"))
 		trace      types.DenomTrace
 		amount     sdk.Int
 		path       *ibctesting.Path

modules/apps/transfer/types/ack_test.go

@@ -3,21 +3,11 @@ 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/transfer/types"
 	ibctesting "github.com/cosmos/ibc-go/v3/testing"
 )
 
-const (
-	gasUsed   = uint64(100)
-	gasWanted = uint64(100)
-)
-
 type TypesTestSuite struct {
 	suite.Suite
 
@@ -37,64 +27,3 @@ func (suite *TypesTestSuite) SetupTest() {
 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

@@ -1,13 +1,20 @@
 package types
 
 import (
+	"fmt"
 	"reflect"
 	"strings"
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
 )
 
+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"
+)
+
 // NewResultAcknowledgement returns a new instance of Acknowledgement using an Acknowledgement_Result
 // type in the Response field.
 func NewResultAcknowledgement(result []byte) Acknowledgement {
@@ -22,10 +29,14 @@ func NewResultAcknowledgement(result []byte) Acknowledgement {
 // 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 {
+func NewErrorAcknowledgement(err error) 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
+
 	return Acknowledgement{
 		Response: &Acknowledgement_Error{
-			Error: err,
+			Error: fmt.Sprintf("ABCI code: %d: %s", code, ackErrorString),
 		},
 	}
 }