Skip to content

refactor: remove SendTransfer, require IBC transfers to be initiated with MsgTransfer #2446

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mergify merged 16 commits into from
Oct 11, 2022
Merged

refactor: remove SendTransfer, require IBC transfers to be initiated with MsgTransfer #2446

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
4db8313
refactor: remove SendTransfer public function in favor of MsgTransfer
colin-axner Sep 29, 2022
d2da1c9
add changelog entry
colin-axner Sep 29, 2022
e87087b
self review: increase code coverage, fix minor bugs
colin-axner Sep 29, 2022
7549663
Merge branch 'main' into colin/1918-remove-SendTransfer-pubclicfunc
colin-axner Sep 29, 2022
05098fb
Update CHANGELOG.md
colin-axner Oct 3, 2022
c7bb641
Update modules/apps/transfer/keeper/msg_server_test.go
colin-axner Oct 3, 2022
1f03958
Merge branch 'main' into colin/1918-remove-SendTransfer-pubclicfunc
colin-axner Oct 3, 2022
a11a447
Merge branch 'main' of github.com:cosmos/ibc-go into colin/1918-remov…
colin-axner Oct 6, 2022
a3d80c7
Merge branch 'colin/1918-remove-SendTransfer-pubclicfunc' of github.c…
colin-axner Oct 6, 2022
1b41b9b
Merge branch 'main' of github.com:cosmos/ibc-go into colin/1918-remov…
colin-axner Oct 6, 2022
9351467
self review fix
colin-axner Oct 6, 2022
dabfc5c
Merge branch 'main' into colin/1918-remove-SendTransfer-pubclicfunc
colin-axner Oct 6, 2022
9c4d411
Merge branch 'main' of github.com:cosmos/ibc-go into colin/1918-remov…
colin-axner Oct 11, 2022
a878376
Merge branch 'colin/1918-remove-SendTransfer-pubclicfunc' of github.c…
colin-axner Oct 11, 2022
cca0b61
chore: remove outdated testing comments
colin-axner Oct 11, 2022
9e03e65
Merge branch 'main' into colin/1918-remove-SendTransfer-pubclicfunc
mergify[bot] Oct 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
14 changes: 8 additions & 6 deletions modules/apps/transfer/keeper/mbt_relay_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
9 changes: 9 additions & 0 deletions modules/apps/transfer/keeper/msg_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand All @@ -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)
Expand Down
55 changes: 34 additions & 21 deletions modules/apps/transfer/keeper/msg_server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand All @@ -43,31 +54,33 @@ func (suite *KeeperTestSuite) TestMsgTransfer() {
}

for _, tc := range testCases {
suite.SetupTest()
suite.Run(tc.name, func() {
Copy link
Copy Markdown
Contributor Author

@colin-axner colin-axner Sep 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like using suite.Run because it adds the test case name to the output if it fails

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)
}
})
}
}
38 changes: 1 addition & 37 deletions modules/apps/transfer/keeper/relay.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand All @@ -87,14 +59,6 @@ func (k Keeper) sendTransfer(
timeoutTimestamp uint64,
metadata []byte,
) (uint64, error) {
if !k.GetSendEnabled(ctx) {
Copy link
Copy Markdown
Contributor

@crodriguezvega crodriguezvega Sep 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason to move these checks to the message server?

Copy link
Copy Markdown
Contributor Author

@colin-axner colin-axner Oct 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the message server was added after send enabled, which is why it ended up within "SendTransfer"

The reasoning for why I moved it is because I view it as auxiliary to the transfer logic. It isn't in the IBC specification for ICS20

I visualize the code like this:

// entry point
MsgTransfer

// basic checks/auxiliary logic
IsSendEnabeld?
IsSenderBlocked?

// perform ics20 logic
sendTransfer

I think this should be true for most of our handlers:

// single entry point, Msg...

// basic checks in msg_server.go
isEnabled?
object.ValidateBasic()

// perform logic (typically relay.go)

Copy link
Copy Markdown
Contributor Author

@colin-axner colin-axner Oct 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general I think we should avoid overloading functions. Basic enablement checks make sense to go directly in the entrypoint (msg_server.go), while functions nested deeper in the handler can focus on ICS logic entirely

Copy link
Copy Markdown
Contributor

@damiannolan damiannolan Oct 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree++

Definitely a good way to reason about things!

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)
Expand Down
Loading