REVIEW: Don't abort after AnteHandler if CheckTx #510
Conversation
Currently we are aborting running a transaction after the AnteHandler in case it is a checkTx. This means that the handler never gets the chance to check the validity of a transaction. Furthermore the AnteHandler should not handle CheckTx logic. The AnteHandler should handle global validation, whereas each Handler should handle module validation.
Codecov Report
@@ Coverage Diff @@
## develop #510 +/- ##
===========================================
+ Coverage 53.89% 53.91% +0.01%
===========================================
Files 27 27
Lines 1542 1547 +5
===========================================
+ Hits 831 834 +3
- Misses 660 661 +1
- Partials 51 52 +1 |
adrianbrink
changed the title
| app.anteHandler = ah | ||
| } | ||
|
|
||
| // nolint - Get functions |
There was a problem hiding this comment.
Why remove this?
Maybe add a proper comment if you don't like the placeholder.
| @@ -172,7 +170,6 @@ func (app *BaseApp) NewContext(isCheckTx bool, header abci.Header) sdk.Context { | |||
|
|
|||
| // Implements ABCI | |||
| func (app *BaseApp) Info(req abci.RequestInfo) abci.ResponseInfo { | |||
|
|
|||
There was a problem hiding this comment.
I actually prefer these leading spaces when there is a long function header.
I think it is personal preference, and maybe you can respect the code style in the repo. These lines came from Jae, and I agree with this style.
| // Run the ante handler. | ||
| newCtx, result, abort := app.anteHandler(ctx, tx) | ||
| if isCheckTx || abort { | ||
| if abort { |
There was a problem hiding this comment.
This is correct, but did you make sure all implementations handle checktx properly?
There was a problem hiding this comment.
What implementations are you referring to?
In the worst case all implementations will now just do extra work against the CheckTx state. So in the case of the EVM, it would now run the full transaction through the EVM instead of returning beforehand.
There was a problem hiding this comment.
CheckTx is supposed to return a non-zero count.
I refer to at least bank/handler. But also other Handlers that are in the codebase.
|
Thanks. I like the spaces after long function sigs too. This is fine - our handlers are all sufficiently simple that we want them to run in full, but we need to better distinguish the difference between check/deliver and how fees work ... see #518 Also it means, before this PR, we cant send coins to someone with 0 coins and then have them send them on within a single block because the transfer doesn't run - so someone should write a test for this ... #517 |
|
I don't mind the spaces after function names. But we should be consistent. |
Original PR opened by @jackzampolin cosmos/gaia#476 Co-authored-by: Alexander Bezobchuk <alexanderbez@users.noreply.github.com> Co-authored-by: Jack Zampolin <jack.zampolin@gmail.com> Co-authored-by: Colin Axner <colin@interchain.berlin> Co-authored-by: Marko Baricevic <marko@interchain.berlin> Co-authored-by: Josh Lee <josh@tendermint.com> Co-authored-by: Gautier Marin <gautier@tendermint.com> Co-authored-by: Sam Hart <sam@hxrts.com> Co-authored-by: Shahan Khatchadourian <shahan.k.code@gmail.com> Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com> Co-authored-by: Mircea Colonescu <mircea@tendermint.com> Co-authored-by: Erik Grinaker <erik@tendermint.com> Co-authored-by: Billy Rennekamp <billy@interchain.berlin> Co-authored-by: Denis Fadeev <denis@tendermint.com> Co-authored-by: Chris Goes <cwgoes@interchain.berlin> Co-authored-by: Jacob Gadikian <jacobgadikian@gmail.com> Co-authored-by: Gianguido Sora <me@gsora.xyz> Co-authored-by: Zaki Manian <zaki@iqlusion.io> Co-authored-by: Chris Remus Co-authored-by: Melea
…#19312) (cosmos#510) * feat(x/gov): implement a minimum amount per deposit (cosmos#18146) Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> Co-authored-by: Julien Robert <julien@rbrt.fr> * Fix * Fix test * Fix e2e * Fix e2e * Fix test * update changelog * Include expedited proposal logic * Update deposit.go --------- Co-authored-by: MSalopek <matija.salopek994@gmail.com> Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> Co-authored-by: Julien Robert <julien@rbrt.fr> Co-authored-by: Adam Tucker <adam@osmosis.team> Co-authored-by: Adam Tucker <adamleetucker@outlook.com>
…cosmos#19312) (cosmos#510)" This reverts commit 106ba23.
Currently we are aborting running a transaction after the AnteHandler in
case it is a checkTx. This means that the handler never gets the chance
to check the validity of a transaction. Furthermore the AnteHandler
should not handle CheckTx logic.
The AnteHandler should handle global validation, whereas each Handler
should handle module validation.
closes #433