-
Notifications
You must be signed in to change notification settings - Fork 586
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.
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
Conversation
… disable relative timeouts by block height
…e transfer CLI inline comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if I would consider this an API breaking change but it definitely has implications on end users. If its going to target v9 anyway I suppose it doesn't matter.
Let me know and I can remove the value which I've currently marked as deprecated.
Published as r4r for e2es, and will manually test tomorrow morning 👍🏻 |
…sistent for both height and timestamp timeouts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @damiannolan!
modules/apps/27-interchain-accounts/controller/client/cli/tx.go
Outdated
Show resolved
Hide resolved
@@ -73,8 +82,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}. |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @damiannolan !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thanks @damiannolan
Description
closes: #3594
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/
) or specification (x/<module>/spec/
).godoc
comments.Files changed
in the Github PR explorer.Codecov Report
in the comment section below once CI passes.