Consolidate usage of NewErrorAcknowledgement (#1565)

chatton · web-flow · commit b40dbc646c71 · 2022-06-28T14:57:57.000+01:00
diff --git a/CHANGELOG.md b/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 non-deterministic writes to application state.
 
 ### State Machine Breaking
 
diff --git a/docs/migrations/v3-to-v4.md b/docs/migrations/v3-to-v4.md
@@ -26,6 +26,10 @@ This is an API breaking change and as such IBC application developers will have
 The `OnChanOpenInit` application callback has been modified.
 The return signature now includes the application version as detailed in the latest IBC [spec changes](https://github.com/cosmos/ibc/pull/629).
 
+The `NewErrorAcknowledgement` method signature has changed.
+It now accepts an `error` rather than a `string`. This was done in order to prevent accidental state changes.
+All error acknowledgements now contain a deterministic ABCI code and error message. It is the responsibility of the application developer to emit error details in events.
+
 ### ICS27 - Interchain Accounts
 
 The `RegisterInterchainAccount` API has been modified to include an additional `version` argument. This change has been made in order to support ICS29 fee middleware, for relayer incentivization of ICS27 packets.
diff --git a/modules/apps/27-interchain-accounts/controller/ibc_middleware.go b/modules/apps/27-interchain-accounts/controller/ibc_middleware.go
@@ -138,7 +138,10 @@ func (im IBCMiddleware) OnRecvPacket(
 	packet channeltypes.Packet,
 	_ sdk.AccAddress,
 ) ibcexported.Acknowledgement {
-	return channeltypes.NewErrorAcknowledgement("cannot receive packet on controller chain")
+	err := sdkerrors.Wrapf(icatypes.ErrInvalidChannelFlow, "cannot receive packet on controller chain")
+	ack := channeltypes.NewErrorAcknowledgement(err)
+	keeper.EmitAcknowledgementEvent(ctx, packet, ack, err)
+	return ack
 }
 
 // OnAcknowledgementPacket implements the IBCMiddleware interface
diff --git a/modules/apps/27-interchain-accounts/controller/keeper/events.go b/modules/apps/27-interchain-accounts/controller/keeper/events.go
@@ -0,0 +1,31 @@
+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) {
+	attributes := []sdk.Attribute{
+		sdk.NewAttribute(sdk.AttributeKeyModule, icatypes.ModuleName),
+		sdk.NewAttribute(icatypes.AttributeKeyControllerChannelID, packet.GetDestChannel()),
+		sdk.NewAttribute(icatypes.AttributeKeyAckSuccess, fmt.Sprintf("%t", ack.Success())),
+	}
+
+	if err != nil {
+		attributes = append(attributes, sdk.NewAttribute(icatypes.AttributeKeyAckError, err.Error()))
+	}
+
+	ctx.EventManager().EmitEvent(
+		sdk.NewEvent(
+			icatypes.EventTypePacket,
+			attributes...,
+		),
+	)
+}
diff --git a/modules/apps/27-interchain-accounts/host/ibc_module.go b/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.
diff --git a/modules/apps/27-interchain-accounts/host/ibc_module_test.go b/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,
 		},
diff --git a/modules/apps/27-interchain-accounts/host/keeper/events.go b/modules/apps/27-interchain-accounts/host/keeper/events.go
@@ -10,18 +10,20 @@ import (
 // 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
+	attributes := []sdk.Attribute{
+		sdk.NewAttribute(sdk.AttributeKeyModule, icatypes.ModuleName),
+		sdk.NewAttribute(icatypes.AttributeKeyHostChannelID, packet.GetDestChannel()),
+		sdk.NewAttribute(icatypes.AttributeKeyAckSuccess, fmt.Sprintf("%t", ack.Success())),
+	}
+
 	if err != nil {
-		errorMsg = err.Error()
+		attributes = append(attributes, sdk.NewAttribute(icatypes.AttributeKeyAckError, 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())),
+			attributes...,
 		),
 	)
 }
diff --git a/modules/apps/27-interchain-accounts/host/types/ack.go b/modules/apps/27-interchain-accounts/host/types/ack.go
diff --git a/modules/apps/27-interchain-accounts/host/types/ack_test.go b/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)
-}
diff --git a/modules/apps/27-interchain-accounts/types/events.go b/modules/apps/27-interchain-accounts/types/events.go
@@ -4,7 +4,8 @@ package types
 const (
 	EventTypePacket = "ics27_packet"
 
-	AttributeKeyAckError      = "error"
-	AttributeKeyHostChannelID = "host_channel_id"
-	AttributeKeyAckSuccess    = "success"
+	AttributeKeyAckError            = "error"
+	AttributeKeyHostChannelID       = "host_channel_id"
+	AttributeKeyControllerChannelID = "controller_channel_id"
+	AttributeKeyAckSuccess          = "success"
 )
diff --git a/modules/apps/transfer/ibc_module.go b/modules/apps/transfer/ibc_module.go
@@ -9,6 +9,7 @@ import (
 	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
 	capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types"
 
+	icatypes "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/types"
 	"github.com/cosmos/ibc-go/v3/modules/apps/transfer/keeper"
 	"github.com/cosmos/ibc-go/v3/modules/apps/transfer/types"
 	channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types"
@@ -178,28 +179,38 @@ func (im IBCModule) OnRecvPacket(
 	ack := channeltypes.NewResultAcknowledgement([]byte{byte(1)})
 
 	var data types.FungibleTokenPacketData
+	var ackErr error
 	if err := types.ModuleCdc.UnmarshalJSON(packet.GetData(), &data); err != nil {
-		ack = channeltypes.NewErrorAcknowledgement("cannot unmarshal ICS-20 transfer packet data")
+		ackErr = sdkerrors.Wrapf(icatypes.ErrInvalidChannelFlow, "cannot unmarshal ICS-20 transfer packet data")
+		ack = channeltypes.NewErrorAcknowledgement(ackErr)
 	}
 
 	// 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)
+			ackErr = err
 		}
 	}
 
+	eventAttributes := []sdk.Attribute{
+		sdk.NewAttribute(sdk.AttributeKeySender, data.Sender),
+		sdk.NewAttribute(types.AttributeKeyReceiver, data.Receiver),
+		sdk.NewAttribute(types.AttributeKeyDenom, data.Denom),
+		sdk.NewAttribute(types.AttributeKeyAmount, data.Amount),
+		sdk.NewAttribute(types.AttributeKeyAckSuccess, fmt.Sprintf("%t", ack.Success())),
+	}
+
+	if ackErr != nil {
+		eventAttributes = append(eventAttributes, sdk.NewAttribute(types.AttributeKeyAckError, ackErr.Error()))
+	}
+
 	ctx.EventManager().EmitEvent(
 		sdk.NewEvent(
 			types.EventTypePacket,
-			sdk.NewAttribute(sdk.AttributeKeyModule, types.ModuleName),
-			sdk.NewAttribute(sdk.AttributeKeySender, data.Sender),
-			sdk.NewAttribute(types.AttributeKeyReceiver, data.Receiver),
-			sdk.NewAttribute(types.AttributeKeyDenom, data.Denom),
-			sdk.NewAttribute(types.AttributeKeyAmount, data.Amount),
-			sdk.NewAttribute(types.AttributeKeyAckSuccess, fmt.Sprintf("%t", ack.Success())),
+			eventAttributes...,
 		),
 	)
 
diff --git a/modules/apps/transfer/keeper/mbt_relay_test.go b/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)
 				}
diff --git a/modules/apps/transfer/keeper/relay_test.go b/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
diff --git a/modules/apps/transfer/types/ack.go b/modules/apps/transfer/types/ack.go
diff --git a/modules/apps/transfer/types/ack_test.go b/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)
-}
diff --git a/modules/core/04-channel/types/acknowledgement.go b/modules/core/04-channel/types/acknowledgement.go
diff --git a/modules/core/04-channel/types/acknowledgement_test.go b/modules/core/04-channel/types/acknowledgement_test.go
diff --git a/testing/mock/mock.go b/testing/mock/mock.go

Original file line number	Diff line number	Diff line change
`@@ -106,13 +106,13 @@ func (im IBCModule) OnRecvPacket(`
`106`	`106`	`_ sdk.AccAddress,`
`107`	`107`	`) ibcexported.Acknowledgement {`
`108`	`108`	`if !im.keeper.IsHostEnabled(ctx) {`
`109`		`- return types.NewErrorAcknowledgement(types.ErrHostSubModuleDisabled)`
	`109`	`+ return channeltypes.NewErrorAcknowledgement(types.ErrHostSubModuleDisabled)`
`110`	`110`	`}`
`111`	`111`
`112`	`112`	`txResponse, err := im.keeper.OnRecvPacket(ctx, packet)`
`113`	`113`	`ack := channeltypes.NewResultAcknowledgement(txResponse)`
`114`	`114`	`if err != nil {`
`115`		`- ack = types.NewErrorAcknowledgement(err)`
	`115`	`+ ack = channeltypes.NewErrorAcknowledgement(err)`
`116`	`116`	`}`
`117`	`117`
`118`	`118`	`// Emit an event indicating a successful or failed acknowledgement.`
Original file line number	Diff line number	Diff line change
`@@ -403,7 +403,7 @@ func (suite *InterchainAccountsTestSuite) TestOnRecvPacket() {`
`403`	`403`	`suite.chainB.GetSimApp().ICAAuthModule.IBCApp.OnRecvPacket = func(`
`404`	`404`	`ctx sdk.Context, packet channeltypes.Packet, relayer sdk.AccAddress,`
`405`	`405`	`) exported.Acknowledgement {`
`406`		`- return channeltypes.NewErrorAcknowledgement("failed OnRecvPacket mock callback")`
	`406`	`+ return channeltypes.NewErrorAcknowledgement(fmt.Errorf("failed OnRecvPacket mock callback"))`
`407`	`407`	`}`
`408`	`408`	`}, true,`
`409`	`409`	`},`