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.

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

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

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)

@webmaster128 webmaster128 mentioned this pull request May 4, 2020
11 tasks
@aaronc aaronc mentioned this pull request May 4, 2020
4 tasks
@aaronc aaronc mentioned this pull request May 13, 2020
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

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

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     

x/ibc-transfer/client/cli/tx.go Outdated Show resolved Hide resolved
client/flags/flags.go Show resolved Hide resolved
x/auth/types/stdsignmsg.go Show resolved Hide resolved
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?

types/errors/errors.go Outdated Show resolved Hide resolved
x/auth/types/stdtx_test.go Show resolved Hide resolved
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
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. C:x/auth
Projects
None yet
Development

Successfully merging this pull request may close these issues.

height timeout for a transaction
5 participants