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

Tx Height Timeout #6089

Merged
merged 47 commits into from
Aug 7, 2020
Merged

Tx Height Timeout #6089

merged 47 commits into from
Aug 7, 2020

Conversation

alexanderbez
Copy link
Contributor

@alexanderbez alexanderbez commented Apr 27, 2020 •
edited
Loading

Description

  • rename SigFeeMemoTx to Tx
    • new name can be up for discussion, but SigFeeMemoTx is really a poor choice
  • introduce new TxHeightTimeoutDecorator ante-handler decorator
  • client updates to allow setting timeout-height

closes: #5339

For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@alexanderbez alexanderbez self-assigned this Apr 27, 2020
@webmaster128 webmaster128 mentioned this pull request May 4, 2020
Merged
11 tasks
@aaronc aaronc mentioned this pull request May 4, 2020
Closed
4 tasks
@aaronc aaronc mentioned this pull request May 13, 2020
Closed
27 tasks
@stale stale bot added stale and removed stale labels May 27, 2020
@stale stale bot added the stale label Jun 26, 2020
@alexanderbez alexanderbez removed the WIP label Jul 7, 2020
@alexanderbez
Copy link
Contributor Author

Can we proceed with this now @aaronc?

@cosmos cosmos deleted a comment from stale bot Jul 29, 2020
@cosmos cosmos deleted a comment from stale bot Jul 29, 2020
@alexanderbez alexanderbez added A:automerge Automatically merge PR once all prerequisites pass. pinned labels Jul 29, 2020
@aaronc
Copy link
Member

aaronc commented Jul 29, 2020

Can we proceed with this now @aaronc?

Yes please!

@aaronc
Copy link
Member

aaronc commented Jul 29, 2020 •
edited
Loading

Because of #6863 we should add the timeout field to StdTx as well.

Although that will require adding to SignDoc as well so as an alternative we can have SIGN_MODE_LEGACY_AMINO_JSON fail if timeout is set.

I don't have a strong preference, but it needs to be addressed.

@codecov
Copy link

codecov bot commented Aug 3, 2020 •
edited
Loading

Codecov Report

Merging #6089 into master will increase coverage by 1.08%.
The diff coverage is 65.11%.

@@            Coverage Diff             @@
##           master    #6089      +/-   ##
==========================================
+ Coverage   60.86%   61.94%   +1.08%     
==========================================
  Files         379      519     +140     
  Lines       24737    32088    +7351     
==========================================
+ Hits        15056    19877    +4821     
- Misses       8506    10597    +2091     
- Partials     1175     1614     +439

AdityaSripal
AdityaSripal approved these changes Aug 5, 2020
View reviewed changes
x/ibc-transfer/client/cli/tx.go Outdated Show resolved Hide resolved
client/flags/flags.go Show resolved Hide resolved
aaronc
aaronc requested changes Aug 6, 2020
View reviewed changes
x/auth/types/stdsignmsg.go Show resolved Hide resolved
colin-axner
colin-axner approved these changes Aug 7, 2020
View reviewed changes
Copy link
Contributor

@colin-axner colin-axner left a comment

Choose a reason for hiding this comment

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

ACK

@alexanderbez
Copy link
Contributor Author

@aaronc, mind taking another look at this?

aaronc
aaronc reviewed Aug 7, 2020
View reviewed changes
types/errors/errors.go Outdated Show resolved Hide resolved
x/auth/types/stdtx_test.go Show resolved Hide resolved
aaronc
aaronc approved these changes Aug 7, 2020
View reviewed changes
Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

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

ACK 🎉

@mergify mergify bot merged commit 9ae1766 into master Aug 7, 2020
@mergify mergify bot deleted the bez/5339-tx-height-timeout branch August 7, 2020 23:32
larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
@alexanderbez @colin-axner
@fedekunze @mergify
Tx Height Timeout (cosmos#6089)
c3a67ff 
* Implement TxHeightTimeoutDecorator

* Update error message

* rename type

* set height timeout

* update tests

* update *SignDoc)

* rename

* remove dup

* fix build

* cli updates

* rest updates

* cl++

* Update x/auth/ante/basic.go

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>

* Update x/auth/ante/basic.go

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>

* Update x/auth/ante/basic.go

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>

* Update x/auth/ante/basic.go

Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com>

* rename

* rename

* remove TimeoutHeight from SignDoc

* updates

* fix tests

* update IBC cmd flags

* use omitempty

* update godoc

* add test case to TestStdSignBytes

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
@y3fers0n y3fers0n mentioned this pull request Feb 8, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aaronc aaronc aaronc approved these changes

@AdityaSripal AdityaSripal AdityaSripal approved these changes

@colin-axner colin-axner colin-axner approved these changes

@fedekunze fedekunze fedekunze approved these changes

@alessio alessio Awaiting requested review from alessio

@clevinson clevinson Awaiting requested review from clevinson

@jgimeno jgimeno Awaiting requested review from jgimeno

@amaury1093 amaury1093 Awaiting requested review from amaury1093

Assignees

@alexanderbez alexanderbez

Labels
A:automerge Automatically merge PR once all prerequisites pass. C:x/auth
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

height timeout for a transaction
5 participants
@alexanderbez @aaronc @AdityaSripal @colin-axner @fedekunze