ADR 010: Modular AnteHandler #4942

AdityaSripal · 2019-08-21T22:07:46Z

ADR to address #4572. Write-up of different proposals of how to make the AnteHandler more modular and customizable. Included the initial per-module AnteHandler approach that would fit nicely with the already existing module-manager pattern. Included Weave's approach which is to use decorators that can be chained together. Included an approach that tries to have the best of both worlds.

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.
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 · 2019-08-21T22:25:14Z

docs/architecture/adr-009-modular-antehandler.md

+
+Each decorator works like a modularized SDK antehandler function, but it can take in a `next` argument that may be another decorator or a Handler (which does not take in a next argument). These decorators can be chained together, one decorator being passed in as the `next` argument of the previous decorator in the chain. The chain ends in a Router which can take a tx and route to the appropriate msg handler.
+
+A key benefit of this approach is that one Decorator can wrap its internal logic around the next Checker/Deliverer. A weave Decorator may do the following:


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 the sdk.Tx interface to include a Gas 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 calling next may help us avoid changing the sdk.Tx interface. However, any chain that uses something other than auth.StdTx will have to implement their own decorator.

codecov · 2019-08-21T22:38:35Z

Codecov Report

Merging #4942 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4942   +/-   ##
=======================================
  Coverage   55.54%   55.54%           
=======================================
  Files         284      284           
  Lines       17462    17462           
=======================================
  Hits         9700     9700           
  Misses       7067     7067           
  Partials      695      695

tnachen · 2019-08-22T05:10:08Z

Do we have any use cases in mind that we have to support? Each of these with these pros and cons is a bit hard to evaluate without knowing what user functionality we need to support now.
Otherwise, we might make this much more complicated without really much mileage.

ethanfrey

Some first comments.

docs/architecture/adr-009-modular-antehandler.md

ethanfrey · 2019-08-22T06:11:01Z

docs/architecture/adr-009-modular-antehandler.md

+##### User Code:
+
+```golang
+moduleManager.SetAnteHandlerOrder([]AnteHandler(AuthModuleAnteHandler, DistrModuleAnteHandler))


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).

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?

ethanfrey · 2019-08-22T06:15:26Z

docs/architecture/adr-009-modular-antehandler.md

+2. Provides a nested modular structure that isn't possible in the solution above, while also allowing for a linear one-after-the-other structure like the solution above.
+
+Cons:
+1. It is hard to understand at first glance the state updates that would occur after a Decorator runs given the `ctx`, `store`, and `tx`. A Decorator can have an arbitrary number of nested Decorators being called within its function body, each possibly doing some pre- and post-processing before calling the next decorator on the chain. Thus to understand what a Decorator is doing, one must also understand what every other decorator further along the chain is also doing. This can get quite complicated to understand. A linear, one-after-the-other approach while less powerful, may be much easier to reason about.


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 add defer() 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.

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.

ethanfrey · 2019-08-22T12:57:34Z

@tnachen The ante handler makes many decisions and it is hard to customize if a zone doesn't have the same idea as the gaia hub. The main question is do we want a very opinionated sdk that works for gaia hub and all zones must make the same trade-offs, or do we want a flexible library to allow many projects to build custom zones.

Some examples;

Abort CheckTx early, or Run the full tx on a scratch db for better filtering. See some discussions here Prevent Spam Txs #4695 If we had something modular, each zone could choose.
Allow governance set min-fee in DeliverTx (opt-in middleware)
Adding product-level min fee based on the messages run (more complex version of this, useful for eg. DEXs charging fees for trades)
Auto-tagging the Result with all keys that were modified in the database (to make it easy to subscribe to / watch any field - accounts, stake delegated, governance proposal state, etc...) without adding code in each handler.

All of these work with the weave framework and have been tested extensively.

ethanfrey · 2019-08-22T13:06:18Z

For a run-through of the design philosophy I suggest, you can look at this video series: https://vimeo.com/showcase/6189877

The first video can be skipped if you understand the abci architecture well.

Videos 2 and 3 go into the whole tx handling flow of weave, much of which could easily be ported to sdk. Or whichever pieces seem useful. Please watch these videos (and look at code linked below) if you want to understand the decorator pattern. This pattern is called middleware by every major web framework, and used in almost every such case. As these frameworks were the source of the idea of the Handler/Router architecture I introduced in the original cosmos-sdk, it makes sense to look at this context, and make a clear argument why another approach is better, after taking into account the experience of many other devs.

Some examples of the decorator patterns.

These could easily be implemented in the chain fashion:

FeeDecorator
Signature Verification
-> This will also show you how you can use interfaces to avoid requiring auth.StdTx or any other hard-coded type

The power of wrapping (and not just running before):

docs/architecture/adr-009-modular-antehandler.md

alexanderbez · 2019-08-22T20:38:04Z

The main question is do we want a very opinionated sdk that works for gaia hub and all zones must make the same trade-offs, or do we want a flexible library to allow many projects to build custom zones.

I'd argue we can and strive for having both (it's opinionated, but we're striving for better flexibility). Namely, we want an opinionated framework much in the sense Golang is, however, we still want it to be modular and flexible enough to provide developers what they need to build expressive and complex state-machine applications using these opinionated constructs.

@AdityaSripal @ethanfrey @tnachen

Here's my 2 satoshis, this ADR is great in expressing the problem set and the proposals. With regards to the proposals, we already use the decorator pattern in various places in the SDK so this wouldn't be anything new to us -- we understand the pros/cons. Also nitpick, middleware ≠ decorators.

That being said and having further thought about this, both the ModuleManager and decorator pattern both sacrifice readability. That being the case, we should go with the more expressive and flexible approach (being opinionated) while maintaining a dev-friendly UX. I can see how decorators would provide the edge here, but they can be abused.

So my recommendation is that we express in this ADR to recommend going with a "simple" decorator approach. And what I mean by simple here is not having to necessarily "port" things from weave (not to say we can't, but rather adopt in the most minimal changes), but rather define what interfaces/abstractions make sense for a modular AnteHandler in general. This should be pretty trivial. The ADR should define the interface or two that it needs and an example.

rigelrozanski

Thanks @AdityaSripal !
Great ideas, left some comments herein

docs/architecture/adr-009-modular-antehandler.md

rigelrozanski · 2019-08-26T19:05:38Z

docs/architecture/adr-009-modular-antehandler.md

+
+Our recommended approach is to split the AnteHandler functionality into tightly scoped "micro-functions", while preserving the one-after-the-other ordering that would come from the ModuleManager approach.
+
+We can then have a way to chain these micro-functions so that they run one after the other. Modules may define multiple ante micro-functions and then also provide a default per-module AnteHandler that implements a default, suggested order for these micro-functions. 


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:

mm.AnteHandlers( stakingModule.AnteHandler() // full ante handler Combo(bank.SignatureMicroFn, distribution.ValidateMemoMicroFn, ... ) // combination of microfunctions mintingModule.AnteHandler() ... )

edit; I see you've gone into more detail, below, pseudo code earlier on would still be nice though

shouldn't it be

Combo(bank.SignatureMicroFn, distribution.ValidateMemoMicroFn, ... ).AnteHandler()

I'm confused to what Combo does/is

Combo is an invented function which just groups MicroFn's into an AnteHandler

rigelrozanski · 2019-08-26T19:11:52Z

docs/architecture/adr-009-modular-antehandler.md

+##### User Code:
+
+```golang
+moduleManager.SetAnteHandlerOrder([]AnteHandler(AuthModuleAnteHandler, DistrModuleAnteHandler))


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)

rigelrozanski · 2019-08-26T19:15:24Z

docs/architecture/adr-009-modular-antehandler.md

+##### User Code:
+
+```golang
+moduleManager.SetAnteHandlerOrder([]AnteHandler(AuthModuleAnteHandler, DistrModuleAnteHandler))


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?

ethanfrey · 2019-08-27T10:57:33Z

docs/architecture/adr-009-modular-antehandler.md

+```golang
+// ChainDecorators will recursively link all of the Decorators in the chain and return a final AnteHandler function
+// This is done to preserve the ability to set a single AnteHandler function in the baseapp.
+func ChainDecorators(chain ...Decorator) AnteHandler {


This does decorate the chain of AnteHandlers.
But returns before the Handler is called.

In such a case, the simple ordered-list of ante handlers makes as much sense.
The point of decorators was to wrap the handler execution and be able to do cleanup after the Handler was called.

If you just want to build a AnteHandler and not call defered code after execution, then the simpler chain approach works well.

Maybe you can illustrate with a defer oog check example @AdityaSripal ?

@ethanfrey I'm not following, this the example code provided here all the ante-handlers have the capability of calling code after the next antehandler was called (as depicted in the UserDefinedDecorator) - I think that is the whole point of using decorators as opposed to just calling a list of non-decorated ante-handlers sequentially.

This decorates the chain of AnteHandlers so that it is possible for one AnteDecorator to wrap and cleanup the next antehandler. It is clearly more powerful than the chained micro-function approach since the chained micro-function cannot wrap over each other (So it cannot do defer-cleanup).

It's true, this does not allow users to decorate a MsgHandler or wrap the entire handler logic with a decorator. It's a deliberate choice to make decorators capable of modifying the authentication and validation work of AnteHandler, since that is where most of the use-case for decorators lie in my opinion.

I don't think there's any reason why a decorator for ModuleA should have the power to modify the results from a MsgHandler in ModuleB. It does make sense that a decorator in ModuleA may perform additional authentication/validation work on a tx however, even if it contains a msg for ModuleB.

ethanfrey · 2019-08-27T10:59:15Z

docs/architecture/adr-009-modular-antehandler.md

+
+Pros:
+
+1. Allows one decorator to pre- and post-process the next AnteHandler, similar to the Weave design.


There is no concept of AnteHandler(); Handler() in weave.
It is Decorator(Handler).

It is not just the AnteHandler. The enormous body of DeliverTx, runTx and runMsg in BaseApp show all the code that cannot be customized. Exactly because there is no other place to put code that runs both before and after all handlers.

I agree all of the functions you mentioned above could do with a refactor that makes them simpler and more readable. I'm not convinced that anything they do should be customizable though.

Why would a chain decide not to catch OOG panics and return with the appropriate result?
Why would a chain decide not to cache the state and only write state updates if msg-handler passes?

I think all chains will want to do this. If you have an example of some baseapp logic that is hardcoded but should be customizable, please share it in a reply.

I think this comes back to the trade-off between being opinionated and being flexible. Given that every chain will be doing this, I don't see why we should make every user remember to add the relevant decorators to their chain and add that unnecessary burden along with the risk that developers forget to do this.

Given that the code in runTx and DeliverTx something every SDK user should be running. I think it makes sense to be opinionated here and have it implemented directly into the DeliverTx implementation.

This along with my comment here is why I think it's better to have decorators deliberately restricted to the AnteHandler, rather than have it wrap the entire Handler execution path.

docs/architecture/adr-009-modular-antehandler.md

fedekunze · 2019-08-27T14:01:42Z

docs/architecture/adr-009-modular-antehandler.md

+
+Our recommended approach is to split the AnteHandler functionality into tightly scoped "micro-functions", while preserving the one-after-the-other ordering that would come from the ModuleManager approach.
+
+We can then have a way to chain these micro-functions so that they run one after the other. Modules may define multiple ante micro-functions and then also provide a default per-module AnteHandler that implements a default, suggested order for these micro-functions. 


shouldn't it be

Combo(bank.SignatureMicroFn, distribution.ValidateMemoMicroFn, ... ).AnteHandler()

I'm confused to what Combo does/is

docs/architecture/adr-009-modular-antehandler.md

alexanderbez

Looks like the Simple Decorators is the approach we're aiming for? @ethanfrey pointed on some issues here. Want to address that @AdityaSripal ?

ethanfrey · 2019-08-27T14:56:25Z

Does anyone else want to respond to Tim?
#4942 (comment)

So we have some clear examples we are trying to solve here.
I added some from my experience with weave, but I think it would be good if you also
add some pain points that lead to the original Modular AnteHandler design.

alexanderbez · 2019-08-27T14:59:59Z

Does anyone else want to respond to Tim?
#4942 (comment)

So we have some clear examples we are trying to solve here.
I added some from my experience with weave, but I think it would be good if you also
add some pain points that lead to the original Modular AnteHandler design.

I believe the Context addresses this...to some extent. The ADR should be concise but we can certainly add a simple example. Any suggestions?

rigelrozanski

reviewed Simple Decorators

docs/architecture/adr-009-modular-antehandler.md

rigelrozanski · 2019-08-27T15:19:55Z

docs/architecture/adr-009-modular-antehandler.md

+```golang
+// ChainDecorators will recursively link all of the Decorators in the chain and return a final AnteHandler function
+// This is done to preserve the ability to set a single AnteHandler function in the baseapp.
+func ChainDecorators(chain ...Decorator) AnteHandler {


@ethanfrey I'm not following, this the example code provided here all the ante-handlers have the capability of calling code after the next antehandler was called (as depicted in the UserDefinedDecorator) - I think that is the whole point of using decorators as opposed to just calling a list of non-decorated ante-handlers sequentially.

docs/architecture/adr-009-modular-antehandler.md

rigelrozanski · 2019-08-27T15:28:35Z

docs/architecture/adr-009-modular-antehandler.md

+Cons:
+1. Cannot wrap antehandlers with decorators like you can with Weave.
+
+### Simple Decorators


K - just reviewed the Simple Decorators approach - seems like not a bad idea, however I'm not a huge fan of the deviation of pattern from the ModuleManager (I'm biased 😛 ). I still like the chained decorator approach the most for a 1-dimentional code path. however My only concern is that we actually need decorators in order to successfully have deferred cleanup for something like OutOfGas errors. If this last statement is correct, then it would seem that decorators are the natural pattern for Ante-Handlers

Seems as though we probably want simple decorators based on slack convo:

Do we by nature need decorators for the ante-handler to perform defer-cleanup. Is there a nice way to accomplish this with the micro-chain approach?

Aditya Sripal 18 minutes ago
The defer runs right before the function it is included in returns. So the defer won’t apply for anything that is further along in the micro-chain since each function will return before the next micro-function in the chain gets run

Aditya Sripal 16 minutes ago
The reason defers work with decorators is that a decorator will return only after all of the nested decorators inside it (the ones further along the chain) return

fp4k 15 minutes ago
That’s how I understood this - which means that if we were to use micro-functions and we wanted to be able to catch all the out-of-gas errors from further ante-handler functions we would need to allow the out-of-gas micro-function to have custom decorator capabilities… and in which case why have a hacky exception right, may as well have that capacity for all ante-handler functions. Seems like we logically need simple decorators

Aditya Sripal 10 minutes ago
Yea, i briefly considered having a best-of-both-worlds approach. There was no good way to do it without making things incredibly ugly and unreadable

Aditya Sripal 9 minutes ago
If we’re allowing both decorators (nested) and micro-functions (one-after-the-other), the execution path for a given chain can be almost comically complicated

alexanderbez

ACK -- the Simple Decorator approach seems like the correct and most straight forward approach.

fedekunze

ACK

docs/architecture/adr-009-modular-antehandler.md

Co-Authored-By: Marko <marbar3778@yahoo.com>

ethanfrey · 2019-08-29T10:09:44Z

You agree on the Simple Decorator approach, right? Which is:

// An AnteDecorator wraps an AnteHandler, and can do pre- and post-processing on the next AnteHandler
type AnteDecorator interface {
    AnteHandle(ctx Context, tx Tx, simulate bool, next AnteHandler) (newCtx Context, err error)
}

correct?

rigelrozanski · 2019-08-29T16:51:40Z

correct

initial adr

2958790

AdityaSripal requested review from alessio, alexanderbez, fedekunze, jackzampolin and rigelrozanski as code owners August 21, 2019 22:07

add ref links

822357c

AdityaSripal commented Aug 21, 2019

View reviewed changes

ethanfrey reviewed Aug 22, 2019

View reviewed changes

alexanderbez suggested changes Aug 22, 2019

View reviewed changes

alexanderbez added 8 commits August 22, 2019 16:18

Update docs/architecture/adr-009-modular-antehandler.md

a966964

Update docs/architecture/adr-009-modular-antehandler.md

05e65e7

Update docs/architecture/adr-009-modular-antehandler.md

142dfb7

Update docs/architecture/adr-009-modular-antehandler.md

3c0475a

Update docs/architecture/adr-009-modular-antehandler.md

46f42cc

Update docs/architecture/adr-009-modular-antehandler.md

02e80c2

Update docs/architecture/adr-009-modular-antehandler.md

318c34d

Update docs/architecture/adr-009-modular-antehandler.md

58c06ce

alexanderbez added T: ADR An issue or PR relating to an architectural decision record R4R labels Aug 22, 2019

address ethan comments. Change AnteHandler return value to ctx, error

fbc6a41

rigelrozanski approved these changes Aug 26, 2019

View reviewed changes

add simple decorators proposal

df805f0

ethanfrey reviewed Aug 27, 2019

View reviewed changes

fedekunze suggested changes Aug 27, 2019

View reviewed changes

alexanderbez suggested changes Aug 27, 2019

View reviewed changes

rigelrozanski reviewed Aug 27, 2019

View reviewed changes

alexanderbez approved these changes Aug 27, 2019

View reviewed changes

alexanderbez requested a review from fedekunze August 27, 2019 16:30

final updates

249e8ef

fedekunze approved these changes Aug 27, 2019

View reviewed changes

tac0turtle reviewed Aug 28, 2019

View reviewed changes

docs/architecture/adr-009-modular-antehandler.md Outdated Show resolved Hide resolved

tac0turtle changed the title ~~ADR 009: Modular AnteHandler~~ ADR 010: Modular AnteHandler Aug 28, 2019

alexanderbez and others added 3 commits August 28, 2019 11:27

Update docs/architecture/adr-009-modular-antehandler.md

c9e3ff7

Co-Authored-By: Marko <marbar3778@yahoo.com>

Merge branch 'master' into aditya/modular-ante-adr

d8825e7

Update TOC

e8e7975

alexanderbez merged commit 10e49c1 into master Aug 28, 2019

alexanderbez deleted the aditya/modular-ante-adr branch August 28, 2019 15:36

This was referenced Aug 28, 2019

Module-Specific AnteHandlers #4572

Closed

WIP: Modular AnteHandler #4583

Closed

AdityaSripal mentioned this pull request Sep 5, 2019

AnteDecorator #5006

Merged

5 tasks


		Each decorator works like a modularized SDK antehandler function, but it can take in a `next` argument that may be another decorator or a Handler (which does not take in a next argument). These decorators can be chained together, one decorator being passed in as the `next` argument of the previous decorator in the chain. The chain ends in a Router which can take a tx and route to the appropriate msg handler.

		A key benefit of this approach is that one Decorator can wrap its internal logic around the next Checker/Deliverer. A weave Decorator may do the following:


		Our recommended approach is to split the AnteHandler functionality into tightly scoped "micro-functions", while preserving the one-after-the-other ordering that would come from the ModuleManager approach.

		We can then have a way to chain these micro-functions so that they run one after the other. Modules may define multiple ante micro-functions and then also provide a default per-module AnteHandler that implements a default, suggested order for these micro-functions.


		Pros:

		1. Allows one decorator to pre- and post-process the next AnteHandler, similar to the Weave design.

ADR 010: Modular AnteHandler #4942

ADR 010: Modular AnteHandler #4942

Conversation

AdityaSripal commented Aug 21, 2019 • edited Loading

AdityaSripal Aug 21, 2019 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Aug 21, 2019 • edited Loading

Codecov Report

tnachen commented Aug 22, 2019

ethanfrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanfrey commented Aug 22, 2019

ethanfrey commented Aug 22, 2019

alexanderbez commented Aug 22, 2019 • edited Loading

rigelrozanski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdityaSripal Aug 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

ethanfrey commented Aug 27, 2019

alexanderbez commented Aug 27, 2019

rigelrozanski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

fedekunze left a comment

Choose a reason for hiding this comment

ethanfrey commented Aug 29, 2019

rigelrozanski commented Aug 29, 2019

AdityaSripal commented Aug 21, 2019 •

edited

Loading

AdityaSripal Aug 21, 2019 •

edited

Loading

codecov bot commented Aug 21, 2019 •

edited

Loading

alexanderbez commented Aug 22, 2019 •

edited

Loading

AdityaSripal Aug 27, 2019 •

edited

Loading