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

…with MsgTransfer (backport #2446) (#2527)

* 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

* fix test

Co-authored-by: colin axnér <[email protected]>
Co-authored-by: Carlos Rodriguez <[email protected]>

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

@@ -339,16 +339,17 @@ 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(0, 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)

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)

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)
+			}
+		})
 	}
 }

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)