refactor: remove SendTransfer, require IBC transfers to be initiated …

…with MsgTransfer (#2446)

## Description

closes: #1918

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

- [x] Targeted PR against correct branch (see [CONTRIBUTING.md](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#pr-targeting))
- [x] Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
- [x] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules/structure.md).
- [x] Wrote unit and integration [tests](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#testing)
- [x] Updated relevant documentation (`docs/`) or specification (`x/<module>/spec/`)
- [x] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code).
- [x] Added a relevant changelog entry to the `Unreleased` section in `CHANGELOG.md`
- [x] Re-reviewed `Files changed` in the Github PR explorer
- [x] Review `Codecov Report` in the comment section below once CI passes

(cherry picked from commit 24b17bd)

# Conflicts:
#	modules/apps/transfer/keeper/mbt_relay_test.go
#	modules/apps/transfer/keeper/msg_server_test.go
#	modules/apps/transfer/keeper/relay.go
#	modules/apps/transfer/keeper/relay_test.go

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.
 * (apps/27-interchain-accounts) [\#2035](https://github.com/cosmos/ibc-go/pull/2035) Interchain accounts host and controller Keepers now expects a keeper which fulfills the expected `ScopedKeeper` interface for the capability keeper.

modules/apps/transfer/keeper/mbt_relay_test.go

@@ -338,15 +338,23 @@ 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,
+<<<<<<< HEAD
 							clienttypes.NewHeight(0, 110),
 							0)
+=======
+							suite.chainA.GetTimeoutHeight(), 0, // only use timeout height
+							nil,
+						)
+
+						_, err = suite.chainB.GetSimApp().TransferKeeper.Transfer(sdk.WrapSDKContext(suite.chainB.GetContext()), msg)
+
+>>>>>>> 24b17bd (refactor: remove SendTransfer, require IBC transfers to be initiated with MsgTransfer (#2446))
 					}
 				case "OnRecvPacket":
 					err = suite.chainB.GetSimApp().TransferKeeper.OnRecvPacket(suite.chainB.GetContext(), packet, tc.packet.Data)

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,
 	)

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,30 +54,43 @@ 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)
 
+<<<<<<< HEAD
 		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
 		)
+=======
+			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"),
+			)
+>>>>>>> 24b17bd (refactor: remove SendTransfer, require IBC transfers to be initiated with MsgTransfer (#2446))
 
-		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)
+			}
+		})
 	}
 }

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,6 +48,7 @@ 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'
+<<<<<<< HEAD
 //
 // Note: An IBC Transfer must be initiated using a MsgTransfer via the Transfer rpc handler
 func (k Keeper) SendTransfer(
@@ -74,6 +75,8 @@ func (k Keeper) SendTransfer(
 }
 
 // sendTransfer handles transfer sending logic.
+=======
+>>>>>>> 24b17bd (refactor: remove SendTransfer, require IBC transfers to be initiated with MsgTransfer (#2446))
 func (k Keeper) sendTransfer(
 	ctx sdk.Context,
 	sourcePort,
@@ -84,6 +87,7 @@ func (k Keeper) sendTransfer(
 	timeoutHeight clienttypes.Height,
 	timeoutTimestamp uint64,
 ) (uint64, error) {
+<<<<<<< HEAD
 	if !k.GetSendEnabled(ctx) {
 		return 0, types.ErrSendDisabled
 	}
@@ -93,6 +97,9 @@ func (k Keeper) sendTransfer(
 	}
 
 	sourceChannelEnd, found := k.channelKeeper.GetChannel(ctx, sourcePort, sourceChannel)
+=======
+	channel, found := k.channelKeeper.GetChannel(ctx, sourcePort, sourceChannel)
+>>>>>>> 24b17bd (refactor: remove SendTransfer, require IBC transfers to be initiated with MsgTransfer (#2446))
 	if !found {
 		return 0, sdkerrors.Wrapf(channeltypes.ErrChannelNotFound, "port ID (%s) channel ID (%s)", sourcePort, sourceChannel)
 	}