Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(tx)!: make timeout_height time based #20870
feat(tx)!: make timeout_height time based #20870
Changes from 28 commits
8d367e6
6de6ec3
cce6b54
6a1fe37
6b142ec
6e2aa27
ae1ca05
59248f5
ce2c7d2
559e0be
a178e9a
02d63dc
efcb0b9
ca0b587
4810fd1
e17ddc8
975c968
2f69f6f
f978027
1013006
c1ba08e
680082e
4c00978
96507c5
d6d2a40
b439a8a
b5b3031
424724b
70f772d
b3a89f5
3724d9d
0fe0792
c74e08f
6edb58e
7b2d554
234d250
4297c62
db37bfa
1657027
55f24fb
9ac9527
e5b2a8e
4323483
0272673
a85d969
f6c0df5
bc94df3
2074128
48c0c5f
4c6c03f
ff273d1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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.
Unrelated to this PR but these types of tests are fragile and hard to maintain.
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.
nit, mark timeout height and timeout timestamp mutually exclusive:
cmd.MarkFlagsMutuallyExclusive(flag1, flag2)
EDIT: should the height flag even stay??
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.
do we need it for ordered tx ? if not then we can remove the height flag all together
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.
cc @tac0turtle
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.
id remove height flag and just go with time. Its slightly breaking but b etter to push people to the better design
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.
should we also remove
timeout_height
in txbody or keep it for nowThere 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.
We shouldn't remove anything from tx body. We can't do that type of breakage in production proto types. We also should still support it in the state machine even if we remove from CLI
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.
gotcha
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.
removed the timeout_height flag
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.
Then we do need to flag this as CLI breaking
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.
so what
timeout_timestamp
is for if we should usetimeout_height
?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.
Add documentation for
ErrTxTimeout
.The new error constant
ErrTxTimeout
is added correctly. However, it lacks documentation. Ensure it follows the existing pattern of documenting errors.