From f6392b402ae83b51d7bf19ac482f96899da8b9a1 Mon Sep 17 00:00:00 2001 From: Carlos Rodriguez Date: Wed, 19 Jun 2024 10:38:42 +0200 Subject: [PATCH 1/7] disallow non-zero timeout height when forwarding tokens --- modules/apps/transfer/ibc_module_test.go | 2 +- modules/apps/transfer/keeper/forwarding.go | 3 +- .../transfer/keeper/relay_forwarding_test.go | 22 ++++---- modules/apps/transfer/types/msgs.go | 13 ++++- modules/apps/transfer/types/msgs_test.go | 55 ++++++++++--------- 5 files changed, 53 insertions(+), 42 deletions(-) diff --git a/modules/apps/transfer/ibc_module_test.go b/modules/apps/transfer/ibc_module_test.go index cbf937dd9a4..f4f883de2ca 100644 --- a/modules/apps/transfer/ibc_module_test.go +++ b/modules/apps/transfer/ibc_module_test.go @@ -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())+1000000000) module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), ibctesting.TransferPort) suite.Require().NoError(err) diff --git a/modules/apps/transfer/keeper/forwarding.go b/modules/apps/transfer/keeper/forwarding.go index 52513f10c4f..72b2e6f2f60 100644 --- a/modules/apps/transfer/keeper/forwarding.go +++ b/modules/apps/transfer/keeper/forwarding.go @@ -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" @@ -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, diff --git a/modules/apps/transfer/keeper/relay_forwarding_test.go b/modules/apps/transfer/keeper/relay_forwarding_test.go index 8cea5d23a0b..89c687f5d52 100644 --- a/modules/apps/transfer/keeper/relay_forwarding_test.go +++ b/modules/apps/transfer/keeper/relay_forwarding_test.go @@ -14,6 +14,8 @@ import ( ibctesting "github.com/cosmos/ibc-go/v8/testing" ) +const defaultTimeoutTimestampDelta = 1000000000 + func (suite *KeeperTestSuite) TestPathForwarding() { amount := sdkmath.NewInt(100) @@ -39,8 +41,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())+, "", forwarding, ) result, err := suite.chainA.SendMsgs(transferMsg) @@ -96,8 +98,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())+defaultTimeoutTimestampDelta, "", forwarding, ) @@ -174,8 +176,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())+defaultTimeoutTimestampDelta, "", forwarding, ) @@ -274,8 +276,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())+defaultTimeoutTimestampDelta, "", forwarding, ) @@ -488,8 +490,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())+defaultTimeoutTimestampDelta, "", forwarding, ) diff --git a/modules/apps/transfer/types/msgs.go b/modules/apps/transfer/types/msgs.go index 9a3d4b821fc..71f4ba59d4e 100644 --- a/modules/apps/transfer/types/msgs.go +++ b/modules/apps/transfer/types/msgs.go @@ -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. + 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. + 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() { diff --git a/modules/apps/transfer/types/msgs_test.go b/modules/apps/transfer/types/msgs_test.go index 49caeb04831..03fc39f61fb 100644 --- a/modules/apps/transfer/types/msgs_test.go +++ b/modules/apps/transfer/types/msgs_test.go @@ -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}, + {"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 { From dafff8563b02d50348d9b8b8b36e51fceae62f3c Mon Sep 17 00:00:00 2001 From: Carlos Rodriguez Date: Wed, 19 Jun 2024 10:40:22 +0200 Subject: [PATCH 2/7] typo --- modules/apps/transfer/keeper/relay_forwarding_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/apps/transfer/keeper/relay_forwarding_test.go b/modules/apps/transfer/keeper/relay_forwarding_test.go index 89c687f5d52..6dcdc871461 100644 --- a/modules/apps/transfer/keeper/relay_forwarding_test.go +++ b/modules/apps/transfer/keeper/relay_forwarding_test.go @@ -42,7 +42,7 @@ func (suite *KeeperTestSuite) TestPathForwarding() { sender.GetAddress().String(), receiver.GetAddress().String(), clienttypes.ZeroHeight(), - uint64(suite.chainA.GetContext().BlockTime().UnixNano())+, "", + uint64(suite.chainA.GetContext().BlockTime().UnixNano())+defaultTimeoutTimestampDelta, "", forwarding, ) result, err := suite.chainA.SendMsgs(transferMsg) From 490f194345ac6d2ed27cfd19282e05ae62599c38 Mon Sep 17 00:00:00 2001 From: Carlos Rodriguez Date: Wed, 19 Jun 2024 10:51:15 +0200 Subject: [PATCH 3/7] move constant --- modules/apps/transfer/ibc_module_test.go | 2 +- .../apps/transfer/keeper/relay_forwarding_test.go | 12 +++++------- testing/values.go | 9 +++++---- 3 files changed, 11 insertions(+), 12 deletions(-) diff --git a/modules/apps/transfer/ibc_module_test.go b/modules/apps/transfer/ibc_module_test.go index f4f883de2ca..2e6b9b55585 100644 --- a/modules/apps/transfer/ibc_module_test.go +++ b/modules/apps/transfer/ibc_module_test.go @@ -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.ZeroHeight(), uint64(suite.chainA.GetContext().BlockTime().UnixNano())+1000000000) + 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) module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), ibctesting.TransferPort) suite.Require().NoError(err) diff --git a/modules/apps/transfer/keeper/relay_forwarding_test.go b/modules/apps/transfer/keeper/relay_forwarding_test.go index 6dcdc871461..63135f8da15 100644 --- a/modules/apps/transfer/keeper/relay_forwarding_test.go +++ b/modules/apps/transfer/keeper/relay_forwarding_test.go @@ -14,8 +14,6 @@ import ( ibctesting "github.com/cosmos/ibc-go/v8/testing" ) -const defaultTimeoutTimestampDelta = 1000000000 - func (suite *KeeperTestSuite) TestPathForwarding() { amount := sdkmath.NewInt(100) @@ -42,7 +40,7 @@ func (suite *KeeperTestSuite) TestPathForwarding() { sender.GetAddress().String(), receiver.GetAddress().String(), clienttypes.ZeroHeight(), - uint64(suite.chainA.GetContext().BlockTime().UnixNano())+defaultTimeoutTimestampDelta, "", + uint64(suite.chainA.GetContext().BlockTime().UnixNano())+ibctesting.DefaultTimeoutTimestampDelta, "", forwarding, ) result, err := suite.chainA.SendMsgs(transferMsg) @@ -99,7 +97,7 @@ func (suite *KeeperTestSuite) TestEscrowsAreSetAfterForwarding() { sender.GetAddress().String(), receiver.GetAddress().String(), clienttypes.ZeroHeight(), - uint64(suite.chainA.GetContext().BlockTime().UnixNano())+defaultTimeoutTimestampDelta, "", + uint64(suite.chainA.GetContext().BlockTime().UnixNano())+ibctesting.DefaultTimeoutTimestampDelta, "", forwarding, ) @@ -177,7 +175,7 @@ func (suite *KeeperTestSuite) TestHappyPathForwarding() { sender.GetAddress().String(), receiver.GetAddress().String(), clienttypes.ZeroHeight(), - uint64(suite.chainA.GetContext().BlockTime().UnixNano())+defaultTimeoutTimestampDelta, "", + uint64(suite.chainA.GetContext().BlockTime().UnixNano())+ibctesting.DefaultTimeoutTimestampDelta, "", forwarding, ) @@ -277,7 +275,7 @@ func (suite *KeeperTestSuite) TestSimplifiedHappyPathForwarding() { sender.GetAddress().String(), receiver.GetAddress().String(), clienttypes.ZeroHeight(), - uint64(suite.chainA.GetContext().BlockTime().UnixNano())+defaultTimeoutTimestampDelta, "", + uint64(suite.chainA.GetContext().BlockTime().UnixNano())+ibctesting.DefaultTimeoutTimestampDelta, "", forwarding, ) @@ -491,7 +489,7 @@ func (suite *KeeperTestSuite) TestAcknowledgementFailureScenario5Forwarding() { sender.GetAddress().String(), receiver.GetAddress().String(), clienttypes.ZeroHeight(), - uint64(suite.chainA.GetContext().BlockTime().UnixNano())+defaultTimeoutTimestampDelta, "", + uint64(suite.chainA.GetContext().BlockTime().UnixNano())+ibctesting.DefaultTimeoutTimestampDelta, "", forwarding, ) diff --git a/testing/values.go b/testing/values.go index e8e6dd006e7..52d856b7097 100644 --- a/testing/values.go +++ b/testing/values.go @@ -28,10 +28,11 @@ const ( FirstConnectionID = "connection-0" // Default params constants used to create a TM client - TrustingPeriod time.Duration = time.Hour * 24 * 7 * 2 - UnbondingPeriod time.Duration = time.Hour * 24 * 7 * 3 - MaxClockDrift time.Duration = time.Second * 10 - DefaultDelayPeriod uint64 = 0 + TrustingPeriod time.Duration = time.Hour * 24 * 7 * 2 + UnbondingPeriod time.Duration = time.Hour * 24 * 7 * 3 + MaxClockDrift time.Duration = time.Second * 10 + DefaultDelayPeriod uint64 = 0 + DefaultTimeoutTimestampDelta = 1 << 12 DefaultChannelVersion = mock.Version InvalidID = "IDisInvalid" From ea33133d42b8b8aafab69a3bb23fe7973584f557 Mon Sep 17 00:00:00 2001 From: Carlos Rodriguez Date: Wed, 19 Jun 2024 14:39:48 +0200 Subject: [PATCH 4/7] use time to create default timeout timestamp delta --- modules/apps/transfer/keeper/relay_forwarding_test.go | 2 +- testing/values.go | 11 ++++++----- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/modules/apps/transfer/keeper/relay_forwarding_test.go b/modules/apps/transfer/keeper/relay_forwarding_test.go index 63135f8da15..97be532c3ce 100644 --- a/modules/apps/transfer/keeper/relay_forwarding_test.go +++ b/modules/apps/transfer/keeper/relay_forwarding_test.go @@ -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. diff --git a/testing/values.go b/testing/values.go index 52d856b7097..4c2685e0793 100644 --- a/testing/values.go +++ b/testing/values.go @@ -28,11 +28,10 @@ const ( FirstConnectionID = "connection-0" // Default params constants used to create a TM client - TrustingPeriod time.Duration = time.Hour * 24 * 7 * 2 - UnbondingPeriod time.Duration = time.Hour * 24 * 7 * 3 - MaxClockDrift time.Duration = time.Second * 10 - DefaultDelayPeriod uint64 = 0 - DefaultTimeoutTimestampDelta = 1 << 12 + TrustingPeriod time.Duration = time.Hour * 24 * 7 * 2 + UnbondingPeriod time.Duration = time.Hour * 24 * 7 * 3 + MaxClockDrift time.Duration = time.Second * 10 + DefaultDelayPeriod uint64 = 0 DefaultChannelVersion = mock.Version InvalidID = "IDisInvalid" @@ -56,6 +55,8 @@ var ( // DefaultTrustLevel sets params variables used to create a TM client DefaultTrustLevel = ibctm.DefaultTrustLevel + DefaultTimeoutTimestampDelta uint64 = uint64(time.Hour.Nanoseconds()) + TestAccAddress = "cosmos17dtl0mjt3t77kpuhg2edqzjpszulwhgzuj9ljs" TestCoin = sdk.NewCoin(sdk.DefaultBondDenom, sdkmath.NewInt(100)) TestCoins = sdk.NewCoins(TestCoin) From 0294c8bbbd1293adb015f26fe2b5691f0d37d72a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?colin=20axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 19 Jun 2024 16:15:34 +0200 Subject: [PATCH 5/7] Apply suggestions from code review --- modules/apps/transfer/types/msgs.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/apps/transfer/types/msgs.go b/modules/apps/transfer/types/msgs.go index 71f4ba59d4e..580ebfa81a2 100644 --- a/modules/apps/transfer/types/msgs.go +++ b/modules/apps/transfer/types/msgs.go @@ -107,12 +107,12 @@ func (msg MsgTransfer) ValidateBasic() error { } if len(msg.Forwarding.Hops) > 0 { - // We cannot have non-zero timeout height and non-empty forwarding hops at the same time. + // when forwarding, the timeout height must not be set 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. + // when forwarding, the memo must be empty 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) } From 0b574b8d2141841be8b9a134410c9bcb28017072 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 19 Jun 2024 16:16:20 +0200 Subject: [PATCH 6/7] refactor: add GetTimeoutTimestamp helper fn to the testing pkg --- modules/apps/transfer/ibc_module_test.go | 2 +- .../apps/transfer/keeper/relay_forwarding_test.go | 12 ++++++------ testing/chain.go | 6 ++++++ 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/modules/apps/transfer/ibc_module_test.go b/modules/apps/transfer/ibc_module_test.go index 2e6b9b55585..171f6d8108f 100644 --- a/modules/apps/transfer/ibc_module_test.go +++ b/modules/apps/transfer/ibc_module_test.go @@ -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.ZeroHeight(), uint64(suite.chainA.GetContext().BlockTime().UnixNano())+ibctesting.DefaultTimeoutTimestampDelta) + packet = channeltypes.NewPacket(packetData.GetBytes(), seq, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, clienttypes.ZeroHeight(), suite.chainA.GetTimeoutTimestamp()) module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), ibctesting.TransferPort) suite.Require().NoError(err) diff --git a/modules/apps/transfer/keeper/relay_forwarding_test.go b/modules/apps/transfer/keeper/relay_forwarding_test.go index 97be532c3ce..dd9bc0f345c 100644 --- a/modules/apps/transfer/keeper/relay_forwarding_test.go +++ b/modules/apps/transfer/keeper/relay_forwarding_test.go @@ -40,7 +40,7 @@ func (suite *KeeperTestSuite) TestPathForwarding() { sender.GetAddress().String(), receiver.GetAddress().String(), clienttypes.ZeroHeight(), - uint64(suite.chainA.GetContext().BlockTime().UnixNano())+ibctesting.DefaultTimeoutTimestampDelta, "", + suite.chainA.GetTimeoutTimestamp(), "", forwarding, ) result, err := suite.chainA.SendMsgs(transferMsg) @@ -97,7 +97,7 @@ func (suite *KeeperTestSuite) TestEscrowsAreSetAfterForwarding() { sender.GetAddress().String(), receiver.GetAddress().String(), clienttypes.ZeroHeight(), - uint64(suite.chainA.GetContext().BlockTime().UnixNano())+ibctesting.DefaultTimeoutTimestampDelta, "", + suite.chainA.GetTimeoutTimestamp(), "", forwarding, ) @@ -175,7 +175,7 @@ func (suite *KeeperTestSuite) TestHappyPathForwarding() { sender.GetAddress().String(), receiver.GetAddress().String(), clienttypes.ZeroHeight(), - uint64(suite.chainA.GetContext().BlockTime().UnixNano())+ibctesting.DefaultTimeoutTimestampDelta, "", + suite.chainA.GetTimeoutTimestamp(), "", forwarding, ) @@ -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.ZeroHeight(), uint64(suite.chainA.GetContext().BlockTime().UnixNano())+ibctesting.DefaultTimeoutTimestampDelta) + packetRecv := channeltypes.NewPacket(data.GetBytes(), 2, path1.EndpointA.ChannelConfig.PortID, path1.EndpointA.ChannelID, path1.EndpointB.ChannelConfig.PortID, path1.EndpointB.ChannelID, clienttypes.ZeroHeight(), suite.chainA.GetTimeoutTimestamp()) err = suite.chainB.GetSimApp().TransferKeeper.OnRecvPacket(suite.chainB.GetContext(), packetRecv, data) // If forwarding has been triggered then the async must be true. @@ -275,7 +275,7 @@ func (suite *KeeperTestSuite) TestSimplifiedHappyPathForwarding() { sender.GetAddress().String(), receiver.GetAddress().String(), clienttypes.ZeroHeight(), - uint64(suite.chainA.GetContext().BlockTime().UnixNano())+ibctesting.DefaultTimeoutTimestampDelta, "", + suite.chainA.GetTimeoutTimestamp(), "", forwarding, ) @@ -489,7 +489,7 @@ func (suite *KeeperTestSuite) TestAcknowledgementFailureScenario5Forwarding() { sender.GetAddress().String(), receiver.GetAddress().String(), clienttypes.ZeroHeight(), - uint64(suite.chainA.GetContext().BlockTime().UnixNano())+ibctesting.DefaultTimeoutTimestampDelta, "", + suite.chainA.GetTimeoutTimestamp(), "", forwarding, ) diff --git a/testing/chain.go b/testing/chain.go index 663e4fe853a..6a264a6e64d 100644 --- a/testing/chain.go +++ b/testing/chain.go @@ -629,6 +629,12 @@ func (chain *TestChain) GetTimeoutHeight() clienttypes.Height { return clienttypes.NewHeight(clienttypes.ParseChainID(chain.ChainID), uint64(chain.GetContext().BlockHeight())+100) } +// GetTimeoutTimestamp is a convenience function which returns a IBC packet timeout timestamp +// to be used for testing. It returns the current block timestamp + default timestamp delta (1 hour). +func (chain *TestChain) GetTimeoutTimestamp() uint64 { + return uint64(chain.GetContext().BlockTime().UnixNano()) + DefaultTimeoutTimestampDelta +} + // DeleteKey deletes the specified key from the ibc store. func (chain *TestChain) DeleteKey(key []byte) { storeKey := chain.GetSimApp().GetKey(exported.StoreKey) From cc70596f9b101939394834d95332b26f00f6323e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 19 Jun 2024 16:22:16 +0200 Subject: [PATCH 7/7] lint --- testing/values.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/testing/values.go b/testing/values.go index 4c2685e0793..d91dcb1c898 100644 --- a/testing/values.go +++ b/testing/values.go @@ -55,7 +55,7 @@ var ( // DefaultTrustLevel sets params variables used to create a TM client DefaultTrustLevel = ibctm.DefaultTrustLevel - DefaultTimeoutTimestampDelta uint64 = uint64(time.Hour.Nanoseconds()) + DefaultTimeoutTimestampDelta = uint64(time.Hour.Nanoseconds()) TestAccAddress = "cosmos17dtl0mjt3t77kpuhg2edqzjpszulwhgzuj9ljs" TestCoin = sdk.NewCoin(sdk.DefaultBondDenom, sdkmath.NewInt(100))