-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
AnteDecorator #5006
Conversation
Currently all decorators require tx be TODOs:
|
Codecov Report
@@ 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 |
…itya/ante-decorator
@@ -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))) |
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 have a better understanding on the increased gas costs now?
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.
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
Agree on 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 |
Co-Authored-By: frog power 4000 <rigel.rozanski@gmail.com>
…mos-sdk into aditya/ante-decorator
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.
utACK
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.
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!
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.
testedACK
implementation of SimpleDecorators approach described in ADR 010: #4942
closes: #4572
docs/
)Unreleased
section inCHANGELOG.md
Files changed
in the github PR explorerFor Admin Use: