Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
ADR 010: Modular AnteHandler #4942
ADR 010: Modular AnteHandler #4942
Changes from 11 commits
2958790
822357c
a966964
05e65e7
142dfb7
3c0475a
46f42cc
02e80c2
318c34d
58c06ce
fbc6a41
df805f0
249e8ef
c9e3ff7
d8825e7
e8e7975
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
After reading this, it's a bit confusing to understand what we're talking about. Message handlers vs chained-antehandler calls?
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 example of where the decorator approach would be useful for us is to have a Decorator that wraps all other AnteHandlers with a defer clause that will catch an
OutOfGas
panic and return the appropriate sdk Result and abort.In the per-module implementation, it was clear that having the same defer clause in every antehandler made no sense. The best way I found to handle this was to set the
ctx.GasMeter
in the module manager's antehandler and then place a defer clause there. However, doing this required changing thesdk.Tx
interface to include aGas
method.This can be seen here.
Having a decorator that wraps all subsequent antehandler functions to set the
GasMeter
and implements the defer clause correctly before callingnext
may help us avoid changing thesdk.Tx
interface. However, any chain that uses something other thanauth.StdTx
will have to implement their own 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.
I think it is not much harder to reason about.
Decorators are short and should be well-documented.
The only difference with the chained approach is they allow for optional cleanup.
In many cases, they will be
do_check(); next()
, as in chain. But you can adddefer()
clauses to handle panics, or various other cleanups - eg. we can wrap the store to record all keys written, and then auto-add tags to the result.But I guess "hard to reason about" is quite subjective here. I would recommend you refer to existing weave decorators you find confusing. There is only one I can think of, required by complex business logic from iov.
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.
This isn't anything specific to weave. Long chains of middleware/decorator calls do not lend themselves to be easily "readable" -- this is what I think @AdityaSripal meant here and I don't think it's too subjective. We already use this pattern in certain places in the SDK and you have to have some context to understand the execution path.
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.
May be worth adding another sentence here or depictionary pseudo-code discussing the various forms which the app may take. I'm assuming based on this first description one couldn't mix usage of the "full" ante-handler and the micro-functions... although maybe this would also be great? Something like:
edit; I see you've gone into more detail, below, pseudo code earlier on would still be nice though
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.
shouldn't it be
I'm confused to what
Combo
does/isThere 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.
Combo is an invented function which just groups MicroFn's into an AnteHandler
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.
I know you mention weave as hard to reason - but here, the actual stack is split over multiple modules, making it quite hard to see all the pieces and reason about it.
At the least, the entire flow should be visible in one place, to reduce the error of forgetting or duplicating some check (or placing in the wrong order).
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.
Yeah maybe? I like the idea of having all the "micro-functions" names listed in one place as you've suggested - which would make all operations visible simultaneously. I also like the idea of enclosing some more of the redundant microfunctions into larger "ante-handlers" default implementations from a module.
It's a tough call - I think allowing for both is not an unreasonable approach but maybe for Gaia we implement them fully exposed for good practice?
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.
I wouldn't mind seeing this piece of code broken out with more items, maybe combining "full" ante-handlers from modules, with combination ante-handlers (as I mentioned in my earlier comment)