diff --git a/CHANGELOG.md b/CHANGELOG.md index 30f1951b8d7..d341d80f74a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,6 +42,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### API Breaking +* (apps/transfer) [\#2446](https://github.com/cosmos/ibc-go/pull/2446) Remove `SendTransfer` function in favor of a private `sendTransfer` function. All IBC transfers must be initiated with `MsgTransfer`. * (apps/29-fee) [\#2395](https://github.com/cosmos/ibc-go/pull/2395) Remove param space from ics29 NewKeeper function. The field was unused. * (apps/27-interchain-accounts) [\#2133](https://github.com/cosmos/ibc-go/pull/2133) Generates genesis protos in a separate directory to avoid circular import errors. The protobuf package name has changed for the genesis types. * (light-clients/tendermint)[\#1768](https://github.com/cosmos/ibc-go/pull/1768) Removed `AllowUpdateAfterExpiry`, `AllowUpdateAfterMisbehaviour` booleans as they are deprecated (see ADR026) diff --git a/modules/apps/transfer/keeper/mbt_relay_test.go b/modules/apps/transfer/keeper/mbt_relay_test.go index a3591fe5124..c96acfa5ac5 100644 --- a/modules/apps/transfer/keeper/mbt_relay_test.go +++ b/modules/apps/transfer/keeper/mbt_relay_test.go @@ -339,16 +339,18 @@ func (suite *KeeperTestSuite) TestModelBasedRelay() { if !ok { panic("MBT failed to parse amount from string") } - err = suite.chainB.GetSimApp().TransferKeeper.SendTransfer( - suite.chainB.GetContext(), + msg := types.NewMsgTransfer( tc.packet.SourcePort, tc.packet.SourceChannel, sdk.NewCoin(denom, amount), - sender, + sender.String(), tc.packet.Data.Receiver, - clienttypes.NewHeight(1, 110), - 0, - nil) + suite.chainA.GetTimeoutHeight(), 0, // only use timeout height + nil, + ) + + _, err = suite.chainB.GetSimApp().TransferKeeper.Transfer(sdk.WrapSDKContext(suite.chainB.GetContext()), msg) + } case "OnRecvPacket": err = suite.chainB.GetSimApp().TransferKeeper.OnRecvPacket(suite.chainB.GetContext(), packet, tc.packet.Data) diff --git a/modules/apps/transfer/keeper/msg_server.go b/modules/apps/transfer/keeper/msg_server.go index f9c83b8c10b..447f785324a 100644 --- a/modules/apps/transfer/keeper/msg_server.go +++ b/modules/apps/transfer/keeper/msg_server.go @@ -4,6 +4,7 @@ import ( "context" sdk "github.com/cosmos/cosmos-sdk/types" + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" "github.com/cosmos/ibc-go/v6/modules/apps/transfer/types" ) @@ -14,11 +15,19 @@ var _ types.MsgServer = Keeper{} func (k Keeper) Transfer(goCtx context.Context, msg *types.MsgTransfer) (*types.MsgTransferResponse, error) { ctx := sdk.UnwrapSDKContext(goCtx) + if !k.GetSendEnabled(ctx) { + return nil, types.ErrSendDisabled + } + sender, err := sdk.AccAddressFromBech32(msg.Sender) if err != nil { return nil, err } + if k.bankKeeper.BlockedAddr(sender) { + return nil, sdkerrors.Wrapf(sdkerrors.ErrUnauthorized, "%s is not allowed to send funds", sender) + } + sequence, err := k.sendTransfer( ctx, msg.SourcePort, msg.SourceChannel, msg.Token, sender, msg.Receiver, msg.TimeoutHeight, msg.TimeoutTimestamp, msg.Metadata) diff --git a/modules/apps/transfer/keeper/msg_server_test.go b/modules/apps/transfer/keeper/msg_server_test.go index 4cbd63506a6..9d6b5a458cc 100644 --- a/modules/apps/transfer/keeper/msg_server_test.go +++ b/modules/apps/transfer/keeper/msg_server_test.go @@ -19,6 +19,17 @@ func (suite *KeeperTestSuite) TestMsgTransfer() { func() {}, true, }, + { + "send transfers disabled", + func() { + suite.chainA.GetSimApp().TransferKeeper.SetParams(suite.chainA.GetContext(), + types.Params{ + SendEnabled: false, + }, + ) + }, + false, + }, { "invalid sender", func() { @@ -43,31 +54,33 @@ func (suite *KeeperTestSuite) TestMsgTransfer() { } for _, tc := range testCases { - suite.SetupTest() + suite.Run(tc.name, func() { + suite.SetupTest() - path := NewTransferPath(suite.chainA, suite.chainB) - suite.coordinator.Setup(path) + path := NewTransferPath(suite.chainA, suite.chainB) + suite.coordinator.Setup(path) - coin := sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100)) - msg = types.NewMsgTransfer( - path.EndpointA.ChannelConfig.PortID, - path.EndpointA.ChannelID, - coin, suite.chainA.SenderAccount.GetAddress().String(), suite.chainB.SenderAccount.GetAddress().String(), - suite.chainB.GetTimeoutHeight(), 0, // only use timeout height - []byte("custom metadata"), - ) + coin := sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100)) + msg = types.NewMsgTransfer( + path.EndpointA.ChannelConfig.PortID, + path.EndpointA.ChannelID, + coin, suite.chainA.SenderAccount.GetAddress().String(), suite.chainB.SenderAccount.GetAddress().String(), + suite.chainB.GetTimeoutHeight(), 0, // only use timeout height + []byte("custom metadata"), + ) - tc.malleate() + tc.malleate() - res, err := suite.chainA.GetSimApp().TransferKeeper.Transfer(sdk.WrapSDKContext(suite.chainA.GetContext()), msg) + res, err := suite.chainA.GetSimApp().TransferKeeper.Transfer(sdk.WrapSDKContext(suite.chainA.GetContext()), msg) - if tc.expPass { - suite.Require().NotEqual(res.Sequence, uint64(0)) - suite.Require().NoError(err) - suite.Require().NotNil(res) - } else { - suite.Require().Error(err) - suite.Require().Nil(res) - } + if tc.expPass { + suite.Require().NoError(err) + suite.Require().NotNil(res) + suite.Require().NotEqual(res.Sequence, uint64(0)) + } else { + suite.Require().Error(err) + suite.Require().Nil(res) + } + }) } } diff --git a/modules/apps/transfer/keeper/relay.go b/modules/apps/transfer/keeper/relay.go index 5ff3d64ef4a..2c621a4152b 100644 --- a/modules/apps/transfer/keeper/relay.go +++ b/modules/apps/transfer/keeper/relay.go @@ -16,7 +16,7 @@ import ( coretypes "github.com/cosmos/ibc-go/v6/modules/core/types" ) -// SendTransfer handles transfer sending logic. There are 2 possible cases: +// sendTransfer handles transfer sending logic. There are 2 possible cases: // // 1. Sender chain is acting as the source zone. The coins are transferred // to an escrow address (i.e locked) on the sender chain and then transferred @@ -48,34 +48,6 @@ import ( // 4. A -> C : sender chain is sink zone. Denom upon receiving: 'C/B/denom' // 5. C -> B : sender chain is sink zone. Denom upon receiving: 'B/denom' // 6. B -> A : sender chain is sink zone. Denom upon receiving: 'denom' -// -// Note: An IBC Transfer must be initiated using a MsgTransfer via the Transfer rpc handler -func (k Keeper) SendTransfer( - ctx sdk.Context, - sourcePort, - sourceChannel string, - token sdk.Coin, - sender sdk.AccAddress, - receiver string, - timeoutHeight clienttypes.Height, - timeoutTimestamp uint64, - metadata []byte, -) error { - _, err := k.sendTransfer( - ctx, - sourcePort, - sourceChannel, - token, - sender, - receiver, - timeoutHeight, - timeoutTimestamp, - metadata, - ) - return err -} - -// sendTransfer handles transfer sending logic. func (k Keeper) sendTransfer( ctx sdk.Context, sourcePort, @@ -87,14 +59,6 @@ func (k Keeper) sendTransfer( timeoutTimestamp uint64, metadata []byte, ) (uint64, error) { - if !k.GetSendEnabled(ctx) { - return 0, types.ErrSendDisabled - } - - if k.bankKeeper.BlockedAddr(sender) { - return 0, sdkerrors.Wrapf(sdkerrors.ErrUnauthorized, "%s is not allowed to send funds", sender) - } - channel, found := k.channelKeeper.GetChannel(ctx, sourcePort, sourceChannel) if !found { return 0, sdkerrors.Wrapf(channeltypes.ErrChannelNotFound, "port ID (%s) channel ID (%s)", sourcePort, sourceChannel) diff --git a/modules/apps/transfer/keeper/relay_test.go b/modules/apps/transfer/keeper/relay_test.go index 35544917fce..f3016b1e222 100644 --- a/modules/apps/transfer/keeper/relay_test.go +++ b/modules/apps/transfer/keeper/relay_test.go @@ -9,7 +9,6 @@ import ( "github.com/cosmos/ibc-go/v6/modules/apps/transfer/types" clienttypes "github.com/cosmos/ibc-go/v6/modules/core/02-client/types" channeltypes "github.com/cosmos/ibc-go/v6/modules/core/04-channel/types" - host "github.com/cosmos/ibc-go/v6/modules/core/24-host" ibctesting "github.com/cosmos/ibc-go/v6/testing" "github.com/cosmos/ibc-go/v6/testing/simapp" ) @@ -18,142 +17,133 @@ import ( // chainA and coin that orignate on chainB func (suite *KeeperTestSuite) TestSendTransfer() { var ( - amount sdk.Coin - path *ibctesting.Path - sender sdk.AccAddress - err error - metadata []byte + coin sdk.Coin + path *ibctesting.Path + sender sdk.AccAddress + timeoutHeight clienttypes.Height + metadata []byte ) testCases := []struct { - msg string - malleate func() - sendFromSource bool - expPass bool + name string + malleate func() + expPass bool }{ { - "successful transfer from source chain", - func() { - suite.coordinator.CreateTransferChannels(path) - amount = sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100)) - }, true, true, + "successful transfer with native token", + func() {}, true, }, { "successful transfer from source chain with metadata", func() { - suite.coordinator.CreateTransferChannels(path) - amount = sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100)) metadata = []byte("metadata") - }, true, true, + }, true, }, { - "successful transfer with coin from counterparty chain", + "successful transfer with IBC token", + func() { - // send coin from chainA back to chainB - suite.coordinator.CreateTransferChannels(path) - amount = types.GetTransferCoin(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, sdk.DefaultBondDenom, sdk.NewInt(100)) - }, false, true, + // send IBC token back to chainB + coin = types.GetTransferCoin(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, coin.Denom, coin.Amount) + }, true, }, { - "successful transfer with coin from counterparty chain with metadata", + "successful transfer with IBC token and metadata", func() { - // send coin from chainA back to chainB - suite.coordinator.CreateTransferChannels(path) - amount = types.GetTransferCoin(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, sdk.DefaultBondDenom, sdk.NewInt(100)) + // send IBC token back to chainB + coin = types.GetTransferCoin(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, coin.Denom, coin.Amount) metadata = []byte("metadata") - }, false, true, + }, true, }, { "source channel not found", func() { // channel references wrong ID - suite.coordinator.CreateTransferChannels(path) path.EndpointA.ChannelID = ibctesting.InvalidID - amount = sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100)) - }, true, false, + }, false, }, { "transfer failed - sender account is blocked", func() { - suite.coordinator.CreateTransferChannels(path) - amount = sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100)) sender = suite.chainA.GetSimApp().AccountKeeper.GetModuleAddress(types.ModuleName) - }, true, false, + }, false, }, - // createOutgoingPacket tests - // - source chain { "send coin failed", func() { - suite.coordinator.CreateTransferChannels(path) - amount = sdk.NewCoin("randomdenom", sdk.NewInt(100)) - }, true, false, + coin = sdk.NewCoin("randomdenom", sdk.NewInt(100)) + }, false, }, - // - receiving chain { - "send from module account failed", + "failed to parse coin denom", func() { - suite.coordinator.CreateTransferChannels(path) - amount = types.GetTransferCoin(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, " randomdenom", sdk.NewInt(100)) - }, false, false, + coin = types.GetTransferCoin(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, "randomdenom", coin.Amount) + }, false, + }, + { + "send from module account failed, insufficient balance", + func() { + coin = types.GetTransferCoin(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, coin.Denom, coin.Amount.Add(sdk.NewInt(1))) + }, false, }, { "channel capability not found", func() { - suite.coordinator.CreateTransferChannels(path) cap := suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) // Release channel capability suite.chainA.GetSimApp().ScopedTransferKeeper.ReleaseCapability(suite.chainA.GetContext(), cap) - amount = sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100)) - }, true, false, + }, false, + }, + { + "SendPacket fails, timeout height and timeout timestamp are zero", + func() { + timeoutHeight = clienttypes.ZeroHeight() + }, false, }, } for _, tc := range testCases { - tc := tc - - suite.Run(fmt.Sprintf("Case %s", tc.msg), func() { + suite.Run(tc.name, func() { suite.SetupTest() // reset + path = NewTransferPath(suite.chainA, suite.chainB) - suite.coordinator.SetupConnections(path) + suite.coordinator.Setup(path) + + coin = sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100)) sender = suite.chainA.SenderAccount.GetAddress() metadata = []byte{} + timeoutHeight = suite.chainB.GetTimeoutHeight() - tc.malleate() - - if !tc.sendFromSource { - // send coin from chainB to chainA - coinFromBToA := sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100)) - transferMsg := types.NewMsgTransfer(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, coinFromBToA, suite.chainB.SenderAccount.GetAddress().String(), suite.chainA.SenderAccount.GetAddress().String(), clienttypes.NewHeight(1, 110), 0, nil) - _, err = suite.chainB.SendMsgs(transferMsg) - suite.Require().NoError(err) // message committed + // create IBC token on chainA + transferMsg := types.NewMsgTransfer(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, coin, suite.chainB.SenderAccount.GetAddress().String(), suite.chainA.SenderAccount.GetAddress().String(), suite.chainA.GetTimeoutHeight(), 0, nil) + result, err := suite.chainB.SendMsgs(transferMsg) + suite.Require().NoError(err) // message committed - // receive coin on chainA from chainB - fungibleTokenPacket := types.NewFungibleTokenPacketData(coinFromBToA.Denom, coinFromBToA.Amount.String(), suite.chainB.SenderAccount.GetAddress().String(), suite.chainA.SenderAccount.GetAddress().String(), nil) - packet := channeltypes.NewPacket(fungibleTokenPacket.GetBytes(), 1, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, clienttypes.NewHeight(1, 110), 0) + packet, err := ibctesting.ParsePacketFromEvents(result.GetEvents()) + suite.Require().NoError(err) - // get proof of packet commitment from chainB - err = path.EndpointA.UpdateClient() - suite.Require().NoError(err) - packetKey := host.PacketCommitmentKey(packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence()) - proof, proofHeight := path.EndpointB.QueryProof(packetKey) + err = path.RelayPacket(packet) + suite.Require().NoError(err) - recvMsg := channeltypes.NewMsgRecvPacket(packet, proof, proofHeight, suite.chainA.SenderAccount.GetAddress().String()) - _, err = suite.chainA.SendMsgs(recvMsg) - suite.Require().NoError(err) // message committed - } + tc.malleate() - err = suite.chainA.GetSimApp().TransferKeeper.SendTransfer( - suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, amount, - sender, suite.chainB.SenderAccount.GetAddress().String(), suite.chainB.GetTimeoutHeight(), 0, + msg := types.NewMsgTransfer( + path.EndpointA.ChannelConfig.PortID, + path.EndpointA.ChannelID, + coin, sender.String(), suite.chainB.SenderAccount.GetAddress().String(), + timeoutHeight, 0, // only use timeout height metadata, ) + res, err := suite.chainA.GetSimApp().TransferKeeper.Transfer(sdk.WrapSDKContext(suite.chainA.GetContext()), msg) + if tc.expPass { suite.Require().NoError(err) + suite.Require().NotNil(res) } else { suite.Require().Error(err) + suite.Require().Nil(res) } }) }