diff --git a/CHANGELOG.md b/CHANGELOG.md index 4ed1bea6378..111714cd3ad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,8 @@ Ref: https://keepachangelog.com/en/1.0.0/ ## [v2.0.3](https://github.com/cosmos/ibc-go/releases/tag/v2.0.2) - 2022-02-03 +* (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 * (channel) [\#692](https://github.com/cosmos/ibc-go/pull/692) Minimize channel logging by only emitting the packet sequence, source port/channel, destination port/channel upon packet receives, acknowledgements and timeouts. 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..202404fff3a --- /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" +) + +// 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-deterministic codespace and log values + + errorString := fmt.Sprintf("ABCI code: %d: %s", code, ackErrorString) + + return channeltypes.NewErrorAcknowledgement(errorString) +} diff --git a/modules/apps/transfer/ibc_module.go b/modules/apps/transfer/ibc_module.go new file mode 100644 index 00000000000..26f1c533434 --- /dev/null +++ b/modules/apps/transfer/ibc_module.go @@ -0,0 +1,280 @@ +package transfer + +import ( + "fmt" + "math" + + sdk "github.com/cosmos/cosmos-sdk/types" + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" + capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/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" + porttypes "github.com/cosmos/ibc-go/v3/modules/core/05-port/types" + host "github.com/cosmos/ibc-go/v3/modules/core/24-host" + ibcexported "github.com/cosmos/ibc-go/v3/modules/core/exported" +) + +// IBCModule implements the ICS26 interface for transfer given the transfer keeper. +type IBCModule struct { + keeper keeper.Keeper +} + +// NewIBCModule creates a new IBCModule given the keeper +func NewIBCModule(k keeper.Keeper) IBCModule { + return IBCModule{ + keeper: k, + } +} + +// ValidateTransferChannelParams does validation of a newly created transfer channel. A transfer +// channel must be UNORDERED, use the correct port (by default 'transfer'), and use the current +// supported version. Only 2^32 channels are allowed to be created. +func ValidateTransferChannelParams( + ctx sdk.Context, + keeper keeper.Keeper, + order channeltypes.Order, + portID string, + channelID string, +) error { + // NOTE: for escrow address security only 2^32 channels are allowed to be created + // Issue: https://github.com/cosmos/cosmos-sdk/issues/7737 + channelSequence, err := channeltypes.ParseChannelSequence(channelID) + if err != nil { + return err + } + if channelSequence > uint64(math.MaxUint32) { + return sdkerrors.Wrapf(types.ErrMaxTransferChannels, "channel sequence %d is greater than max allowed transfer channels %d", channelSequence, uint64(math.MaxUint32)) + } + if order != channeltypes.UNORDERED { + return sdkerrors.Wrapf(channeltypes.ErrInvalidChannelOrdering, "expected %s channel, got %s ", channeltypes.UNORDERED, order) + } + + // Require portID is the portID transfer module is bound to + boundPort := keeper.GetPort(ctx) + if boundPort != portID { + return sdkerrors.Wrapf(porttypes.ErrInvalidPort, "invalid port: %s, expected %s", portID, boundPort) + } + + return nil +} + +// OnChanOpenInit implements the IBCModule interface +func (im IBCModule) OnChanOpenInit( + ctx sdk.Context, + order channeltypes.Order, + connectionHops []string, + portID string, + channelID string, + chanCap *capabilitytypes.Capability, + counterparty channeltypes.Counterparty, + version string, +) error { + if err := ValidateTransferChannelParams(ctx, im.keeper, order, portID, channelID); err != nil { + return err + } + + if version != types.Version { + return sdkerrors.Wrapf(types.ErrInvalidVersion, "got %s, expected %s", version, types.Version) + } + + // Claim channel capability passed back by IBC module + if err := im.keeper.ClaimCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)); err != nil { + return err + } + + return nil +} + +// OnChanOpenTry implements the IBCModule interface. +func (im IBCModule) OnChanOpenTry( + ctx sdk.Context, + order channeltypes.Order, + connectionHops []string, + portID, + channelID string, + chanCap *capabilitytypes.Capability, + counterparty channeltypes.Counterparty, + counterpartyVersion string, +) (string, error) { + if err := ValidateTransferChannelParams(ctx, im.keeper, order, portID, channelID); err != nil { + return "", err + } + + if counterpartyVersion != types.Version { + return "", sdkerrors.Wrapf(types.ErrInvalidVersion, "invalid counterparty version: got: %s, expected %s", counterpartyVersion, types.Version) + } + + // Module may have already claimed capability in OnChanOpenInit in the case of crossing hellos + // (ie chainA and chainB both call ChanOpenInit before one of them calls ChanOpenTry) + // If module can already authenticate the capability then module already owns it so we don't need to claim + // Otherwise, module does not have channel capability and we must claim it from IBC + if !im.keeper.AuthenticateCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)) { + // Only claim channel capability passed back by IBC module if we do not already own it + if err := im.keeper.ClaimCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)); err != nil { + return "", err + } + } + + return types.Version, nil +} + +// OnChanOpenAck implements the IBCModule interface +func (im IBCModule) OnChanOpenAck( + ctx sdk.Context, + portID, + channelID string, + counterpartyVersion string, +) error { + if counterpartyVersion != types.Version { + return sdkerrors.Wrapf(types.ErrInvalidVersion, "invalid counterparty version: %s, expected %s", counterpartyVersion, types.Version) + } + return nil +} + +// OnChanOpenConfirm implements the IBCModule interface +func (im IBCModule) OnChanOpenConfirm( + ctx sdk.Context, + portID, + channelID string, +) error { + return nil +} + +// OnChanCloseInit implements the IBCModule interface +func (im IBCModule) OnChanCloseInit( + ctx sdk.Context, + portID, + channelID string, +) error { + // Disallow user-initiated channel closing for transfer channels + return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "user cannot close channel") +} + +// OnChanCloseConfirm implements the IBCModule interface +func (im IBCModule) OnChanCloseConfirm( + ctx sdk.Context, + portID, + channelID string, +) error { + return nil +} + +// OnRecvPacket implements the IBCModule interface. A successful acknowledgement +// is returned if the packet data is succesfully decoded and the receive application +// logic returns without error. +func (im IBCModule) OnRecvPacket( + ctx sdk.Context, + packet channeltypes.Packet, + relayer sdk.AccAddress, +) ibcexported.Acknowledgement { + ack := channeltypes.NewResultAcknowledgement([]byte{byte(1)}) + + var data types.FungibleTokenPacketData + if err := types.ModuleCdc.UnmarshalJSON(packet.GetData(), &data); err != nil { + ack = channeltypes.NewErrorAcknowledgement("cannot unmarshal ICS-20 transfer packet data") + } + + // 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) + } + } + + ctx.EventManager().EmitEvent( + sdk.NewEvent( + types.EventTypePacket, + sdk.NewAttribute(sdk.AttributeKeyModule, types.ModuleName), + 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())), + ), + ) + + // NOTE: acknowledgement will be written synchronously during IBC handler execution. + return ack +} + +// OnAcknowledgementPacket implements the IBCModule interface +func (im IBCModule) OnAcknowledgementPacket( + ctx sdk.Context, + packet channeltypes.Packet, + acknowledgement []byte, + relayer sdk.AccAddress, +) error { + var ack channeltypes.Acknowledgement + if err := types.ModuleCdc.UnmarshalJSON(acknowledgement, &ack); err != nil { + return sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "cannot unmarshal ICS-20 transfer packet acknowledgement: %v", err) + } + var data types.FungibleTokenPacketData + if err := types.ModuleCdc.UnmarshalJSON(packet.GetData(), &data); err != nil { + return sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "cannot unmarshal ICS-20 transfer packet data: %s", err.Error()) + } + + if err := im.keeper.OnAcknowledgementPacket(ctx, packet, data, ack); err != nil { + return err + } + + ctx.EventManager().EmitEvent( + sdk.NewEvent( + types.EventTypePacket, + sdk.NewAttribute(sdk.AttributeKeyModule, types.ModuleName), + sdk.NewAttribute(types.AttributeKeyReceiver, data.Receiver), + sdk.NewAttribute(types.AttributeKeyDenom, data.Denom), + sdk.NewAttribute(types.AttributeKeyAmount, data.Amount), + sdk.NewAttribute(types.AttributeKeyAck, ack.String()), + ), + ) + + switch resp := ack.Response.(type) { + case *channeltypes.Acknowledgement_Result: + ctx.EventManager().EmitEvent( + sdk.NewEvent( + types.EventTypePacket, + sdk.NewAttribute(types.AttributeKeyAckSuccess, string(resp.Result)), + ), + ) + case *channeltypes.Acknowledgement_Error: + ctx.EventManager().EmitEvent( + sdk.NewEvent( + types.EventTypePacket, + sdk.NewAttribute(types.AttributeKeyAckError, resp.Error), + ), + ) + } + + return nil +} + +// OnTimeoutPacket implements the IBCModule interface +func (im IBCModule) OnTimeoutPacket( + ctx sdk.Context, + packet channeltypes.Packet, + relayer sdk.AccAddress, +) error { + var data types.FungibleTokenPacketData + if err := types.ModuleCdc.UnmarshalJSON(packet.GetData(), &data); err != nil { + return sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "cannot unmarshal ICS-20 transfer packet data: %s", err.Error()) + } + // refund tokens + if err := im.keeper.OnTimeoutPacket(ctx, packet, data); err != nil { + return err + } + + ctx.EventManager().EmitEvent( + sdk.NewEvent( + types.EventTypeTimeout, + sdk.NewAttribute(sdk.AttributeKeyModule, types.ModuleName), + sdk.NewAttribute(types.AttributeKeyRefundReceiver, data.Sender), + sdk.NewAttribute(types.AttributeKeyRefundDenom, data.Denom), + sdk.NewAttribute(types.AttributeKeyRefundAmount, data.Amount), + ), + ) + + return nil +} diff --git a/modules/apps/transfer/types/ack.go b/modules/apps/transfer/types/ack.go new file mode 100644 index 00000000000..6512f2e8371 --- /dev/null +++ b/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) +} diff --git a/modules/apps/transfer/types/ack_test.go b/modules/apps/transfer/types/ack_test.go new file mode 100644 index 00000000000..bc4e2d07afc --- /dev/null +++ b/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) + +} diff --git a/modules/core/04-channel/types/acknowledgement.go b/modules/core/04-channel/types/acknowledgement.go index cfc088ab0c9..b46de2b981d 100644 --- a/modules/core/04-channel/types/acknowledgement.go +++ b/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{