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

disallow timeout height usage when forwarding packets #6641

Merged
merged 9 commits into from
Jun 19, 2024
Merged

disallow timeout height usage when forwarding packets #6641

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f6392b4
disallow non-zero timeout height when forwarding tokens
crodriguezvega Jun 19, 2024
dafff85
typo
crodriguezvega Jun 19, 2024
490f194
move constant
crodriguezvega Jun 19, 2024
ea33133
use time to create default timeout timestamp delta
crodriguezvega Jun 19, 2024
0294c8b
Apply suggestions from code review
colin-axner Jun 19, 2024
0b574b8
refactor: add GetTimeoutTimestamp helper fn to the testing pkg
colin-axner Jun 19, 2024
cc70596
lint
colin-axner Jun 19, 2024
6470c7c
Merge branch 'carlos/6632-disallow-timeout-height-usage-when-forwardi…
colin-axner Jun 19, 2024
3ee74dc
Merge branch 'feat/ics20-v2-path-forwarding' of github.com:cosmos/ibc…
colin-axner Jun 19, 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
2 changes: 1 addition & 1 deletion modules/apps/transfer/ibc_module_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,7 @@ func (suite *TransferTestSuite) TestOnRecvPacket() {
}

seq := uint64(1)
packet = channeltypes.NewPacket(packetData.GetBytes(), seq, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, clienttypes.NewHeight(1, 100), 0)
packet = channeltypes.NewPacket(packetData.GetBytes(), seq, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, clienttypes.ZeroHeight(), uint64(suite.chainA.GetContext().BlockTime().UnixNano())+ibctesting.DefaultTimeoutTimestampDelta)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could it be an idea to create a function to get this value? With all the uses in relay_forwarding_test as well it might make things a bit more tidy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, @gjermundgaraba. If you have something concrete in mind, feel free to push a commit. Otherwise I will merge the PR tonight.

Copy link
Contributor

Choose a reason for hiding this comment

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

I got it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, @colin-axner!


module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), ibctesting.TransferPort)
suite.Require().NoError(err)
Expand Down
3 changes: 2 additions & 1 deletion modules/apps/transfer/keeper/forwarding.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
sdk "github.com/cosmos/cosmos-sdk/types"

"github.com/cosmos/ibc-go/v8/modules/apps/transfer/types"
clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types"
channeltypes "github.com/cosmos/ibc-go/v8/modules/core/04-channel/types"
host "github.com/cosmos/ibc-go/v8/modules/core/24-host"
ibcerrors "github.com/cosmos/ibc-go/v8/modules/core/errors"
Expand All @@ -33,7 +34,7 @@ func (k Keeper) forwardPacket(ctx sdk.Context, data types.FungibleTokenPacketDat
receivedCoins,
sender.String(),
data.Receiver,
packet.TimeoutHeight,
clienttypes.ZeroHeight(),
packet.TimeoutTimestamp,
memo,
nextForwardingPath,
Expand Down
22 changes: 11 additions & 11 deletions modules/apps/transfer/keeper/relay_forwarding_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,8 @@ func (suite *KeeperTestSuite) TestPathForwarding() {
sdk.NewCoins(coin),
sender.GetAddress().String(),
receiver.GetAddress().String(),
suite.chainA.GetTimeoutHeight(),
0, "",
clienttypes.ZeroHeight(),
uint64(suite.chainA.GetContext().BlockTime().UnixNano())+ibctesting.DefaultTimeoutTimestampDelta, "",
forwarding,
)
result, err := suite.chainA.SendMsgs(transferMsg)
Expand Down Expand Up @@ -96,8 +96,8 @@ func (suite *KeeperTestSuite) TestEscrowsAreSetAfterForwarding() {
sdk.NewCoins(coin),
sender.GetAddress().String(),
receiver.GetAddress().String(),
suite.chainA.GetTimeoutHeight(),
0, "",
clienttypes.ZeroHeight(),
uint64(suite.chainA.GetContext().BlockTime().UnixNano())+ibctesting.DefaultTimeoutTimestampDelta, "",
forwarding,
)

Expand Down Expand Up @@ -174,8 +174,8 @@ func (suite *KeeperTestSuite) TestHappyPathForwarding() {
sdk.NewCoins(coin),
sender.GetAddress().String(),
receiver.GetAddress().String(),
suite.chainA.GetTimeoutHeight(),
0, "",
clienttypes.ZeroHeight(),
uint64(suite.chainA.GetContext().BlockTime().UnixNano())+ibctesting.DefaultTimeoutTimestampDelta, "",
forwarding,
)

Expand All @@ -195,7 +195,7 @@ func (suite *KeeperTestSuite) TestHappyPathForwarding() {
Amount: amount.String(),
},
}, sender.GetAddress().String(), receiver.GetAddress().String(), "", forwarding)
packetRecv := channeltypes.NewPacket(data.GetBytes(), 2, path1.EndpointA.ChannelConfig.PortID, path1.EndpointA.ChannelID, path1.EndpointB.ChannelConfig.PortID, path1.EndpointB.ChannelID, clienttypes.NewHeight(1, 100), 0)
packetRecv := channeltypes.NewPacket(data.GetBytes(), 2, path1.EndpointA.ChannelConfig.PortID, path1.EndpointA.ChannelID, path1.EndpointB.ChannelConfig.PortID, path1.EndpointB.ChannelID, clienttypes.ZeroHeight(), uint64(suite.chainA.GetContext().BlockTime().UnixNano())+ibctesting.DefaultTimeoutTimestampDelta)

err = suite.chainB.GetSimApp().TransferKeeper.OnRecvPacket(suite.chainB.GetContext(), packetRecv, data)
// If forwarding has been triggered then the async must be true.
Expand Down Expand Up @@ -274,8 +274,8 @@ func (suite *KeeperTestSuite) TestSimplifiedHappyPathForwarding() {
sdk.NewCoins(coinOnA),
sender.GetAddress().String(),
receiver.GetAddress().String(),
suite.chainA.GetTimeoutHeight(),
0, "",
clienttypes.ZeroHeight(),
uint64(suite.chainA.GetContext().BlockTime().UnixNano())+ibctesting.DefaultTimeoutTimestampDelta, "",
forwarding,
)

Expand Down Expand Up @@ -488,8 +488,8 @@ func (suite *KeeperTestSuite) TestAcknowledgementFailureScenario5Forwarding() {
sdk.NewCoins(coin),
sender.GetAddress().String(),
receiver.GetAddress().String(),
suite.chainA.GetTimeoutHeight(),
0, "",
clienttypes.ZeroHeight(),
uint64(suite.chainA.GetContext().BlockTime().UnixNano())+ibctesting.DefaultTimeoutTimestampDelta, "",
forwarding,
)

Expand Down
13 changes: 10 additions & 3 deletions modules/apps/transfer/types/msgs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,9 +106,16 @@ func (msg MsgTransfer) ValidateBasic() error {
return err
}

// We cannot have non-empty memo and non-empty forwarding hops at the same time.
if len(msg.Forwarding.Hops) > 0 && msg.Memo != "" {
return errorsmod.Wrapf(ErrInvalidMemo, "memo must be empty if forwarding path hops is not empty: %s, %s", msg.Memo, msg.Forwarding.Hops)
if len(msg.Forwarding.Hops) > 0 {
// We cannot have non-zero timeout height and non-empty forwarding hops at the same time.
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
if !msg.TimeoutHeight.IsZero() {
return errorsmod.Wrapf(ErrInvalidPacketTimeout, "timeout height must not be set if forwarding path hops is not empty: %s, %s", msg.TimeoutHeight, msg.Forwarding.Hops)
}

// We cannot have non-empty memo and non-empty forwarding hops at the same time.
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
if msg.Memo != "" {
return errorsmod.Wrapf(ErrInvalidMemo, "memo must be empty if forwarding path hops is not empty: %s, %s", msg.Memo, msg.Forwarding.Hops)
}
}

for _, coin := range msg.GetCoins() {
Expand Down
55 changes: 28 additions & 27 deletions modules/apps/transfer/types/msgs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,33 +58,34 @@ func TestMsgTransferValidation(t *testing.T) {
msg *types.MsgTransfer
expError error
}{
{"valid msg with base denom", types.NewMsgTransfer(validPort, validChannel, coins, sender, receiver, timeoutHeight, 0, "", emptyForwarding), nil},
{"valid msg with trace hash", types.NewMsgTransfer(validPort, validChannel, ibcCoins, sender, receiver, timeoutHeight, 0, "", emptyForwarding), nil},
{"multidenom", types.NewMsgTransfer(validPort, validChannel, coins.Add(ibcCoins...), sender, receiver, timeoutHeight, 0, "", emptyForwarding), nil},
{"invalid ibc denom", types.NewMsgTransfer(validPort, validChannel, invalidIBCCoins, sender, receiver, timeoutHeight, 0, "", emptyForwarding), ibcerrors.ErrInvalidCoins},
{"too short port id", types.NewMsgTransfer(invalidShortPort, validChannel, coins, sender, receiver, timeoutHeight, 0, "", emptyForwarding), host.ErrInvalidID},
{"too long port id", types.NewMsgTransfer(invalidLongPort, validChannel, coins, sender, receiver, timeoutHeight, 0, "", emptyForwarding), host.ErrInvalidID},
{"port id contains non-alpha", types.NewMsgTransfer(invalidPort, validChannel, coins, sender, receiver, timeoutHeight, 0, "", emptyForwarding), host.ErrInvalidID},
{"too short channel id", types.NewMsgTransfer(validPort, invalidShortChannel, coins, sender, receiver, timeoutHeight, 0, "", emptyForwarding), host.ErrInvalidID},
{"too long channel id", types.NewMsgTransfer(validPort, invalidLongChannel, coins, sender, receiver, timeoutHeight, 0, "", emptyForwarding), host.ErrInvalidID},
{"too long memo", types.NewMsgTransfer(validPort, validChannel, coins, sender, receiver, timeoutHeight, 0, ibctesting.GenerateString(types.MaximumMemoLength+1), emptyForwarding), types.ErrInvalidMemo},
{"channel id contains non-alpha", types.NewMsgTransfer(validPort, invalidChannel, coins, sender, receiver, timeoutHeight, 0, "", emptyForwarding), host.ErrInvalidID},
{"invalid denom", types.NewMsgTransfer(validPort, validChannel, invalidDenomCoins, sender, receiver, timeoutHeight, 0, "", emptyForwarding), ibcerrors.ErrInvalidCoins},
{"zero coins", types.NewMsgTransfer(validPort, validChannel, zeroCoins, sender, receiver, timeoutHeight, 0, "", emptyForwarding), ibcerrors.ErrInvalidCoins},
{"missing sender address", types.NewMsgTransfer(validPort, validChannel, coins, emptyAddr, receiver, timeoutHeight, 0, "", emptyForwarding), ibcerrors.ErrInvalidAddress},
{"missing recipient address", types.NewMsgTransfer(validPort, validChannel, coins, sender, "", timeoutHeight, 0, "", emptyForwarding), ibcerrors.ErrInvalidAddress},
{"too long recipient address", types.NewMsgTransfer(validPort, validChannel, coins, sender, ibctesting.GenerateString(types.MaximumReceiverLength+1), timeoutHeight, 0, "", emptyForwarding), ibcerrors.ErrInvalidAddress},
{"empty coins", types.NewMsgTransfer(validPort, validChannel, sdk.NewCoins(), sender, receiver, timeoutHeight, 0, "", emptyForwarding), ibcerrors.ErrInvalidCoins},
{"multidenom: invalid denom", types.NewMsgTransfer(validPort, validChannel, coins.Add(invalidDenomCoins...), sender, receiver, timeoutHeight, 0, "", emptyForwarding), ibcerrors.ErrInvalidCoins},
{"multidenom: invalid ibc denom", types.NewMsgTransfer(validPort, validChannel, coins.Add(invalidIBCCoins...), sender, receiver, timeoutHeight, 0, "", emptyForwarding), ibcerrors.ErrInvalidCoins},
{"multidenom: zero coins", types.NewMsgTransfer(validPort, validChannel, zeroCoins, sender, receiver, timeoutHeight, 0, "", emptyForwarding), ibcerrors.ErrInvalidCoins},
{"multidenom: too many coins", types.NewMsgTransfer(validPort, validChannel, make([]sdk.Coin, types.MaximumTokensLength+1), sender, receiver, timeoutHeight, 0, "", emptyForwarding), ibcerrors.ErrInvalidCoins},
{"multidenom: both token and tokens are set", &types.MsgTransfer{validPort, validChannel, coin, sender, receiver, timeoutHeight, 0, "", coins, emptyForwarding}, ibcerrors.ErrInvalidCoins},
{"memo must be empty if forwarding path hops is not empty", types.NewMsgTransfer(validPort, validChannel, coins, sender, receiver, timeoutHeight, 0, "memo", types.NewForwarding("", validHop)), types.ErrInvalidMemo},
{"invalid forwarding info port", types.NewMsgTransfer(validPort, validChannel, coins, sender, receiver, timeoutHeight, 0, "", types.NewForwarding("", types.Hop{PortId: invalidPort, ChannelId: validChannel})), host.ErrInvalidID},
{"invalid forwarding info channel", types.NewMsgTransfer(validPort, validChannel, coins, sender, receiver, timeoutHeight, 0, "", types.NewForwarding("", types.Hop{PortId: validPort, ChannelId: invalidChannel})), host.ErrInvalidID},
{"invalid forwarding info too many hops", types.NewMsgTransfer(validPort, validChannel, coins, sender, receiver, timeoutHeight, 0, "", types.NewForwarding("", generateHops(types.MaximumNumberOfForwardingHops+1)...)), types.ErrInvalidForwarding},
{"invalid forwarding info too long memo", types.NewMsgTransfer(validPort, validChannel, coins, sender, receiver, timeoutHeight, 0, "", types.NewForwarding(ibctesting.GenerateString(types.MaximumMemoLength+1), validHop)), types.ErrInvalidMemo},
{"valid msg with base denom", types.NewMsgTransfer(validPort, validChannel, coins, sender, receiver, clienttypes.ZeroHeight(), 100, "", emptyForwarding), nil},
{"valid msg with trace hash", types.NewMsgTransfer(validPort, validChannel, ibcCoins, sender, receiver, clienttypes.ZeroHeight(), 100, "", emptyForwarding), nil},
{"multidenom", types.NewMsgTransfer(validPort, validChannel, coins.Add(ibcCoins...), sender, receiver, clienttypes.ZeroHeight(), 100, "", emptyForwarding), nil},
{"invalid ibc denom", types.NewMsgTransfer(validPort, validChannel, invalidIBCCoins, sender, receiver, clienttypes.ZeroHeight(), 100, "", emptyForwarding), ibcerrors.ErrInvalidCoins},
{"too short port id", types.NewMsgTransfer(invalidShortPort, validChannel, coins, sender, receiver, clienttypes.ZeroHeight(), 100, "", emptyForwarding), host.ErrInvalidID},
{"too long port id", types.NewMsgTransfer(invalidLongPort, validChannel, coins, sender, receiver, clienttypes.ZeroHeight(), 100, "", emptyForwarding), host.ErrInvalidID},
{"port id contains non-alpha", types.NewMsgTransfer(invalidPort, validChannel, coins, sender, receiver, clienttypes.ZeroHeight(), 100, "", emptyForwarding), host.ErrInvalidID},
{"too short channel id", types.NewMsgTransfer(validPort, invalidShortChannel, coins, sender, receiver, clienttypes.ZeroHeight(), 100, "", emptyForwarding), host.ErrInvalidID},
{"too long channel id", types.NewMsgTransfer(validPort, invalidLongChannel, coins, sender, receiver, clienttypes.ZeroHeight(), 100, "", emptyForwarding), host.ErrInvalidID},
{"too long memo", types.NewMsgTransfer(validPort, validChannel, coins, sender, receiver, clienttypes.ZeroHeight(), 100, ibctesting.GenerateString(types.MaximumMemoLength+1), emptyForwarding), types.ErrInvalidMemo},
{"channel id contains non-alpha", types.NewMsgTransfer(validPort, invalidChannel, coins, sender, receiver, clienttypes.ZeroHeight(), 100, "", emptyForwarding), host.ErrInvalidID},
{"invalid denom", types.NewMsgTransfer(validPort, validChannel, invalidDenomCoins, sender, receiver, clienttypes.ZeroHeight(), 100, "", emptyForwarding), ibcerrors.ErrInvalidCoins},
{"zero coins", types.NewMsgTransfer(validPort, validChannel, zeroCoins, sender, receiver, clienttypes.ZeroHeight(), 100, "", emptyForwarding), ibcerrors.ErrInvalidCoins},
{"missing sender address", types.NewMsgTransfer(validPort, validChannel, coins, emptyAddr, receiver, clienttypes.ZeroHeight(), 100, "", emptyForwarding), ibcerrors.ErrInvalidAddress},
{"missing recipient address", types.NewMsgTransfer(validPort, validChannel, coins, sender, "", clienttypes.ZeroHeight(), 100, "", emptyForwarding), ibcerrors.ErrInvalidAddress},
{"too long recipient address", types.NewMsgTransfer(validPort, validChannel, coins, sender, ibctesting.GenerateString(types.MaximumReceiverLength+1), clienttypes.ZeroHeight(), 100, "", emptyForwarding), ibcerrors.ErrInvalidAddress},
{"empty coins", types.NewMsgTransfer(validPort, validChannel, sdk.NewCoins(), sender, receiver, clienttypes.ZeroHeight(), 100, "", emptyForwarding), ibcerrors.ErrInvalidCoins},
{"multidenom: invalid denom", types.NewMsgTransfer(validPort, validChannel, coins.Add(invalidDenomCoins...), sender, receiver, clienttypes.ZeroHeight(), 100, "", emptyForwarding), ibcerrors.ErrInvalidCoins},
{"multidenom: invalid ibc denom", types.NewMsgTransfer(validPort, validChannel, coins.Add(invalidIBCCoins...), sender, receiver, clienttypes.ZeroHeight(), 100, "", emptyForwarding), ibcerrors.ErrInvalidCoins},
{"multidenom: zero coins", types.NewMsgTransfer(validPort, validChannel, zeroCoins, sender, receiver, clienttypes.ZeroHeight(), 100, "", emptyForwarding), ibcerrors.ErrInvalidCoins},
{"multidenom: too many coins", types.NewMsgTransfer(validPort, validChannel, make([]sdk.Coin, types.MaximumTokensLength+1), sender, receiver, clienttypes.ZeroHeight(), 100, "", emptyForwarding), ibcerrors.ErrInvalidCoins},
{"multidenom: both token and tokens are set", &types.MsgTransfer{validPort, validChannel, coin, sender, receiver, clienttypes.ZeroHeight(), 100, "", coins, emptyForwarding}, ibcerrors.ErrInvalidCoins},
{"timeout height must be zero if forwarding path hops is not empty", types.NewMsgTransfer(validPort, validChannel, coins, sender, receiver, timeoutHeight, 100, "memo", types.NewForwarding("", validHop)), types.ErrInvalidPacketTimeout},
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 is the test that was added and the rest were changed to pass a zero height.

{"memo must be empty if forwarding path hops is not empty", types.NewMsgTransfer(validPort, validChannel, coins, sender, receiver, clienttypes.ZeroHeight(), 100, "memo", types.NewForwarding("", validHop)), types.ErrInvalidMemo},
{"invalid forwarding info port", types.NewMsgTransfer(validPort, validChannel, coins, sender, receiver, clienttypes.ZeroHeight(), 100, "", types.NewForwarding("", types.Hop{PortId: invalidPort, ChannelId: validChannel})), host.ErrInvalidID},
{"invalid forwarding info channel", types.NewMsgTransfer(validPort, validChannel, coins, sender, receiver, clienttypes.ZeroHeight(), 100, "", types.NewForwarding("", types.Hop{PortId: validPort, ChannelId: invalidChannel})), host.ErrInvalidID},
{"invalid forwarding info too many hops", types.NewMsgTransfer(validPort, validChannel, coins, sender, receiver, clienttypes.ZeroHeight(), 100, "", types.NewForwarding("", generateHops(types.MaximumNumberOfForwardingHops+1)...)), types.ErrInvalidForwarding},
{"invalid forwarding info too long memo", types.NewMsgTransfer(validPort, validChannel, coins, sender, receiver, clienttypes.ZeroHeight(), 100, "", types.NewForwarding(ibctesting.GenerateString(types.MaximumMemoLength+1), validHop)), types.ErrInvalidMemo},
}

for _, tc := range testCases {
Expand Down
2 changes: 2 additions & 0 deletions testing/values.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@
// DefaultTrustLevel sets params variables used to create a TM client
DefaultTrustLevel = ibctm.DefaultTrustLevel

DefaultTimeoutTimestampDelta uint64 = uint64(time.Hour.Nanoseconds())

Check failure on line 58 in testing/values.go

View workflow job for this annotation

GitHub Actions / lint 

var-declaration: should omit type uint64 from declaration of var DefaultTimeoutTimestampDelta; it will be inferred from the right-hand side (revive)

Check failure on line 58 in testing/values.go

View workflow job for this annotation

GitHub Actions / lint 

var-declaration: should omit type uint64 from declaration of var DefaultTimeoutTimestampDelta; it will be inferred from the right-hand side (revive)

TestAccAddress = "cosmos17dtl0mjt3t77kpuhg2edqzjpszulwhgzuj9ljs"
TestCoin = sdk.NewCoin(sdk.DefaultBondDenom, sdkmath.NewInt(100))
TestCoins = sdk.NewCoins(TestCoin)
Expand Down
Loading