ICS 20 Cleanup and Tests (#5577)

* Add comments, remove unused code and attempt to point to places where the spec is being implemented

* close channel when transfer fails

* rename packer data transfer to align to spec; refactor table tests

* ICS 20 implementation cleanup work (#5602)

* Simulation docs (#5033)

* simulation docs

* update docs with the latest simulation changes

* minor imporvments

* clean up of simulation.md

* expand section on weights

* minor reword

* minor wording fix

Co-authored-by: Marko <[email protected]>

* Merge PR #5597: Include Amount in Complete Unbonding/Redelegation Events

* Add bank alias for gaia

* Moar bank alias gaia

* Moar bank alias gaia

* Call `TimeoutExecuted`, add wrappers

* Remove unused `MsgRecvPacket`

Co-authored-by: Federico Kunze <[email protected]>
Co-authored-by: Marko <[email protected]>
Co-authored-by: Alexander Bezobchuk <[email protected]>
Co-authored-by: Jack Zampolin <[email protected]>

* Merge PR #5603: Remove acknowledgement interface in favour of []byte

* fixes and cleanup

* spec compliance

* refactor relay prefixes and tests

* Fix test compilation

* cleanup; add notes and additional test case

* Receive transfer test

* Apply suggestions from code review

Co-Authored-By: Bot from GolangCI <[email protected]>

* Fix autolinter application

* Add testcase with incorrect prefix

* golangcibot fixes

* delete extra comment

Co-authored-by: Federico Kunze <[email protected]>
Co-authored-by: Christopher Goes <[email protected]>
Co-authored-by: Marko <[email protected]>
Co-authored-by: Alexander Bezobchuk <[email protected]>
Co-authored-by: Bot from GolangCI <[email protected]>

types/coin.go

@@ -580,8 +580,8 @@ func (coins Coins) Sort() Coins {
 // Parsing
 
 var (
-	// Denominations can be 3 ~ 32 characters long.
-	reDnmString = `[a-z][a-z0-9/]{2,31}`
+	// Denominations can be 3 ~ 64 characters long.
+	reDnmString = `[a-z][a-z0-9/]{2,63}`
 	reAmt       = `[[:digit:]]+`
 	reDecAmt    = `[[:digit:]]*\.[[:digit:]]+`
 	reSpc       = `[[:space:]]*`

x/ibc/04-channel/alias.go

@@ -37,6 +37,7 @@ var (
 	ErrPacketTimeout             = types.ErrPacketTimeout
 	ErrInvalidChannel            = types.ErrInvalidChannel
 	ErrInvalidChannelState       = types.ErrInvalidChannelState
+	ErrAcknowledgementTooLong    = types.ErrAcknowledgementTooLong
 	NewMsgChannelOpenInit        = types.NewMsgChannelOpenInit
 	NewMsgChannelOpenTry         = types.NewMsgChannelOpenTry
 	NewMsgChannelOpenAck         = types.NewMsgChannelOpenAck

x/ibc/04-channel/exported/exported.go

@@ -46,6 +46,11 @@ type PacketDataI interface {
 	Type() string         // Type returns human readable identifier, implements sdk.Msg
 }
 
+// PacketAcknowledgementI defines the interface for IBC packet acknowledgements.
+type PacketAcknowledgementI interface {
+	GetBytes() []byte
+}
+
 // Order defines if a channel is ORDERED or UNORDERED
 type Order byte
 

x/ibc/04-channel/keeper/handshake.go

@@ -331,7 +331,7 @@ func (k Keeper) ChanCloseInit(
 
 	channel.State = exported.CLOSED
 	k.SetChannel(ctx, portID, channelID, channel)
-
+	k.Logger(ctx).Info("channel close initialized: portID (%s), channelID (%s)", portID, channelID)
 	return nil
 }
 

x/ibc/04-channel/keeper/packet.go

@@ -178,7 +178,7 @@ func (k Keeper) RecvPacket(
 func (k Keeper) PacketExecuted(
 	ctx sdk.Context,
 	packet exported.PacketI,
-	acknowledgement exported.PacketDataI,
+	acknowledgement exported.PacketAcknowledgementI,
 ) error {
 	channel, found := k.GetChannel(ctx, packet.GetDestPort(), packet.GetDestChannel())
 	if !found {
@@ -231,7 +231,7 @@ func (k Keeper) PacketExecuted(
 func (k Keeper) AcknowledgePacket(
 	ctx sdk.Context,
 	packet exported.PacketI,
-	acknowledgement []byte,
+	acknowledgement exported.PacketAcknowledgementI,
 	proof commitment.ProofI,
 	proofHeight uint64,
 ) (exported.PacketI, error) {
@@ -286,7 +286,7 @@ func (k Keeper) AcknowledgePacket(
 
 	if err := k.connectionKeeper.VerifyPacketAcknowledgement(
 		ctx, connectionEnd, proofHeight, proof, packet.GetDestPort(), packet.GetDestChannel(),
-		packet.GetSequence(), acknowledgement,
+		packet.GetSequence(), acknowledgement.GetBytes(),
 	); err != nil {
 		return nil, sdkerrors.Wrap(err, "invalid acknowledgement on counterparty chain")
 	}

x/ibc/04-channel/keeper/packet_test.go

@@ -6,6 +6,7 @@ import (
 	connectionexported "github.com/cosmos/cosmos-sdk/x/ibc/03-connection/exported"
 	"github.com/cosmos/cosmos-sdk/x/ibc/04-channel/exported"
 	"github.com/cosmos/cosmos-sdk/x/ibc/04-channel/types"
+	transfertypes "github.com/cosmos/cosmos-sdk/x/ibc/20-transfer/types"
 	ibctypes "github.com/cosmos/cosmos-sdk/x/ibc/types"
 )
 
@@ -212,7 +213,7 @@ func (suite *KeeperTestSuite) TestAcknowledgePacket() {
 	counterparty := types.NewCounterparty(testPort2, testChannel2)
 	var packet types.Packet
 
-	ack := []byte("ack")
+	ack := transfertypes.AckDataTransfer{}
 
 	testCases := []testCase{
 		{"success", func() {

x/ibc/04-channel/types/codec.go

@@ -22,6 +22,7 @@ func init() {
 func RegisterCodec(cdc *codec.Codec) {
 	cdc.RegisterInterface((*exported.PacketI)(nil), nil)
 	cdc.RegisterInterface((*exported.PacketDataI)(nil), nil)
+	cdc.RegisterInterface((*exported.PacketAcknowledgementI)(nil), nil)
 	cdc.RegisterConcrete(Channel{}, "ibc/channel/Channel", nil)
 	cdc.RegisterConcrete(Packet{}, "ibc/channel/Packet", nil)
 

x/ibc/04-channel/types/errors.go

@@ -18,4 +18,5 @@ var (
 	ErrInvalidPacket             = sdkerrors.Register(SubModuleName, 10, "invalid packet")
 	ErrPacketTimeout             = sdkerrors.Register(SubModuleName, 11, "packet timeout")
 	ErrTooManyConnectionHops     = sdkerrors.Register(SubModuleName, 12, "too many connection hops")
+	ErrAcknowledgementTooLong    = sdkerrors.Register(SubModuleName, 13, "acknowledgement too long")
 )

x/ibc/04-channel/types/msgs.go

@@ -446,7 +446,7 @@ func (msg MsgPacket) GetSigners() []sdk.AccAddress {
 
 var _ sdk.Msg = MsgTimeout{}
 
-// MsgTimeout receives timeouted packet
+// MsgTimeout receives timed-out packet
 type MsgTimeout struct {
 	Packet           `json:"packet" yaml:"packet"`
 	NextSequenceRecv uint64            `json:"next_sequence_recv" yaml:"next_sequence_recv"`
@@ -504,14 +504,14 @@ var _ sdk.Msg = MsgAcknowledgement{}
 // MsgAcknowledgement receives incoming IBC acknowledgement
 type MsgAcknowledgement struct {
 	Packet          `json:"packet" yaml:"packet"`
-	Acknowledgement exported.PacketDataI `json:"acknowledgement" yaml:"acknowledgement"`
-	Proof           commitment.ProofI    `json:"proof" yaml:"proof"`
-	ProofHeight     uint64               `json:"proof_height" yaml:"proof_height"`
-	Signer          sdk.AccAddress       `json:"signer" yaml:"signer"`
+	Acknowledgement exported.PacketAcknowledgementI `json:"acknowledgement" yaml:"acknowledgement"`
+	Proof           commitment.ProofI               `json:"proof" yaml:"proof"`
+	ProofHeight     uint64                          `json:"proof_height" yaml:"proof_height"`
+	Signer          sdk.AccAddress                  `json:"signer" yaml:"signer"`
 }
 
 // NewMsgAcknowledgement constructs a new MsgAcknowledgement
-func NewMsgAcknowledgement(packet Packet, ack exported.PacketDataI, proof commitment.ProofI, proofHeight uint64, signer sdk.AccAddress) MsgAcknowledgement {
+func NewMsgAcknowledgement(packet Packet, ack exported.PacketAcknowledgementI, proof commitment.ProofI, proofHeight uint64, signer sdk.AccAddress) MsgAcknowledgement {
 	return MsgAcknowledgement{
 		Packet:          packet,
 		Acknowledgement: ack,
@@ -534,12 +534,12 @@ func (msg MsgAcknowledgement) ValidateBasic() error {
 	if err := msg.Proof.ValidateBasic(); err != nil {
 		return sdkerrors.Wrap(err, "proof ack cannot be nil")
 	}
+	if len(msg.Acknowledgement.GetBytes()) > 100 {
+		return sdkerrors.Wrap(ErrAcknowledgementTooLong, "acknowledgement cannot exceed 100 bytes")
+	}
 	if msg.ProofHeight == 0 {
 		return sdkerrors.Wrap(ibctypes.ErrInvalidHeight, "proof height must be > 0")
 	}
-	if err := msg.Acknowledgement.ValidateBasic(); err != nil {
-		return err
-	}
 	if msg.Signer.Empty() {
 		return sdkerrors.ErrInvalidAddress
 	}

x/ibc/04-channel/types/msgs_test.go

@@ -392,10 +392,19 @@ func (invalidPacketT) Type() string {
 	return "invalid"
 }
 
+var _ exported.PacketAcknowledgementI = invalidAckT{}
+
+type invalidAckT struct{}
+
+func (invalidAckT) GetBytes() []byte {
+	return []byte("123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890")
+}
+
 // define variables used for testing
 var (
 	packet        = NewPacket(validPacketT{}, 1, portid, chanid, cpportid, cpchanid)
 	invalidPacket = NewPacket(invalidPacketT{}, 0, portid, chanid, cpportid, cpchanid)
+	invalidAck    = invalidAckT{}
 
 	emptyProof     = commitment.Proof{Proof: nil}
 	invalidProofs1 = commitment.ProofI(nil)
@@ -521,7 +530,7 @@ func (suite *MsgTestSuite) TestMsgAcknowledgement() {
 		NewMsgAcknowledgement(packet, packet.GetData(), proof, 1, emptyAddr),
 		NewMsgAcknowledgement(packet, packet.GetData(), emptyProof, 1, addr),
 		NewMsgAcknowledgement(invalidPacket, packet.GetData(), proof, 1, addr),
-		NewMsgAcknowledgement(packet, invalidPacket.GetData(), proof, 1, addr),
+		NewMsgAcknowledgement(packet, invalidAck, proof, 1, addr),
 		NewMsgAcknowledgement(packet, packet.GetData(), invalidProofs1, 1, addr),
 	}
 

x/ibc/04-channel/types/packet.go

@@ -22,7 +22,7 @@ func CommitPacket(data exported.PacketDataI) []byte {
 }
 
 // CommitAcknowledgement returns the hash of commitment bytes
-func CommitAcknowledgement(data exported.PacketDataI) []byte {
+func CommitAcknowledgement(data exported.PacketAcknowledgementI) []byte {
 	return tmhash.Sum(data.GetBytes())
 }
 

x/ibc/20-transfer/alias.go

@@ -14,7 +14,7 @@ import (
 const (
 	DefaultPacketTimeout = keeper.DefaultPacketTimeout
 	AttributeKeyReceiver = types.AttributeKeyReceiver
-	SubModuleName        = types.SubModuleName
+	ModuleName           = types.ModuleName
 	StoreKey             = types.StoreKey
 	RouterKey            = types.RouterKey
 	QuerierRoute         = types.QuerierRoute
@@ -35,13 +35,13 @@ var (
 )
 
 type (
-	Keeper             = keeper.Keeper
-	BankKeeper         = types.BankKeeper
-	ChannelKeeper      = types.ChannelKeeper
-	ClientKeeper       = types.ClientKeeper
-	ConnectionKeeper   = types.ConnectionKeeper
-	SupplyKeeper       = types.SupplyKeeper
-	MsgTransfer        = types.MsgTransfer
-	MsgRecvPacket      = types.MsgRecvPacket
-	PacketDataTransfer = types.PacketDataTransfer
+	Keeper                  = keeper.Keeper
+	BankKeeper              = types.BankKeeper
+	ChannelKeeper           = types.ChannelKeeper
+	ClientKeeper            = types.ClientKeeper
+	ConnectionKeeper        = types.ConnectionKeeper
+	SupplyKeeper            = types.SupplyKeeper
+	FungibleTokenPacketData = types.FungibleTokenPacketData
+	MsgTransfer             = types.MsgTransfer
+	AckDataTransfer         = types.AckDataTransfer
 )

x/ibc/20-transfer/client/cli/cli.go

@@ -30,7 +30,6 @@ func GetTxCmd(cdc *codec.Codec) *cobra.Command {
 
 	ics20TransferTxCmd.AddCommand(flags.PostCommands(
 		GetTransferTxCmd(cdc),
-		// GetMsgRecvPacketCmd(cdc),
 	)...)
 
 	return ics20TransferTxCmd

x/ibc/20-transfer/client/rest/swagger.go

@@ -12,16 +12,9 @@ type (
 	}
 
 	PostTransfer struct {
-		Msgs       []types.MsgTransfer `json:"msg" yaml:"msg"`
+		Msgs       []types.MsgTransfer      `json:"msg" yaml:"msg"`
 		Fee        authtypes.StdFee         `json:"fee" yaml:"fee"`
 		Signatures []authtypes.StdSignature `json:"signatures" yaml:"signatures"`
-		Memo       string              `json:"memo" yaml:"memo"`
-	}
-
-	PostRecvPacket struct {
-		Msgs       []types.MsgRecvPacket `json:"msg" yaml:"msg"`
-		Fee        authtypes.StdFee           `json:"fee" yaml:"fee"`
-		Signatures []authtypes.StdSignature   `json:"signatures" yaml:"signatures"`
-		Memo       string                `json:"memo" yaml:"memo"`
+		Memo       string                   `json:"memo" yaml:"memo"`
 	}
 )

x/ibc/20-transfer/handler.go

@@ -3,7 +3,6 @@ package transfer
 import (
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
-	"github.com/cosmos/cosmos-sdk/x/ibc/20-transfer/types"
 
 	channeltypes "github.com/cosmos/cosmos-sdk/x/ibc/04-channel/types"
 )
@@ -16,36 +15,38 @@ func NewHandler(k Keeper) sdk.Handler {
 			return handleMsgTransfer(ctx, k, msg)
 		case channeltypes.MsgPacket:
 			switch data := msg.Data.(type) {
-			case PacketDataTransfer: // i.e fulfills the Data interface
+			case FungibleTokenPacketData:
 				return handlePacketDataTransfer(ctx, k, msg, data)
 			default:
-				return nil, sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "unrecognized ics20 packet data type: %T", msg)
+				return nil, sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "unrecognized ICS-20 transfer packet data type: %T", data)
 			}
 		case channeltypes.MsgTimeout:
 			switch data := msg.Data.(type) {
-			case PacketDataTransfer:
+			case FungibleTokenPacketData:
 				return handleTimeoutDataTransfer(ctx, k, msg, data)
 			default:
-				return nil, sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "unrecognized ics20 packet data type: %T", data)
+				return nil, sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "unrecognized ICS-20 transfer packet data type: %T", data)
 			}
 		default:
-			return nil, sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "unrecognized ics20 message type: %T", msg)
+			return nil, sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "unrecognized ICS-20 transfer message type: %T", msg)
 		}
 	}
 }
 
+// See createOutgoingPacket in spec:https://github.com/cosmos/ics/tree/master/spec/ics-020-fungible-token-transfer#packet-relay
 func handleMsgTransfer(ctx sdk.Context, k Keeper, msg MsgTransfer) (*sdk.Result, error) {
-	err := k.SendTransfer(ctx, msg.SourcePort, msg.SourceChannel, msg.DestHeight, msg.Amount, msg.Sender, msg.Receiver, msg.Source)
-	if err != nil {
+	if err := k.SendTransfer(
+		ctx, msg.SourcePort, msg.SourceChannel, msg.DestHeight, msg.Amount, msg.Sender, msg.Receiver, msg.Source,
+	); err != nil {
 		return nil, err
 	}
 
 	ctx.EventManager().EmitEvent(
 		sdk.NewEvent(
 			sdk.EventTypeMessage,
-			sdk.NewAttribute(sdk.AttributeKeyModule, types.AttributeValueCategory),
+			sdk.NewAttribute(sdk.AttributeKeyModule, AttributeValueCategory),
 			sdk.NewAttribute(sdk.AttributeKeySender, msg.Sender.String()),
-			sdk.NewAttribute(types.AttributeKeyReceiver, msg.Receiver.String()),
+			sdk.NewAttribute(AttributeKeyReceiver, msg.Receiver.String()),
 		),
 	)
 
@@ -54,43 +55,57 @@ func handleMsgTransfer(ctx sdk.Context, k Keeper, msg MsgTransfer) (*sdk.Result,
 	}, nil
 }
 
-func handlePacketDataTransfer(ctx sdk.Context, k Keeper, msg channeltypes.MsgPacket, data types.PacketDataTransfer) (*sdk.Result, error) {
-	packet := msg.Packet
-	err := k.ReceiveTransfer(ctx, packet.SourcePort, packet.SourceChannel, packet.DestinationPort, packet.DestinationChannel, data)
-	if err != nil {
-		panic(err)
-		// TODO: Source chain sent invalid packet, shutdown channel
+// See onRecvPacket in spec: https://github.com/cosmos/ics/tree/master/spec/ics-020-fungible-token-transfer#packet-relay
+func handlePacketDataTransfer(
+	ctx sdk.Context, k Keeper, msg channeltypes.MsgPacket, data FungibleTokenPacketData,
+) (*sdk.Result, error) {
+	if err := k.ReceiveTransfer(ctx, msg.Packet, data); err != nil {
+		// NOTE (cwgoes): How do we want to handle this case? Maybe we should be more lenient,
+		// it's safe to leave the channel open I think.
+
+		// TODO: handle packet receipt that due to an error (specify)
+		// the receiving chain couldn't process the transfer
+
+		// source chain sent invalid packet, shutdown our channel end
+		if err := k.ChanCloseInit(ctx, msg.Packet.DestinationPort, msg.Packet.DestinationChannel); err != nil {
+			return nil, err
+		}
+		return nil, err
 	}
 
-	acknowledgement := types.AckDataTransfer{}
-	err = k.PacketExecuted(ctx, packet, acknowledgement)
-	if err != nil {
-		panic(err)
-		// TODO: This should not happen
+	acknowledgement := AckDataTransfer{}
+	if err := k.PacketExecuted(ctx, msg.Packet, acknowledgement); err != nil {
+		return nil, err
 	}
 
 	ctx.EventManager().EmitEvent(
 		sdk.NewEvent(
 			sdk.EventTypeMessage,
-			sdk.NewAttribute(sdk.AttributeKeyModule, types.AttributeValueCategory),
+			sdk.NewAttribute(sdk.AttributeKeyModule, AttributeValueCategory),
 			sdk.NewAttribute(sdk.AttributeKeySender, msg.Signer.String()),
 		),
 	)
 
-	// packet receiving should not fail
 	return &sdk.Result{
 		Events: ctx.EventManager().Events(),
 	}, nil
 }
 
-func handleTimeoutDataTransfer(ctx sdk.Context, k Keeper, msg channeltypes.MsgTimeout, data types.PacketDataTransfer) (*sdk.Result, error) {
-	packet := msg.Packet
-	err := k.TimeoutTransfer(ctx, packet.SourcePort, packet.SourceChannel, packet.DestinationPort, packet.DestinationChannel, data)
-	if err != nil {
-		// This chain sent invalid packet
+// See onTimeoutPacket in spec: https://github.com/cosmos/ics/tree/master/spec/ics-020-fungible-token-transfer#packet-relay
+func handleTimeoutDataTransfer(
+	ctx sdk.Context, k Keeper, msg channeltypes.MsgTimeout, data FungibleTokenPacketData,
+) (*sdk.Result, error) {
+	if err := k.TimeoutTransfer(ctx, msg.Packet, data); err != nil {
+		// This shouldn't happen, since we've already validated that we've sent the packet.
 		panic(err)
 	}
-	// packet timeout should not fail
+
+	if err := k.TimeoutExecuted(ctx, msg.Packet); err != nil {
+		// This shouldn't happen, since we've already validated that we've sent the packet.
+		// TODO: Figure out what happens if the capability authorisation changes.
+		panic(err)
+	}
+
 	return &sdk.Result{
 		Events: ctx.EventManager().Events(),
 	}, nil