diff --git a/modules/apps/27-interchain-accounts/host/ibc_module.go b/modules/apps/27-interchain-accounts/host/ibc_module.go index 15606eb9e74..cec21d258c3 100644 --- a/modules/apps/27-interchain-accounts/host/ibc_module.go +++ b/modules/apps/27-interchain-accounts/host/ibc_module.go @@ -111,7 +111,7 @@ func (im IBCModule) OnRecvPacket( ack := channeltypes.NewResultAcknowledgement([]byte{byte(1)}) if err := im.keeper.OnRecvPacket(ctx, packet); err != nil { - ack = channeltypes.NewErrorAcknowledgement(icatypes.AcknowledgementError) + ack = types.NewErrorAcknowledgement(err) // Emit an event including the error msg keeper.EmitWriteErrorAcknowledgementEvent(ctx, packet, err) diff --git a/modules/apps/27-interchain-accounts/host/types/ack.go b/modules/apps/27-interchain-accounts/host/types/ack.go new file mode 100644 index 00000000000..047a7063e46 --- /dev/null +++ b/modules/apps/27-interchain-accounts/host/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 host chain: see events for details" +) + +// AcknowledgementErrorString 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 + + errorString := fmt.Sprintf("ABCI code: %d: %s", code, ackErrorString) + + return channeltypes.NewErrorAcknowledgement(errorString) +} diff --git a/modules/apps/27-interchain-accounts/host/types/ack_test.go b/modules/apps/27-interchain-accounts/host/types/ack_test.go new file mode 100644 index 00000000000..bc4e2d07afc --- /dev/null +++ b/modules/apps/27-interchain-accounts/host/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) + +} diff --git a/modules/apps/27-interchain-accounts/types/errors.go b/modules/apps/27-interchain-accounts/types/errors.go index 3f2cd77d488..e0a5c141de9 100644 --- a/modules/apps/27-interchain-accounts/types/errors.go +++ b/modules/apps/27-interchain-accounts/types/errors.go @@ -4,12 +4,6 @@ import ( sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" ) -const ( - // AcknowledgementError defines a string constant included in error acknowledgements - // NOTE: Changing this const is state machine breaking as acknowledgements are written into state - AcknowledgementError = "error handling packet on host chain: see events for details" -) - var ( ErrUnknownDataType = sdkerrors.Register(ModuleName, 2, "unknown data type") ErrAccountAlreadyExist = sdkerrors.Register(ModuleName, 3, "account already exist")