From d3ee967bddf2b49c59f295fbc4f5ead98599e4e6 Mon Sep 17 00:00:00 2001 From: Jack Zampolin Date: Mon, 27 Jan 2020 15:15:08 -0800 Subject: [PATCH 01/16] Add comments, remove unused code and attempt to point to places where the spec is being implemented --- x/ibc/04-channel/types/msgs.go | 2 +- x/ibc/20-transfer/alias.go | 1 - x/ibc/20-transfer/client/cli/cli.go | 1 - x/ibc/20-transfer/client/cli/tx.go | 70 --------- x/ibc/20-transfer/client/rest/swagger.go | 7 - x/ibc/20-transfer/genesis.go | 1 + x/ibc/20-transfer/handler.go | 4 + x/ibc/20-transfer/keeper/callbacks.go | 172 --------------------- x/ibc/20-transfer/keeper/callbacks_test.go | 51 ------ x/ibc/20-transfer/keeper/relay.go | 4 + x/ibc/20-transfer/module.go | 24 ++- x/ibc/20-transfer/types/msgs.go | 60 +------ x/ibc/20-transfer/types/msgs_test.go | 34 ++-- x/ibc/20-transfer/types/packet.go | 25 +-- 14 files changed, 65 insertions(+), 391 deletions(-) delete mode 100644 x/ibc/20-transfer/keeper/callbacks.go delete mode 100644 x/ibc/20-transfer/keeper/callbacks_test.go diff --git a/x/ibc/04-channel/types/msgs.go b/x/ibc/04-channel/types/msgs.go index c655cfda0f45..966b982b2ad2 100644 --- a/x/ibc/04-channel/types/msgs.go +++ b/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"` diff --git a/x/ibc/20-transfer/alias.go b/x/ibc/20-transfer/alias.go index d819b6ed2399..ad109cc00a38 100644 --- a/x/ibc/20-transfer/alias.go +++ b/x/ibc/20-transfer/alias.go @@ -42,6 +42,5 @@ type ( ConnectionKeeper = types.ConnectionKeeper SupplyKeeper = types.SupplyKeeper MsgTransfer = types.MsgTransfer - MsgRecvPacket = types.MsgRecvPacket PacketDataTransfer = types.PacketDataTransfer ) diff --git a/x/ibc/20-transfer/client/cli/cli.go b/x/ibc/20-transfer/client/cli/cli.go index 45a41239f974..03b26c5cb2b6 100644 --- a/x/ibc/20-transfer/client/cli/cli.go +++ b/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 diff --git a/x/ibc/20-transfer/client/cli/tx.go b/x/ibc/20-transfer/client/cli/tx.go index b1845266b480..9d708ffca3ed 100644 --- a/x/ibc/20-transfer/client/cli/tx.go +++ b/x/ibc/20-transfer/client/cli/tx.go @@ -65,73 +65,3 @@ func GetTransferTxCmd(cdc *codec.Codec) *cobra.Command { cmd.Flags().Bool(FlagSource, false, "Pass flag for sending token from the source chain") return cmd } - -// // GetMsgRecvPacketCmd returns the command to create a MsgRecvTransferPacket transaction -// func GetMsgRecvPacketCmd(cdc *codec.Codec) *cobra.Command { -// cmd := &cobra.Command{ -// Use: "recv-packet [sending-port-id] [sending-channel-id] [client-id]", -// Short: "Creates and sends a SendPacket message", -// Args: cobra.ExactArgs(3), -// RunE: func(cmd *cobra.Command, args []string) error { -// inBuf := bufio.NewReader(cmd.InOrStdin()) -// txBldr := auth.NewTxBuilderFromCLI(inBuf).WithTxEncoder(authclient.GetTxEncoder(cdc)) -// cliCtx := context.NewCLIContextWithInput(inBuf).WithCodec(cdc).WithBroadcastMode(flags.BroadcastBlock) - -// prove := viper.GetBool(flags.FlagProve) -// node2 := viper.GetString(FlagNode2) -// cid1 := viper.GetString(flags.FlagChainID) -// cid2 := viper.GetString(FlagChainID2) -// cliCtx2 := context.NewCLIContextIBC(cliCtx.GetFromAddress().String(), cid2, node2). -// WithCodec(cdc). -// WithBroadcastMode(flags.BroadcastBlock) - -// header, _, err := clientutils.QueryTendermintHeader(cliCtx2) -// if err != nil { -// return err -// } - -// sourcePort, sourceChannel, clientid := args[0], args[1], args[2] - -// passphrase, err := keys.GetPassphrase(viper.GetString(flags.FlagFrom)) -// if err != nil { -// return nil -// } - -// viper.Set(flags.FlagChainID, cid1) -// msgUpdateClient := clienttypes.NewMsgUpdateClient(clientid, header, cliCtx.GetFromAddress()) -// if err := msgUpdateClient.ValidateBasic(); err != nil { -// return err -// } - -// res, err := utils.CompleteAndBroadcastTx(txBldr, cliCtx, []sdk.Msg{msgUpdateClient}, passphrase) -// if err != nil || !res.IsOK() { -// return err -// } - -// viper.Set(flags.FlagChainID, cid2) -// sequence := uint64(viper.GetInt(FlagSequence)) -// packetRes, err := channelutils.QueryPacket(cliCtx2.WithHeight(header.Height-1), sourcePort, sourceChannel, sequence, uint64(viper.GetInt(FlagTimeout)), prove) -// if err != nil { -// return err -// } - -// viper.Set(flags.FlagChainID, cid1) - -// msg := types.NewMsgRecvPacket(packetRes.Packet, []commitment.Proof{packetRes.Proof}, packetRes.ProofHeight, cliCtx.GetFromAddress()) -// if err := msg.ValidateBasic(); err != nil { -// return err -// } - -// return authclient.GenerateOrBroadcastMsgs(cliCtx, txBldr, []sdk.Msg{msg}) -// }, -// } - -// cmd = client.PostCommands(cmd)[0] -// cmd.Flags().Bool(FlagSource, false, "Pass flag for sending token from the source chain") -// cmd.Flags().String(FlagNode2, "tcp://localhost:26657", "RPC port for the second chain") -// cmd.Flags().String(FlagChainID2, "", "chain-id for the second chain") -// cmd.Flags().String(FlagSequence, "", "sequence for the packet") -// cmd.Flags().String(FlagTimeout, "", "timeout for the packet") -// cmd.Flags().Bool(flags.FlagProve, true, "show proofs for the query results") -// return cmd -// } diff --git a/x/ibc/20-transfer/client/rest/swagger.go b/x/ibc/20-transfer/client/rest/swagger.go index c36f5d9e9822..fba0ccb3409e 100644 --- a/x/ibc/20-transfer/client/rest/swagger.go +++ b/x/ibc/20-transfer/client/rest/swagger.go @@ -17,11 +17,4 @@ type ( Signatures []auth.StdSignature `json:"signatures" yaml:"signatures"` Memo string `json:"memo" yaml:"memo"` } - - PostRecvPacket struct { - Msgs []types.MsgRecvPacket `json:"msg" yaml:"msg"` - Fee auth.StdFee `json:"fee" yaml:"fee"` - Signatures []auth.StdSignature `json:"signatures" yaml:"signatures"` - Memo string `json:"memo" yaml:"memo"` - } ) diff --git a/x/ibc/20-transfer/genesis.go b/x/ibc/20-transfer/genesis.go index facde722a27d..974be35f2c7a 100644 --- a/x/ibc/20-transfer/genesis.go +++ b/x/ibc/20-transfer/genesis.go @@ -10,6 +10,7 @@ import ( // InitGenesis sets distribution information for genesis func InitGenesis(ctx sdk.Context, keeper Keeper) { // check if the module account exists + // TODO: should we create the module account if it doesn't exist? moduleAcc := keeper.GetTransferAccount(ctx) if moduleAcc == nil { panic(fmt.Sprintf("%s module account has not been set", types.GetModuleAccountName())) diff --git a/x/ibc/20-transfer/handler.go b/x/ibc/20-transfer/handler.go index 877d20f01984..dcc9447c70f5 100644 --- a/x/ibc/20-transfer/handler.go +++ b/x/ibc/20-transfer/handler.go @@ -9,6 +9,7 @@ import ( ) // NewHandler returns sdk.Handler for IBC token transfer module messages +// See NewHandler function in ADR15: https://github.com/cosmos/ics/tree/master/spec/ics-020-fungible-token-transfer#packet-relay func NewHandler(k Keeper) sdk.Handler { return func(ctx sdk.Context, msg sdk.Msg) (*sdk.Result, error) { switch msg := msg.(type) { @@ -34,6 +35,7 @@ func NewHandler(k Keeper) sdk.Handler { } } +// 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.Amount, msg.Sender, msg.Receiver, msg.Source) if err != nil { @@ -54,6 +56,7 @@ func handleMsgTransfer(ctx sdk.Context, k Keeper, msg MsgTransfer) (*sdk.Result, }, nil } +// 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 types.PacketDataTransfer) (*sdk.Result, error) { packet := msg.Packet err := k.ReceiveTransfer(ctx, packet.SourcePort, packet.SourceChannel, packet.DestinationPort, packet.DestinationChannel, data) @@ -83,6 +86,7 @@ func handlePacketDataTransfer(ctx sdk.Context, k Keeper, msg channeltypes.MsgPac }, nil } +// 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 types.PacketDataTransfer) (*sdk.Result, error) { packet := msg.Packet err := k.TimeoutTransfer(ctx, packet.SourcePort, packet.SourceChannel, packet.DestinationPort, packet.DestinationChannel, data) diff --git a/x/ibc/20-transfer/keeper/callbacks.go b/x/ibc/20-transfer/keeper/callbacks.go deleted file mode 100644 index 3e478a16c6c7..000000000000 --- a/x/ibc/20-transfer/keeper/callbacks.go +++ /dev/null @@ -1,172 +0,0 @@ -package keeper - -// NOTE: -// OnChanOpenInit, OnChanOpenTry, OnChanOpenAck, OnChanOpenConfirm, OnChanCLoseConfirm -// will be implemented according to ADR15 in the future PRs. Code left for reference. -// -// OnRecvPacket, OnAcknowledgementPacket, OnTimeoutPacket has been implemented according -// to ADR15. - -/* -import ( - "strings" - - sdk "github.com/cosmos/cosmos-sdk/types" - sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" - channel "github.com/cosmos/cosmos-sdk/x/ibc/04-channel" - port "github.com/cosmos/cosmos-sdk/x/ibc/05-port" - "github.com/cosmos/cosmos-sdk/x/ibc/20-transfer/types" - ibctypes "github.com/cosmos/cosmos-sdk/x/ibc/types" -) - -// nolint: unused -func (k Keeper) OnChanOpenInit( - ctx sdk.Context, - order channel.Order, - connectionHops []string, - portID, - channelID string, - counterparty channel.Counterparty, - version string, -) error { - if order != channel.UNORDERED { - return sdkerrors.Wrap(channel.ErrInvalidChannel, "channel must be UNORDERED") - } - - // NOTE: here the capability key name defines the port ID of the counterparty - if counterparty.PortID != k.boundedCapability.Name() { - return sdkerrors.Wrapf( - port.ErrInvalidPort, - "counterparty port ID doesn't match the capability key (%s ≠ %s)", counterparty.PortID, k.boundedCapability.Name(), - ) - } - - if strings.TrimSpace(version) != "" { - return sdkerrors.Wrap(ibctypes.ErrInvalidVersion, "version must be blank") - } - - // NOTE: as the escrow address is generated from both the port and channel IDs - // there's no need to store it on a map. - return nil -} - -// nolint: unused -func (k Keeper) OnChanOpenTry( - ctx sdk.Context, - order channel.Order, - connectionHops []string, - portID, - channelID string, - counterparty channel.Counterparty, - version string, - counterpartyVersion string, -) error { - if order != channel.UNORDERED { - return sdkerrors.Wrap(channel.ErrInvalidChannel, "channel must be UNORDERED") - } - - // NOTE: here the capability key name defines the port ID of the counterparty - if counterparty.PortID != k.boundedCapability.Name() { - return sdkerrors.Wrapf( - port.ErrInvalidPort, - "counterparty port ID doesn't match the capability key (%s ≠ %s)", counterparty.PortID, k.boundedCapability.Name(), - ) - } - - if strings.TrimSpace(version) != "" { - return sdkerrors.Wrap(ibctypes.ErrInvalidVersion, "version must be blank") - } - - if strings.TrimSpace(counterpartyVersion) != "" { - return sdkerrors.Wrap(ibctypes.ErrInvalidVersion, "counterparty version must be blank") - } - - // NOTE: as the escrow address is generated from both the port and channel IDs - // there's no need to store it on a map. - return nil -} - -// nolint: unused -func (k Keeper) OnChanOpenAck( - ctx sdk.Context, - portID, - channelID string, - version string, -) error { - if strings.TrimSpace(version) != "" { - return sdkerrors.Wrap(ibctypes.ErrInvalidVersion, "version must be blank") - } - - return nil -} - -// nolint: unused -func (k Keeper) OnChanOpenConfirm( - ctx sdk.Context, - portID, - channelID string, -) error { - // no-op - return nil -} - -// nolint: unused -func (k Keeper) OnChanCloseInit( - ctx sdk.Context, - portID, - channelID string, -) error { - // no-op - return nil -} - -// nolint: unused -func (k Keeper) OnChanCloseConfirm( - ctx sdk.Context, - portID, - channelID string, -) error { - // no-op - return nil -} - -// nolint: unused -func (k Keeper) OnTimeoutPacket( - ctx sdk.Context, - packet channelexported.PacketI, -) error { - var data types.PacketData - - err := k.cdc.UnmarshalBinaryBare(packet.GetData(), &data) - if err != nil { - return sdkerrors.Wrap(err, "invalid packet data") - } - - // check the denom prefix - prefix := types.GetDenomPrefix(packet.GetSourcePort(), packet.GetSourceChannel()) - coins := make(sdk.Coins, len(data.Amount)) - for i, coin := range data.Amount { - coin := coin - if !strings.HasPrefix(coin.Denom, prefix) { - return sdkerrors.Wrapf( - sdkerrors.ErrInvalidCoins, - "%s doesn't contain the prefix '%s'", coin.Denom, prefix, - ) - } - coins[i] = sdk.NewCoin(coin.Denom[len(prefix):], coin.Amount) - } - - if data.Source { - escrowAddress := types.GetEscrowAddress(packet.GetDestPort(), packet.GetDestChannel()) - return k.bankKeeper.SendCoins(ctx, escrowAddress, data.Sender, coins) - } - - // mint from supply - err = k.supplyKeeper.MintCoins(ctx, types.GetModuleAccountName(), data.Amount) - if err != nil { - return err - } - - return k.supplyKeeper.SendCoinsFromModuleToAccount(ctx, types.GetModuleAccountName(), data.Sender, data.Amount) -} -*/ diff --git a/x/ibc/20-transfer/keeper/callbacks_test.go b/x/ibc/20-transfer/keeper/callbacks_test.go deleted file mode 100644 index 171cb99e7306..000000000000 --- a/x/ibc/20-transfer/keeper/callbacks_test.go +++ /dev/null @@ -1,51 +0,0 @@ -package keeper_test - -// TODO: move to 04-channel after CheckOpen implementation in the following PR -/* -func (suite *KeeperTestSuite) TestOnChanOpenInit() { - invalidOrder := channel.ORDERED - - counterparty := channel.NewCounterparty(testPort2, testChannel2) - err := suite.app.IBCKeeper.TransferKeeper.OnChanOpenInit(suite.ctx, invalidOrder, []string{testConnection}, testPort1, testChannel1, counterparty, "") - suite.Error(err) // invalid channel order - - err = suite.app.IBCKeeper.TransferKeeper.OnChanOpenInit(suite.ctx, testChannelOrder, []string{testConnection}, testPort1, testChannel1, counterparty, "") - suite.Error(err) // invalid counterparty port ID - - counterparty = channel.NewCounterparty(testPort1, testChannel2) - err = suite.app.IBCKeeper.TransferKeeper.OnChanOpenInit(suite.ctx, testChannelOrder, []string{testConnection}, testPort1, testChannel1, counterparty, testChannelVersion) - suite.Error(err) // invalid version - - err = suite.app.IBCKeeper.TransferKeeper.OnChanOpenInit(suite.ctx, testChannelOrder, []string{testConnection}, testPort1, testChannel1, counterparty, "") - suite.NoError(err) // successfully executed -} - -func (suite *KeeperTestSuite) TestOnChanOpenTry() { - invalidOrder := channel.ORDERED - - counterparty := channel.NewCounterparty(testPort2, testChannel2) - err := suite.app.IBCKeeper.TransferKeeper.OnChanOpenTry(suite.ctx, invalidOrder, []string{testConnection}, testPort1, testChannel1, counterparty, "", "") - suite.Error(err) // invalid channel order - - err = suite.app.IBCKeeper.TransferKeeper.OnChanOpenTry(suite.ctx, testChannelOrder, []string{testConnection}, testPort1, testChannel1, counterparty, "", "") - suite.Error(err) // invalid counterparty port ID - - counterparty = channel.NewCounterparty(testPort1, testChannel2) - err = suite.app.IBCKeeper.TransferKeeper.OnChanOpenTry(suite.ctx, testChannelOrder, []string{testConnection}, testPort1, testChannel1, counterparty, testChannelVersion, "") - suite.Error(err) // invalid version - - err = suite.app.IBCKeeper.TransferKeeper.OnChanOpenTry(suite.ctx, testChannelOrder, []string{testConnection}, testPort1, testChannel1, counterparty, "", testChannelVersion) - suite.Error(err) // invalid counterparty version - - err = suite.app.IBCKeeper.TransferKeeper.OnChanOpenTry(suite.ctx, testChannelOrder, []string{testConnection}, testPort1, testChannel1, counterparty, "", "") - suite.NoError(err) // successfully executed -} - -func (suite *KeeperTestSuite) TestOnChanOpenAck() { - err := suite.app.IBCKeeper.TransferKeeper.OnChanOpenAck(suite.ctx, testPort1, testChannel1, testChannelVersion) - suite.Error(err) // invalid version - - err = suite.app.IBCKeeper.TransferKeeper.OnChanOpenAck(suite.ctx, testPort1, testChannel1, "") - suite.NoError(err) // successfully executed -} -*/ diff --git a/x/ibc/20-transfer/keeper/relay.go b/x/ibc/20-transfer/keeper/relay.go index 8bfa6efcfb97..ea90cd7c340d 100644 --- a/x/ibc/20-transfer/keeper/relay.go +++ b/x/ibc/20-transfer/keeper/relay.go @@ -133,6 +133,7 @@ func (k Keeper) TimeoutTransfer( return k.supplyKeeper.SendCoinsFromModuleToAccount(ctx, types.GetModuleAccountName(), data.Sender, data.Amount) } +// See spec for this function: https://github.com/cosmos/ics/tree/master/spec/ics-020-fungible-token-transfer#packet-relay func (k Keeper) createOutgoingPacket( ctx sdk.Context, seq uint64, @@ -149,6 +150,7 @@ func (k Keeper) createOutgoingPacket( // escrow tokens if the destination chain is the same as the sender's escrowAddress := types.GetEscrowAddress(sourcePort, sourceChannel) + // construct receiving denominations, check correctness prefix := types.GetDenomPrefix(destinationPort, destinationChannel) coins := make(sdk.Coins, len(amount)) for i, coin := range amount { @@ -161,6 +163,7 @@ func (k Keeper) createOutgoingPacket( coins[i] = sdk.NewCoin(coin.Denom[len(prefix):], coin.Amount) } + // escrow source tokens (assumed to fail if balance insufficient) err := k.bankKeeper.SendCoins(ctx, sender, escrowAddress, coins) if err != nil { return err @@ -168,6 +171,7 @@ func (k Keeper) createOutgoingPacket( } else { // burn vouchers from the sender's balance if the source is from another chain + // construct receiving denomination, check correctness prefix := types.GetDenomPrefix(sourcePort, sourceChannel) for _, coin := range amount { if !strings.HasPrefix(coin.Denom, prefix) { diff --git a/x/ibc/20-transfer/module.go b/x/ibc/20-transfer/module.go index 914f4d148c71..2a4e593f61c3 100644 --- a/x/ibc/20-transfer/module.go +++ b/x/ibc/20-transfer/module.go @@ -18,6 +18,7 @@ import ( ) const ( + // ModuleName declares the name of the module ModuleName = types.SubModuleName ) @@ -26,84 +27,103 @@ var ( _ module.AppModuleBasic = AppModuleBasic{} ) +// AppModuleBasic is the 20-transfer appmodulebasic type AppModuleBasic struct{} -// Name returns the IBC transfer ICS name +// Name implements AppModuleBasic interface func (AppModuleBasic) Name() string { - return SubModuleName + return ModuleName } +// RegisterCodec implements AppModuleBasic interface func (AppModuleBasic) RegisterCodec(cdc *codec.Codec) { RegisterCodec(cdc) } +// DefaultGenesis implements AppModuleBasic interface func (AppModuleBasic) DefaultGenesis() json.RawMessage { return nil } +// ValidateGenesis implements AppModuleBasic interface func (AppModuleBasic) ValidateGenesis(bz json.RawMessage) error { return nil } +// RegisterRESTRoutes implements AppModuleBasic interface func (AppModuleBasic) RegisterRESTRoutes(ctx context.CLIContext, rtr *mux.Router) { rest.RegisterRoutes(ctx, rtr) } +// GetTxCmd implements AppModuleBasic interface func (AppModuleBasic) GetTxCmd(cdc *codec.Codec) *cobra.Command { return cli.GetTxCmd(cdc) } +// GetQueryCmd implements AppModuleBasic interface func (AppModuleBasic) GetQueryCmd(cdc *codec.Codec) *cobra.Command { return cli.GetQueryCmd(cdc, QuerierRoute) } +// AppModule represents the AppModule for this module type AppModule struct { AppModuleBasic keeper Keeper } +// NewAppModule creates a new 20-transfer module func NewAppModule(k Keeper) AppModule { return AppModule{ keeper: k, } } +// RegisterInvariants implements the AppModule interface func (AppModule) RegisterInvariants(ir sdk.InvariantRegistry) { // TODO } +// Route implements the AppModule interface func (AppModule) Route() string { return RouterKey } +// NewHandler implements the AppModule interface func (am AppModule) NewHandler() sdk.Handler { return NewHandler(am.keeper) } +// QuerierRoute implements the AppModule interface func (AppModule) QuerierRoute() string { return QuerierRoute } +// NewQuerierHandler implements the AppModule interface func (am AppModule) NewQuerierHandler() sdk.Querier { return nil } // InitGenesis performs genesis initialization for the staking module. It returns // no validator updates. +// InitGenesis implements the AppModule interface func (am AppModule) InitGenesis(ctx sdk.Context, data json.RawMessage) []abci.ValidatorUpdate { // check if the IBC transfer module account is set + // TODO: Should we automatically set the account if it is not set? InitGenesis(ctx, am.keeper) return []abci.ValidatorUpdate{} } +// ExportGenesis implements the AppModule interface func (am AppModule) ExportGenesis(ctx sdk.Context) json.RawMessage { return nil } +// BeginBlock implements the AppModule interface func (am AppModule) BeginBlock(ctx sdk.Context, req abci.RequestBeginBlock) { } +// EndBlock implements the AppModule interface func (am AppModule) EndBlock(ctx sdk.Context, req abci.RequestEndBlock) []abci.ValidatorUpdate { return []abci.ValidatorUpdate{} } diff --git a/x/ibc/20-transfer/types/msgs.go b/x/ibc/20-transfer/types/msgs.go index 42a56e0f30a6..ad3732fec518 100644 --- a/x/ibc/20-transfer/types/msgs.go +++ b/x/ibc/20-transfer/types/msgs.go @@ -3,12 +3,12 @@ package types import ( sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" - channelexported "github.com/cosmos/cosmos-sdk/x/ibc/04-channel/exported" - commitment "github.com/cosmos/cosmos-sdk/x/ibc/23-commitment" host "github.com/cosmos/cosmos-sdk/x/ibc/24-host" ibctypes "github.com/cosmos/cosmos-sdk/x/ibc/types" ) +// MsgTransfer is for transfering packets between ICS20 enabled chains +// See ICS Spec here: https://github.com/cosmos/ics/tree/master/spec/ics-020-fungible-token-transfer#data-structures type MsgTransfer struct { SourcePort string `json:"source_port" yaml:"source_port"` // the port on which the packet will be sent SourceChannel string `json:"source_channel" yaml:"source_channel"` // the channel by which the packet will be sent @@ -74,59 +74,3 @@ func (msg MsgTransfer) GetSignBytes() []byte { func (msg MsgTransfer) GetSigners() []sdk.AccAddress { return []sdk.AccAddress{msg.Sender} } - -type MsgRecvPacket struct { - Packet channelexported.PacketI `json:"packet" yaml:"packet"` - Proofs []commitment.Proof `json:"proofs" yaml:"proofs"` - Height uint64 `json:"height" yaml:"height"` - Signer sdk.AccAddress `json:"signer" yaml:"signer"` -} - -// NewMsgRecvPacket creates a new MsgRecvPacket instance -func NewMsgRecvPacket(packet channelexported.PacketI, proofs []commitment.Proof, height uint64, signer sdk.AccAddress) MsgRecvPacket { - return MsgRecvPacket{ - Packet: packet, - Proofs: proofs, - Height: height, - Signer: signer, - } -} - -// Route implements sdk.Msg -func (MsgRecvPacket) Route() string { - return ibctypes.RouterKey -} - -// Type implements sdk.Msg -func (MsgRecvPacket) Type() string { - return "recv_packet" -} - -// ValidateBasic implements sdk.Msg -func (msg MsgRecvPacket) ValidateBasic() error { - if msg.Height == 0 { - return sdkerrors.Wrap(ibctypes.ErrInvalidHeight, "height must be > 0") - } - if msg.Proofs == nil || len(msg.Proofs) == 0 { - return sdkerrors.Wrap(commitment.ErrInvalidProof, "missing proof") - } - for _, proof := range msg.Proofs { - if err := proof.ValidateBasic(); err != nil { - return err - } - } - if msg.Signer.Empty() { - return sdkerrors.Wrap(sdkerrors.ErrInvalidAddress, "missing sender address") - } - return msg.Packet.ValidateBasic() -} - -// GetSignBytes implements sdk.Msg -func (msg MsgRecvPacket) GetSignBytes() []byte { - return sdk.MustSortJSON(ModuleCdc.MustMarshalJSON(msg)) -} - -// GetSigners implements sdk.Msg -func (msg MsgRecvPacket) GetSigners() []sdk.AccAddress { - return []sdk.AccAddress{msg.Signer} -} diff --git a/x/ibc/20-transfer/types/msgs_test.go b/x/ibc/20-transfer/types/msgs_test.go index 42e55d32677c..b2c867bdfb92 100644 --- a/x/ibc/20-transfer/types/msgs_test.go +++ b/x/ibc/20-transfer/types/msgs_test.go @@ -12,10 +12,12 @@ import ( // define constants used for testing const ( + validPort = "testportid" invalidPort = "invalidport1" invalidShortPort = "p" invalidLongPort = "invalidlongportinvalidlongport" + validChannel = "testchannel" invalidChannel = "invalidchannel1" invalidShortChannel = "invalidch" invalidLongChannel = "invalidlongchannelinvalidlongchannel" @@ -33,14 +35,14 @@ var ( // TestMsgTransferRoute tests Route for MsgTransfer func TestMsgTransferRoute(t *testing.T) { - msg := NewMsgTransfer("testportid", "testchannel", coins, addr1, addr2, true) + msg := NewMsgTransfer(validPort, validChannel, coins, addr1, addr2, true) require.Equal(t, ibctypes.RouterKey, msg.Route()) } // TestMsgTransferType tests Type for MsgTransfer func TestMsgTransferType(t *testing.T) { - msg := NewMsgTransfer("testportid", "testchannel", coins, addr1, addr2, true) + msg := NewMsgTransfer(validPort, validChannel, coins, addr1, addr2, true) require.Equal(t, "transfer", msg.Type()) } @@ -48,18 +50,18 @@ func TestMsgTransferType(t *testing.T) { // TestMsgTransferValidation tests ValidateBasic for MsgTransfer func TestMsgTransferValidation(t *testing.T) { testMsgs := []MsgTransfer{ - NewMsgTransfer("testportid", "testchannel", coins, addr1, addr2, true), // valid msg - NewMsgTransfer(invalidShortPort, "testchannel", coins, addr1, addr2, true), // too short port id - NewMsgTransfer(invalidLongPort, "testchannel", coins, addr1, addr2, true), // too long port id - NewMsgTransfer(invalidPort, "testchannel", coins, addr1, addr2, true), // port id contains non-alpha - NewMsgTransfer("testportid", invalidShortChannel, coins, addr1, addr2, true), // too short channel id - NewMsgTransfer("testportid", invalidLongChannel, coins, addr1, addr2, false), // too long channel id - NewMsgTransfer("testportid", invalidChannel, coins, addr1, addr2, false), // channel id contains non-alpha - NewMsgTransfer("testportid", "testchannel", invalidDenomCoins, addr1, addr2, false), // invalid amount - NewMsgTransfer("testportid", "testchannel", negativeCoins, addr1, addr2, false), // amount contains negative coin - NewMsgTransfer("testportid", "testchannel", coins, emptyAddr, addr2, false), // missing sender address - NewMsgTransfer("testportid", "testchannel", coins, addr1, emptyAddr, false), // missing recipient address - NewMsgTransfer("testportid", "testchannel", sdk.Coins{}, addr1, addr2, false), // not possitive coin + NewMsgTransfer(validPort, validChannel, coins, addr1, addr2, true), // valid msg + NewMsgTransfer(invalidShortPort, validChannel, coins, addr1, addr2, true), // too short port id + NewMsgTransfer(invalidLongPort, validChannel, coins, addr1, addr2, true), // too long port id + NewMsgTransfer(invalidPort, validChannel, coins, addr1, addr2, true), // port id contains non-alpha + NewMsgTransfer(validPort, invalidShortChannel, coins, addr1, addr2, true), // too short channel id + NewMsgTransfer(validPort, invalidLongChannel, coins, addr1, addr2, false), // too long channel id + NewMsgTransfer(validPort, invalidChannel, coins, addr1, addr2, false), // channel id contains non-alpha + NewMsgTransfer(validPort, validChannel, invalidDenomCoins, addr1, addr2, false), // invalid amount + NewMsgTransfer(validPort, validChannel, negativeCoins, addr1, addr2, false), // amount contains negative coin + NewMsgTransfer(validPort, validChannel, coins, emptyAddr, addr2, false), // missing sender address + NewMsgTransfer(validPort, validChannel, coins, addr1, emptyAddr, false), // missing recipient address + NewMsgTransfer(validPort, validChannel, sdk.Coins{}, addr1, addr2, false), // not possitive coin } testCases := []struct { @@ -92,7 +94,7 @@ func TestMsgTransferValidation(t *testing.T) { // TestMsgTransferGetSignBytes tests GetSignBytes for MsgTransfer func TestMsgTransferGetSignBytes(t *testing.T) { - msg := NewMsgTransfer("testportid", "testchannel", coins, addr1, addr2, true) + msg := NewMsgTransfer(validPort, validChannel, coins, addr1, addr2, true) res := msg.GetSignBytes() expected := `{"type":"ibc/transfer/MsgTransfer","value":{"amount":[{"amount":"100","denom":"atom"}],"receiver":"cosmos1w3jhxarpv3j8yvs7f9y7g","sender":"cosmos1w3jhxarpv3j8yvg4ufs4x","source":true,"source_channel":"testchannel","source_port":"testportid"}}` @@ -101,7 +103,7 @@ func TestMsgTransferGetSignBytes(t *testing.T) { // TestMsgTransferGetSigners tests GetSigners for MsgTransfer func TestMsgTransferGetSigners(t *testing.T) { - msg := NewMsgTransfer("testportid", "testchannel", coins, addr1, addr2, true) + msg := NewMsgTransfer(validPort, validChannel, coins, addr1, addr2, true) res := msg.GetSigners() expected := "[746573746164647231]" diff --git a/x/ibc/20-transfer/types/packet.go b/x/ibc/20-transfer/types/packet.go index 189ae9b40c63..116bc3e6ede8 100644 --- a/x/ibc/20-transfer/types/packet.go +++ b/x/ibc/20-transfer/types/packet.go @@ -12,6 +12,7 @@ import ( var _ channelexported.PacketDataI = PacketDataTransfer{} // PacketDataTransfer defines a struct for the packet payload +// See FungibleTokenPacketData spec: https://github.com/cosmos/ics/tree/master/spec/ics-020-fungible-token-transfer#data-structures type PacketDataTransfer struct { Amount sdk.Coins `json:"amount" yaml:"amount"` // the tokens to be transferred Sender sdk.AccAddress `json:"sender" yaml:"sender"` // the sender address @@ -31,6 +32,7 @@ func NewPacketDataTransfer(amount sdk.Coins, sender, receiver sdk.AccAddress, so } } +// String returns a string representation of PacketDataTransfer func (pd PacketDataTransfer) String() string { return fmt.Sprintf(`PacketDataTransfer: Amount: %s @@ -44,8 +46,7 @@ func (pd PacketDataTransfer) String() string { ) } -// Implements channelexported.PacketDataI -// ValidateBasic performs a basic check of the packet fields +// ValidateBasic implements channelexported.PacketDataI func (pd PacketDataTransfer) ValidateBasic() error { if !pd.Amount.IsAllPositive() { return sdkerrors.ErrInsufficientFunds @@ -62,43 +63,43 @@ func (pd PacketDataTransfer) ValidateBasic() error { return nil } -// Implements channelexported.PacketDataI +// GetBytes implements channelexported.PacketDataI func (pd PacketDataTransfer) GetBytes() []byte { return sdk.MustSortJSON(ModuleCdc.MustMarshalJSON(pd)) } -// Implements channelexported.PacketDataI +// GetTimeoutHeight implements channelexported.PacketDataI func (pd PacketDataTransfer) GetTimeoutHeight() uint64 { return pd.Timeout } -// Implements channelexported.PacketDataI +// Type implements channelexported.PacketDataI func (pd PacketDataTransfer) Type() string { return "ics20/transfer" } var _ channelexported.PacketDataI = AckDataTransfer{} -type AckDataTransfer struct { -} +// AckDataTransfer is a no-op packet +// See spec for onAcknowledgePacket: https://github.com/cosmos/ics/tree/master/spec/ics-020-fungible-token-transfer#packet-relay +type AckDataTransfer struct{} -// Implements channelexported.PacketDataI -// ValidateBasic performs a basic check of the packet fields +// ValidateBasic implements channelexported.PacketDataI func (ack AckDataTransfer) ValidateBasic() error { return nil } -// Implements channelexported.PacketDataI +// GetBytes implements channelexported.PacketDataI func (ack AckDataTransfer) GetBytes() []byte { return []byte("ok") } -// Implements channelexported.PacketDataI +// GetTimeoutHeight implements channelexported.PacketDataI func (ack AckDataTransfer) GetTimeoutHeight() uint64 { return 0 } -// Implements channelexported.PacketDataI +// Type implements channelexported.PacketDataI func (ack AckDataTransfer) Type() string { return "ics20/transfer/ack" } From c6e232179a9b7c56dd153467c1c0e565494ac600 Mon Sep 17 00:00:00 2001 From: Federico Kunze Date: Fri, 31 Jan 2020 16:46:32 +0100 Subject: [PATCH 02/16] close channel when transfer fails --- x/ibc/04-channel/keeper/handshake.go | 2 +- x/ibc/20-transfer/genesis.go | 1 - x/ibc/20-transfer/handler.go | 45 ++++++++++++--------- x/ibc/20-transfer/keeper/keeper.go | 6 +++ x/ibc/20-transfer/keeper/relay.go | 25 +++++++----- x/ibc/20-transfer/types/expected_keepers.go | 1 + 6 files changed, 50 insertions(+), 30 deletions(-) diff --git a/x/ibc/04-channel/keeper/handshake.go b/x/ibc/04-channel/keeper/handshake.go index 9668cafe55ef..40bfb79e90d1 100644 --- a/x/ibc/04-channel/keeper/handshake.go +++ b/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 } diff --git a/x/ibc/20-transfer/genesis.go b/x/ibc/20-transfer/genesis.go index 974be35f2c7a..facde722a27d 100644 --- a/x/ibc/20-transfer/genesis.go +++ b/x/ibc/20-transfer/genesis.go @@ -10,7 +10,6 @@ import ( // InitGenesis sets distribution information for genesis func InitGenesis(ctx sdk.Context, keeper Keeper) { // check if the module account exists - // TODO: should we create the module account if it doesn't exist? moduleAcc := keeper.GetTransferAccount(ctx) if moduleAcc == nil { panic(fmt.Sprintf("%s module account has not been set", types.GetModuleAccountName())) diff --git a/x/ibc/20-transfer/handler.go b/x/ibc/20-transfer/handler.go index dcc9447c70f5..14f2b6abb87b 100644 --- a/x/ibc/20-transfer/handler.go +++ b/x/ibc/20-transfer/handler.go @@ -20,25 +20,26 @@ func NewHandler(k Keeper) sdk.Handler { case PacketDataTransfer: // i.e fulfills the Data interface 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", msg) } case channeltypes.MsgTimeout: switch data := msg.Data.(type) { case PacketDataTransfer: 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.Amount, msg.Sender, msg.Receiver, msg.Source) - if err != nil { + if err := k.SendTransfer( + ctx, msg.SourcePort, msg.SourceChannel, msg.Amount, msg.Sender, msg.Receiver, msg.Source, + ); err != nil { return nil, err } @@ -57,19 +58,26 @@ func handleMsgTransfer(ctx sdk.Context, k Keeper, msg MsgTransfer) (*sdk.Result, } // 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 types.PacketDataTransfer) (*sdk.Result, error) { +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 + if err := k.ReceiveTransfer( + ctx, packet.SourcePort, packet.SourceChannel, packet.DestinationPort, packet.DestinationChannel, data, + ); err != nil { + // 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, packet.DestinationPort, 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 + if err := k.PacketExecuted(ctx, packet, acknowledgement); err != nil { + return nil, err } ctx.EventManager().EmitEvent( @@ -89,11 +97,12 @@ func handlePacketDataTransfer(ctx sdk.Context, k Keeper, msg channeltypes.MsgPac // 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 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 - panic(err) + if err := k.TimeoutTransfer( + ctx, packet.SourcePort, packet.SourceChannel, packet.DestinationPort, packet.DestinationChannel, data, + ); err != nil { + return nil, err } + // packet timeout should not fail return &sdk.Result{ Events: ctx.EventManager().Events(), diff --git a/x/ibc/20-transfer/keeper/keeper.go b/x/ibc/20-transfer/keeper/keeper.go index c57ca802fa4c..25e9e847a543 100644 --- a/x/ibc/20-transfer/keeper/keeper.go +++ b/x/ibc/20-transfer/keeper/keeper.go @@ -68,3 +68,9 @@ func (k Keeper) GetTransferAccount(ctx sdk.Context) supplyexported.ModuleAccount func (k Keeper) PacketExecuted(ctx sdk.Context, packet channelexported.PacketI, acknowledgement channelexported.PacketDataI) error { return k.channelKeeper.PacketExecuted(ctx, packet, acknowledgement) } + +// ChanCloseInit defines a wrapper function for the channel Keeper's function +// in order to expose it to the ICS20 trasfer handler. +func (k Keeper) ChanCloseInit( ctx sdk.Context, portID, channelID string) error { + return k.channelKeeper.ChanCloseInit(ctx,portID, channelID) +} \ No newline at end of file diff --git a/x/ibc/20-transfer/keeper/relay.go b/x/ibc/20-transfer/keeper/relay.go index ea90cd7c340d..c0841adc5578 100644 --- a/x/ibc/20-transfer/keeper/relay.go +++ b/x/ibc/20-transfer/keeper/relay.go @@ -70,8 +70,9 @@ func (k Keeper) ReceiveTransfer( } // mint new tokens if the source of the transfer is the same chain - err := k.supplyKeeper.MintCoins(ctx, types.GetModuleAccountName(), data.Amount) - if err != nil { + if err := k.supplyKeeper.MintCoins( + ctx, types.GetModuleAccountName(), data.Amount, + ); err != nil { return err } @@ -125,8 +126,9 @@ func (k Keeper) TimeoutTransfer( } // mint from supply - err := k.supplyKeeper.MintCoins(ctx, types.GetModuleAccountName(), data.Amount) - if err != nil { + if err := k.supplyKeeper.MintCoins( + ctx, types.GetModuleAccountName(), data.Amount, + ); err != nil { return err } @@ -164,8 +166,9 @@ func (k Keeper) createOutgoingPacket( } // escrow source tokens (assumed to fail if balance insufficient) - err := k.bankKeeper.SendCoins(ctx, sender, escrowAddress, coins) - if err != nil { + if err := k.bankKeeper.SendCoins( + ctx, sender, escrowAddress, coins, + ); err != nil { return err } @@ -183,14 +186,16 @@ func (k Keeper) createOutgoingPacket( } // transfer the coins to the module account and burn them - err := k.supplyKeeper.SendCoinsFromAccountToModule(ctx, sender, types.GetModuleAccountName(), amount) - if err != nil { + if err := k.supplyKeeper.SendCoinsFromAccountToModule( + ctx, sender, types.GetModuleAccountName(), amount, + ); err != nil { return err } // burn from supply - err = k.supplyKeeper.BurnCoins(ctx, types.GetModuleAccountName(), amount) - if err != nil { + if err := k.supplyKeeper.BurnCoins( + ctx, types.GetModuleAccountName(), amount, + ); err != nil { return err } } diff --git a/x/ibc/20-transfer/types/expected_keepers.go b/x/ibc/20-transfer/types/expected_keepers.go index 1b41921815e1..4a3adef2bca3 100644 --- a/x/ibc/20-transfer/types/expected_keepers.go +++ b/x/ibc/20-transfer/types/expected_keepers.go @@ -20,6 +20,7 @@ type ChannelKeeper interface { GetNextSequenceSend(ctx sdk.Context, portID, channelID string) (uint64, bool) SendPacket(ctx sdk.Context, packet channelexported.PacketI) error PacketExecuted(ctx sdk.Context, packet channelexported.PacketI, acknowledgement channelexported.PacketDataI) error + ChanCloseInit(ctx sdk.Context, portID, channelID string) error } // ClientKeeper defines the expected IBC client keeper From 965440f2bfadb2d78fadc67f69f238f8772a53fa Mon Sep 17 00:00:00 2001 From: Federico Kunze Date: Mon, 3 Feb 2020 12:25:04 +0100 Subject: [PATCH 03/16] rename packer data transfer to align to spec; refactor table tests --- x/ibc/20-transfer/alias.go | 19 +-- x/ibc/20-transfer/handler.go | 17 +-- x/ibc/20-transfer/keeper/keeper.go | 8 +- x/ibc/20-transfer/keeper/keeper_test.go | 81 +++++++++++ x/ibc/20-transfer/keeper/relay.go | 12 +- x/ibc/20-transfer/keeper/relay_test.go | 186 +++++++----------------- x/ibc/20-transfer/module.go | 6 - x/ibc/20-transfer/types/codec.go | 2 +- x/ibc/20-transfer/types/errors.go | 10 ++ x/ibc/20-transfer/types/events.go | 2 +- x/ibc/20-transfer/types/keys.go | 12 +- x/ibc/20-transfer/types/packet.go | 51 ++++--- x/ibc/20-transfer/types/packet_test.go | 18 +-- 13 files changed, 222 insertions(+), 202 deletions(-) create mode 100644 x/ibc/20-transfer/types/errors.go diff --git a/x/ibc/20-transfer/alias.go b/x/ibc/20-transfer/alias.go index ad109cc00a38..d76c3a765e92 100644 --- a/x/ibc/20-transfer/alias.go +++ b/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,12 +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 - PacketDataTransfer = types.PacketDataTransfer + Keeper = keeper.Keeper + BankKeeper = types.BankKeeper + ChannelKeeper = types.ChannelKeeper + ClientKeeper = types.ClientKeeper + ConnectionKeeper = types.ConnectionKeeper + SupplyKeeper = types.SupplyKeeper + MsgTransfer = types.MsgTransfer + FungibleTokenPacketData = types.FungibleTokenPacketData + AckDataTransfer = types.AckDataTransfer ) diff --git a/x/ibc/20-transfer/handler.go b/x/ibc/20-transfer/handler.go index 14f2b6abb87b..dd4b3662cea2 100644 --- a/x/ibc/20-transfer/handler.go +++ b/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" ) @@ -17,14 +16,14 @@ 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: // i.e fulfills the Data interface return handlePacketDataTransfer(ctx, k, msg, data) default: return nil, sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "unrecognized ICS-20 transfer packet data type: %T", msg) } 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 ICS-20 transfer packet data type: %T", data) @@ -46,9 +45,9 @@ func handleMsgTransfer(ctx sdk.Context, k Keeper, msg MsgTransfer) (*sdk.Result, 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()), ), ) @@ -59,7 +58,7 @@ func handleMsgTransfer(ctx sdk.Context, k Keeper, msg MsgTransfer) (*sdk.Result, // 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 types.PacketDataTransfer, + ctx sdk.Context, k Keeper, msg channeltypes.MsgPacket, data FungibleTokenPacketData, ) (*sdk.Result, error) { packet := msg.Packet if err := k.ReceiveTransfer( @@ -75,7 +74,7 @@ func handlePacketDataTransfer( return nil, err } - acknowledgement := types.AckDataTransfer{} + acknowledgement := AckDataTransfer{} if err := k.PacketExecuted(ctx, packet, acknowledgement); err != nil { return nil, err } @@ -83,7 +82,7 @@ func handlePacketDataTransfer( 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()), ), ) @@ -95,7 +94,7 @@ func handlePacketDataTransfer( } // 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 types.PacketDataTransfer) (*sdk.Result, error) { +func handleTimeoutDataTransfer(ctx sdk.Context, k Keeper, msg channeltypes.MsgTimeout, data FungibleTokenPacketData) (*sdk.Result, error) { packet := msg.Packet if err := k.TimeoutTransfer( ctx, packet.SourcePort, packet.SourceChannel, packet.DestinationPort, packet.DestinationChannel, data, diff --git a/x/ibc/20-transfer/keeper/keeper.go b/x/ibc/20-transfer/keeper/keeper.go index 25e9e847a543..126a634e9c8e 100644 --- a/x/ibc/20-transfer/keeper/keeper.go +++ b/x/ibc/20-transfer/keeper/keeper.go @@ -55,7 +55,7 @@ func NewKeeper( // Logger returns a module-specific logger. func (k Keeper) Logger(ctx sdk.Context) log.Logger { - return ctx.Logger().With("module", fmt.Sprintf("x/%s/%s", ibctypes.ModuleName, types.SubModuleName)) + return ctx.Logger().With("module", fmt.Sprintf("x/%s/%s", ibctypes.ModuleName, types.ModuleName)) } // GetTransferAccount returns the ICS20 - transfers ModuleAccount @@ -71,6 +71,6 @@ func (k Keeper) PacketExecuted(ctx sdk.Context, packet channelexported.PacketI, // ChanCloseInit defines a wrapper function for the channel Keeper's function // in order to expose it to the ICS20 trasfer handler. -func (k Keeper) ChanCloseInit( ctx sdk.Context, portID, channelID string) error { - return k.channelKeeper.ChanCloseInit(ctx,portID, channelID) -} \ No newline at end of file +func (k Keeper) ChanCloseInit(ctx sdk.Context, portID, channelID string) error { + return k.channelKeeper.ChanCloseInit(ctx, portID, channelID) +} diff --git a/x/ibc/20-transfer/keeper/keeper_test.go b/x/ibc/20-transfer/keeper/keeper_test.go index af4ea22a8e64..a746225d6b2a 100644 --- a/x/ibc/20-transfer/keeper/keeper_test.go +++ b/x/ibc/20-transfer/keeper/keeper_test.go @@ -13,9 +13,14 @@ import ( "github.com/cosmos/cosmos-sdk/simapp" sdk "github.com/cosmos/cosmos-sdk/types" clientexported "github.com/cosmos/cosmos-sdk/x/ibc/02-client/exported" + connection "github.com/cosmos/cosmos-sdk/x/ibc/03-connection" connectionexported "github.com/cosmos/cosmos-sdk/x/ibc/03-connection/exported" + channel "github.com/cosmos/cosmos-sdk/x/ibc/04-channel" channelexported "github.com/cosmos/cosmos-sdk/x/ibc/04-channel/exported" + tendermint "github.com/cosmos/cosmos-sdk/x/ibc/07-tendermint" "github.com/cosmos/cosmos-sdk/x/ibc/20-transfer/types" + commitment "github.com/cosmos/cosmos-sdk/x/ibc/23-commitment" + ibctypes "github.com/cosmos/cosmos-sdk/x/ibc/types" ) // define constants used for testing @@ -84,3 +89,79 @@ func (suite *KeeperTestSuite) TestGetTransferAccount() { func TestKeeperTestSuite(t *testing.T) { suite.Run(t, new(KeeperTestSuite)) } + +func (suite *KeeperTestSuite) createClient() { + suite.app.Commit() + commitID := suite.app.LastCommitID() + + suite.app.BeginBlock(abci.RequestBeginBlock{Header: abci.Header{Height: suite.app.LastBlockHeight() + 1}}) + suite.ctx = suite.app.BaseApp.NewContext(false, abci.Header{}) + + consensusState := tendermint.ConsensusState{ + Root: commitment.NewRoot(commitID.Hash), + ValidatorSetHash: suite.valSet.Hash(), + } + + _, err := suite.app.IBCKeeper.ClientKeeper.CreateClient(suite.ctx, testClient, testClientType, consensusState) + suite.NoError(err) +} + +func (suite *KeeperTestSuite) updateClient() { + // always commit and begin a new block on updateClient + suite.app.Commit() + commitID := suite.app.LastCommitID() + + suite.app.BeginBlock(abci.RequestBeginBlock{Header: abci.Header{Height: suite.app.LastBlockHeight() + 1}}) + suite.ctx = suite.app.BaseApp.NewContext(false, abci.Header{}) + + state := tendermint.ConsensusState{ + Root: commitment.NewRoot(commitID.Hash), + } + + suite.app.IBCKeeper.ClientKeeper.SetClientConsensusState(suite.ctx, testClient, 1, state) +} + +func (suite *KeeperTestSuite) createConnection(state connectionexported.State) { + connection := connection.ConnectionEnd{ + State: state, + ClientID: testClient, + Counterparty: connection.Counterparty{ + ClientID: testClient, + ConnectionID: testConnection, + Prefix: suite.app.IBCKeeper.ConnectionKeeper.GetCommitmentPrefix(), + }, + Versions: connection.GetCompatibleVersions(), + } + + suite.app.IBCKeeper.ConnectionKeeper.SetConnection(suite.ctx, testConnection, connection) +} + +func (suite *KeeperTestSuite) createChannel(portID string, chanID string, connID string, counterpartyPort string, counterpartyChan string, state channelexported.State) { + ch := channel.Channel{ + State: state, + Ordering: testChannelOrder, + Counterparty: channel.Counterparty{ + PortID: counterpartyPort, + ChannelID: counterpartyChan, + }, + ConnectionHops: []string{connID}, + Version: testChannelVersion, + } + + suite.app.IBCKeeper.ChannelKeeper.SetChannel(suite.ctx, portID, chanID, ch) +} + +func (suite *KeeperTestSuite) queryProof(key []byte) (proof commitment.Proof, height int64) { + res := suite.app.Query(abci.RequestQuery{ + Path: fmt.Sprintf("store/%s/key", ibctypes.StoreKey), + Data: key, + Prove: true, + }) + + height = res.Height + proof = commitment.Proof{ + Proof: res.Proof, + } + + return +} diff --git a/x/ibc/20-transfer/keeper/relay.go b/x/ibc/20-transfer/keeper/relay.go index c0841adc5578..06073cd7efb9 100644 --- a/x/ibc/20-transfer/keeper/relay.go +++ b/x/ibc/20-transfer/keeper/relay.go @@ -1,6 +1,7 @@ package keeper import ( + "fmt" "strings" sdk "github.com/cosmos/cosmos-sdk/types" @@ -35,10 +36,10 @@ func (k Keeper) SendTransfer( } coins := make(sdk.Coins, len(amount)) - prefix := types.GetDenomPrefix(destinationPort, destinationChannel) switch { case isSourceChain: // build the receiving denomination prefix + prefix := types.GetDenomPrefix(destinationPort, destinationChannel) for i, coin := range amount { coins[i] = sdk.NewCoin(prefix+coin.Denom, coin.Amount) } @@ -56,7 +57,7 @@ func (k Keeper) ReceiveTransfer( sourceChannel, destinationPort, destinationChannel string, - data types.PacketDataTransfer, + data types.FungibleTokenPacketData, ) error { if data.Source { prefix := types.GetDenomPrefix(destinationPort, destinationChannel) @@ -107,7 +108,7 @@ func (k Keeper) TimeoutTransfer( sourceChannel, destinationPort, destinationChannel string, - data types.PacketDataTransfer, + data types.FungibleTokenPacketData, ) error { // check the denom prefix prefix := types.GetDenomPrefix(sourcePort, sourceChannel) @@ -154,6 +155,7 @@ func (k Keeper) createOutgoingPacket( // construct receiving denominations, check correctness prefix := types.GetDenomPrefix(destinationPort, destinationChannel) + fmt.Println(prefix) coins := make(sdk.Coins, len(amount)) for i, coin := range amount { if !strings.HasPrefix(coin.Denom, prefix) { @@ -200,7 +202,9 @@ func (k Keeper) createOutgoingPacket( } } - packetData := types.NewPacketDataTransfer(amount, sender, receiver, isSourceChain, uint64(ctx.BlockHeight())+DefaultPacketTimeout) + packetData := types.NewFungibleTokenPacketData( + amount, sender, receiver, isSourceChain, uint64(ctx.BlockHeight())+DefaultPacketTimeout, + ) packet := channel.NewPacket( packetData, diff --git a/x/ibc/20-transfer/keeper/relay_test.go b/x/ibc/20-transfer/keeper/relay_test.go index a9c6ebd0a3f6..102a028a8fda 100644 --- a/x/ibc/20-transfer/keeper/relay_test.go +++ b/x/ibc/20-transfer/keeper/relay_test.go @@ -3,146 +3,72 @@ package keeper_test import ( "fmt" - abci "github.com/tendermint/tendermint/abci/types" - sdk "github.com/cosmos/cosmos-sdk/types" - connection "github.com/cosmos/cosmos-sdk/x/ibc/03-connection" - connectionexported "github.com/cosmos/cosmos-sdk/x/ibc/03-connection/exported" - channel "github.com/cosmos/cosmos-sdk/x/ibc/04-channel" channelexported "github.com/cosmos/cosmos-sdk/x/ibc/04-channel/exported" - tendermint "github.com/cosmos/cosmos-sdk/x/ibc/07-tendermint" "github.com/cosmos/cosmos-sdk/x/ibc/20-transfer/types" - commitment "github.com/cosmos/cosmos-sdk/x/ibc/23-commitment" - ibctypes "github.com/cosmos/cosmos-sdk/x/ibc/types" - "github.com/cosmos/cosmos-sdk/x/supply" ) -func (suite *KeeperTestSuite) createClient() { - suite.app.Commit() - commitID := suite.app.LastCommitID() - - suite.app.BeginBlock(abci.RequestBeginBlock{Header: abci.Header{Height: suite.app.LastBlockHeight() + 1}}) - suite.ctx = suite.app.BaseApp.NewContext(false, abci.Header{}) - - consensusState := tendermint.ConsensusState{ - Root: commitment.NewRoot(commitID.Hash), - ValidatorSetHash: suite.valSet.Hash(), - } - - _, err := suite.app.IBCKeeper.ClientKeeper.CreateClient(suite.ctx, testClient, testClientType, consensusState) - suite.NoError(err) -} - -func (suite *KeeperTestSuite) updateClient() { - // always commit and begin a new block on updateClient - suite.app.Commit() - commitID := suite.app.LastCommitID() - - suite.app.BeginBlock(abci.RequestBeginBlock{Header: abci.Header{Height: suite.app.LastBlockHeight() + 1}}) - suite.ctx = suite.app.BaseApp.NewContext(false, abci.Header{}) - - state := tendermint.ConsensusState{ - Root: commitment.NewRoot(commitID.Hash), - } - - suite.app.IBCKeeper.ClientKeeper.SetClientConsensusState(suite.ctx, testClient, 1, state) -} - -func (suite *KeeperTestSuite) createConnection(state connectionexported.State) { - connection := connection.ConnectionEnd{ - State: state, - ClientID: testClient, - Counterparty: connection.Counterparty{ - ClientID: testClient, - ConnectionID: testConnection, - Prefix: suite.app.IBCKeeper.ConnectionKeeper.GetCommitmentPrefix(), - }, - Versions: connection.GetCompatibleVersions(), +func (suite *KeeperTestSuite) TestSendTransfer() { + testCases := []struct { + msg string + sourcePort string + sourceChannel string + amount sdk.Coins + sender sdk.AccAddress + receiver sdk.AccAddress + isSourceChain bool + malleate func() + expPass bool + }{ + // {"sucess transfer from source chain", testPort1, testChannel1, testCoins, testAddr1, testAddr2, + // true, func() {}, true}, + // {"sucess transfer from external chain", testPort1, testChannel1, testCoins, testAddr1, testAddr2, + // true, func() {}, true}, + {"source channel not found", testPort1, testChannel1, testCoins, testAddr1, testAddr2, + true, func() {}, false}, + {"next seq send not found", testPort1, testChannel1, testCoins, testAddr1, testAddr2, + true, func() { + suite.createChannel(testPort1, testChannel1, testConnection, testPort2, testChannel2, channelexported.OPEN) + }, false}, + // createOutgoingPacket tests + // - source chain + {"no prefix on transfer amount", testPort1, testChannel1, testCoins, testAddr1, testAddr2, + true, func() { + suite.createChannel(testPort1, testChannel1, testConnection, testPort2, testChannel2, channelexported.OPEN) + suite.app.IBCKeeper.ChannelKeeper.SetNextSequenceSend(suite.ctx, testPort1, testChannel1, 1) + }, false}, + {"send coins failed", testPort1, testChannel1, testCoins, testAddr1, testAddr2, + true, func() { + suite.createChannel(testPort1, testChannel1, testConnection, testPort2, testChannel2, channelexported.OPEN) + suite.app.IBCKeeper.ChannelKeeper.SetNextSequenceSend(suite.ctx, testPort1, testChannel1, 1) + }, true}, + // - receiving chain + // {"no prefix on transfer amount", testPort1, testChannel1, testCoins, testAddr1, testAddr2, + // false, func() {}, false}, + // {"send from module account dailed", testPort1, testChannel1, testCoins, testAddr1, testAddr2, + // false, func() {}, false}, + // {"tokens burn failed", testPort1, testChannel1, testCoins, testAddr1, testAddr2, + // false, func() {}, false}, } - suite.app.IBCKeeper.ConnectionKeeper.SetConnection(suite.ctx, testConnection, connection) -} - -func (suite *KeeperTestSuite) createChannel(portID string, chanID string, connID string, counterpartyPort string, counterpartyChan string, state channelexported.State) { - ch := channel.Channel{ - State: state, - Ordering: testChannelOrder, - Counterparty: channel.Counterparty{ - PortID: counterpartyPort, - ChannelID: counterpartyChan, - }, - ConnectionHops: []string{connID}, - Version: testChannelVersion, - } + for i, tc := range testCases { + tc := tc + suite.Run(fmt.Sprintf("Case %s", tc.msg), func() { + suite.SetupTest() // reset - suite.app.IBCKeeper.ChannelKeeper.SetChannel(suite.ctx, portID, chanID, ch) -} + tc.malleate() -func (suite *KeeperTestSuite) queryProof(key []byte) (proof commitment.Proof, height int64) { - res := suite.app.Query(abci.RequestQuery{ - Path: fmt.Sprintf("store/%s/key", ibctypes.StoreKey), - Data: key, - Prove: true, - }) + err := suite.app.TransferKeeper.SendTransfer( + suite.ctx, tc.sourcePort, tc.sourceChannel, tc.amount, tc.sender, tc.receiver, tc.isSourceChain, + ) - height = res.Height - proof = commitment.Proof{ - Proof: res.Proof, + if tc.expPass { + suite.Require().NoError(err, "valid test case %d failed: %s", i, tc.msg) + } else { + suite.Require().Error(err, "invalid test case %d passed: %s", i, tc.msg) + } + }) } - - return -} - -func (suite *KeeperTestSuite) TestSendTransfer() { - // test the situation where the source is true - isSourceChain := true - - err := suite.app.TransferKeeper.SendTransfer(suite.ctx, testPort1, testChannel1, testCoins, testAddr1, testAddr2, isSourceChain) - suite.Error(err) // channel does not exist - - suite.createChannel(testPort1, testChannel1, testConnection, testPort2, testChannel2, channelexported.OPEN) - err = suite.app.TransferKeeper.SendTransfer(suite.ctx, testPort1, testChannel1, testCoins, testAddr1, testAddr2, isSourceChain) - suite.Error(err) // next send sequence not found - - nextSeqSend := uint64(1) - suite.app.IBCKeeper.ChannelKeeper.SetNextSequenceSend(suite.ctx, testPort1, testChannel1, nextSeqSend) - err = suite.app.TransferKeeper.SendTransfer(suite.ctx, testPort1, testChannel1, testCoins, testAddr1, testAddr2, isSourceChain) - suite.Error(err) // sender has insufficient coins - - _ = suite.app.BankKeeper.SetBalances(suite.ctx, testAddr1, testCoins) - err = suite.app.TransferKeeper.SendTransfer(suite.ctx, testPort1, testChannel1, testCoins, testAddr1, testAddr2, isSourceChain) - suite.NoError(err) // successfully executed - - senderCoins := suite.app.BankKeeper.GetAllBalances(suite.ctx, testAddr1) - suite.Equal(sdk.Coins(nil), senderCoins) - - escrowCoins := suite.app.BankKeeper.GetAllBalances(suite.ctx, types.GetEscrowAddress(testPort1, testChannel1)) - suite.Equal(testCoins, escrowCoins) - - newNextSeqSend, found := suite.app.IBCKeeper.ChannelKeeper.GetNextSequenceSend(suite.ctx, testPort1, testChannel1) - suite.True(found) - suite.Equal(nextSeqSend+1, newNextSeqSend) - - packetCommitment := suite.app.IBCKeeper.ChannelKeeper.GetPacketCommitment(suite.ctx, testPort1, testChannel1, nextSeqSend) - suite.NotNil(packetCommitment) - - // test the situation where the source is false - isSourceChain = false - - _ = suite.app.BankKeeper.SetBalances(suite.ctx, testAddr1, testPrefixedCoins2) - err = suite.app.TransferKeeper.SendTransfer(suite.ctx, testPort1, testChannel1, testPrefixedCoins2, testAddr1, testAddr2, isSourceChain) - suite.Error(err) // incorrect denom prefix - - suite.app.SupplyKeeper.SetSupply(suite.ctx, supply.NewSupply(testPrefixedCoins1)) - _ = suite.app.BankKeeper.SetBalances(suite.ctx, testAddr1, testPrefixedCoins1) - err = suite.app.TransferKeeper.SendTransfer(suite.ctx, testPort1, testChannel1, testPrefixedCoins1, testAddr1, testAddr2, isSourceChain) - suite.NoError(err) // successfully executed - - senderCoins = suite.app.BankKeeper.GetAllBalances(suite.ctx, testAddr1) - suite.Equal(sdk.Coins(nil), senderCoins) - - totalSupply := suite.app.SupplyKeeper.GetSupply(suite.ctx) - suite.Equal(sdk.Coins(nil), totalSupply.GetTotal()) // supply should be deflated } func (suite *KeeperTestSuite) TestReceiveTransfer() { @@ -150,7 +76,7 @@ func (suite *KeeperTestSuite) TestReceiveTransfer() { source := true packetTimeout := uint64(100) - packetData := types.NewPacketDataTransfer(testPrefixedCoins1, testAddr1, testAddr2, source, packetTimeout) + packetData := types.NewFungibleTokenPacketData(testPrefixedCoins1, testAddr1, testAddr2, source, packetTimeout) err := suite.app.TransferKeeper.ReceiveTransfer(suite.ctx, testPort1, testChannel1, testPort2, testChannel2, packetData) suite.Error(err) // incorrect denom prefix diff --git a/x/ibc/20-transfer/module.go b/x/ibc/20-transfer/module.go index 2a4e593f61c3..00c1306f87d8 100644 --- a/x/ibc/20-transfer/module.go +++ b/x/ibc/20-transfer/module.go @@ -14,12 +14,6 @@ import ( "github.com/cosmos/cosmos-sdk/types/module" "github.com/cosmos/cosmos-sdk/x/ibc/20-transfer/client/cli" "github.com/cosmos/cosmos-sdk/x/ibc/20-transfer/client/rest" - "github.com/cosmos/cosmos-sdk/x/ibc/20-transfer/types" -) - -const ( - // ModuleName declares the name of the module - ModuleName = types.SubModuleName ) var ( diff --git a/x/ibc/20-transfer/types/codec.go b/x/ibc/20-transfer/types/codec.go index ff87c7fcfeaf..489bd6cd9dce 100644 --- a/x/ibc/20-transfer/types/codec.go +++ b/x/ibc/20-transfer/types/codec.go @@ -12,7 +12,7 @@ var ModuleCdc = codec.New() // RegisterCodec registers the IBC transfer types func RegisterCodec(cdc *codec.Codec) { cdc.RegisterConcrete(MsgTransfer{}, "ibc/transfer/MsgTransfer", nil) - cdc.RegisterConcrete(PacketDataTransfer{}, "ibc/transfer/PacketDataTransfer", nil) + cdc.RegisterConcrete(FungibleTokenPacketData{}, "ibc/transfer/PacketDataTransfer", nil) } func init() { diff --git a/x/ibc/20-transfer/types/errors.go b/x/ibc/20-transfer/types/errors.go new file mode 100644 index 000000000000..c0f40d1171b9 --- /dev/null +++ b/x/ibc/20-transfer/types/errors.go @@ -0,0 +1,10 @@ +package types + +import ( + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" +) + +// IBC channel sentinel errors +var ( + ErrInvalidPacketTimeout = sdkerrors.Register(ModuleName, 1, "invalid packet timeout") +) diff --git a/x/ibc/20-transfer/types/events.go b/x/ibc/20-transfer/types/events.go index 759548ae1827..f233b49377ff 100644 --- a/x/ibc/20-transfer/types/events.go +++ b/x/ibc/20-transfer/types/events.go @@ -13,5 +13,5 @@ const ( // IBC transfer events vars var ( - AttributeValueCategory = fmt.Sprintf("%s_%s", ibctypes.ModuleName, SubModuleName) + AttributeValueCategory = fmt.Sprintf("%s_%s", ibctypes.ModuleName, ModuleName) ) diff --git a/x/ibc/20-transfer/types/keys.go b/x/ibc/20-transfer/types/keys.go index 1a3a8458ae53..322f1500349a 100644 --- a/x/ibc/20-transfer/types/keys.go +++ b/x/ibc/20-transfer/types/keys.go @@ -10,17 +10,17 @@ import ( ) const ( - // SubModuleName defines the IBC transfer name - SubModuleName = "transfer" + // ModuleName defines the IBC transfer name + ModuleName = "transfer" // StoreKey is the store key string for IBC transfer - StoreKey = SubModuleName + StoreKey = ModuleName // RouterKey is the message route for IBC transfer - RouterKey = SubModuleName + RouterKey = ModuleName // QuerierRoute is the querier route for IBC transfer - QuerierRoute = SubModuleName + QuerierRoute = ModuleName ) // GetEscrowAddress returns the escrow address for the specified channel @@ -39,5 +39,5 @@ func GetDenomPrefix(portID, channelID string) string { // GetModuleAccountName returns the IBC transfer module account name for supply func GetModuleAccountName() string { - return fmt.Sprintf("%s/%s", ibctypes.ModuleName, SubModuleName) + return fmt.Sprintf("%s/%s", ibctypes.ModuleName, ModuleName) } diff --git a/x/ibc/20-transfer/types/packet.go b/x/ibc/20-transfer/types/packet.go index 116bc3e6ede8..7ff77f4384cb 100644 --- a/x/ibc/20-transfer/types/packet.go +++ b/x/ibc/20-transfer/types/packet.go @@ -9,11 +9,11 @@ import ( channelexported "github.com/cosmos/cosmos-sdk/x/ibc/04-channel/exported" ) -var _ channelexported.PacketDataI = PacketDataTransfer{} +var _ channelexported.PacketDataI = FungibleTokenPacketData{} -// PacketDataTransfer defines a struct for the packet payload +// FungibleTokenPacketData defines a struct for the packet payload // See FungibleTokenPacketData spec: https://github.com/cosmos/ics/tree/master/spec/ics-020-fungible-token-transfer#data-structures -type PacketDataTransfer struct { +type FungibleTokenPacketData struct { Amount sdk.Coins `json:"amount" yaml:"amount"` // the tokens to be transferred Sender sdk.AccAddress `json:"sender" yaml:"sender"` // the sender address Receiver sdk.AccAddress `json:"receiver" yaml:"receiver"` // the recipient address on the destination chain @@ -21,9 +21,11 @@ type PacketDataTransfer struct { Timeout uint64 `json:"timeout" yaml:"timeout"` } -// NewPacketDataTransfer contructs a new PacketDataTransfer -func NewPacketDataTransfer(amount sdk.Coins, sender, receiver sdk.AccAddress, source bool, timeout uint64) PacketDataTransfer { - return PacketDataTransfer{ +// NewFungibleTokenPacketData contructs a new FungibleTokenPacketData instance +func NewFungibleTokenPacketData( + amount sdk.Coins, sender, receiver sdk.AccAddress, + source bool, timeout uint64) FungibleTokenPacketData { + return FungibleTokenPacketData{ Amount: amount, Sender: sender, Receiver: receiver, @@ -32,49 +34,52 @@ func NewPacketDataTransfer(amount sdk.Coins, sender, receiver sdk.AccAddress, so } } -// String returns a string representation of PacketDataTransfer -func (pd PacketDataTransfer) String() string { - return fmt.Sprintf(`PacketDataTransfer: +// String returns a string representation of FungibleTokenPacketData +func (ftpd FungibleTokenPacketData) String() string { + return fmt.Sprintf(`FungibleTokenPacketData: Amount: %s Sender: %s Receiver: %s Source: %v`, - pd.Amount.String(), - pd.Sender, - pd.Receiver, - pd.Source, + ftpd.Amount.String(), + ftpd.Sender, + ftpd.Receiver, + ftpd.Source, ) } // ValidateBasic implements channelexported.PacketDataI -func (pd PacketDataTransfer) ValidateBasic() error { - if !pd.Amount.IsAllPositive() { +func (ftpd FungibleTokenPacketData) ValidateBasic() error { + if !ftpd.Amount.IsAllPositive() { return sdkerrors.ErrInsufficientFunds } - if !pd.Amount.IsValid() { + if !ftpd.Amount.IsValid() { return sdkerrors.ErrInvalidCoins } - if pd.Sender.Empty() { + if ftpd.Sender.Empty() { return sdkerrors.Wrap(sdkerrors.ErrInvalidAddress, "missing sender address") } - if pd.Receiver.Empty() { + if ftpd.Receiver.Empty() { return sdkerrors.Wrap(sdkerrors.ErrInvalidAddress, "missing receiver address") } + if ftpd.Timeout == 0 { + return sdkerrors.Wrap(ErrInvalidPacketTimeout, "timeout cannot be 0") + } return nil } // GetBytes implements channelexported.PacketDataI -func (pd PacketDataTransfer) GetBytes() []byte { - return sdk.MustSortJSON(ModuleCdc.MustMarshalJSON(pd)) +func (ftpd FungibleTokenPacketData) GetBytes() []byte { + return sdk.MustSortJSON(ModuleCdc.MustMarshalJSON(ftpd)) } // GetTimeoutHeight implements channelexported.PacketDataI -func (pd PacketDataTransfer) GetTimeoutHeight() uint64 { - return pd.Timeout +func (ftpd FungibleTokenPacketData) GetTimeoutHeight() uint64 { + return ftpd.Timeout } // Type implements channelexported.PacketDataI -func (pd PacketDataTransfer) Type() string { +func (ftpd FungibleTokenPacketData) Type() string { return "ics20/transfer" } diff --git a/x/ibc/20-transfer/types/packet_test.go b/x/ibc/20-transfer/types/packet_test.go index 1a3c1b46fc16..adc86ff42b11 100644 --- a/x/ibc/20-transfer/types/packet_test.go +++ b/x/ibc/20-transfer/types/packet_test.go @@ -6,18 +6,18 @@ import ( "github.com/stretchr/testify/require" ) -// TestPacketDataTransferValidation tests ValidateBasic for PacketDataTransfer -func TestPacketDataTransferValidation(t *testing.T) { - testPacketDataTransfer := []PacketDataTransfer{ - NewPacketDataTransfer(coins, addr1, addr2, true, 100), // valid msg - NewPacketDataTransfer(invalidDenomCoins, addr1, addr2, true, 100), // invalid amount - NewPacketDataTransfer(negativeCoins, addr1, addr2, false, 100), // amount contains negative coin - NewPacketDataTransfer(coins, emptyAddr, addr2, false, 100), // missing sender address - NewPacketDataTransfer(coins, addr1, emptyAddr, false, 100), // missing recipient address +// TestFungibleTokenPacketDataValidateBasic tests ValidateBasic for FungibleTokenPacketData +func TestFungibleTokenPacketDataValidateBasic(t *testing.T) { + testPacketDataTransfer := []FungibleTokenPacketData{ + NewFungibleTokenPacketData(coins, addr1, addr2, true, 100), // valid msg + NewFungibleTokenPacketData(invalidDenomCoins, addr1, addr2, true, 100), // invalid amount + NewFungibleTokenPacketData(negativeCoins, addr1, addr2, false, 100), // amount contains negative coin + NewFungibleTokenPacketData(coins, emptyAddr, addr2, false, 100), // missing sender address + NewFungibleTokenPacketData(coins, addr1, emptyAddr, false, 100), // missing recipient address } testCases := []struct { - packetData PacketDataTransfer + packetData FungibleTokenPacketData expPass bool errMsg string }{ From 05fee3f6ef60d27db54753d3d0911f71d434fe6a Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Mon, 3 Feb 2020 12:40:33 +0100 Subject: [PATCH 04/16] 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 * 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 <31522760+fedekunze@users.noreply.github.com> Co-authored-by: Marko Co-authored-by: Alexander Bezobchuk Co-authored-by: Jack Zampolin --- CHANGELOG.md | 3 + docs/building-modules/simulator.md | 123 ++++++++++++++++++++ docs/building-modules/structure.md | 17 ++- docs/package.json | 2 +- docs/using-the-sdk/README.md | 1 - docs/using-the-sdk/simulation.md | 89 +++++++++----- x/bank/alias.go | 30 ++--- x/ibc/20-transfer/alias.go | 4 +- x/ibc/20-transfer/handler.go | 16 ++- x/ibc/20-transfer/keeper/keeper.go | 8 +- x/ibc/20-transfer/types/expected_keepers.go | 1 + x/staking/keeper/delegation.go | 59 +++++++--- x/staking/keeper/val_state_change.go | 12 +- x/staking/spec/07_events.md | 26 +++-- 14 files changed, 301 insertions(+), 90 deletions(-) create mode 100644 docs/building-modules/simulator.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 1764c8d33313..fc45a176b45d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -62,6 +62,9 @@ balances or a single balance by denom when the `denom` query parameter is presen ### Improvements +* (modules) [\#5597](https://github.com/cosmos/cosmos-sdk/pull/5597) Add `amount` event attribute to the `complete_unbonding` +and `complete_redelegation` events that reflect the total balances of the completed unbondings and redelegations +respectively. * (types) [\#5581](https://github.com/cosmos/cosmos-sdk/pull/5581) Add convenience functions {,Must}Bech32ifyAddressBytes. * (staking) [\#5584](https://github.com/cosmos/cosmos-sdk/pull/5584) Add util function `ToTmValidator` that converts a `staking.Validator` type to `*tmtypes.Validator`. * (client) [\#5585](https://github.com/cosmos/cosmos-sdk/pull/5585) IBC additions: diff --git a/docs/building-modules/simulator.md b/docs/building-modules/simulator.md new file mode 100644 index 000000000000..d8b7aca42307 --- /dev/null +++ b/docs/building-modules/simulator.md @@ -0,0 +1,123 @@ +# Module Simulation + +## Prerequisites + +* [Cosmos Blockchain Simulator](./../using-the-sdk/simulation.md) + +## Synopsis + +This document details how to define each module simulation functions to be +integrated with the application `SimulationManager`. + +* [Simulation package](#simulation-package) + * [Store decoders](#store-decoders) + * [Randomized genesis](#randomized-genesis) + * [Randomized parameters](#randomized-parameters) + * [Random weighted operations](#random-weighted-operations) + * [Random proposal contents](#random-proposal-contents) +* [Registering the module simulation functions](#registering-simulation-functions) +* [App simulator manager](#app-simulator-manager) +* [Simulation tests](#simulation-tests) + +## Simulation package + +Every module that implements the SDK simulator needs to have a `x//simulation` +package which contains the primary functions required by the fuzz tests: store +decoders, randomized genesis state and parameters, weighted operations and proposal +contents. + +### Store decoders + +Registering the store decoders is required for the `AppImportExport`. This allows +for the key-value pairs from the stores to be decoded (_i.e_ unmarshalled) +to their corresponding types. In particular, it matches the key to a concrete type +and then unmarshals the value from the `KVPair` to the type provided. + +You can use the example [here](https://github.com/cosmos/cosmos-sdk/blob/release%2Fv0.38.0/x/distribution/simulation/decoder.go) from the distribution module to implement your store decoders. + +### Randomized genesis + +The simulator tests different scenarios and values for genesis parameters +in order to fully test the edge cases of specific modules. The `simulator` package from each module must expose a `RandomizedGenState` function to generate the initial random `GenesisState` from a given seed. In + +Once the module genesis parameter are generated randomly (or with the key and +values defined in a `params` file), they are marshaled to JSON format and added +to the app genesis JSON to use it on the simulations. + +You can check an example on how to create the randomized genesis [here](https://github.com/cosmos/cosmos-sdk/blob/release%2Fv0.38.0/x/staking/simulation/genesis.go). + +### Randomized parameter changes + +The simulator is able to test parameter changes at random. The simulator package from each module must contain a `RandomizedParams` func that will simulate parameter changes of the module throughout the simulations lifespan. + +You can see how an example of what is needed to fully test parameter changes [here](https://github.com/cosmos/cosmos-sdk/blob/release%2Fv0.38.0/x/staking/simulation/params.go) + +### Random weighted operations + +Operations are one of the crucial parts of the SDK simulation. They are the transactions +(`Msg`) that are simulated with random field values. The sender of the operation +is also assigned randomly. + +Operations on the simulation are simulated using the full [transaction cycle](../core/transactions.md) of a +`ABCI` application that exposes the `BaseApp`. + +Shown below is how weights are set: + ++++ https://github.com/cosmos/cosmos-sdk/blob/release%2Fv0.38.0/x/staking/simulation/operations.go#L18-L92 + +As you can see the weights are predefined in this case but there are options on how to override this behavior with different weights. One is allowing `*rand.Rand` to define a random weight for the operation, or you can inject your own predefined weights. + +Here is how one can override the above package `simappparams`. + ++++ https://github.com/cosmos/gaia/blob/master/sims.mk#L9-L22 + +For the last test a tool called runsim is used, this is used to parallelize go test instances, provide info to Github and slack integrations to provide information to your team on how the simulations are running. + +### Random proposal contents + +Randomized governance proposals are also supported on the SDK simulator. Each +module must define the governance proposal `Content`s that they expose and register +them to be used on the parameters. + +## Registering simulation functions + +Now that all the required functions are defined, we need to integrate them into the module pattern within the `module.go`: + ++++ https://github.com/cosmos/cosmos-sdk/blob/release%2Fv0.38.0/x/distribution/module.go#L156-L185 + +## App Simulator manager + +The following step is setting up the `SimulatorManager` at the app level. This +is required for the simulation test files on the next step. + +```go +type CustomApp struct { + ... + sm *module.SimulationManager +} +``` + +Then at the instantiation of the application, we create the `SimulationManager` +instance in the same way we create the `ModuleManager` but this time we only pass +the modules that implement the simulation functions from the `AppModuleSimulation` +interface described above. + +```go +func NewCustomApp(...) { + // create the simulation manager and define the order of the modules for deterministic simulations + app.sm = module.NewSimulationManager( + auth.NewAppModule(app.accountKeeper), + bank.NewAppModule(app.bankKeeper, app.accountKeeper), + supply.NewAppModule(app.supplyKeeper, app.accountKeeper), + ov.NewAppModule(app.govKeeper, app.accountKeeper, app.supplyKeeper), + mint.NewAppModule(app.mintKeeper), + distr.NewAppModule(app.distrKeeper, app.accountKeeper, app.supplyKeeper, app.stakingKeeper), + staking.NewAppModule(app.stakingKeeper, app.accountKeeper, app.supplyKeeper), + slashing.NewAppModule(app.slashingKeeper, app.accountKeeper, app.stakingKeeper), + ) + + // register the store decoders for simulation tests + app.sm.RegisterStoreDecoders() + ... +} +``` diff --git a/docs/building-modules/structure.md b/docs/building-modules/structure.md index 6b6d2f889a14..47d9a82b19ff 100644 --- a/docs/building-modules/structure.md +++ b/docs/building-modules/structure.md @@ -37,13 +37,18 @@ x/{module} │   ├── params.go │   ├── ... │   └── querier.go +├── simulation +│   ├── decoder.go +│   ├── genesis.go +│   ├── operations.go +│   ├── params.go +│   └── proposals.go ├── abci.go ├── alias.go ├── genesis.go ├── handler.go -├── module.go ├── ... -└── simulation.go +└── module.go ``` - `abci.go`: The module's `BeginBlocker` and `EndBlocker` implementations (if any). @@ -53,7 +58,7 @@ there is nothing preventing developers from importing other packages from the mo (excluding`internal/`) but it is recommended that `alias.go` have everything exposed that other modules may need. The majority of the exported values here will typically come from `internal/` (see below). -- `client/`: The module's CLI and REST client functionality implementation and +- `client/`: The module's CLI and REST client functionality implementation and testing. - `exported/`: The module's exported types -- typically type interfaces. If a module relies on other module keepers, it is expected to receive them as interface @@ -79,8 +84,10 @@ allows for greater freedom of development while maintaining API stability. implementations such as the querier and invariants. - `module.go`: The module's implementation of the `AppModule` and `AppModuleBasic` interfaces. -- `simulation.go`: The module's simulation messages and related types (if any). +- `simulation/`: The module's simulation package defines all the required functions +used on the blockchain simulator: randomized genesis state, parameters, weigthed +operations, proposal contents and types decoders. ## Next {hide} -Learn about [interfaces](../interfaces/interfaces-intro.md) {hide} \ No newline at end of file +Learn about [interfaces](../interfaces/interfaces-intro.md) {hide} diff --git a/docs/package.json b/docs/package.json index 06255f68eb6a..4189d8bd368c 100644 --- a/docs/package.json +++ b/docs/package.json @@ -19,4 +19,4 @@ "vuepress-plugin-smooth-scroll": "0.0.9", "vuepress-theme-cosmos": "^1.0.113" } -} \ No newline at end of file +} diff --git a/docs/using-the-sdk/README.md b/docs/using-the-sdk/README.md index 5254bf991ae5..747dfd500553 100644 --- a/docs/using-the-sdk/README.md +++ b/docs/using-the-sdk/README.md @@ -7,4 +7,3 @@ parent: - [Modules](../../x/README.md) - [Simulation](./simulation.md) - diff --git a/docs/using-the-sdk/simulation.md b/docs/using-the-sdk/simulation.md index 05147050933e..e8fcb8088f07 100644 --- a/docs/using-the-sdk/simulation.md +++ b/docs/using-the-sdk/simulation.md @@ -1,49 +1,65 @@ # Cosmos Blockchain Simulator -The Cosmos SDK offers a full fledged simulation framework to fuzz test every message defined by a module. +The Cosmos SDK offers a full fledged simulation framework to fuzz test every +message defined by a module. -This functionality is provided by the[`SimApp`](https://github.com/cosmos/cosmos-sdk/blob/master/simapp/app.go), -which is a dummy application that is used for running the [`simulation`](https://github.com/cosmos/cosmos-sdk/tree/master/x/simulation) module. -This module defines all the simulation logic as well as the operations for randomized parameters like accounts, balances etc. +On the SDK, this functionality is provided by the[`SimApp`](https://github.com/cosmos/cosmos-sdk/blob/master/simapp/app.go), which is a +`Baseapp` application that is used for running the [`simulation`](https://github.com/cosmos/cosmos-sdk/tree/master/x/simulation) module. +This module defines all the simulation logic as well as the operations for +randomized parameters like accounts, balances etc. ## Goals -The blockchain simulator tests how the blockchain application would behave under real life circumstances by generating and sending randomized messages. -The goal of this is to detect and debug failures that could halt a live chain, by providing logs and statistics about the operations run by the simulator as well as exporting the latest application state when a failure was found. +The blockchain simulator tests how the blockchain application would behave under +real life circumstances by generating and sending randomized messages. +The goal of this is to detect and debug failures that could halt a live chain, +by providing logs and statistics about the operations run by the simulator as +well as exporting the latest application state when a failure was found. -Its main difference with integration testing is that the simulator app allows you to pass parameters to customize the chain that's being simulated. -This comes in handy when trying to reproduce bugs that were generated in the provided operations (randomized or not). +Its main difference with integration testing is that the simulator app allows +you to pass parameters to customize the chain that's being simulated. +This comes in handy when trying to reproduce bugs that were generated in the +provided operations (randomized or not). ## Simulation commands -The simulation app has different commands, each of which tests a different failure type: +The simulation app has different commands, each of which tests a different +failure type: -- `AppImportExport`: The simulator exports the initial app state and then it creates a new app with the exported `genesis.json` as an input, checking for inconsistencies between the stores. -- `AppSimulationAfterImport`: Queues two simulations together. The first one provides the app state (_i.e_ genesis) to the second. Useful to test software upgrades or hard-forks from a live chain. -- `AppStateDeterminism`: Checks that all the nodes return the same values, in the same order. -- `BenchmarkInvariants`: Analyses the performance of running all the modules' invariants (_i.e_ secuentially runs a [benchmark](https://golang.org/pkg/testing/#hdr-Benchmarks) test). An invariant checks for differences between the values that are on the store and the passive tracker. Eg: total coins held by accounts vs total supply tracker. -- `FullAppSimulation`: General simulation mode. Runs the chain and the specified operations for a given number of blocks. Tests that there're no `panics` on the simulation. It does also run invariant checks on every `Period` but they are not benchmarked. +* `AppImportExport`: The simulator exports the initial app state and then it +creates a new app with the exported `genesis.json` as an input, checking for +inconsistencies between the stores. +* `AppSimulationAfterImport`: Queues two simulations together. The first one provides the app state (_i.e_ genesis) to the second. Useful to test software upgrades or hard-forks from a live chain. +* `AppStateDeterminism`: Checks that all the nodes return the same values, in the same order. +* `BenchmarkInvariants`: Analysis of the performance of running all modules' invariants (_i.e_ sequentially runs a [benchmark](https://golang.org/pkg/testing/#hdr-Benchmarks) test). An invariant checks for +differences between the values that are on the store and the passive tracker. Eg: total coins held by accounts vs total supply tracker. +* `FullAppSimulation`: General simulation mode. Runs the chain and the specified operations for a given number of blocks. Tests that there're no `panics` on the simulation. It does also run invariant checks on every `Period` but they are not benchmarked. -Each simulation must receive a set of inputs (_i.e_ flags) such as the number of blocks that the simulation is run, seed, block size, etc. +Each simulation must receive a set of inputs (_i.e_ flags) such as the number of +blocks that the simulation is run, seed, block size, etc. Check the full list of flags [here](https://github.com/cosmos/cosmos-sdk/blob/adf6ddd4a807c8363e33083a3281f6a5e112ab89/simapp/sim_test.go#L34-L50). ## Simulator Modes In addition to the various inputs and commands, the simulator runs in three modes: -1. Completely random where the initial state, module parameters and simulation parameters are **pseudo-randomly generated**. +1. Completely random where the initial state, module parameters and simulation +parameters are **pseudo-randomly generated**. 2. From a `genesis.json` file where the initial state and the module parameters are defined. This mode is helpful for running simulations on a known state such as a live network export where a new (mostly likely breaking) version of the application needs to be tested. 3. From a `params.json` file where the initial state is pseudo-randomly generated but the module and simulation parameters can be provided manually. -This allows for a more controlled and deterministic simulation setup while allowing the state space to still be pseudo-randomly simulated. The list of available parameters is listed [here](https://github.com/cosmos/cosmos-sdk/blob/adf6ddd4a807c8363e33083a3281f6a5e112ab89/x/simulation/params.go#L170-L178). +This allows for a more controlled and deterministic simulation setup while allowing the state space to still be pseudo-randomly simulated. +The list of available parameters are listed [here](https://github.com/cosmos/cosmos-sdk/blob/adf6ddd4a807c8363e33083a3281f6a5e112ab89/x/simulation/params.go#L170-L178). ::: tip -These modes are not mutually exclusive. So you can for example run a randomly generated genesis state (`1`) with manually generated simulation params (`3`). +These modes are not mutually exclusive. So you can for example run a randomly +generated genesis state (`1`) with manually generated simulation params (`3`). ::: ## Usage -This is a general example of how simulations are run. For more specific examples check the SDK [Makefile](https://github.com/cosmos/cosmos-sdk/blob/adf6ddd4a807c8363e33083a3281f6a5e112ab89/Makefile#L88-L123). +This is a general example of how simulations are run. For more specific examples +check the SDK [Makefile](https://github.com/cosmos/cosmos-sdk/blob/adf6ddd4a807c8363e33083a3281f6a5e112ab89/Makefile#L88-L123). ```bash $ go test -mod=readonly github.com/cosmos/cosmos-sdk/simapp \ @@ -56,15 +72,26 @@ This is a general example of how simulations are run. For more specific examples Here are some suggestions when encountering a simulation failure: -- Export the app state at the height were the failure was found. You can do this by passing the `-ExportStatePath` flag to the simulator. -- Use `-Verbose` logs. They could give you a better hint on all the operations involved. - -- Reduce the simulation `-Period`. This will run the invariants checks more frequently. -- Print all the failed invariants at once with `-PrintAllInvariants`. -- Try using another `-Seed`. If it can reproduce the same error and if it fails sooner you will spend less time running the simulations. -- Reduce the `-NumBlocks` . How's the app state at the height previous to the failure? -- Run invariants on every operation with `-SimulateEveryOperation`. _Note_: this will slow down your simulation **a lot**. -- Try adding logs to operations that are not logged. You will have to define a [Logger](https://github.com/cosmos/cosmos-sdk/blob/adf6ddd4a807c8363e33083a3281f6a5e112ab89/x/staking/keeper/keeper.go#L65:17) on your `Keeper`. - - - +* Export the app state at the height were the failure was found. You can do this +by passing the `-ExportStatePath` flag to the simulator. +* Use `-Verbose` logs. They could give you a better hint on all the operations +involved. +* Reduce the simulation `-Period`. This will run the invariants checks more +frequently. +* Print all the failed invariants at once with `-PrintAllInvariants`. +* Try using another `-Seed`. If it can reproduce the same error and if it fails +sooner you will spend less time running the simulations. +* Reduce the `-NumBlocks` . How's the app state at the height previous to the +failure? +* Run invariants on every operation with `-SimulateEveryOperation`. _Note_: this +will slow down your simulation **a lot**. +* Try adding logs to operations that are not logged. You will have to define a +[Logger](https://github.com/cosmos/cosmos-sdk/blob/adf6ddd4a807c8363e33083a3281f6a5e112ab89/x/staking/keeper/keeper.go#L65:17) on your `Keeper`. + +## Use simulation in your SDK-based application + +Learn how you can integrate the simulation into your SDK-based application: + +* Application Simulation Manager +* [Building modules: Simulator](../building-modules/simulator.md) +* Simulator tests diff --git a/x/bank/alias.go b/x/bank/alias.go index 2fdfa6602b82..4ddeda71f50e 100644 --- a/x/bank/alias.go +++ b/x/bank/alias.go @@ -51,21 +51,23 @@ var ( ParamStoreKeySendEnabled = types.ParamStoreKeySendEnabled BalancesPrefix = types.BalancesPrefix AddressFromBalancesStore = types.AddressFromBalancesStore + GetGenesisStateFromAppState = types.GetGenesisStateFromAppState ) type ( - Keeper = keeper.Keeper - BaseKeeper = keeper.BaseKeeper - SendKeeper = keeper.SendKeeper - BaseSendKeeper = keeper.BaseSendKeeper - ViewKeeper = keeper.ViewKeeper - BaseViewKeeper = keeper.BaseViewKeeper - GenesisState = types.GenesisState - Balance = types.Balance - MsgSend = types.MsgSend - MsgMultiSend = types.MsgMultiSend - Input = types.Input - Output = types.Output - QueryBalanceParams = types.QueryBalanceParams - QueryAllBalancesParams = types.QueryAllBalancesParams + Keeper = keeper.Keeper + BaseKeeper = keeper.BaseKeeper + SendKeeper = keeper.SendKeeper + BaseSendKeeper = keeper.BaseSendKeeper + ViewKeeper = keeper.ViewKeeper + BaseViewKeeper = keeper.BaseViewKeeper + GenesisState = types.GenesisState + Balance = types.Balance + MsgSend = types.MsgSend + MsgMultiSend = types.MsgMultiSend + Input = types.Input + Output = types.Output + QueryBalanceParams = types.QueryBalanceParams + QueryAllBalancesParams = types.QueryAllBalancesParams + GenesisBalancesIterator = types.GenesisBalancesIterator ) diff --git a/x/ibc/20-transfer/alias.go b/x/ibc/20-transfer/alias.go index d76c3a765e92..09347c55a556 100644 --- a/x/ibc/20-transfer/alias.go +++ b/x/ibc/20-transfer/alias.go @@ -41,7 +41,7 @@ type ( ClientKeeper = types.ClientKeeper ConnectionKeeper = types.ConnectionKeeper SupplyKeeper = types.SupplyKeeper - MsgTransfer = types.MsgTransfer FungibleTokenPacketData = types.FungibleTokenPacketData - AckDataTransfer = types.AckDataTransfer + MsgTransfer = types.MsgTransfer + PacketDataTransfer = types.PacketDataTransfer ) diff --git a/x/ibc/20-transfer/handler.go b/x/ibc/20-transfer/handler.go index dd4b3662cea2..a3b0e3886e4c 100644 --- a/x/ibc/20-transfer/handler.go +++ b/x/ibc/20-transfer/handler.go @@ -87,7 +87,6 @@ func handlePacketDataTransfer( ), ) - // packet receiving should not fail return &sdk.Result{ Events: ctx.EventManager().Events(), }, nil @@ -96,13 +95,18 @@ func handlePacketDataTransfer( // 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) { packet := msg.Packet - if err := k.TimeoutTransfer( - ctx, packet.SourcePort, packet.SourceChannel, packet.DestinationPort, packet.DestinationChannel, data, - ); err != nil { - return nil, err + err := k.TimeoutTransfer(ctx, packet.SourcePort, packet.SourceChannel, packet.DestinationPort, packet.DestinationChannel, data) + if err != nil { + // TODO: This chain sent invalid packet + panic(err) + } + + err = k.TimeoutExecuted(ctx, packet) + if err != nil { + // TODO: This should not happen + panic(err) } - // packet timeout should not fail return &sdk.Result{ Events: ctx.EventManager().Events(), }, nil diff --git a/x/ibc/20-transfer/keeper/keeper.go b/x/ibc/20-transfer/keeper/keeper.go index 126a634e9c8e..4c1bcc3ba0b9 100644 --- a/x/ibc/20-transfer/keeper/keeper.go +++ b/x/ibc/20-transfer/keeper/keeper.go @@ -64,7 +64,7 @@ func (k Keeper) GetTransferAccount(ctx sdk.Context) supplyexported.ModuleAccount } // PacketExecuted defines a wrapper function for the channel Keeper's function -// in order to expose it to the ICS20 trasfer handler. +// in order to expose it to the ICS20 transfer handler. func (k Keeper) PacketExecuted(ctx sdk.Context, packet channelexported.PacketI, acknowledgement channelexported.PacketDataI) error { return k.channelKeeper.PacketExecuted(ctx, packet, acknowledgement) } @@ -74,3 +74,9 @@ func (k Keeper) PacketExecuted(ctx sdk.Context, packet channelexported.PacketI, func (k Keeper) ChanCloseInit(ctx sdk.Context, portID, channelID string) error { return k.channelKeeper.ChanCloseInit(ctx, portID, channelID) } + +// TimeoutExecuted defines a wrapper function for the channel Keeper's function +// in order to expose it to the ICS20 transfer handler. +func (k Keeper) TimeoutExecuted(ctx sdk.Context, packet channelexported.PacketI) error { + return k.channelKeeper.TimeoutExecuted(ctx, packet) +} diff --git a/x/ibc/20-transfer/types/expected_keepers.go b/x/ibc/20-transfer/types/expected_keepers.go index 4a3adef2bca3..14fda1412c93 100644 --- a/x/ibc/20-transfer/types/expected_keepers.go +++ b/x/ibc/20-transfer/types/expected_keepers.go @@ -21,6 +21,7 @@ type ChannelKeeper interface { SendPacket(ctx sdk.Context, packet channelexported.PacketI) error PacketExecuted(ctx sdk.Context, packet channelexported.PacketI, acknowledgement channelexported.PacketDataI) error ChanCloseInit(ctx sdk.Context, portID, channelID string) error + TimeoutExecuted(ctx sdk.Context, packet channelexported.PacketI) error } // ClientKeeper defines the expected IBC client keeper diff --git a/x/staking/keeper/delegation.go b/x/staking/keeper/delegation.go index faf8ba415ef7..219beef71271 100644 --- a/x/staking/keeper/delegation.go +++ b/x/staking/keeper/delegation.go @@ -660,14 +660,17 @@ func (k Keeper) Undelegate( return completionTime, nil } -// CompleteUnbonding completes the unbonding of all mature entries in the -// retrieved unbonding delegation object. -func (k Keeper) CompleteUnbonding(ctx sdk.Context, delAddr sdk.AccAddress, valAddr sdk.ValAddress) error { +// CompleteUnbondingWithAmount completes the unbonding of all mature entries in +// the retrieved unbonding delegation object and returns the total unbonding +// balance or an error upon failure. +func (k Keeper) CompleteUnbondingWithAmount(ctx sdk.Context, delAddr sdk.AccAddress, valAddr sdk.ValAddress) (sdk.Coins, error) { ubd, found := k.GetUnbondingDelegation(ctx, delAddr, valAddr) if !found { - return types.ErrNoUnbondingDelegation + return nil, types.ErrNoUnbondingDelegation } + bondDenom := k.GetParams(ctx).BondDenom + balances := sdk.NewCoins() ctxTime := ctx.BlockHeader().Time // loop through all the entries and complete unbonding mature entries @@ -679,11 +682,15 @@ func (k Keeper) CompleteUnbonding(ctx sdk.Context, delAddr sdk.AccAddress, valAd // track undelegation only when remaining or truncated shares are non-zero if !entry.Balance.IsZero() { - amt := sdk.NewCoins(sdk.NewCoin(k.GetParams(ctx).BondDenom, entry.Balance)) - err := k.supplyKeeper.UndelegateCoinsFromModuleToAccount(ctx, types.NotBondedPoolName, ubd.DelegatorAddress, amt) + amt := sdk.NewCoin(bondDenom, entry.Balance) + err := k.supplyKeeper.UndelegateCoinsFromModuleToAccount( + ctx, types.NotBondedPoolName, ubd.DelegatorAddress, sdk.NewCoins(amt), + ) if err != nil { - return err + return nil, err } + + balances = balances.Add(amt) } } } @@ -695,7 +702,14 @@ func (k Keeper) CompleteUnbonding(ctx sdk.Context, delAddr sdk.AccAddress, valAd k.SetUnbondingDelegation(ctx, ubd) } - return nil + return balances, nil +} + +// CompleteUnbonding performs the same logic as CompleteUnbondingWithAmount except +// it does not return the total unbonding amount. +func (k Keeper) CompleteUnbonding(ctx sdk.Context, delAddr sdk.AccAddress, valAddr sdk.ValAddress) error { + _, err := k.CompleteUnbondingWithAmount(ctx, delAddr, valAddr) + return err } // begin unbonding / redelegation; create a redelegation record @@ -755,17 +769,20 @@ func (k Keeper) BeginRedelegation( return completionTime, nil } -// CompleteRedelegation completes the unbonding of all mature entries in the -// retrieved unbonding delegation object. -func (k Keeper) CompleteRedelegation( +// CompleteRedelegationWithAmount completes the redelegations of all mature entries in the +// retrieved redelegation object and returns the total redelegation (initial) +// balance or an error upon failure. +func (k Keeper) CompleteRedelegationWithAmount( ctx sdk.Context, delAddr sdk.AccAddress, valSrcAddr, valDstAddr sdk.ValAddress, -) error { +) (sdk.Coins, error) { red, found := k.GetRedelegation(ctx, delAddr, valSrcAddr, valDstAddr) if !found { - return types.ErrNoRedelegation + return nil, types.ErrNoRedelegation } + bondDenom := k.GetParams(ctx).BondDenom + balances := sdk.NewCoins() ctxTime := ctx.BlockHeader().Time // loop through all the entries and complete mature redelegation entries @@ -774,6 +791,10 @@ func (k Keeper) CompleteRedelegation( if entry.IsMature(ctxTime) { red.RemoveEntry(int64(i)) i-- + + if !entry.InitialBalance.IsZero() { + balances = balances.Add(sdk.NewCoin(bondDenom, entry.InitialBalance)) + } } } @@ -784,7 +805,17 @@ func (k Keeper) CompleteRedelegation( k.SetRedelegation(ctx, red) } - return nil + return balances, nil +} + +// CompleteRedelegation performs the same logic as CompleteRedelegationWithAmount +// except it does not return the total redelegation amount. +func (k Keeper) CompleteRedelegation( + ctx sdk.Context, delAddr sdk.AccAddress, valSrcAddr, valDstAddr sdk.ValAddress, +) error { + + _, err := k.CompleteRedelegationWithAmount(ctx, delAddr, valSrcAddr, valDstAddr) + return err } // ValidateUnbondAmount validates that a given unbond or redelegation amount is diff --git a/x/staking/keeper/val_state_change.go b/x/staking/keeper/val_state_change.go index ed630f15ce22..101bc139215a 100644 --- a/x/staking/keeper/val_state_change.go +++ b/x/staking/keeper/val_state_change.go @@ -31,7 +31,7 @@ func (k Keeper) BlockValidatorUpdates(ctx sdk.Context) []abci.ValidatorUpdate { // Remove all mature unbonding delegations from the ubd queue. matureUnbonds := k.DequeueAllMatureUBDQueue(ctx, ctx.BlockHeader().Time) for _, dvPair := range matureUnbonds { - err := k.CompleteUnbonding(ctx, dvPair.DelegatorAddress, dvPair.ValidatorAddress) + balances, err := k.CompleteUnbondingWithAmount(ctx, dvPair.DelegatorAddress, dvPair.ValidatorAddress) if err != nil { continue } @@ -39,6 +39,7 @@ func (k Keeper) BlockValidatorUpdates(ctx sdk.Context) []abci.ValidatorUpdate { ctx.EventManager().EmitEvent( sdk.NewEvent( types.EventTypeCompleteUnbonding, + sdk.NewAttribute(sdk.AttributeKeyAmount, balances.String()), sdk.NewAttribute(types.AttributeKeyValidator, dvPair.ValidatorAddress.String()), sdk.NewAttribute(types.AttributeKeyDelegator, dvPair.DelegatorAddress.String()), ), @@ -48,8 +49,12 @@ func (k Keeper) BlockValidatorUpdates(ctx sdk.Context) []abci.ValidatorUpdate { // Remove all mature redelegations from the red queue. matureRedelegations := k.DequeueAllMatureRedelegationQueue(ctx, ctx.BlockHeader().Time) for _, dvvTriplet := range matureRedelegations { - err := k.CompleteRedelegation(ctx, dvvTriplet.DelegatorAddress, - dvvTriplet.ValidatorSrcAddress, dvvTriplet.ValidatorDstAddress) + balances, err := k.CompleteRedelegationWithAmount( + ctx, + dvvTriplet.DelegatorAddress, + dvvTriplet.ValidatorSrcAddress, + dvvTriplet.ValidatorDstAddress, + ) if err != nil { continue } @@ -57,6 +62,7 @@ func (k Keeper) BlockValidatorUpdates(ctx sdk.Context) []abci.ValidatorUpdate { ctx.EventManager().EmitEvent( sdk.NewEvent( types.EventTypeCompleteRedelegation, + sdk.NewAttribute(sdk.AttributeKeyAmount, balances.String()), sdk.NewAttribute(types.AttributeKeyDelegator, dvvTriplet.DelegatorAddress.String()), sdk.NewAttribute(types.AttributeKeySrcValidator, dvvTriplet.ValidatorSrcAddress.String()), sdk.NewAttribute(types.AttributeKeyDstValidator, dvvTriplet.ValidatorDstAddress.String()), diff --git a/x/staking/spec/07_events.md b/x/staking/spec/07_events.md index e16cf009570d..fe716baf9180 100644 --- a/x/staking/spec/07_events.md +++ b/x/staking/spec/07_events.md @@ -8,20 +8,22 @@ The staking module emits the following events: ## EndBlocker -| Type | Attribute Key | Attribute Value | -|-----------------------|-----------------------|-----------------------| -| complete_unbonding | validator | {validatorAddress} | -| complete_unbonding | delegator | {delegatorAddress} | -| complete_redelegation | source_validator | {srcValidatorAddress} | -| complete_redelegation | destination_validator | {dstValidatorAddress} | -| complete_redelegation | delegator | {delegatorAddress} | +| Type | Attribute Key | Attribute Value | +| --------------------- | --------------------- | ------------------------- | +| complete_unbonding | amount | {totalUnbondingAmount} | +| complete_unbonding | validator | {validatorAddress} | +| complete_unbonding | delegator | {delegatorAddress} | +| complete_redelegation | amount | {totalRedelegationAmount} | +| complete_redelegation | source_validator | {srcValidatorAddress} | +| complete_redelegation | destination_validator | {dstValidatorAddress} | +| complete_redelegation | delegator | {delegatorAddress} | ## Handlers ### MsgCreateValidator | Type | Attribute Key | Attribute Value | -|------------------|---------------|--------------------| +| ---------------- | ------------- | ------------------ | | create_validator | validator | {validatorAddress} | | create_validator | amount | {delegationAmount} | | message | module | staking | @@ -31,7 +33,7 @@ The staking module emits the following events: ### MsgEditValidator | Type | Attribute Key | Attribute Value | -|----------------|---------------------|---------------------| +| -------------- | ------------------- | ------------------- | | edit_validator | commission_rate | {commissionRate} | | edit_validator | min_self_delegation | {minSelfDelegation} | | message | module | staking | @@ -41,7 +43,7 @@ The staking module emits the following events: ### MsgDelegate | Type | Attribute Key | Attribute Value | -|----------|---------------|--------------------| +| -------- | ------------- | ------------------ | | delegate | validator | {validatorAddress} | | delegate | amount | {delegationAmount} | | message | module | staking | @@ -51,7 +53,7 @@ The staking module emits the following events: ### MsgUndelegate | Type | Attribute Key | Attribute Value | -|---------|---------------------|--------------------| +| ------- | ------------------- | ------------------ | | unbond | validator | {validatorAddress} | | unbond | amount | {unbondAmount} | | unbond | completion_time [0] | {completionTime} | @@ -64,7 +66,7 @@ The staking module emits the following events: ### MsgBeginRedelegate | Type | Attribute Key | Attribute Value | -|------------|-----------------------|-----------------------| +| ---------- | --------------------- | --------------------- | | redelegate | source_validator | {srcValidatorAddress} | | redelegate | destination_validator | {dstValidatorAddress} | | redelegate | amount | {unbondAmount} | From 7dfd6b9ccb87f7802f796c2c61b385f499108d76 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Tue, 18 Feb 2020 22:14:36 +0100 Subject: [PATCH 05/16] Merge PR #5603: Remove acknowledgement interface in favour of []byte --- x/ibc/04-channel/alias.go | 1 + x/ibc/04-channel/exported/exported.go | 5 +++++ x/ibc/04-channel/keeper/packet.go | 4 ++-- x/ibc/04-channel/types/codec.go | 1 + x/ibc/04-channel/types/errors.go | 1 + x/ibc/04-channel/types/msgs.go | 16 ++++++++-------- x/ibc/04-channel/types/msgs_test.go | 11 ++++++++++- x/ibc/04-channel/types/packet.go | 2 +- x/ibc/20-transfer/alias.go | 2 +- x/ibc/20-transfer/handler.go | 7 +++++-- x/ibc/20-transfer/keeper/keeper.go | 2 +- x/ibc/20-transfer/keeper/relay.go | 3 +-- x/ibc/20-transfer/types/expected_keepers.go | 2 +- x/ibc/20-transfer/types/packet.go | 21 +++------------------ 14 files changed, 41 insertions(+), 37 deletions(-) diff --git a/x/ibc/04-channel/alias.go b/x/ibc/04-channel/alias.go index cf71557f2ecc..c24c077d9c60 100644 --- a/x/ibc/04-channel/alias.go +++ b/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 diff --git a/x/ibc/04-channel/exported/exported.go b/x/ibc/04-channel/exported/exported.go index ad4f75cacfa8..ba9dc7efdf6b 100644 --- a/x/ibc/04-channel/exported/exported.go +++ b/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 diff --git a/x/ibc/04-channel/keeper/packet.go b/x/ibc/04-channel/keeper/packet.go index 0ba1186ccd4a..9cac26dec99e 100644 --- a/x/ibc/04-channel/keeper/packet.go +++ b/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) { diff --git a/x/ibc/04-channel/types/codec.go b/x/ibc/04-channel/types/codec.go index 2bb785418b67..e8bb91bf33be 100644 --- a/x/ibc/04-channel/types/codec.go +++ b/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) diff --git a/x/ibc/04-channel/types/errors.go b/x/ibc/04-channel/types/errors.go index 9e6ee6fcf74c..a184314bbd56 100644 --- a/x/ibc/04-channel/types/errors.go +++ b/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") ) diff --git a/x/ibc/04-channel/types/msgs.go b/x/ibc/04-channel/types/msgs.go index 966b982b2ad2..3f52db670f97 100644 --- a/x/ibc/04-channel/types/msgs.go +++ b/x/ibc/04-channel/types/msgs.go @@ -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 } diff --git a/x/ibc/04-channel/types/msgs_test.go b/x/ibc/04-channel/types/msgs_test.go index 0e43f58d503b..f071b7a17fcb 100644 --- a/x/ibc/04-channel/types/msgs_test.go +++ b/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), } diff --git a/x/ibc/04-channel/types/packet.go b/x/ibc/04-channel/types/packet.go index df9b78dd0cd5..cd5d1e271c5f 100644 --- a/x/ibc/04-channel/types/packet.go +++ b/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()) } diff --git a/x/ibc/20-transfer/alias.go b/x/ibc/20-transfer/alias.go index 09347c55a556..033d9b18611a 100644 --- a/x/ibc/20-transfer/alias.go +++ b/x/ibc/20-transfer/alias.go @@ -43,5 +43,5 @@ type ( SupplyKeeper = types.SupplyKeeper FungibleTokenPacketData = types.FungibleTokenPacketData MsgTransfer = types.MsgTransfer - PacketDataTransfer = types.PacketDataTransfer + AckDataTransfer = types.AckDataTransfer ) diff --git a/x/ibc/20-transfer/handler.go b/x/ibc/20-transfer/handler.go index a3b0e3886e4c..44ba3a60cf4b 100644 --- a/x/ibc/20-transfer/handler.go +++ b/x/ibc/20-transfer/handler.go @@ -64,6 +64,8 @@ func handlePacketDataTransfer( if err := k.ReceiveTransfer( ctx, packet.SourcePort, packet.SourceChannel, packet.DestinationPort, packet.DestinationChannel, data, ); err != nil { + // 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 @@ -97,13 +99,14 @@ func handleTimeoutDataTransfer(ctx sdk.Context, k Keeper, msg channeltypes.MsgTi packet := msg.Packet err := k.TimeoutTransfer(ctx, packet.SourcePort, packet.SourceChannel, packet.DestinationPort, packet.DestinationChannel, data) if err != nil { - // TODO: This chain sent invalid packet + // This shouldn't happen, since we've already validated that we've sent the packet. panic(err) } err = k.TimeoutExecuted(ctx, packet) if err != nil { - // TODO: This should not happen + // 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) } diff --git a/x/ibc/20-transfer/keeper/keeper.go b/x/ibc/20-transfer/keeper/keeper.go index 4c1bcc3ba0b9..e5225cbe7137 100644 --- a/x/ibc/20-transfer/keeper/keeper.go +++ b/x/ibc/20-transfer/keeper/keeper.go @@ -65,7 +65,7 @@ func (k Keeper) GetTransferAccount(ctx sdk.Context) supplyexported.ModuleAccount // PacketExecuted defines a wrapper function for the channel Keeper's function // in order to expose it to the ICS20 transfer handler. -func (k Keeper) PacketExecuted(ctx sdk.Context, packet channelexported.PacketI, acknowledgement channelexported.PacketDataI) error { +func (k Keeper) PacketExecuted(ctx sdk.Context, packet channelexported.PacketI, acknowledgement channelexported.PacketAcknowledgementI) error { return k.channelKeeper.PacketExecuted(ctx, packet, acknowledgement) } diff --git a/x/ibc/20-transfer/keeper/relay.go b/x/ibc/20-transfer/keeper/relay.go index 06073cd7efb9..0d29dcb7826f 100644 --- a/x/ibc/20-transfer/keeper/relay.go +++ b/x/ibc/20-transfer/keeper/relay.go @@ -81,8 +81,6 @@ func (k Keeper) ReceiveTransfer( return k.supplyKeeper.SendCoinsFromModuleToAccount(ctx, types.GetModuleAccountName(), data.Receiver, data.Amount) } - // unescrow tokens - // check the denom prefix prefix := types.GetDenomPrefix(sourcePort, sourceChannel) coins := make(sdk.Coins, len(data.Amount)) @@ -96,6 +94,7 @@ func (k Keeper) ReceiveTransfer( coins[i] = sdk.NewCoin(coin.Denom[len(prefix):], coin.Amount) } + // unescrow tokens escrowAddress := types.GetEscrowAddress(destinationPort, destinationChannel) return k.bankKeeper.SendCoins(ctx, escrowAddress, data.Receiver, coins) diff --git a/x/ibc/20-transfer/types/expected_keepers.go b/x/ibc/20-transfer/types/expected_keepers.go index 14fda1412c93..a347fb4e476f 100644 --- a/x/ibc/20-transfer/types/expected_keepers.go +++ b/x/ibc/20-transfer/types/expected_keepers.go @@ -19,7 +19,7 @@ type ChannelKeeper interface { GetChannel(ctx sdk.Context, srcPort, srcChan string) (channel channel.Channel, found bool) GetNextSequenceSend(ctx sdk.Context, portID, channelID string) (uint64, bool) SendPacket(ctx sdk.Context, packet channelexported.PacketI) error - PacketExecuted(ctx sdk.Context, packet channelexported.PacketI, acknowledgement channelexported.PacketDataI) error + PacketExecuted(ctx sdk.Context, packet channelexported.PacketI, acknowledgement channelexported.PacketAcknowledgementI) error ChanCloseInit(ctx sdk.Context, portID, channelID string) error TimeoutExecuted(ctx sdk.Context, packet channelexported.PacketI) error } diff --git a/x/ibc/20-transfer/types/packet.go b/x/ibc/20-transfer/types/packet.go index 7ff77f4384cb..edee9f60ae82 100644 --- a/x/ibc/20-transfer/types/packet.go +++ b/x/ibc/20-transfer/types/packet.go @@ -83,28 +83,13 @@ func (ftpd FungibleTokenPacketData) Type() string { return "ics20/transfer" } -var _ channelexported.PacketDataI = AckDataTransfer{} +var _ channelexported.PacketAcknowledgementI = AckDataTransfer{} // AckDataTransfer is a no-op packet // See spec for onAcknowledgePacket: https://github.com/cosmos/ics/tree/master/spec/ics-020-fungible-token-transfer#packet-relay type AckDataTransfer struct{} -// ValidateBasic implements channelexported.PacketDataI -func (ack AckDataTransfer) ValidateBasic() error { - return nil -} - -// GetBytes implements channelexported.PacketDataI +// GetBytes implements channelexported.PacketAcknowledgementI func (ack AckDataTransfer) GetBytes() []byte { - return []byte("ok") -} - -// GetTimeoutHeight implements channelexported.PacketDataI -func (ack AckDataTransfer) GetTimeoutHeight() uint64 { - return 0 -} - -// Type implements channelexported.PacketDataI -func (ack AckDataTransfer) Type() string { - return "ics20/transfer/ack" + return []byte("fungible token transfer ack") } From 6f5ad5cc7835375e500c0f6fc48a7309f687e987 Mon Sep 17 00:00:00 2001 From: Federico Kunze Date: Wed, 19 Feb 2020 10:49:33 +0100 Subject: [PATCH 06/16] fixes and cleanup --- x/ibc/04-channel/keeper/packet.go | 2 +- x/ibc/20-transfer/client/rest/swagger.go | 11 +----- x/ibc/20-transfer/handler.go | 4 +- x/ibc/20-transfer/keeper/keeper_test.go | 45 ++++++++++++----------- x/ibc/20-transfer/keeper/relay.go | 5 +-- x/ibc/20-transfer/keeper/relay_test.go | 47 ------------------------ x/ibc/20-transfer/types/msgs.go | 2 +- x/ibc/20-transfer/types/packet_test.go | 4 +- x/ibc/ante/ante.go | 2 +- 9 files changed, 35 insertions(+), 87 deletions(-) diff --git a/x/ibc/04-channel/keeper/packet.go b/x/ibc/04-channel/keeper/packet.go index 9cac26dec99e..614da9d0ebcf 100644 --- a/x/ibc/04-channel/keeper/packet.go +++ b/x/ibc/04-channel/keeper/packet.go @@ -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") } diff --git a/x/ibc/20-transfer/client/rest/swagger.go b/x/ibc/20-transfer/client/rest/swagger.go index 5714d06dcaaf..1514502c6b66 100644 --- a/x/ibc/20-transfer/client/rest/swagger.go +++ b/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"` } ) diff --git a/x/ibc/20-transfer/handler.go b/x/ibc/20-transfer/handler.go index 44ba3a60cf4b..8a5c685e9bed 100644 --- a/x/ibc/20-transfer/handler.go +++ b/x/ibc/20-transfer/handler.go @@ -8,7 +8,6 @@ import ( ) // NewHandler returns sdk.Handler for IBC token transfer module messages -// See NewHandler function in ADR15: https://github.com/cosmos/ics/tree/master/spec/ics-020-fungible-token-transfer#packet-relay func NewHandler(k Keeper) sdk.Handler { return func(ctx sdk.Context, msg sdk.Msg) (*sdk.Result, error) { switch msg := msg.(type) { @@ -64,7 +63,8 @@ func handlePacketDataTransfer( if err := k.ReceiveTransfer( ctx, packet.SourcePort, packet.SourceChannel, packet.DestinationPort, packet.DestinationChannel, data, ); err != nil { - // How do we want to handle this case? Maybe we should be more lenient, it's safe to leave the channel open I think. + // 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 diff --git a/x/ibc/20-transfer/keeper/keeper_test.go b/x/ibc/20-transfer/keeper/keeper_test.go index b3d72cca2919..603b1eba60a7 100644 --- a/x/ibc/20-transfer/keeper/keeper_test.go +++ b/x/ibc/20-transfer/keeper/keeper_test.go @@ -18,7 +18,7 @@ import ( connectionexported "github.com/cosmos/cosmos-sdk/x/ibc/03-connection/exported" channel "github.com/cosmos/cosmos-sdk/x/ibc/04-channel" channelexported "github.com/cosmos/cosmos-sdk/x/ibc/04-channel/exported" - tendermint "github.com/cosmos/cosmos-sdk/x/ibc/07-tendermint" + ibctmtypes "github.com/cosmos/cosmos-sdk/x/ibc/07-tendermint/types" "github.com/cosmos/cosmos-sdk/x/ibc/20-transfer/types" commitment "github.com/cosmos/cosmos-sdk/x/ibc/23-commitment" ibctypes "github.com/cosmos/cosmos-sdk/x/ibc/types" @@ -79,21 +79,6 @@ func (suite *KeeperTestSuite) SetupTest() { suite.createConnection(connectionexported.OPEN) } -func (suite *KeeperTestSuite) TestGetTransferAccount() { - expectedMaccName := types.GetModuleAccountName() - expectedMaccAddr := sdk.AccAddress(crypto.AddressHash([]byte(expectedMaccName))) - - macc := suite.app.TransferKeeper.GetTransferAccount(suite.ctx) - - suite.NotNil(macc) - suite.Equal(expectedMaccName, macc.GetName()) - suite.Equal(expectedMaccAddr, macc.GetAddress()) -} - -func TestKeeperTestSuite(t *testing.T) { - suite.Run(t, new(KeeperTestSuite)) -} - func (suite *KeeperTestSuite) createClient() { suite.app.Commit() commitID := suite.app.LastCommitID() @@ -101,15 +86,18 @@ func (suite *KeeperTestSuite) createClient() { suite.app.BeginBlock(abci.RequestBeginBlock{Header: abci.Header{Height: suite.app.LastBlockHeight() + 1}}) suite.ctx = suite.app.BaseApp.NewContext(false, abci.Header{}) - consensusState := tendermint.ConsensusState{ - Root: commitment.NewRoot(commitID.Hash), - ValidatorSetHash: suite.valSet.Hash(), + consensusState := ibctmtypes.ConsensusState{ + Root: commitment.NewRoot(commitID.Hash), + ValidatorSet: suite.valSet, } - _, err := suite.app.IBCKeeper.ClientKeeper.CreateClient(suite.ctx, testClient, testClientType, consensusState) + clientState, err := ibctmtypes.Initialize(testClient, testClient, consensusState, trustingPeriod, ubdPeriod) + suite.NoError(err) + _, err = suite.app.IBCKeeper.ClientKeeper.CreateClient(suite.ctx, clientState, consensusState) suite.NoError(err) } +// nolint: unused func (suite *KeeperTestSuite) updateClient() { // always commit and begin a new block on updateClient suite.app.Commit() @@ -118,7 +106,7 @@ func (suite *KeeperTestSuite) updateClient() { suite.app.BeginBlock(abci.RequestBeginBlock{Header: abci.Header{Height: suite.app.LastBlockHeight() + 1}}) suite.ctx = suite.app.BaseApp.NewContext(false, abci.Header{}) - state := tendermint.ConsensusState{ + state := ibctmtypes.ConsensusState{ Root: commitment.NewRoot(commitID.Hash), } @@ -169,3 +157,18 @@ func (suite *KeeperTestSuite) queryProof(key []byte) (proof commitment.Proof, he return } + +func (suite *KeeperTestSuite) TestGetTransferAccount() { + expectedMaccName := types.GetModuleAccountName() + expectedMaccAddr := sdk.AccAddress(crypto.AddressHash([]byte(expectedMaccName))) + + macc := suite.app.TransferKeeper.GetTransferAccount(suite.ctx) + + suite.NotNil(macc) + suite.Equal(expectedMaccName, macc.GetName()) + suite.Equal(expectedMaccAddr, macc.GetAddress()) +} + +func TestKeeperTestSuite(t *testing.T) { + suite.Run(t, new(KeeperTestSuite)) +} diff --git a/x/ibc/20-transfer/keeper/relay.go b/x/ibc/20-transfer/keeper/relay.go index 0d29dcb7826f..04a1a14eaaae 100644 --- a/x/ibc/20-transfer/keeper/relay.go +++ b/x/ibc/20-transfer/keeper/relay.go @@ -1,7 +1,6 @@ package keeper import ( - "fmt" "strings" sdk "github.com/cosmos/cosmos-sdk/types" @@ -154,7 +153,6 @@ func (k Keeper) createOutgoingPacket( // construct receiving denominations, check correctness prefix := types.GetDenomPrefix(destinationPort, destinationChannel) - fmt.Println(prefix) coins := make(sdk.Coins, len(amount)) for i, coin := range amount { if !strings.HasPrefix(coin.Denom, prefix) { @@ -166,13 +164,12 @@ func (k Keeper) createOutgoingPacket( coins[i] = sdk.NewCoin(coin.Denom[len(prefix):], coin.Amount) } - // escrow source tokens (assumed to fail if balance insufficient) + // escrow source tokens. It fails if balance insufficient. if err := k.bankKeeper.SendCoins( ctx, sender, escrowAddress, coins, ); err != nil { return err } - } else { // burn vouchers from the sender's balance if the source is from another chain // construct receiving denomination, check correctness diff --git a/x/ibc/20-transfer/keeper/relay_test.go b/x/ibc/20-transfer/keeper/relay_test.go index aac75ffdea80..102a028a8fda 100644 --- a/x/ibc/20-transfer/keeper/relay_test.go +++ b/x/ibc/20-transfer/keeper/relay_test.go @@ -5,56 +5,9 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" channelexported "github.com/cosmos/cosmos-sdk/x/ibc/04-channel/exported" - ibctmtypes "github.com/cosmos/cosmos-sdk/x/ibc/07-tendermint/types" "github.com/cosmos/cosmos-sdk/x/ibc/20-transfer/types" ) -func (suite *KeeperTestSuite) createClient() { - suite.app.Commit() - commitID := suite.app.LastCommitID() - - suite.app.BeginBlock(abci.RequestBeginBlock{Header: abci.Header{Height: suite.app.LastBlockHeight() + 1}}) - suite.ctx = suite.app.BaseApp.NewContext(false, abci.Header{}) - - consensusState := ibctmtypes.ConsensusState{ - Root: commitment.NewRoot(commitID.Hash), - ValidatorSet: suite.valSet, - } - - clientState, err := ibctmtypes.Initialize(testClient, testClient, consensusState, trustingPeriod, ubdPeriod) - suite.NoError(err) - _, err = suite.app.IBCKeeper.ClientKeeper.CreateClient(suite.ctx, clientState, consensusState) - suite.NoError(err) -} - -func (suite *KeeperTestSuite) updateClient() { - // always commit and begin a new block on updateClient - suite.app.Commit() - commitID := suite.app.LastCommitID() - - suite.app.BeginBlock(abci.RequestBeginBlock{Header: abci.Header{Height: suite.app.LastBlockHeight() + 1}}) - suite.ctx = suite.app.BaseApp.NewContext(false, abci.Header{}) - - state := ibctmtypes.ConsensusState{ - Root: commitment.NewRoot(commitID.Hash), - } - - suite.app.IBCKeeper.ClientKeeper.SetClientConsensusState(suite.ctx, testClient, 1, state) -} - -func (suite *KeeperTestSuite) createConnection(state connectionexported.State) { - connection := connection.ConnectionEnd{ - State: state, - ClientID: testClient, - Counterparty: connection.Counterparty{ - ClientID: testClient, - ConnectionID: testConnection, - Prefix: suite.app.IBCKeeper.ConnectionKeeper.GetCommitmentPrefix(), - }, - Versions: connection.GetCompatibleVersions(), - } -} - func (suite *KeeperTestSuite) TestSendTransfer() { testCases := []struct { msg string diff --git a/x/ibc/20-transfer/types/msgs.go b/x/ibc/20-transfer/types/msgs.go index ad3732fec518..8bdd20c4a8f2 100644 --- a/x/ibc/20-transfer/types/msgs.go +++ b/x/ibc/20-transfer/types/msgs.go @@ -7,7 +7,7 @@ import ( ibctypes "github.com/cosmos/cosmos-sdk/x/ibc/types" ) -// MsgTransfer is for transfering packets between ICS20 enabled chains +// MsgTransfer defines a msg to transfer fungible tokens (i.e Coins) between ICS20 enabled chains. // See ICS Spec here: https://github.com/cosmos/ics/tree/master/spec/ics-020-fungible-token-transfer#data-structures type MsgTransfer struct { SourcePort string `json:"source_port" yaml:"source_port"` // the port on which the packet will be sent diff --git a/x/ibc/20-transfer/types/packet_test.go b/x/ibc/20-transfer/types/packet_test.go index adc86ff42b11..fc188708e60f 100644 --- a/x/ibc/20-transfer/types/packet_test.go +++ b/x/ibc/20-transfer/types/packet_test.go @@ -13,7 +13,8 @@ func TestFungibleTokenPacketDataValidateBasic(t *testing.T) { NewFungibleTokenPacketData(invalidDenomCoins, addr1, addr2, true, 100), // invalid amount NewFungibleTokenPacketData(negativeCoins, addr1, addr2, false, 100), // amount contains negative coin NewFungibleTokenPacketData(coins, emptyAddr, addr2, false, 100), // missing sender address - NewFungibleTokenPacketData(coins, addr1, emptyAddr, false, 100), // missing recipient address + NewFungibleTokenPacketData(coins, addr1, emptyAddr, false, 100), + NewFungibleTokenPacketData(coins, addr1, emptyAddr, false, 0), // missing recipient address } testCases := []struct { @@ -26,6 +27,7 @@ func TestFungibleTokenPacketDataValidateBasic(t *testing.T) { {testPacketDataTransfer[2], false, "amount contains negative coin"}, {testPacketDataTransfer[3], false, "missing sender address"}, {testPacketDataTransfer[4], false, "missing recipient address"}, + {testPacketDataTransfer[5], false, "timeout is 0"}, } for i, tc := range testCases { diff --git a/x/ibc/ante/ante.go b/x/ibc/ante/ante.go index 97f9a99a630f..4301e95c5a10 100644 --- a/x/ibc/ante/ante.go +++ b/x/ibc/ante/ante.go @@ -34,7 +34,7 @@ func (pvr ProofVerificationDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, sim case channel.MsgPacket: _, err = pvr.channelKeeper.RecvPacket(ctx, msg.Packet, msg.Proof, msg.ProofHeight) case channel.MsgAcknowledgement: - _, err = pvr.channelKeeper.AcknowledgePacket(ctx, msg.Packet, msg.Acknowledgement.GetBytes(), msg.Proof, msg.ProofHeight) + _, err = pvr.channelKeeper.AcknowledgePacket(ctx, msg.Packet, msg.Acknowledgement, msg.Proof, msg.ProofHeight) case channel.MsgTimeout: _, err = pvr.channelKeeper.TimeoutPacket(ctx, msg.Packet, msg.Proof, msg.ProofHeight, msg.NextSequenceRecv) } From f56fd3107fd6903c3371429fb4654d95a02885f3 Mon Sep 17 00:00:00 2001 From: Federico Kunze Date: Wed, 19 Feb 2020 11:50:42 +0100 Subject: [PATCH 07/16] spec compliance --- x/ibc/20-transfer/handler.go | 26 ++-- x/ibc/20-transfer/keeper/relay.go | 171 +++++++++++++------------ x/ibc/20-transfer/keeper/relay_test.go | 36 +++--- 3 files changed, 120 insertions(+), 113 deletions(-) diff --git a/x/ibc/20-transfer/handler.go b/x/ibc/20-transfer/handler.go index 6e9be274ac72..f5dc5eaf54f6 100644 --- a/x/ibc/20-transfer/handler.go +++ b/x/ibc/20-transfer/handler.go @@ -15,15 +15,15 @@ func NewHandler(k Keeper) sdk.Handler { return handleMsgTransfer(ctx, k, msg) case channeltypes.MsgPacket: switch data := msg.Data.(type) { - case FungibleTokenPacketData: // i.e fulfills the Data interface - return handlePacketDataTransfer(ctx, k, msg, data) + case FungibleTokenPacketData: + return handlePacketDataTransfer(ctx, k, msg) default: - return nil, sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "unrecognized ICS-20 transfer 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 FungibleTokenPacketData: - return handleTimeoutDataTransfer(ctx, k, msg, data) + return handleTimeoutDataTransfer(ctx, k, msg) default: return nil, sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "unrecognized ICS-20 transfer packet data type: %T", data) } @@ -57,12 +57,9 @@ func handleMsgTransfer(ctx sdk.Context, k Keeper, msg MsgTransfer) (*sdk.Result, // 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, + ctx sdk.Context, k Keeper, msg channeltypes.MsgPacket, ) (*sdk.Result, error) { - packet := msg.Packet - if err := k.ReceiveTransfer( - ctx, packet.SourcePort, packet.SourceChannel, packet.DestinationPort, packet.DestinationChannel, data, - ); err != nil { + if err := k.ReceiveTransfer(ctx, msg.Packet); 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. @@ -70,14 +67,14 @@ func handlePacketDataTransfer( // the receiving chain couldn't process the transfer // source chain sent invalid packet, shutdown our channel end - if err := k.ChanCloseInit(ctx, packet.DestinationPort, packet.DestinationChannel); err != nil { + if err := k.ChanCloseInit(ctx, msg.Packet.DestinationPort, msg.Packet.DestinationChannel); err != nil { return nil, err } return nil, err } acknowledgement := AckDataTransfer{} - if err := k.PacketExecuted(ctx, packet, acknowledgement); err != nil { + if err := k.PacketExecuted(ctx, msg.Packet, acknowledgement); err != nil { return nil, err } @@ -95,15 +92,14 @@ func handlePacketDataTransfer( } // 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) { - packet := msg.Packet - err := k.TimeoutTransfer(ctx, packet.SourcePort, packet.SourceChannel, packet.DestinationPort, packet.DestinationChannel, data) +func handleTimeoutDataTransfer(ctx sdk.Context, k Keeper, msg channeltypes.MsgTimeout) (*sdk.Result, error) { + err := k.TimeoutTransfer(ctx, msg.Packet) if err != nil { // This shouldn't happen, since we've already validated that we've sent the packet. panic(err) } - err = k.TimeoutExecuted(ctx, packet) + err = k.TimeoutExecuted(ctx, msg.Packet) if 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. diff --git a/x/ibc/20-transfer/keeper/relay.go b/x/ibc/20-transfer/keeper/relay.go index fe193364412d..1a3ef0d7dc8d 100644 --- a/x/ibc/20-transfer/keeper/relay.go +++ b/x/ibc/20-transfer/keeper/relay.go @@ -50,89 +50,14 @@ func (k Keeper) SendTransfer( return k.createOutgoingPacket(ctx, sequence, sourcePort, sourceChannel, destinationPort, destinationChannel, destHeight, coins, sender, receiver, isSourceChain) } -// ReceiveTransfer handles transfer receiving logic -func (k Keeper) ReceiveTransfer( - ctx sdk.Context, - sourcePort, - sourceChannel, - destinationPort, - destinationChannel string, - data types.FungibleTokenPacketData, -) error { - if data.Source { - prefix := types.GetDenomPrefix(destinationPort, destinationChannel) - for _, coin := range data.Amount { - if !strings.HasPrefix(coin.Denom, prefix) { - return sdkerrors.Wrapf( - sdkerrors.ErrInvalidCoins, - "%s doesn't contain the prefix '%s'", coin.Denom, prefix, - ) - } - } - - // mint new tokens if the source of the transfer is the same chain - if err := k.supplyKeeper.MintCoins( - ctx, types.GetModuleAccountName(), data.Amount, - ); err != nil { - return err - } - - // send to receiver - return k.supplyKeeper.SendCoinsFromModuleToAccount(ctx, types.GetModuleAccountName(), data.Receiver, data.Amount) - } - - // check the denom prefix - prefix := types.GetDenomPrefix(sourcePort, sourceChannel) - coins := make(sdk.Coins, len(data.Amount)) - for i, coin := range data.Amount { - if !strings.HasPrefix(coin.Denom, prefix) { - return sdkerrors.Wrapf( - sdkerrors.ErrInvalidCoins, - "%s doesn't contain the prefix '%s'", coin.Denom, prefix, - ) - } - coins[i] = sdk.NewCoin(coin.Denom[len(prefix):], coin.Amount) - } - - // unescrow tokens - escrowAddress := types.GetEscrowAddress(destinationPort, destinationChannel) - return k.bankKeeper.SendCoins(ctx, escrowAddress, data.Receiver, coins) - +// ReceiveTransfer handles transfer receiving logic. +func (k Keeper) ReceiveTransfer(ctx sdk.Context, packet channel.Packet) error { + return k.onRecvPacket(ctx, packet) } // TimeoutTransfer handles transfer timeout logic -func (k Keeper) TimeoutTransfer( - ctx sdk.Context, - sourcePort, - sourceChannel, - destinationPort, - destinationChannel string, - data types.FungibleTokenPacketData, -) error { - // check the denom prefix - prefix := types.GetDenomPrefix(sourcePort, sourceChannel) - coins := make(sdk.Coins, len(data.Amount)) - for i, coin := range data.Amount { - coin := coin - if !strings.HasPrefix(coin.Denom, prefix) { - return sdkerrors.Wrapf(sdkerrors.ErrInvalidCoins, "%s doesn't contain the prefix '%s'", coin.Denom, prefix) - } - coins[i] = sdk.NewCoin(coin.Denom[len(prefix):], coin.Amount) - } - - if data.Source { - escrowAddress := types.GetEscrowAddress(destinationPort, destinationChannel) - return k.bankKeeper.SendCoins(ctx, escrowAddress, data.Sender, coins) - } - - // mint from supply - if err := k.supplyKeeper.MintCoins( - ctx, types.GetModuleAccountName(), data.Amount, - ); err != nil { - return err - } - - return k.supplyKeeper.SendCoinsFromModuleToAccount(ctx, types.GetModuleAccountName(), data.Sender, data.Amount) +func (k Keeper) TimeoutTransfer(ctx sdk.Context, packet channel.Packet) error { + return k.onTimeoutPacket(ctx, packet) } // See spec for this function: https://github.com/cosmos/ics/tree/master/spec/ics-020-fungible-token-transfer#packet-relay @@ -215,3 +140,89 @@ func (k Keeper) createOutgoingPacket( return k.channelKeeper.SendPacket(ctx, packet) } + +func (k Keeper) onRecvPacket(ctx sdk.Context, packet channel.Packet) error { + data, ok := packet.GetData().(types.FungibleTokenPacketData) + if !ok { + return sdkerrors.Wrap( + channel.ErrInvalidPacket, + "data doesn't correspond to fungible token transfer", + ) + } + + if data.Source { + prefix := types.GetDenomPrefix(packet.GetDestChannel(), packet.GetDestChannel()) + for _, coin := range data.Amount { + if !strings.HasPrefix(coin.Denom, prefix) { + return sdkerrors.Wrapf( + sdkerrors.ErrInvalidCoins, + "%s doesn't contain the prefix '%s'", coin.Denom, prefix, + ) + } + } + + // mint new tokens if the source of the transfer is the same chain + if err := k.supplyKeeper.MintCoins( + ctx, types.GetModuleAccountName(), data.Amount, + ); err != nil { + return err + } + + // send to receiver + return k.supplyKeeper.SendCoinsFromModuleToAccount( + ctx, types.GetModuleAccountName(), data.Receiver, data.Amount, + ) + } + + // check the denom prefix + prefix := types.GetDenomPrefix(packet.GetSourcePort(), packet.GetSourceChannel()) + coins := make(sdk.Coins, len(data.Amount)) + for i, coin := range data.Amount { + if !strings.HasPrefix(coin.Denom, prefix) { + return sdkerrors.Wrapf( + sdkerrors.ErrInvalidCoins, + "%s doesn't contain the prefix '%s'", coin.Denom, prefix, + ) + } + coins[i] = sdk.NewCoin(coin.Denom[len(prefix):], coin.Amount) + } + + // unescrow tokens + escrowAddress := types.GetEscrowAddress(packet.GetDestPort(), packet.GetDestChannel()) + return k.bankKeeper.SendCoins(ctx, escrowAddress, data.Receiver, coins) +} + +func (k Keeper) onTimeoutPacket(ctx sdk.Context, packet channel.Packet) error { + data, ok := packet.GetData().(types.FungibleTokenPacketData) + if !ok { + return sdkerrors.Wrap( + channel.ErrInvalidPacket, + "data doesn't correspond to fungible token transfer", + ) + } + + // check the denom prefix + prefix := types.GetDenomPrefix(packet.GetSourcePort(), packet.GetSourceChannel()) + coins := make(sdk.Coins, len(data.Amount)) + for i, coin := range data.Amount { + coin := coin + if !strings.HasPrefix(coin.Denom, prefix) { + return sdkerrors.Wrapf(sdkerrors.ErrInvalidCoins, "%s doesn't contain the prefix '%s'", coin.Denom, prefix) + } + coins[i] = sdk.NewCoin(coin.Denom[len(prefix):], coin.Amount) + } + + if data.Source { + escrowAddress := types.GetEscrowAddress(packet.GetDestPort(), packet.GetDestChannel()) + return k.bankKeeper.SendCoins(ctx, escrowAddress, data.Sender, coins) + } + + // mint from supply + if err := k.supplyKeeper.MintCoins( + ctx, types.GetModuleAccountName(), data.Amount, + ); err != nil { + return err + } + + return k.supplyKeeper.SendCoinsFromModuleToAccount(ctx, types.GetModuleAccountName(), data.Sender, data.Amount) +} diff --git a/x/ibc/20-transfer/keeper/relay_test.go b/x/ibc/20-transfer/keeper/relay_test.go index 27a26c58e744..0a6b97864b92 100644 --- a/x/ibc/20-transfer/keeper/relay_test.go +++ b/x/ibc/20-transfer/keeper/relay_test.go @@ -11,43 +11,43 @@ import ( func (suite *KeeperTestSuite) TestSendTransfer() { testCases := []struct { msg string - sourcePort string - sourceChannel string amount sdk.Coins - sender sdk.AccAddress - receiver sdk.AccAddress isSourceChain bool malleate func() expPass bool }{ - // {"sucess transfer from source chain", testPort1, testChannel1, testCoins, testAddr1, testAddr2, + // {"sucess transfer from source chain", testCoins, // true, func() {}, true}, - // {"sucess transfer from external chain", testPort1, testChannel1, testCoins, testAddr1, testAddr2, - // true, func() {}, true}, - {"source channel not found", testPort1, testChannel1, testCoins, testAddr1, testAddr2, + // {"sucess transfer from external chain", testCoins, + // false, func() {}, true}, + {"source channel not found", testCoins, true, func() {}, false}, - {"next seq send not found", testPort1, testChannel1, testCoins, testAddr1, testAddr2, + {"next seq send not found", testCoins, true, func() { suite.createChannel(testPort1, testChannel1, testConnection, testPort2, testChannel2, channelexported.OPEN) }, false}, // createOutgoingPacket tests // - source chain - {"no prefix on transfer amount", testPort1, testChannel1, testCoins, testAddr1, testAddr2, + {"no prefix on transfer amount", testCoins, true, func() { + suite.createChannel(testPort1, testChannel1, testConnection, testPort2, testChannel2, channelexported.OPEN) suite.app.IBCKeeper.ChannelKeeper.SetNextSequenceSend(suite.ctx, testPort1, testChannel1, 1) - }, false}, - {"send coins failed", testPort1, testChannel1, testCoins, testAddr1, testAddr2, + }, true}, + {"send coins failed", testCoins, true, func() { suite.createChannel(testPort1, testChannel1, testConnection, testPort2, testChannel2, channelexported.OPEN) suite.app.IBCKeeper.ChannelKeeper.SetNextSequenceSend(suite.ctx, testPort1, testChannel1, 1) - }, true}, + }, false}, // - receiving chain - // {"no prefix on transfer amount", testPort1, testChannel1, testCoins, testAddr1, testAddr2, - // false, func() {}, false}, - // {"send from module account dailed", testPort1, testChannel1, testCoins, testAddr1, testAddr2, + {"no prefix on transfer amount", testCoins, + false, func() { + suite.createChannel(testPort1, testChannel1, testConnection, testPort2, testChannel2, channelexported.OPEN) + suite.app.IBCKeeper.ChannelKeeper.SetNextSequenceSend(suite.ctx, testPort1, testChannel1, 1) + }, false}, + // {"send from module account dailed", testCoins, // false, func() {}, false}, - // {"tokens burn failed", testPort1, testChannel1, testCoins, testAddr1, testAddr2, + // {"tokens burn failed", testCoins, // false, func() {}, false}, } @@ -59,7 +59,7 @@ func (suite *KeeperTestSuite) TestSendTransfer() { tc.malleate() err := suite.app.TransferKeeper.SendTransfer( - suite.ctx, tc.sourcePort, tc.sourceChannel, 100, tc.amount, tc.sender, tc.receiver, tc.isSourceChain, + suite.ctx, testPort1, testChannel1, 100, tc.amount, testAddr1, testAddr2, tc.isSourceChain, ) if tc.expPass { From 1068bb5375a48a1d89160b14d1b61b686bd8d762 Mon Sep 17 00:00:00 2001 From: Federico Kunze Date: Wed, 19 Feb 2020 13:32:02 +0100 Subject: [PATCH 08/16] refactor relay prefixes and tests --- x/ibc/20-transfer/handler.go | 6 +- x/ibc/20-transfer/keeper/keeper_test.go | 1 + x/ibc/20-transfer/keeper/relay.go | 45 +++++-------- x/ibc/20-transfer/keeper/relay_test.go | 88 ++++++++++--------------- 4 files changed, 55 insertions(+), 85 deletions(-) diff --git a/x/ibc/20-transfer/handler.go b/x/ibc/20-transfer/handler.go index f5dc5eaf54f6..89ba32895433 100644 --- a/x/ibc/20-transfer/handler.go +++ b/x/ibc/20-transfer/handler.go @@ -93,14 +93,12 @@ func handlePacketDataTransfer( // 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) (*sdk.Result, error) { - err := k.TimeoutTransfer(ctx, msg.Packet) - if err != nil { + if err := k.TimeoutTransfer(ctx, msg.Packet); err != nil { // This shouldn't happen, since we've already validated that we've sent the packet. panic(err) } - err = k.TimeoutExecuted(ctx, msg.Packet) - if err != nil { + 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) diff --git a/x/ibc/20-transfer/keeper/keeper_test.go b/x/ibc/20-transfer/keeper/keeper_test.go index 603b1eba60a7..15048a3177f5 100644 --- a/x/ibc/20-transfer/keeper/keeper_test.go +++ b/x/ibc/20-transfer/keeper/keeper_test.go @@ -49,6 +49,7 @@ var ( testAddr2 = sdk.AccAddress([]byte("testaddr2")) testCoins, _ = sdk.ParseCoins("100atom") + destCoins = sdk.NewCoins(sdk.NewInt64Coin("bank/firstchannel/atom", 1000)) testPrefixedCoins1, _ = sdk.ParseCoins(fmt.Sprintf("100%satom", types.GetDenomPrefix(testPort1, testChannel1))) testPrefixedCoins2, _ = sdk.ParseCoins(fmt.Sprintf("100%satom", types.GetDenomPrefix(testPort2, testChannel2))) ) diff --git a/x/ibc/20-transfer/keeper/relay.go b/x/ibc/20-transfer/keeper/relay.go index 1a3ef0d7dc8d..ffc723b65aa4 100644 --- a/x/ibc/20-transfer/keeper/relay.go +++ b/x/ibc/20-transfer/keeper/relay.go @@ -9,7 +9,8 @@ import ( "github.com/cosmos/cosmos-sdk/x/ibc/20-transfer/types" ) -// SendTransfer handles transfer sending logic +// SendTransfer handles transfer sending logic. +// The amount MUST be prefixed if it func (k Keeper) SendTransfer( ctx sdk.Context, sourcePort, @@ -37,11 +38,13 @@ func (k Keeper) SendTransfer( coins := make(sdk.Coins, len(amount)) switch { - case isSourceChain: + case !isSourceChain: // build the receiving denomination prefix - prefix := types.GetDenomPrefix(destinationPort, destinationChannel) + prefix := types.GetDenomPrefix(sourcePort, sourceChannel) for i, coin := range amount { - coins[i] = sdk.NewCoin(prefix+coin.Denom, coin.Amount) + if !strings.HasPrefix(coin.Denom, prefix) { + coins[i] = sdk.NewCoin(prefix+coin.Denom, coin.Amount) + } } default: coins = amount @@ -55,7 +58,7 @@ func (k Keeper) ReceiveTransfer(ctx sdk.Context, packet channel.Packet) error { return k.onRecvPacket(ctx, packet) } -// TimeoutTransfer handles transfer timeout logic +// TimeoutTransfer handles transfer timeout logic. func (k Keeper) TimeoutTransfer(ctx sdk.Context, packet channel.Packet) error { return k.onTimeoutPacket(ctx, packet) } @@ -78,37 +81,20 @@ func (k Keeper) createOutgoingPacket( // escrow tokens if the destination chain is the same as the sender's escrowAddress := types.GetEscrowAddress(sourcePort, sourceChannel) - // construct receiving denominations, check correctness - prefix := types.GetDenomPrefix(destinationPort, destinationChannel) - coins := make(sdk.Coins, len(amount)) - for i, coin := range amount { - if !strings.HasPrefix(coin.Denom, prefix) { - return sdkerrors.Wrapf( - sdkerrors.ErrInvalidCoins, - "%s doesn't contain the prefix '%s'", coin.Denom, prefix, - ) - } - coins[i] = sdk.NewCoin(coin.Denom[len(prefix):], coin.Amount) - } + // NOTE: we can omit the destination prefix correctness since it's already populated + // internally on the SendTransfer function. // escrow source tokens. It fails if balance insufficient. if err := k.bankKeeper.SendCoins( - ctx, sender, escrowAddress, coins, + ctx, sender, escrowAddress, amount, ); err != nil { return err } } else { // burn vouchers from the sender's balance if the source is from another chain - // construct receiving denomination, check correctness - prefix := types.GetDenomPrefix(sourcePort, sourceChannel) - for _, coin := range amount { - if !strings.HasPrefix(coin.Denom, prefix) { - return sdkerrors.Wrapf( - sdkerrors.ErrInvalidCoins, - "%s doesn't contain the prefix '%s'", coin.Denom, prefix, - ) - } - } + + // NOTE: we can omit the source prefix correctness since it's already populated + // internally on the SendTransfer function. // transfer the coins to the module account and burn them if err := k.supplyKeeper.SendCoinsFromAccountToModule( @@ -121,6 +107,9 @@ func (k Keeper) createOutgoingPacket( if err := k.supplyKeeper.BurnCoins( ctx, types.GetModuleAccountName(), amount, ); err != nil { + // NOTE: should not happen as the module account was + // retrieved on the step above and it has enough balace + // to burn. return err } } diff --git a/x/ibc/20-transfer/keeper/relay_test.go b/x/ibc/20-transfer/keeper/relay_test.go index 0a6b97864b92..5d8c00a33c86 100644 --- a/x/ibc/20-transfer/keeper/relay_test.go +++ b/x/ibc/20-transfer/keeper/relay_test.go @@ -5,7 +5,8 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" channelexported "github.com/cosmos/cosmos-sdk/x/ibc/04-channel/exported" - "github.com/cosmos/cosmos-sdk/x/ibc/20-transfer/types" + channel "github.com/cosmos/cosmos-sdk/x/ibc/04-channel/types" + "github.com/cosmos/cosmos-sdk/x/supply" ) func (suite *KeeperTestSuite) TestSendTransfer() { @@ -16,10 +17,19 @@ func (suite *KeeperTestSuite) TestSendTransfer() { malleate func() expPass bool }{ - // {"sucess transfer from source chain", testCoins, - // true, func() {}, true}, - // {"sucess transfer from external chain", testCoins, - // false, func() {}, true}, + {"sucess transfer from source chain", testCoins, + true, func() { + suite.app.BankKeeper.AddCoins(suite.ctx, testAddr1, testCoins) + suite.createChannel(testPort1, testChannel1, testConnection, testPort2, testChannel2, channelexported.OPEN) + suite.app.IBCKeeper.ChannelKeeper.SetNextSequenceSend(suite.ctx, testPort1, testChannel1, 1) + }, true}, + {"sucess transfer from external chain", testCoins, + false, func() { + suite.app.SupplyKeeper.SetSupply(suite.ctx, supply.NewSupply(destCoins)) + suite.app.BankKeeper.AddCoins(suite.ctx, testAddr1, destCoins) + suite.createChannel(testPort1, testChannel1, testConnection, testPort2, testChannel2, channelexported.OPEN) + suite.app.IBCKeeper.ChannelKeeper.SetNextSequenceSend(suite.ctx, testPort1, testChannel1, 1) + }, true}, {"source channel not found", testCoins, true, func() {}, false}, {"next seq send not found", testCoins, @@ -28,27 +38,17 @@ func (suite *KeeperTestSuite) TestSendTransfer() { }, false}, // createOutgoingPacket tests // - source chain - {"no prefix on transfer amount", testCoins, - true, func() { - - suite.createChannel(testPort1, testChannel1, testConnection, testPort2, testChannel2, channelexported.OPEN) - suite.app.IBCKeeper.ChannelKeeper.SetNextSequenceSend(suite.ctx, testPort1, testChannel1, 1) - }, true}, {"send coins failed", testCoins, true, func() { suite.createChannel(testPort1, testChannel1, testConnection, testPort2, testChannel2, channelexported.OPEN) suite.app.IBCKeeper.ChannelKeeper.SetNextSequenceSend(suite.ctx, testPort1, testChannel1, 1) }, false}, // - receiving chain - {"no prefix on transfer amount", testCoins, + {"send from module account dailed", testCoins, false, func() { suite.createChannel(testPort1, testChannel1, testConnection, testPort2, testChannel2, channelexported.OPEN) suite.app.IBCKeeper.ChannelKeeper.SetNextSequenceSend(suite.ctx, testPort1, testChannel1, 1) }, false}, - // {"send from module account dailed", testCoins, - // false, func() {}, false}, - // {"tokens burn failed", testCoins, - // false, func() {}, false}, } for i, tc := range testCases { @@ -72,44 +72,26 @@ func (suite *KeeperTestSuite) TestSendTransfer() { } func (suite *KeeperTestSuite) TestReceiveTransfer() { - // test the situation where the source is true - source := true - packetTimeout := uint64(100) - - packetData := types.NewFungibleTokenPacketData(testPrefixedCoins1, testAddr1, testAddr2, source, packetTimeout) - err := suite.app.TransferKeeper.ReceiveTransfer(suite.ctx, testPort1, testChannel1, testPort2, testChannel2, packetData) - suite.Error(err) // incorrect denom prefix - - packetData.Amount = testPrefixedCoins2 - err = suite.app.TransferKeeper.ReceiveTransfer(suite.ctx, testPort1, testChannel1, testPort2, testChannel2, packetData) - suite.NoError(err) // successfully executed - - totalSupply := suite.app.SupplyKeeper.GetSupply(suite.ctx) - suite.Equal(testPrefixedCoins2, totalSupply.GetTotal()) // supply should be inflated - - receiverCoins := suite.app.BankKeeper.GetAllBalances(suite.ctx, packetData.Receiver) - suite.Equal(testPrefixedCoins2, receiverCoins) - - // test the situation where the source is false - packetData.Source = false - - packetData.Amount = testPrefixedCoins2 - err = suite.app.TransferKeeper.ReceiveTransfer(suite.ctx, testPort1, testChannel1, testPort2, testChannel2, packetData) - suite.Error(err) // incorrect denom prefix - - packetData.Amount = testPrefixedCoins1 - err = suite.app.TransferKeeper.ReceiveTransfer(suite.ctx, testPort1, testChannel1, testPort2, testChannel2, packetData) - suite.Error(err) // insufficient coins in the corresponding escrow account + var packet channel.Packet + testCases := []struct { + msg string + malleate func() + expPass bool + }{} - escrowAddress := types.GetEscrowAddress(testPort2, testChannel2) - _ = suite.app.BankKeeper.SetBalances(suite.ctx, escrowAddress, testCoins) - _ = suite.app.BankKeeper.SetBalances(suite.ctx, packetData.Receiver, sdk.Coins{}) - err = suite.app.TransferKeeper.ReceiveTransfer(suite.ctx, testPort1, testChannel1, testPort2, testChannel2, packetData) - suite.NoError(err) // successfully executed + for i, tc := range testCases { + tc := tc + suite.Run(fmt.Sprintf("Case %s", tc.msg), func() { + suite.SetupTest() // reset + tc.malleate() - escrowCoins := suite.app.BankKeeper.GetAllBalances(suite.ctx, escrowAddress) - suite.Equal(sdk.Coins(nil), escrowCoins) + err := suite.app.TransferKeeper.ReceiveTransfer(suite.ctx, packet) - receiverCoins = suite.app.BankKeeper.GetAllBalances(suite.ctx, packetData.Receiver) - suite.Equal(testCoins, receiverCoins) + if tc.expPass { + suite.Require().NoError(err, "valid test case %d failed: %s", i, tc.msg) + } else { + suite.Require().Error(err, "invalid test case %d passed: %s", i, tc.msg) + } + }) + } } From 37436b1f5b4424ee78ae44354a6e9123e6f1f10c Mon Sep 17 00:00:00 2001 From: Jack Zampolin Date: Wed, 19 Feb 2020 13:24:39 -0800 Subject: [PATCH 09/16] Fix test compilation --- x/ibc/04-channel/keeper/packet_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/x/ibc/04-channel/keeper/packet_test.go b/x/ibc/04-channel/keeper/packet_test.go index e8901ec1c4e4..511e3affb617 100644 --- a/x/ibc/04-channel/keeper/packet_test.go +++ b/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" + xfertypes "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 := xfertypes.AckDataTransfer{} testCases := []testCase{ {"success", func() { From 05b3e48afe8e155c956f0f8a14973b4fe8dcf954 Mon Sep 17 00:00:00 2001 From: Federico Kunze Date: Thu, 20 Feb 2020 13:32:29 +0100 Subject: [PATCH 10/16] cleanup; add notes and additional test case --- x/ibc/04-channel/keeper/packet_test.go | 4 +- x/ibc/20-transfer/handler.go | 14 ++-- x/ibc/20-transfer/keeper/keeper_test.go | 2 + x/ibc/20-transfer/keeper/relay.go | 103 +++++++++++++----------- x/ibc/20-transfer/keeper/relay_test.go | 14 +++- 5 files changed, 77 insertions(+), 60 deletions(-) diff --git a/x/ibc/04-channel/keeper/packet_test.go b/x/ibc/04-channel/keeper/packet_test.go index 511e3affb617..950bf320776b 100644 --- a/x/ibc/04-channel/keeper/packet_test.go +++ b/x/ibc/04-channel/keeper/packet_test.go @@ -6,7 +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" - xfertypes "github.com/cosmos/cosmos-sdk/x/ibc/20-transfer/types" + transfertypes "github.com/cosmos/cosmos-sdk/x/ibc/20-transfer/types" ibctypes "github.com/cosmos/cosmos-sdk/x/ibc/types" ) @@ -213,7 +213,7 @@ func (suite *KeeperTestSuite) TestAcknowledgePacket() { counterparty := types.NewCounterparty(testPort2, testChannel2) var packet types.Packet - ack := xfertypes.AckDataTransfer{} + ack := transfertypes.AckDataTransfer{} testCases := []testCase{ {"success", func() { diff --git a/x/ibc/20-transfer/handler.go b/x/ibc/20-transfer/handler.go index 89ba32895433..08dd6a5927d0 100644 --- a/x/ibc/20-transfer/handler.go +++ b/x/ibc/20-transfer/handler.go @@ -16,14 +16,14 @@ func NewHandler(k Keeper) sdk.Handler { case channeltypes.MsgPacket: switch data := msg.Data.(type) { case FungibleTokenPacketData: - return handlePacketDataTransfer(ctx, k, msg) + return handlePacketDataTransfer(ctx, k, msg, data) default: return nil, sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "unrecognized ICS-20 transfer packet data type: %T", data) } case channeltypes.MsgTimeout: switch data := msg.Data.(type) { case FungibleTokenPacketData: - return handleTimeoutDataTransfer(ctx, k, msg) + return handleTimeoutDataTransfer(ctx, k, msg, data) default: return nil, sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "unrecognized ICS-20 transfer packet data type: %T", data) } @@ -57,9 +57,9 @@ func handleMsgTransfer(ctx sdk.Context, k Keeper, msg MsgTransfer) (*sdk.Result, // 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, + ctx sdk.Context, k Keeper, msg channeltypes.MsgPacket, data FungibleTokenPacketData, ) (*sdk.Result, error) { - if err := k.ReceiveTransfer(ctx, msg.Packet); err != nil { + 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. @@ -92,8 +92,10 @@ func handlePacketDataTransfer( } // 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) (*sdk.Result, error) { - if err := k.TimeoutTransfer(ctx, msg.Packet); err != nil { +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) } diff --git a/x/ibc/20-transfer/keeper/keeper_test.go b/x/ibc/20-transfer/keeper/keeper_test.go index 15048a3177f5..f335ace53c0b 100644 --- a/x/ibc/20-transfer/keeper/keeper_test.go +++ b/x/ibc/20-transfer/keeper/keeper_test.go @@ -50,6 +50,7 @@ var ( testCoins, _ = sdk.ParseCoins("100atom") destCoins = sdk.NewCoins(sdk.NewInt64Coin("bank/firstchannel/atom", 1000)) + destCoins2 = sdk.NewCoins(sdk.NewInt64Coin("testportid/secondchannel/atom", 100)) testPrefixedCoins1, _ = sdk.ParseCoins(fmt.Sprintf("100%satom", types.GetDenomPrefix(testPort1, testChannel1))) testPrefixedCoins2, _ = sdk.ParseCoins(fmt.Sprintf("100%satom", types.GetDenomPrefix(testPort2, testChannel2))) ) @@ -144,6 +145,7 @@ func (suite *KeeperTestSuite) createChannel(portID string, chanID string, connID suite.app.IBCKeeper.ChannelKeeper.SetChannel(suite.ctx, portID, chanID, ch) } +// nolint: unused func (suite *KeeperTestSuite) queryProof(key []byte) (proof commitment.Proof, height int64) { res := suite.app.Query(abci.RequestQuery{ Path: fmt.Sprintf("store/%s/key", ibctypes.StoreKey), diff --git a/x/ibc/20-transfer/keeper/relay.go b/x/ibc/20-transfer/keeper/relay.go index ffc723b65aa4..15303bbe3aef 100644 --- a/x/ibc/20-transfer/keeper/relay.go +++ b/x/ibc/20-transfer/keeper/relay.go @@ -9,8 +9,16 @@ import ( "github.com/cosmos/cosmos-sdk/x/ibc/20-transfer/types" ) -// SendTransfer handles transfer sending logic. -// The amount MUST be prefixed if it +// SendTransfer handles transfer sending logic. There are 2 possible cases: +// +// 1. Sender chain is the source chain of the coins (i.e where they were minted): the coins +// are transfered to an escrow address (i.e locked) on the sender chain and then +// transfered to the destination chain (i.e not the source chain) via a packet +// with the corresponding fungible token data. +// +// 2. Coins are not native from the sender chain (i.e tokens sent where transfered over +// through IBC already): the coins are burned and then a packet is sent to the +// source chain of the tokens. func (k Keeper) SendTransfer( ctx sdk.Context, sourcePort, @@ -19,7 +27,7 @@ func (k Keeper) SendTransfer( amount sdk.Coins, sender, receiver sdk.AccAddress, - isSourceChain bool, + isSourceChain bool, // is the packet sender the source chain of the token? ) error { // get the port and channel of the counterparty sourceChan, found := k.channelKeeper.GetChannel(ctx, sourcePort, sourceChannel) @@ -35,32 +43,17 @@ func (k Keeper) SendTransfer( if !found { return channel.ErrSequenceSendNotFound } - - coins := make(sdk.Coins, len(amount)) - switch { - case !isSourceChain: - // build the receiving denomination prefix - prefix := types.GetDenomPrefix(sourcePort, sourceChannel) - for i, coin := range amount { - if !strings.HasPrefix(coin.Denom, prefix) { - coins[i] = sdk.NewCoin(prefix+coin.Denom, coin.Amount) - } - } - default: - coins = amount - } - - return k.createOutgoingPacket(ctx, sequence, sourcePort, sourceChannel, destinationPort, destinationChannel, destHeight, coins, sender, receiver, isSourceChain) + return k.createOutgoingPacket(ctx, sequence, sourcePort, sourceChannel, destinationPort, destinationChannel, destHeight, amount, sender, receiver, isSourceChain) } // ReceiveTransfer handles transfer receiving logic. -func (k Keeper) ReceiveTransfer(ctx sdk.Context, packet channel.Packet) error { - return k.onRecvPacket(ctx, packet) +func (k Keeper) ReceiveTransfer(ctx sdk.Context, packet channel.Packet, data types.FungibleTokenPacketData) error { + return k.onRecvPacket(ctx, packet, data) } // TimeoutTransfer handles transfer timeout logic. -func (k Keeper) TimeoutTransfer(ctx sdk.Context, packet channel.Packet) error { - return k.onTimeoutPacket(ctx, packet) +func (k Keeper) TimeoutTransfer(ctx sdk.Context, packet channel.Packet, data types.FungibleTokenPacketData) error { + return k.onTimeoutPacket(ctx, packet, data) } // See spec for this function: https://github.com/cosmos/ics/tree/master/spec/ics-020-fungible-token-transfer#packet-relay @@ -77,24 +70,45 @@ func (k Keeper) createOutgoingPacket( receiver sdk.AccAddress, isSourceChain bool, ) error { + // NOTE: + // - Coins transferred from the destination chain should have their denomination + // prefixed with source port and channel IDs. + // - Coins transferred from the source chain can have their denomination + // clear from prefixes when transfered to the escrow account (i.e when they are + // locked) BUT MUST have the destination port and channel ID when constructing + // the packet data. + var prefix string + if isSourceChain { + // clear the denomination from the prefix to send the coins to the escrow account + coins := make(sdk.Coins, len(amount)) + prefix = types.GetDenomPrefix(destinationPort, destinationChannel) + for i, coin := range amount { + if strings.HasPrefix(coin.Denom, prefix) { + coins[i] = sdk.NewCoin(coin.Denom[len(prefix):], coin.Amount) + } else { + coins[i] = amount[i] + } + } + // escrow tokens if the destination chain is the same as the sender's escrowAddress := types.GetEscrowAddress(sourcePort, sourceChannel) - // NOTE: we can omit the destination prefix correctness since it's already populated - // internally on the SendTransfer function. - // escrow source tokens. It fails if balance insufficient. if err := k.bankKeeper.SendCoins( - ctx, sender, escrowAddress, amount, + ctx, sender, escrowAddress, coins, ); err != nil { return err } - } else { - // burn vouchers from the sender's balance if the source is from another chain - // NOTE: we can omit the source prefix correctness since it's already populated - // internally on the SendTransfer function. + } else { + // build the receiving denomination prefix if it's not present + prefix = types.GetDenomPrefix(sourcePort, sourceChannel) + for i, coin := range amount { + if !strings.HasPrefix(coin.Denom, prefix) { + amount[i] = sdk.NewCoin(prefix+coin.Denom, coin.Amount) + } + } // transfer the coins to the module account and burn them if err := k.supplyKeeper.SendCoinsFromAccountToModule( @@ -103,7 +117,7 @@ func (k Keeper) createOutgoingPacket( return err } - // burn from supply + // burn vouchers from the sender's balance if the source is from another chain if err := k.supplyKeeper.BurnCoins( ctx, types.GetModuleAccountName(), amount, ); err != nil { @@ -114,8 +128,11 @@ func (k Keeper) createOutgoingPacket( } } + // NOTE: isSourceChain is negated since the counterparty chain + // + packetData := types.NewFungibleTokenPacketData( - amount, sender, receiver, isSourceChain, destHeight+DefaultPacketTimeout, + amount, sender, receiver, !isSourceChain, destHeight+DefaultPacketTimeout, ) packet := channel.NewPacket( @@ -130,14 +147,8 @@ func (k Keeper) createOutgoingPacket( return k.channelKeeper.SendPacket(ctx, packet) } -func (k Keeper) onRecvPacket(ctx sdk.Context, packet channel.Packet) error { - data, ok := packet.GetData().(types.FungibleTokenPacketData) - if !ok { - return sdkerrors.Wrap( - channel.ErrInvalidPacket, - "data doesn't correspond to fungible token transfer", - ) - } +func (k Keeper) onRecvPacket(ctx sdk.Context, packet channel.Packet, data types.FungibleTokenPacketData) error { + // NOTE: packet data type already checked in handler.go if data.Source { prefix := types.GetDenomPrefix(packet.GetDestChannel(), packet.GetDestChannel()) @@ -181,14 +192,8 @@ func (k Keeper) onRecvPacket(ctx sdk.Context, packet channel.Packet) error { return k.bankKeeper.SendCoins(ctx, escrowAddress, data.Receiver, coins) } -func (k Keeper) onTimeoutPacket(ctx sdk.Context, packet channel.Packet) error { - data, ok := packet.GetData().(types.FungibleTokenPacketData) - if !ok { - return sdkerrors.Wrap( - channel.ErrInvalidPacket, - "data doesn't correspond to fungible token transfer", - ) - } +func (k Keeper) onTimeoutPacket(ctx sdk.Context, packet channel.Packet, data types.FungibleTokenPacketData) error { + // NOTE: packet data type already checked in handler.go // check the denom prefix prefix := types.GetDenomPrefix(packet.GetSourcePort(), packet.GetSourceChannel()) diff --git a/x/ibc/20-transfer/keeper/relay_test.go b/x/ibc/20-transfer/keeper/relay_test.go index 5d8c00a33c86..96b9ad09881b 100644 --- a/x/ibc/20-transfer/keeper/relay_test.go +++ b/x/ibc/20-transfer/keeper/relay_test.go @@ -5,7 +5,8 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" channelexported "github.com/cosmos/cosmos-sdk/x/ibc/04-channel/exported" - channel "github.com/cosmos/cosmos-sdk/x/ibc/04-channel/types" + channeltypes "github.com/cosmos/cosmos-sdk/x/ibc/04-channel/types" + "github.com/cosmos/cosmos-sdk/x/ibc/20-transfer/types" "github.com/cosmos/cosmos-sdk/x/supply" ) @@ -23,6 +24,12 @@ func (suite *KeeperTestSuite) TestSendTransfer() { suite.createChannel(testPort1, testChannel1, testConnection, testPort2, testChannel2, channelexported.OPEN) suite.app.IBCKeeper.ChannelKeeper.SetNextSequenceSend(suite.ctx, testPort1, testChannel1, 1) }, true}, + {"sucess transfer from source chain with denom prefix", destCoins2, + true, func() { + suite.app.BankKeeper.AddCoins(suite.ctx, testAddr1, testCoins) + suite.createChannel(testPort1, testChannel1, testConnection, testPort2, testChannel2, channelexported.OPEN) + suite.app.IBCKeeper.ChannelKeeper.SetNextSequenceSend(suite.ctx, testPort1, testChannel1, 1) + }, true}, {"sucess transfer from external chain", testCoins, false, func() { suite.app.SupplyKeeper.SetSupply(suite.ctx, supply.NewSupply(destCoins)) @@ -72,7 +79,8 @@ func (suite *KeeperTestSuite) TestSendTransfer() { } func (suite *KeeperTestSuite) TestReceiveTransfer() { - var packet channel.Packet + var packet channeltypes.Packet + var data types.FungibleTokenPacketData testCases := []struct { msg string malleate func() @@ -85,7 +93,7 @@ func (suite *KeeperTestSuite) TestReceiveTransfer() { suite.SetupTest() // reset tc.malleate() - err := suite.app.TransferKeeper.ReceiveTransfer(suite.ctx, packet) + err := suite.app.TransferKeeper.ReceiveTransfer(suite.ctx, packet, data) if tc.expPass { suite.Require().NoError(err, "valid test case %d failed: %s", i, tc.msg) From 4f9763ea7b8b393da184bb68848fff013be46885 Mon Sep 17 00:00:00 2001 From: Federico Kunze Date: Thu, 20 Feb 2020 16:31:19 +0100 Subject: [PATCH 11/16] Receive transfer test --- types/coin.go | 4 +-- x/ibc/20-transfer/keeper/keeper_test.go | 8 ++--- x/ibc/20-transfer/keeper/relay.go | 24 ++++++------- x/ibc/20-transfer/keeper/relay_test.go | 48 ++++++++++++++++++++----- 4 files changed, 57 insertions(+), 27 deletions(-) diff --git a/types/coin.go b/types/coin.go index 6a759d569387..8f51f640872c 100644 --- a/types/coin.go +++ b/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:]]*` diff --git a/x/ibc/20-transfer/keeper/keeper_test.go b/x/ibc/20-transfer/keeper/keeper_test.go index f335ace53c0b..082aa2b12f0b 100644 --- a/x/ibc/20-transfer/keeper/keeper_test.go +++ b/x/ibc/20-transfer/keeper/keeper_test.go @@ -48,11 +48,9 @@ var ( testAddr1 = sdk.AccAddress([]byte("testaddr1")) testAddr2 = sdk.AccAddress([]byte("testaddr2")) - testCoins, _ = sdk.ParseCoins("100atom") - destCoins = sdk.NewCoins(sdk.NewInt64Coin("bank/firstchannel/atom", 1000)) - destCoins2 = sdk.NewCoins(sdk.NewInt64Coin("testportid/secondchannel/atom", 100)) - testPrefixedCoins1, _ = sdk.ParseCoins(fmt.Sprintf("100%satom", types.GetDenomPrefix(testPort1, testChannel1))) - testPrefixedCoins2, _ = sdk.ParseCoins(fmt.Sprintf("100%satom", types.GetDenomPrefix(testPort2, testChannel2))) + testCoins, _ = sdk.ParseCoins("100atom") + prefixCoins = sdk.NewCoins(sdk.NewCoin("bank/firstchannel/atom", sdk.NewInt(100))) + prefixCoins2 = sdk.NewCoins(sdk.NewCoin("testportid/secondchannel/atom", sdk.NewInt(100))) ) type KeeperTestSuite struct { diff --git a/x/ibc/20-transfer/keeper/relay.go b/x/ibc/20-transfer/keeper/relay.go index 15303bbe3aef..bea201756a40 100644 --- a/x/ibc/20-transfer/keeper/relay.go +++ b/x/ibc/20-transfer/keeper/relay.go @@ -1,6 +1,7 @@ package keeper import ( + "fmt" "strings" sdk "github.com/cosmos/cosmos-sdk/types" @@ -29,20 +30,20 @@ func (k Keeper) SendTransfer( receiver sdk.AccAddress, isSourceChain bool, // is the packet sender the source chain of the token? ) error { - // get the port and channel of the counterparty - sourceChan, found := k.channelKeeper.GetChannel(ctx, sourcePort, sourceChannel) + sourceChannelEnd, found := k.channelKeeper.GetChannel(ctx, sourcePort, sourceChannel) if !found { return sdkerrors.Wrap(channel.ErrChannelNotFound, sourceChannel) } - destinationPort := sourceChan.Counterparty.PortID - destinationChannel := sourceChan.Counterparty.ChannelID + destinationPort := sourceChannelEnd.Counterparty.PortID + destinationChannel := sourceChannelEnd.Counterparty.ChannelID // get the next sequence sequence, found := k.channelKeeper.GetNextSequenceSend(ctx, sourcePort, sourceChannel) if !found { return channel.ErrSequenceSendNotFound } + return k.createOutgoingPacket(ctx, sequence, sourcePort, sourceChannel, destinationPort, destinationChannel, destHeight, amount, sender, receiver, isSourceChain) } @@ -60,14 +61,11 @@ func (k Keeper) TimeoutTransfer(ctx sdk.Context, packet channel.Packet, data typ func (k Keeper) createOutgoingPacket( ctx sdk.Context, seq uint64, - sourcePort, - sourceChannel, - destinationPort, - destinationChannel string, + sourcePort, sourceChannel, + destinationPort, destinationChannel string, destHeight uint64, amount sdk.Coins, - sender sdk.AccAddress, - receiver sdk.AccAddress, + sender, receiver sdk.AccAddress, isSourceChain bool, ) error { // NOTE: @@ -87,10 +85,12 @@ func (k Keeper) createOutgoingPacket( if strings.HasPrefix(coin.Denom, prefix) { coins[i] = sdk.NewCoin(coin.Denom[len(prefix):], coin.Amount) } else { - coins[i] = amount[i] + coins[i] = coin } } + fmt.Println(coins) + // escrow tokens if the destination chain is the same as the sender's escrowAddress := types.GetEscrowAddress(sourcePort, sourceChannel) @@ -151,7 +151,7 @@ func (k Keeper) onRecvPacket(ctx sdk.Context, packet channel.Packet, data types. // NOTE: packet data type already checked in handler.go if data.Source { - prefix := types.GetDenomPrefix(packet.GetDestChannel(), packet.GetDestChannel()) + prefix := types.GetDenomPrefix(packet.GetDestPort(), packet.GetDestChannel()) for _, coin := range data.Amount { if !strings.HasPrefix(coin.Denom, prefix) { return sdkerrors.Wrapf( diff --git a/x/ibc/20-transfer/keeper/relay_test.go b/x/ibc/20-transfer/keeper/relay_test.go index 96b9ad09881b..f60fc5aa0419 100644 --- a/x/ibc/20-transfer/keeper/relay_test.go +++ b/x/ibc/20-transfer/keeper/relay_test.go @@ -11,6 +11,7 @@ import ( ) func (suite *KeeperTestSuite) TestSendTransfer() { + testCoins2 := sdk.NewCoins(sdk.NewCoin("testportid/secondchannel/atom", sdk.NewInt(100))) testCases := []struct { msg string amount sdk.Coins @@ -24,16 +25,18 @@ func (suite *KeeperTestSuite) TestSendTransfer() { suite.createChannel(testPort1, testChannel1, testConnection, testPort2, testChannel2, channelexported.OPEN) suite.app.IBCKeeper.ChannelKeeper.SetNextSequenceSend(suite.ctx, testPort1, testChannel1, 1) }, true}, - {"sucess transfer from source chain with denom prefix", destCoins2, + {"sucess transfer from source chain with denom prefix", testCoins2, true, func() { - suite.app.BankKeeper.AddCoins(suite.ctx, testAddr1, testCoins) + _, err := suite.app.BankKeeper.AddCoins(suite.ctx, testAddr1, testCoins) + suite.Require().NoError(err) suite.createChannel(testPort1, testChannel1, testConnection, testPort2, testChannel2, channelexported.OPEN) suite.app.IBCKeeper.ChannelKeeper.SetNextSequenceSend(suite.ctx, testPort1, testChannel1, 1) }, true}, {"sucess transfer from external chain", testCoins, false, func() { - suite.app.SupplyKeeper.SetSupply(suite.ctx, supply.NewSupply(destCoins)) - suite.app.BankKeeper.AddCoins(suite.ctx, testAddr1, destCoins) + suite.app.SupplyKeeper.SetSupply(suite.ctx, supply.NewSupply(prefixCoins)) + _, err := suite.app.BankKeeper.AddCoins(suite.ctx, testAddr1, prefixCoins) + suite.Require().NoError(err) suite.createChannel(testPort1, testChannel1, testConnection, testPort2, testChannel2, channelexported.OPEN) suite.app.IBCKeeper.ChannelKeeper.SetNextSequenceSend(suite.ctx, testPort1, testChannel1, 1) }, true}, @@ -51,7 +54,7 @@ func (suite *KeeperTestSuite) TestSendTransfer() { suite.app.IBCKeeper.ChannelKeeper.SetNextSequenceSend(suite.ctx, testPort1, testChannel1, 1) }, false}, // - receiving chain - {"send from module account dailed", testCoins, + {"send from module account failed", testCoins, false, func() { suite.createChannel(testPort1, testChannel1, testConnection, testPort2, testChannel2, channelexported.OPEN) suite.app.IBCKeeper.ChannelKeeper.SetNextSequenceSend(suite.ctx, testPort1, testChannel1, 1) @@ -79,13 +82,42 @@ func (suite *KeeperTestSuite) TestSendTransfer() { } func (suite *KeeperTestSuite) TestReceiveTransfer() { - var packet channeltypes.Packet - var data types.FungibleTokenPacketData + data := types.NewFungibleTokenPacketData(prefixCoins2, testAddr1, testAddr2, true, 100) + testCases := []struct { msg string malleate func() expPass bool - }{} + }{ + {"sucess receive from source chain", + func() {}, true}, + // onRecvPacket + // - source chain + {"no dest prefix on coin denom", + func() { + data.Amount = testCoins + }, false}, + {"mint failed", + func() { + data.Amount = prefixCoins2 + data.Amount[0].Amount = sdk.ZeroInt() + }, false}, + // - receiving chain + {"no source prefix on coin denom", + func() { + data.Source = false + }, false}, + {"sucess receive from external chain", + func() { + data.Source = false + data.Amount = prefixCoins + escrow := types.GetEscrowAddress(testPort2, testChannel2) + _, err := suite.app.BankKeeper.AddCoins(suite.ctx, escrow, testCoins) + suite.Require().NoError(err) + }, true}, + } + + packet := channeltypes.NewPacket(data, 1, testPort1, testChannel1, testPort2, testChannel2) for i, tc := range testCases { tc := tc From 4db79ba7d3dd7c046cda2aabdda0ddebbe90862b Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Thu, 20 Feb 2020 16:40:05 +0100 Subject: [PATCH 12/16] Apply suggestions from code review Co-Authored-By: Bot from GolangCI <42910462+golangcibot@users.noreply.github.com> --- x/ibc/20-transfer/keeper/relay.go | 3 --- x/ibc/20-transfer/keeper/relay_test.go | 3 --- 2 files changed, 6 deletions(-) diff --git a/x/ibc/20-transfer/keeper/relay.go b/x/ibc/20-transfer/keeper/relay.go index bea201756a40..29a99b530f40 100644 --- a/x/ibc/20-transfer/keeper/relay.go +++ b/x/ibc/20-transfer/keeper/relay.go @@ -13,11 +13,8 @@ import ( // SendTransfer handles transfer sending logic. There are 2 possible cases: // // 1. Sender chain is the source chain of the coins (i.e where they were minted): the coins -// are transfered to an escrow address (i.e locked) on the sender chain and then -// transfered to the destination chain (i.e not the source chain) via a packet // with the corresponding fungible token data. // -// 2. Coins are not native from the sender chain (i.e tokens sent where transfered over // through IBC already): the coins are burned and then a packet is sent to the // source chain of the tokens. func (k Keeper) SendTransfer( diff --git a/x/ibc/20-transfer/keeper/relay_test.go b/x/ibc/20-transfer/keeper/relay_test.go index f60fc5aa0419..7b7f3c020444 100644 --- a/x/ibc/20-transfer/keeper/relay_test.go +++ b/x/ibc/20-transfer/keeper/relay_test.go @@ -19,20 +19,17 @@ func (suite *KeeperTestSuite) TestSendTransfer() { malleate func() expPass bool }{ - {"sucess transfer from source chain", testCoins, true, func() { suite.app.BankKeeper.AddCoins(suite.ctx, testAddr1, testCoins) suite.createChannel(testPort1, testChannel1, testConnection, testPort2, testChannel2, channelexported.OPEN) suite.app.IBCKeeper.ChannelKeeper.SetNextSequenceSend(suite.ctx, testPort1, testChannel1, 1) }, true}, - {"sucess transfer from source chain with denom prefix", testCoins2, true, func() { _, err := suite.app.BankKeeper.AddCoins(suite.ctx, testAddr1, testCoins) suite.Require().NoError(err) suite.createChannel(testPort1, testChannel1, testConnection, testPort2, testChannel2, channelexported.OPEN) suite.app.IBCKeeper.ChannelKeeper.SetNextSequenceSend(suite.ctx, testPort1, testChannel1, 1) }, true}, - {"sucess transfer from external chain", testCoins, false, func() { suite.app.SupplyKeeper.SetSupply(suite.ctx, supply.NewSupply(prefixCoins)) _, err := suite.app.BankKeeper.AddCoins(suite.ctx, testAddr1, prefixCoins) From e8cca1556de90e50a2518af0c87a2dba196b945a Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Thu, 20 Feb 2020 16:43:17 +0100 Subject: [PATCH 13/16] Fix autolinter application --- x/ibc/20-transfer/keeper/relay.go | 3 +++ x/ibc/20-transfer/keeper/relay_test.go | 3 +++ 2 files changed, 6 insertions(+) diff --git a/x/ibc/20-transfer/keeper/relay.go b/x/ibc/20-transfer/keeper/relay.go index 29a99b530f40..ca4dadac5fd8 100644 --- a/x/ibc/20-transfer/keeper/relay.go +++ b/x/ibc/20-transfer/keeper/relay.go @@ -13,8 +13,11 @@ import ( // SendTransfer handles transfer sending logic. There are 2 possible cases: // // 1. Sender chain is the source chain of the coins (i.e where they were minted): the coins +// are transferred to an escrow address (i.e locked) on the sender chain and then +// transferred to the destination chain (i.e not the source chain) via a packet // with the corresponding fungible token data. // +// 2. Coins are not native from the sender chain (i.e tokens sent where transferred over // through IBC already): the coins are burned and then a packet is sent to the // source chain of the tokens. func (k Keeper) SendTransfer( diff --git a/x/ibc/20-transfer/keeper/relay_test.go b/x/ibc/20-transfer/keeper/relay_test.go index 7b7f3c020444..a6b7e128e8a2 100644 --- a/x/ibc/20-transfer/keeper/relay_test.go +++ b/x/ibc/20-transfer/keeper/relay_test.go @@ -19,17 +19,20 @@ func (suite *KeeperTestSuite) TestSendTransfer() { malleate func() expPass bool }{ + {"successful transfer from source chain", testCoins, true, func() { suite.app.BankKeeper.AddCoins(suite.ctx, testAddr1, testCoins) suite.createChannel(testPort1, testChannel1, testConnection, testPort2, testChannel2, channelexported.OPEN) suite.app.IBCKeeper.ChannelKeeper.SetNextSequenceSend(suite.ctx, testPort1, testChannel1, 1) }, true}, + {"successful transfer from source chain with denom prefix", testCoins2, true, func() { _, err := suite.app.BankKeeper.AddCoins(suite.ctx, testAddr1, testCoins) suite.Require().NoError(err) suite.createChannel(testPort1, testChannel1, testConnection, testPort2, testChannel2, channelexported.OPEN) suite.app.IBCKeeper.ChannelKeeper.SetNextSequenceSend(suite.ctx, testPort1, testChannel1, 1) }, true}, + {"successful transfer from external chain", testCoins, false, func() { suite.app.SupplyKeeper.SetSupply(suite.ctx, supply.NewSupply(prefixCoins)) _, err := suite.app.BankKeeper.AddCoins(suite.ctx, testAddr1, prefixCoins) From 4d14d36712cc424fa3fac0b75f17049621477283 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Thu, 20 Feb 2020 16:45:48 +0100 Subject: [PATCH 14/16] Add testcase with incorrect prefix --- x/ibc/20-transfer/keeper/relay_test.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/x/ibc/20-transfer/keeper/relay_test.go b/x/ibc/20-transfer/keeper/relay_test.go index a6b7e128e8a2..70739fb2265c 100644 --- a/x/ibc/20-transfer/keeper/relay_test.go +++ b/x/ibc/20-transfer/keeper/relay_test.go @@ -97,6 +97,11 @@ func (suite *KeeperTestSuite) TestReceiveTransfer() { func() { data.Amount = testCoins }, false}, + {"incorrect dest prefix on coin denom", + func() { + data.Source = false + data.Amount = prefixCoins2 + }, false}, {"mint failed", func() { data.Amount = prefixCoins2 From b31d01b873116306d8b729758f4a7b5aea4bdffe Mon Sep 17 00:00:00 2001 From: Federico Kunze Date: Thu, 20 Feb 2020 17:00:24 +0100 Subject: [PATCH 15/16] golangcibot fixes --- x/ibc/20-transfer/keeper/relay.go | 11 +++---- x/ibc/20-transfer/keeper/relay_test.go | 40 +++++++++++++------------- 2 files changed, 24 insertions(+), 27 deletions(-) diff --git a/x/ibc/20-transfer/keeper/relay.go b/x/ibc/20-transfer/keeper/relay.go index bea201756a40..6d2bdaf25831 100644 --- a/x/ibc/20-transfer/keeper/relay.go +++ b/x/ibc/20-transfer/keeper/relay.go @@ -1,7 +1,6 @@ package keeper import ( - "fmt" "strings" sdk "github.com/cosmos/cosmos-sdk/types" @@ -13,11 +12,11 @@ import ( // SendTransfer handles transfer sending logic. There are 2 possible cases: // // 1. Sender chain is the source chain of the coins (i.e where they were minted): the coins -// are transfered to an escrow address (i.e locked) on the sender chain and then -// transfered to the destination chain (i.e not the source chain) via a packet +// are transferred to an escrow address (i.e locked) on the sender chain and then +// transferred to the destination chain (i.e not the source chain) via a packet // with the corresponding fungible token data. // -// 2. Coins are not native from the sender chain (i.e tokens sent where transfered over +// 2. Coins are not native from the sender chain (i.e tokens sent where transferred over // through IBC already): the coins are burned and then a packet is sent to the // source chain of the tokens. func (k Keeper) SendTransfer( @@ -72,7 +71,7 @@ func (k Keeper) createOutgoingPacket( // - Coins transferred from the destination chain should have their denomination // prefixed with source port and channel IDs. // - Coins transferred from the source chain can have their denomination - // clear from prefixes when transfered to the escrow account (i.e when they are + // clear from prefixes when transferred to the escrow account (i.e when they are // locked) BUT MUST have the destination port and channel ID when constructing // the packet data. var prefix string @@ -89,8 +88,6 @@ func (k Keeper) createOutgoingPacket( } } - fmt.Println(coins) - // escrow tokens if the destination chain is the same as the sender's escrowAddress := types.GetEscrowAddress(sourcePort, sourceChannel) diff --git a/x/ibc/20-transfer/keeper/relay_test.go b/x/ibc/20-transfer/keeper/relay_test.go index f60fc5aa0419..e4e6fccf3602 100644 --- a/x/ibc/20-transfer/keeper/relay_test.go +++ b/x/ibc/20-transfer/keeper/relay_test.go @@ -15,50 +15,50 @@ func (suite *KeeperTestSuite) TestSendTransfer() { testCases := []struct { msg string amount sdk.Coins - isSourceChain bool malleate func() + isSourceChain bool expPass bool }{ - {"sucess transfer from source chain", testCoins, - true, func() { + {"success transfer from source chain", testCoins, + func() { suite.app.BankKeeper.AddCoins(suite.ctx, testAddr1, testCoins) suite.createChannel(testPort1, testChannel1, testConnection, testPort2, testChannel2, channelexported.OPEN) suite.app.IBCKeeper.ChannelKeeper.SetNextSequenceSend(suite.ctx, testPort1, testChannel1, 1) - }, true}, - {"sucess transfer from source chain with denom prefix", testCoins2, - true, func() { + }, true, true}, + {"success transfer from source chain with denom prefix", testCoins2, + func() { _, err := suite.app.BankKeeper.AddCoins(suite.ctx, testAddr1, testCoins) suite.Require().NoError(err) suite.createChannel(testPort1, testChannel1, testConnection, testPort2, testChannel2, channelexported.OPEN) suite.app.IBCKeeper.ChannelKeeper.SetNextSequenceSend(suite.ctx, testPort1, testChannel1, 1) - }, true}, - {"sucess transfer from external chain", testCoins, - false, func() { + }, true, true}, + {"success transfer from external chain", testCoins, + func() { suite.app.SupplyKeeper.SetSupply(suite.ctx, supply.NewSupply(prefixCoins)) _, err := suite.app.BankKeeper.AddCoins(suite.ctx, testAddr1, prefixCoins) suite.Require().NoError(err) suite.createChannel(testPort1, testChannel1, testConnection, testPort2, testChannel2, channelexported.OPEN) suite.app.IBCKeeper.ChannelKeeper.SetNextSequenceSend(suite.ctx, testPort1, testChannel1, 1) - }, true}, + }, false, true}, {"source channel not found", testCoins, - true, func() {}, false}, + func() {}, true, false}, {"next seq send not found", testCoins, - true, func() { + func() { suite.createChannel(testPort1, testChannel1, testConnection, testPort2, testChannel2, channelexported.OPEN) - }, false}, + }, true, false}, // createOutgoingPacket tests // - source chain {"send coins failed", testCoins, - true, func() { + func() { suite.createChannel(testPort1, testChannel1, testConnection, testPort2, testChannel2, channelexported.OPEN) suite.app.IBCKeeper.ChannelKeeper.SetNextSequenceSend(suite.ctx, testPort1, testChannel1, 1) - }, false}, + }, true, false}, // - receiving chain {"send from module account failed", testCoins, - false, func() { + func() { suite.createChannel(testPort1, testChannel1, testConnection, testPort2, testChannel2, channelexported.OPEN) suite.app.IBCKeeper.ChannelKeeper.SetNextSequenceSend(suite.ctx, testPort1, testChannel1, 1) - }, false}, + }, false, false}, } for i, tc := range testCases { @@ -82,14 +82,14 @@ func (suite *KeeperTestSuite) TestSendTransfer() { } func (suite *KeeperTestSuite) TestReceiveTransfer() { - data := types.NewFungibleTokenPacketData(prefixCoins2, testAddr1, testAddr2, true, 100) + data := types.NewFungibleTokenPacketData(prefixCoins2, testAddr1, testAddr2, 100) testCases := []struct { msg string malleate func() expPass bool }{ - {"sucess receive from source chain", + {"success receive from source chain", func() {}, true}, // onRecvPacket // - source chain @@ -107,7 +107,7 @@ func (suite *KeeperTestSuite) TestReceiveTransfer() { func() { data.Source = false }, false}, - {"sucess receive from external chain", + {"success receive from external chain", func() { data.Source = false data.Amount = prefixCoins From 74815b6ff0012c3c54bd8f6dff7bcca6a4bd6a11 Mon Sep 17 00:00:00 2001 From: Federico Kunze Date: Thu, 20 Feb 2020 17:23:31 +0100 Subject: [PATCH 16/16] delete extra comment --- x/ibc/20-transfer/keeper/relay.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/x/ibc/20-transfer/keeper/relay.go b/x/ibc/20-transfer/keeper/relay.go index e0bb1cb0bdfe..bf2553576eb9 100644 --- a/x/ibc/20-transfer/keeper/relay.go +++ b/x/ibc/20-transfer/keeper/relay.go @@ -125,9 +125,6 @@ func (k Keeper) createOutgoingPacket( } } - // NOTE: isSourceChain is negated since the counterparty chain - // - packetData := types.NewFungibleTokenPacketData( amount, sender, receiver, !isSourceChain, destHeight+DefaultPacketTimeout, )