Skip to content
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

Jump to bottom

chore(api)!: move checks from Transfer to OnSendPacket #7068

Merged
merged 19 commits into from
Aug 8, 2024
Merged

chore(api)!: move checks from Transfer to OnSendPacket #7068

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
3425726
deps: bump cosmos-sdk to v0.50.9 (#6828)
damiannolan Aug 7, 2024
7285344
chore(api!): move checks from Transfer to OnSendPacket
bznein Aug 7, 2024
ca07c45
Merge branch 'feat/port-router' into bznein/6949/msgTransferWrapperTo…
bznein Aug 7, 2024
f0f21ff
Fix merge error
bznein Aug 7, 2024
e0fc870
fix to test
bznein Aug 7, 2024
e98e568
Add Require() back
bznein Aug 7, 2024
3fec066
Update modules/apps/transfer/ibc_module.go
bznein Aug 7, 2024
4d15a77
Linter
bznein Aug 7, 2024
503e790
Removed wrong merge results
bznein Aug 7, 2024
ffa3343
reintroduce test
bznein Aug 7, 2024
3dd7deb
Move check for isSendEnabledCoins to keeper
bznein Aug 7, 2024
36e4b05
new tests
bznein Aug 8, 2024
be9f558
Merge remote-tracking branch 'origin' into bznein/6949/msgTransferWra…
bznein Aug 8, 2024
77fbb3a
fixed test
bznein Aug 8, 2024
ae8ba6b
linter
bznein Aug 8, 2024
a2d0b74
Removed checks and added comment
bznein Aug 8, 2024
4d87e21
removed extra parentheses
bznein Aug 8, 2024
04de448
Revert "Merge remote-tracking branch 'origin' into bznein/6949/msgTra…
bznein Aug 8, 2024
87b2411
Delete commented out code
bznein Aug 8, 2024
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
19 changes: 19 additions & 0 deletions modules/apps/transfer/ibc_module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,14 @@ func (im IBCModule) OnSendPacket(
dataBz []byte,
signer sdk.AccAddress,
) error {
if !im.keeper.GetParams(ctx).SendEnabled {
return types.ErrSendDisabled
}
if im.keeper.IsBlockedAddr(signer) {
return errorsmod.Wrapf(ibcerrors.ErrUnauthorized, "%s is not allowed to send funds", signer)
}


ics20Version, found := im.keeper.GetICS4Wrapper().GetAppVersion(ctx, portID, channelID)
if !found {
return errorsmod.Wrapf(ibcerrors.ErrNotFound, "app version not found for port %s and channel %s", portID, channelID)
Expand All @@ -182,6 +190,17 @@ func (im IBCModule) OnSendPacket(
return err
}

if ics20Version == types.V1 {
// ics20-1 only supports a single coin, so if that is the current version, we must only process a single coin.
if len(data.Tokens) > 1 {
return errorsmod.Wrapf(ibcerrors.ErrInvalidRequest, "cannot transfer multiple coins with %s", types.V1)
}

if len(data.Forwarding.Hops) > 1 {
bznein marked this conversation as resolved.
Show resolved Hide resolved
return errorsmod.Wrapf(ibcerrors.ErrInvalidRequest, "cannot forward coins with %s", types.V1)
}

}
if data.Sender != signer.String() {
return errorsmod.Wrapf(ibcerrors.ErrInvalidAddress, "invalid signer address: expected %s, got %s", data.Sender, signer)
}
Expand Down
5 changes: 0 additions & 5 deletions modules/apps/transfer/keeper/export_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,11 +54,6 @@ func (k Keeper) GetAllForwardedPackets(ctx sdk.Context) []types.ForwardedPacket
return k.getAllForwardedPackets(ctx)
}

// IsBlockedAddr is a wrapper around isBlockedAddr for testing purposes
func (k Keeper) IsBlockedAddr(addr sdk.AccAddress) bool {
return k.isBlockedAddr(addr)
}

// CreatePacketDataBytesFromVersion is a wrapper around createPacketDataBytesFromVersion for testing purposes
func CreatePacketDataBytesFromVersion(appVersion, sender, receiver, memo string, tokens types.Tokens, hops []types.Hop) ([]byte, error) {
return createPacketDataBytesFromVersion(appVersion, sender, receiver, memo, tokens, hops)
Expand Down
2 changes: 1 addition & 1 deletion modules/apps/transfer/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,7 +391,7 @@ func (k Keeper) iterateForwardedPackets(ctx sdk.Context, cb func(packet types.Fo

// IsBlockedAddr checks if the given address is allowed to send or receive tokens.
// The module account is always allowed to send and receive tokens.
func (k Keeper) isBlockedAddr(addr sdk.AccAddress) bool {
func (k Keeper) IsBlockedAddr(addr sdk.AccAddress) bool {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was needed in order to be able to call it from OnSendPacket

moduleAddr := k.authKeeper.GetModuleAddress(types.ModuleName)
if addr.Equals(moduleAddr) {
return false
Expand Down
31 changes: 5 additions & 26 deletions modules/apps/transfer/keeper/msg_server.go
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

there is some commented out code on L54 that can be removed now!

// packetData := types.NewFungibleTokenPacketData(
// 	fullDenomPath, msg.Token.Amount.String(), sender.String(), msg.Receiver, msg.Memo,
// )

Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,6 @@ var _ types.MsgServer = (*Keeper)(nil)
func (k Keeper) Transfer(goCtx context.Context, msg *types.MsgTransfer) (*types.MsgTransferResponse, error) {
ctx := sdk.UnwrapSDKContext(goCtx)

if !k.GetParams(ctx).SendEnabled {
return nil, types.ErrSendDisabled
}

sender, err := sdk.AccAddressFromBech32(msg.Sender)
if err != nil {
return nil, err
Expand All @@ -33,27 +29,6 @@ func (k Keeper) Transfer(goCtx context.Context, msg *types.MsgTransfer) (*types.
return nil, errorsmod.Wrapf(types.ErrSendDisabled, err.Error())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This check was left in this function as the logic is not easy to implement on OnSendPacket, we would have to export BankKeeper but also add a lot of logic to convert coins back to tokens

Copy link
Contributor

Choose a reason for hiding this comment

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

This check is necessary to be moved. We can move it into keeper.OnSendPacket. There's already logic to convert tokens to coin there

Copy link
Contributor

Choose a reason for hiding this comment

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

Note: going forward, users can invoke OnSendPacket without using MsgTransfer, so it's unsafe to have any necessary checks remain in MsgTransfer

}

if k.isBlockedAddr(sender) {
return nil, errorsmod.Wrapf(ibcerrors.ErrUnauthorized, "%s is not allowed to send funds", sender)
}

appVersion, found := k.ics4Wrapper.GetAppVersion(ctx, msg.SourcePort, msg.SourceChannel)
if !found {
return nil, errorsmod.Wrapf(ibcerrors.ErrInvalidRequest, "application version not found for source port: %s and source channel: %s", msg.SourcePort, msg.SourceChannel)
}

if appVersion == types.V1 {
// ics20-1 only supports a single coin, so if that is the current version, we must only process a single coin.
if len(msg.Tokens) > 1 {
return nil, errorsmod.Wrapf(ibcerrors.ErrInvalidRequest, "cannot transfer multiple coins with %s", types.V1)
}

// ics20-1 does not support forwarding, so if that is the current version, we must reject the transfer.
if msg.HasForwarding() {
return nil, errorsmod.Wrapf(ibcerrors.ErrInvalidRequest, "cannot forward coins with %s", types.V1)
}
}

if msg.Forwarding.GetUnwind() {
msg, err = k.unwindHops(ctx, msg)
if err != nil {
Expand All @@ -71,6 +46,10 @@ func (k Keeper) Transfer(goCtx context.Context, msg *types.MsgTransfer) (*types.
tokens = append(tokens, token)
}

appVersion, found := k.ics4Wrapper.GetAppVersion(ctx, msg.SourcePort, msg.SourceChannel)
if !found {
return nil, errorsmod.Wrapf(ibcerrors.ErrInvalidRequest, "application version not found for source port: %s and source channel: %s", msg.SourcePort, msg.SourceChannel)
}
packetDataBz, err := createPacketDataBytesFromVersion(appVersion, sender.String(), msg.Receiver, msg.Memo, tokens, msg.Forwarding.GetHops())
if err != nil {
return nil, err
Expand Down Expand Up @@ -149,7 +128,7 @@ func (k Keeper) unwindHops(ctx sdk.Context, msg *types.MsgTransfer) (*types.MsgT
msg.Forwarding.Hops = append(unwindHops[1:], msg.Forwarding.Hops...)
msg.Forwarding.Unwind = false

// Message is validate again, this would only fail if hops now exceeds maximum allowed.
// Message is validated again, this would only fail if hops now exceeds maximum allowed.
if err := msg.ValidateBasic(); err != nil {
return nil, err
}
Expand Down
2 changes: 1 addition & 1 deletion 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 @@ -107,7 +107,7 @@ func (suite *KeeperTestSuite) TestMsgTransfer() {
// explicitly set to ics20-1 which does not support multi-denom
path.EndpointA.UpdateChannel(func(channel *channeltypes.Channel) { channel.Version = types.V1 })
},
ibcerrors.ErrInvalidRequest,
types.ErrInvalidVersion,
},
{
"failure: cannot unwind native tokens",
Expand Down
10 changes: 7 additions & 3 deletions modules/apps/transfer/keeper/relay.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ func (k Keeper) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet, data t
return err
}

if k.isBlockedAddr(receiver) {
if k.IsBlockedAddr(receiver) {
return errorsmod.Wrapf(ibcerrors.ErrUnauthorized, "%s is not allowed to receive funds", receiver)
}

Expand Down Expand Up @@ -323,7 +323,7 @@ func (k Keeper) refundPacketTokens(ctx sdk.Context, packet channeltypes.Packet,
if err != nil {
return err
}
if k.isBlockedAddr(sender) {
if k.IsBlockedAddr(sender) {
return errorsmod.Wrapf(ibcerrors.ErrUnauthorized, "%s is not allowed to receive funds", sender)
}

Expand All @@ -347,6 +347,10 @@ func (k Keeper) refundPacketTokens(ctx sdk.Context, packet channeltypes.Packet,
return err
}

if k.IsBlockedAddr(sender) {
return errorsmod.Wrapf(ibcerrors.ErrUnauthorized, "%s is not allowed to send funds", sender)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

this is an unnecessary addition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No idea how this got in, sorry :/

if err := k.bankKeeper.SendCoins(ctx, moduleAccountAddr, sender, sdk.NewCoins(coin)); err != nil {
panic(fmt.Errorf("unable to send coins from module to account despite previously minting coins to module account: %v", err))
}
Expand Down Expand Up @@ -430,7 +434,7 @@ func createPacketDataBytesFromVersion(appVersion, sender, receiver, memo string,
case types.V1:
// Sanity check, tokens must always be of length 1 if using app version V1.
if len(tokens) != 1 {
return nil, errorsmod.Wrapf(ibcerrors.ErrInvalidRequest, "cannot transfer multiple coins with %s", types.V1)
return nil, errorsmod.Wrapf(types.ErrInvalidVersion, "length of tokens must be equal to 1 if using %s version", types.V1)
Copy link
Contributor

Choose a reason for hiding this comment

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

would recommend doing changes like this against main since it is unrelated to the port router changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was the result of a weird merge, because we used to panic here so I added a similar change but not identical, will revert

}

token := tokens[0]
Expand Down
17 changes: 1 addition & 16 deletions modules/apps/transfer/keeper/relay_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,21 +169,6 @@ func (suite *KeeperTestSuite) TestSendTransfer() {
// },
// channeltypes.ErrInvalidPacket,
// },
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test now doesn't succeed due to the following logic:

  • Transfer does not check for the version/forwarding combination anymore
  • the function that creates the packet data will ignore everything in forwarding when the version is V1.
    Note that there is still a check on OnSendPacket but it will not be triggered because Hops will not be present.

I'll open an issue to add tests specifically for OnSendPacket since we currently do not have them

Copy link
Contributor

Choose a reason for hiding this comment

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

It should? I wonder if this was not working because of the 1 -> 0 change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ouch thanks, yeah it does. I'm trying to give up coffee starting today and you can see the results :(

"failure: forwarding hops is not empty with ics20-1",
func() {
// Set version to isc20-1.
path.EndpointA.UpdateChannel(func(channel *channeltypes.Channel) {
channel.Version = types.V1
})

forwarding = types.NewForwarding(false, types.NewHop(
path.EndpointA.ChannelConfig.PortID,
path.EndpointA.ChannelID,
))
},
ibcerrors.ErrInvalidRequest,
},
}

for _, tc := range testCases {
Expand Down Expand Up @@ -1259,7 +1244,7 @@ func (suite *KeeperTestSuite) TestCreatePacketDataBytesFromVersion() {
},
func(bz []byte, err error) {
suite.Require().Nil(bz)
suite.Require().ErrorIs(err, ibcerrors.ErrInvalidRequest)
suite.Require().ErrorIs(err, types.ErrInvalidVersion)
},
},
{
Expand Down
Loading