Skip to content

Commit

Permalink
chore: replace error string in transfer acks with const (#818)
Browse files Browse the repository at this point in the history
* fix: adding ack error string const for transfer

* updating godoc

* adding warning note to godoc in 04-channel

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

* adding changelog entry

(cherry picked from commit ac46ac0)

# Conflicts:
#	CHANGELOG.md
#	modules/apps/27-interchain-accounts/host/types/ack.go
#	modules/apps/transfer/ibc_module.go
  • Loading branch information
damiannolan authored and mergify-bot committed Feb 23, 2022
1 parent 32c935e commit ed751e4
Show file tree
Hide file tree
Showing 6 changed files with 474 additions and 0 deletions.
37 changes: 37 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,44 @@ Ref: https://keepachangelog.com/en/1.0.0/

# Changelog

<<<<<<< HEAD
## [v1.3.0](https://github.com/cosmos/ibc-go/releases/tag/v1.3.0) - 2022-03-01
=======
## [Unreleased]

### Dependencies

* [\#404](https://github.com/cosmos/ibc-go/pull/404) Bump Go version to 1.17
* (core) [\#709](https://github.com/cosmos/ibc-go/pull/709) Replace github.com/pkg/errors with stdlib errors

### API Breaking

* (testing( [\#813](https://github.com/cosmos/ibc-go/pull/813) The `ack` argument to the testing function `RelayPacket` has been removed as it is no longer needed.
* (testing) [\#774](https://github.com/cosmos/ibc-go/pull/774) Added `ChainID` arg to `SetupWithGenesisValSet` on the testing app. `Coordinator` generated ChainIDs now starts at index 1
* (transfer) [\#675](https://github.com/cosmos/ibc-go/pull/675) Transfer `NewKeeper` now takes in an ICS4Wrapper. The ICS4Wrapper may be the IBC Channel Keeper when ICS20 is not used in a middleware stack. The ICS4Wrapper is required for applications wishing to connect middleware to ICS20.
* (core) [\#650](https://github.com/cosmos/ibc-go/pull/650) Modify `OnChanOpenTry` IBC application module callback to return the negotiated app version. The version passed into the `MsgChanOpenTry` has been deprecated and will be ignored by core IBC.
* (core) [\#629](https://github.com/cosmos/ibc-go/pull/629) Removes the `GetProofSpecs` from the ClientState interface. This function was previously unused by core IBC.
* (transfer) [\#517](https://github.com/cosmos/ibc-go/pull/517) Separates the ICS 26 callback functions from `AppModule` into a new type `IBCModule` for ICS 20 transfer.
* (modules/core/02-client) [\#536](https://github.com/cosmos/ibc-go/pull/536) `GetSelfConsensusState` return type changed from bool to error.
* (channel) [\#644](https://github.com/cosmos/ibc-go/pull/644) Removes `CounterpartyHops` function from the ChannelKeeper.
* (testing) [\#776](https://github.com/cosmos/ibc-go/pull/776) Adding helper fn to generate capability name for testing callbacks


### State Machine Breaking

* (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

* (testing) [\#810](https://github.com/cosmos/ibc-go/pull/810) Additional testing function added to `Endpoint` type called `RecvPacketWithResult`. Performs the same functionality as the existing `RecvPacket` function but also returns the message result. `path.RelayPacket` no longer uses the provided acknowledgement argument and instead obtains the acknowledgement via MsgRecvPacket events.
* (connection) [\#721](https://github.com/cosmos/ibc-go/pull/721) Simplify connection handshake error messages when unpacking client state.
* (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.
* [\#383](https://github.com/cosmos/ibc-go/pull/383) Adds helper functions for merging and splitting middleware versions from the underlying app version.
* (modules/core/05-port) [\#288](https://github.com/cosmos/ibc-go/issues/288) Making the 05-port keeper function IsBound public. The IsBound function checks if the provided portID is already binded to a module.
* (channel) [\#644](https://github.com/cosmos/ibc-go/pull/644) Adds `GetChannelConnection` to the ChannelKeeper. This function returns the connectionID and connection state associated with a channel.
* (channel) [\647](https://github.com/cosmos/ibc-go/pull/647) Reorganizes channel handshake handling to set channel state after IBC application callbacks.
* (client) [\#724](https://github.com/cosmos/ibc-go/pull/724) `IsRevisionFormat` and `IsClientIDFormat` have been updated to disallow newlines before the dash used to separate the chainID and revision number, and the client type and client sequence.
>>>>>>> ac46ac0 (chore: replace error string in transfer acks with const (#818))
### Features

Expand Down
27 changes: 27 additions & 0 deletions modules/apps/27-interchain-accounts/host/types/ack.go
Original file line number Diff line number Diff line change
@@ -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)
}
280 changes: 280 additions & 0 deletions modules/apps/transfer/ibc_module.go
Original file line number Diff line number Diff line change
@@ -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
}
27 changes: 27 additions & 0 deletions modules/apps/transfer/types/ack.go
Original file line number Diff line number Diff line change
@@ -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)
}
Loading

0 comments on commit ed751e4

Please sign in to comment.