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

AnteDecorator #5006

Merged
merged 42 commits into from
Oct 10, 2019
Merged

AnteDecorator #5006

merged 42 commits into from
Oct 10, 2019

Conversation

AdityaSripal
Copy link
Member

@AdityaSripal AdityaSripal commented Sep 5, 2019

implementation of SimpleDecorators approach described in ADR 010: #4942

closes: #4572


  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote tests
  • Updated relevant documentation (docs/)
  • 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, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@AdityaSripal AdityaSripal added T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). WIP labels Sep 9, 2019
x/auth/ante/sigverify_test.go Outdated Show resolved Hide resolved
x/auth/ante/setup_test.go Outdated Show resolved Hide resolved
x/auth/ante/sigverify_test.go Outdated Show resolved Hide resolved
x/auth/ante/basic_test.go Outdated Show resolved Hide resolved
x/auth/ante/sigverify_test.go Outdated Show resolved Hide resolved
x/auth/ante/sigverify_test.go Outdated Show resolved Hide resolved
x/auth/ante/sigverify_test.go Outdated Show resolved Hide resolved
@AdityaSripal
Copy link
Member Author

Currently all decorators require tx be auth.StdTx which makes this change a bit useless.
Solution: Define specific interfaces for each decorator that tx must fulfill for that decorator to be used

TODOs:

  • Define Decorator-specific tx interfaces
  • Documentation

@codecov
Copy link

codecov bot commented Sep 18, 2019

Codecov Report

Merging #5006 into master will decrease coverage by 0.03%.
The diff coverage is 70.9%.

@@            Coverage Diff             @@
##           master    #5006      +/-   ##
==========================================
- Coverage   54.98%   54.94%   -0.04%     
==========================================
  Files         297      302       +5     
  Lines       18255    18357     +102     
==========================================
+ Hits        10037    10087      +50     
- Misses       7478     7517      +39     
- Partials      740      753      +13

x/auth/ante/sigverify_test.go Outdated Show resolved Hide resolved
x/auth/ante/sigverify_test.go Outdated Show resolved Hide resolved
x/auth/ante/sigverify_test.go Outdated Show resolved Hide resolved
x/auth/ante/sigverify_test.go Outdated Show resolved Hide resolved
x/auth/ante/sigverify_test.go Outdated Show resolved Hide resolved
x/auth/ante/ante_test.go Outdated Show resolved Hide resolved
x/auth/ante/fee.go Outdated Show resolved Hide resolved
x/auth/types/stdtx.go Outdated Show resolved Hide resolved
types/handler.go Show resolved Hide resolved
types/handler.go Show resolved Hide resolved
@@ -354,12 +344,12 @@ func TestAnteHandlerMemoGas(t *testing.T) {
checkInvalidTx(t, anteHandler, ctx, tx, false, sdk.CodeOutOfGas)

// memo too large
fee = types.NewStdFee(9000, sdk.NewCoins(sdk.NewInt64Coin("atom", 0)))
fee = types.NewStdFee(50000, sdk.NewCoins(sdk.NewInt64Coin("atom", 0)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have a better understanding on the increased gas costs now?

x/auth/ante/basic.go Show resolved Hide resolved
Copy link
Contributor

@rigelrozanski rigelrozanski left a comment

Choose a reason for hiding this comment

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

Based on phone call, we should

  • move the decorators to a new package x/auth/ante/decorators
  • add a type assertion of standard tx for each decorator custom tx interface requirements

types/handler.go Show resolved Hide resolved
@alexanderbez
Copy link
Contributor

Agree on StdTx assertions ++

I don't see the rationale though for having a subdirectory or additional nesting. This doesn't introduce any burden as far as I can see. Check out Go's os package.

@AdityaSripal AdityaSripal mentioned this pull request Oct 8, 2019
4 tasks
Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

utACK

Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

One final note on the ChainAnteDecorators godoc. In addition, this definitely warrants changelog entries -- one for any breaking changes clients should know about and one for an improvement stating the cool benefits of this new architecture!

types/handler.go Show resolved Hide resolved
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

testedACK

@alexanderbez alexanderbez merged commit c0223a4 into master Oct 10, 2019
@alexanderbez alexanderbez deleted the aditya/ante-decorator branch October 10, 2019 12:46
@tac0turtle tac0turtle mentioned this pull request Oct 12, 2019
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Module-Specific AnteHandlers
8 participants