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

refactor: use local clock time only for transfer CLI relative timeouts #5908

Merged
merged 11 commits into from
Mar 4, 2024
Merged

refactor: use local clock time only for transfer CLI relative timeouts #5908

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
78e49d6
refactor: use local clock time for relative timeouts in transfer CLI.…
damiannolan Feb 26, 2024
d2f8360
refactor: add absolute timeout timestamps in ica MsgSendTx CLI. Updat…
damiannolan Feb 26, 2024
9f8f941
chore: rm DefaultRelativePacketTimeoutHeight var from transfer pkg types
damiannolan Feb 27, 2024
de2cbf1
imp: update the transfer cli long help to be more informative and con…
damiannolan Feb 27, 2024
6774cb9
refactor: adjust timeout logic from pr suggestions and move default t…
damiannolan Feb 28, 2024
8413176
refactor: ica cli to be consistent with transfer
damiannolan Feb 28, 2024
3f2d0bb
chore: mv ica packet timeout default to cli pkg
damiannolan Feb 28, 2024
bb74388
Merge branch 'main' into damian/3594-use-local-clocktime-for-clis
damiannolan Feb 28, 2024
5627e49
fix: correctly support absolute timeouts in ica sendTx cli
damiannolan Feb 29, 2024
7f673fb
imp: long desc for ica sendTx cli
damiannolan Mar 1, 2024
f59ad22
Merge branch 'main' into damian/3594-use-local-clocktime-for-clis
chatton Mar 4, 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
36 changes: 29 additions & 7 deletions modules/apps/27-interchain-accounts/controller/client/cli/tx.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
package cli

import (
"errors"
"fmt"
"os"
"strings"
"time"

"github.com/spf13/cobra"

Expand All @@ -22,8 +24,9 @@ const (
// The controller chain channel version
flagVersion = "version"
// The channel ordering
flagOrdering = "ordering"
flagRelativePacketTimeout = "relative-packet-timeout"
flagOrdering = "ordering"
flagPacketTimeoutTimestamp = "packet-timeout-timestamp"
flagAbsoluteTimeouts = "absolute-timeouts"
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
)

func newRegisterInterchainAccountCmd() *cobra.Command {
Expand Down Expand Up @@ -73,8 +76,8 @@ func newSendTxCmd() *cobra.Command {
Use: "send-tx [connection-id] [path/to/packet_msg.json]",
Short: "Send an interchain account tx on the provided connection.",
Long: strings.TrimSpace(`Submits pre-built packet data containing messages to be executed on the host chain
and attempts to send the packet. Packet data is provided as json, file or string. An
appropriate relative timeoutTimestamp must be provided with flag {relative-packet-timeout}`),
and attempts to send the packet. Packet data is provided as json, file or string. A relative timeout timestamp can be provided with flag {packet-timeout-timestamp}.
Copy link
Contributor

Choose a reason for hiding this comment

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

If it's correct that absolute timeout timestamps can be used when flagAbsoluteTimeouts is true and flagPacketTimeoutTimestamp is also set, should we update this doc string to clarify that? At the moment, the way it reads I think a reader would think flagPacketTimeoutTimestamp can only be used for relative timeouts...

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated in 7f673fb

feel free to push another commit if you want to change it.

If no timeout value is set then a default relative timeout value of 10 minutes is used.`),
Args: cobra.ExactArgs(2),
RunE: func(cmd *cobra.Command, args []string) error {
clientCtx, err := client.GetClientTxContext(cmd)
Expand All @@ -101,18 +104,37 @@ appropriate relative timeoutTimestamp must be provided with flag {relative-packe
}
}

relativeTimeoutTimestamp, err := cmd.Flags().GetUint64(flagRelativePacketTimeout)
timeoutTimestamp, err := cmd.Flags().GetUint64(flagPacketTimeoutTimestamp)
if err != nil {
return err
}

msg := types.NewMsgSendTx(owner, connectionID, relativeTimeoutTimestamp, icaMsgData)
absoluteTimeouts, err := cmd.Flags().GetBool(flagAbsoluteTimeouts)
if err != nil {
return err
}

// NOTE: relative timeouts using block height are not supported.
if !absoluteTimeouts {
// use local clock time as reference time for calculating timeout timestamp.
if timeoutTimestamp != 0 {
damiannolan marked this conversation as resolved.
Show resolved Hide resolved
now := time.Now().UnixNano()
if now > 0 {
timeoutTimestamp = uint64(now) + timeoutTimestamp
} else {
return errors.New("local clock time is not greater than Jan 1st, 1970 12:00 AM")
}
}
}

msg := types.NewMsgSendTx(owner, connectionID, timeoutTimestamp, icaMsgData)

return tx.GenerateOrBroadcastTxCLI(clientCtx, cmd.Flags(), msg)
},
}

cmd.Flags().Uint64(flagRelativePacketTimeout, icatypes.DefaultRelativePacketTimeoutTimestamp, "Relative packet timeout in nanoseconds from now. Default is 10 minutes.")
cmd.Flags().Uint64(flagPacketTimeoutTimestamp, icatypes.DefaultRelativePacketTimeoutTimestamp, "Packet timeout timestamp in nanoseconds from now. Default is 10 minutes.")
cmd.Flags().Bool(flagAbsoluteTimeouts, false, "Timeout flags are used as absolute timeouts.")
flags.AddTxFlagsToCmd(cmd)

return cmd
Expand Down
68 changes: 10 additions & 58 deletions modules/apps/transfer/client/cli/tx.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,7 @@ import (
"github.com/cosmos/cosmos-sdk/version"

"github.com/cosmos/ibc-go/v8/modules/apps/transfer/types"
clientutils "github.com/cosmos/ibc-go/v8/modules/core/02-client/client/utils"
clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types"
channelutils "github.com/cosmos/ibc-go/v8/modules/core/04-channel/client/utils"
"github.com/cosmos/ibc-go/v8/modules/core/exported"
)

const (
Expand All @@ -34,11 +31,9 @@ func NewTransferTxCmd() *cobra.Command {
Use: "transfer [src-port] [src-channel] [receiver] [amount]",
Short: "Transfer a fungible token through IBC",
Long: strings.TrimSpace(`Transfer a fungible token through IBC. Timeouts can be specified
as absolute or relative using the "absolute-timeouts" flag. Timeout height can be set by passing in the height string
in the form {revision}-{height} using the "packet-timeout-height" flag. Relative timeout height is added to the block
height queried from the latest consensus state corresponding to the counterparty channel. Relative timeout timestamp
is added to the greater value of the local clock time and the block timestamp queried from the latest consensus state
corresponding to the counterparty channel. Any timeout set to 0 is disabled.`),
as absolute using the "absolute-timeouts" flag. Timeout height can be set by passing in the height string
in the form {revision}-{height} using the "packet-timeout-height" flag. Note, relative timeout height is not supported. Relative timeout timestamp
damiannolan marked this conversation as resolved.
Show resolved Hide resolved
is added to the value of the system user's local clock time. If no timeout value is set then a default relative timeout value of 10 minutes is used.`),
Example: fmt.Sprintf("%s tx ibc-transfer transfer [src-port] [src-channel] [receiver] [amount]", version.AppName),
Args: cobra.ExactArgs(4),
RunE: func(cmd *cobra.Command, args []string) error {
Expand All @@ -65,6 +60,7 @@ corresponding to the counterparty channel. Any timeout set to 0 is disabled.`),
if err != nil {
return err
}

timeoutHeight, err := clienttypes.ParseHeight(timeoutHeightStr)
if err != nil {
return err
Expand All @@ -85,62 +81,18 @@ corresponding to the counterparty channel. Any timeout set to 0 is disabled.`),
return err
}

// if the timeouts are not absolute, retrieve latest block height and block timestamp
// for the consensus state connected to the destination port/channel.
// localhost clients must rely solely on local clock time in order to use relative timestamps.
// NOTE: relative timeouts using block height are not supported.
// if the timeouts are not absolute, CLI users rely solely on local clock time in order to use relative timestamps.
if !absoluteTimeouts {
clientRes, err := channelutils.QueryChannelClientState(clientCtx, srcPort, srcChannel, false)
if err != nil {
return err
}

var clientState exported.ClientState
if err := clientCtx.InterfaceRegistry.UnpackAny(clientRes.IdentifiedClientState.ClientState, &clientState); err != nil {
return err
}

clientHeight, ok := clientState.GetLatestHeight().(clienttypes.Height)
if !ok {
return fmt.Errorf("invalid height type. expected type: %T, got: %T", clienttypes.Height{}, clientState.GetLatestHeight())
}

var consensusState exported.ConsensusState
if clientState.ClientType() != exported.Localhost {
consensusStateRes, err := clientutils.QueryConsensusState(clientCtx, clientRes.IdentifiedClientState.ClientId, clientHeight, false, true)
if err != nil {
return err
}

if err := clientCtx.InterfaceRegistry.UnpackAny(consensusStateRes.ConsensusState, &consensusState); err != nil {
return err
}
}

if !timeoutHeight.IsZero() {
absoluteHeight := clientHeight
absoluteHeight.RevisionNumber += timeoutHeight.RevisionNumber
absoluteHeight.RevisionHeight += timeoutHeight.RevisionHeight
timeoutHeight = absoluteHeight
return errors.New("relative timeouts using block height is not supported")
damiannolan marked this conversation as resolved.
Show resolved Hide resolved
}

// use local clock time as reference time if it is later than the
// consensus state timestamp of the counterparty chain, otherwise
// still use consensus state timestamp as reference.
// for localhost clients local clock time is always used.
// use local clock time as reference time for calculating timeout timestamp.
if timeoutTimestamp != 0 {
damiannolan marked this conversation as resolved.
Show resolved Hide resolved
var consensusStateTimestamp uint64
if consensusState != nil {
consensusStateTimestamp = consensusState.GetTimestamp()
}

now := time.Now().UnixNano()
if now > 0 {
now := uint64(now)
if now > consensusStateTimestamp {
timeoutTimestamp = now + timeoutTimestamp
} else {
timeoutTimestamp = consensusStateTimestamp + timeoutTimestamp
}
timeoutTimestamp = uint64(now) + timeoutTimestamp
} else {
return errors.New("local clock time is not greater than Jan 1st, 1970 12:00 AM")
}
Expand All @@ -154,7 +106,7 @@ corresponding to the counterparty channel. Any timeout set to 0 is disabled.`),
},
}

cmd.Flags().String(flagPacketTimeoutHeight, types.DefaultRelativePacketTimeoutHeight, "Packet timeout block height. The timeout is disabled when set to 0-0.")
cmd.Flags().String(flagPacketTimeoutHeight, "0-0", "Packet timeout block height in the format {revision}-{height}. The timeout is disabled when set to 0-0.")
cmd.Flags().Uint64(flagPacketTimeoutTimestamp, types.DefaultRelativePacketTimeoutTimestamp, "Packet timeout timestamp in nanoseconds from now. Default is 10 minutes. The timeout is disabled when set to 0.")
cmd.Flags().Bool(flagAbsoluteTimeouts, false, "Timeout flags are used as absolute timeouts.")
cmd.Flags().String(flagMemo, "", "Memo to be sent along with the packet.")
Expand Down
17 changes: 5 additions & 12 deletions modules/apps/transfer/types/packet.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,18 +18,11 @@ var (
_ ibcexported.PacketDataProvider = (*FungibleTokenPacketData)(nil)
)

var (
// DefaultRelativePacketTimeoutHeight is the default packet timeout height (in blocks) relative
// to the current block height of the counterparty chain provided by the client state. The
// timeout is disabled when set to 0.
DefaultRelativePacketTimeoutHeight = "0-1000"

// DefaultRelativePacketTimeoutTimestamp is the default packet timeout timestamp (in nanoseconds)
// relative to the current block timestamp of the counterparty chain provided by the client
// state. The timeout is disabled when set to 0. The default is currently set to a 10 minute
// timeout.
DefaultRelativePacketTimeoutTimestamp = uint64((time.Duration(10) * time.Minute).Nanoseconds())
)
// DefaultRelativePacketTimeoutTimestamp is the default packet timeout timestamp (in nanoseconds)
// relative to the current block timestamp of the counterparty chain provided by the client
// state. The timeout is disabled when set to 0. The default is currently set to a 10 minute
// timeout.
var DefaultRelativePacketTimeoutTimestamp = uint64((time.Duration(10) * time.Minute).Nanoseconds())
damiannolan marked this conversation as resolved.
Show resolved Hide resolved

// NewFungibleTokenPacketData constructs a new FungibleTokenPacketData instance
func NewFungibleTokenPacketData(
Expand Down
Loading