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

ADR 010: Modular AnteHandler #4942

Merged
merged 16 commits into from
Aug 28, 2019
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 88 additions & 0 deletions docs/architecture/adr-009-modular-antehandler.md
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,94 @@ Pros:
Cons:
1. Cannot wrap antehandlers with decorators like you can with Weave.

### Simple Decorators
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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


This approach takes inspiration from Weave's decorator design while trying to minimize the number of breaking changes to the SDK and maximizing simplicity.
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved

Allow Decorator interface that can be chained together to create an SDK AnteHandler.

This allows users to choose between implementing an AnteHandler by themselves and setting it in the baseapp, or use the decorator pattern to chain their custom decorators with SDK provided decorators in the order they wish.
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved

```golang
// A Decorator wraps an AnteHandler, and can do pre- and post-processing on the next AnteHandler
type Decorator interface {
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved
AnteHandle(ctx Context, tx Tx, simulate bool, next AnteHandler) (newCtx Context, err error)
}
```

```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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@AdityaSripal AdityaSripal Aug 27, 2019

Choose a reason for hiding this comment

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

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.

if len(chain) == 1 {
return func(ctx Context, tx Tx, simulate bool) {
chain[0].AnteHandle(ctx, tx, simulate, nil)
}
}
return func(ctx Context, tx Tx, simulate bool) {
chain[0].AnteHandle(ctx, tx, simulate, ChainDecorators(chain[1:]))
}
}
```

#### Example Code

```golang
// Setup GasMeter, catch OutOfGasPanic and handle appropriately
type SetupDecorator struct{}

func (sud SetupDecorator) AnteHandle(ctx Context, tx Tx, simulate bool, next AnteHandler) (newCtx Context, err error) {
ctx.GasMeter = NewGasMeter(tx.Gas)

defer func() {
// recover from OutOfGas panic and handle appropriately
}

return next(ctx, tx, simulate)
}

// Signature Verification decorator. Verify Signatures and move on
type SigVerifyDecorator struct{}

func (svd SigVerifyDecorator) AnteHandle(ctx Context, tx Tx, simulate bool, next AnteHandler) (newCtx Context, err error) {
// verify sigs. Return error if invalid

// call next antehandler if sigs ok
return next(ctx, tx, simulate)
}

// User-defined Decorator. Can choose to pre- and post-process on AnteHandler
type UserDefinedDecorator struct{
// custom fields
}

func (udd UserDefinedDecorator) AnteHandle(ctx Context, tx Tx, simulate bool, next AnteHandler) (newCtx Context, err error) {
// pre-processing logic

ctx, err = next(ctx, tx, simulate)

// post-processing logic
}
```

```golang
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved
// Create final antehandler by chaining the decorators together
antehandler := ChainDecorators(NewSetupDecorator(), NewSigVerifyDecorator(), NewUserDefinedDecorator())

// Set chained Antehandler in the baseapp
bapp.SetAnteHandler(antehandler)
```

Pros:

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

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

2. Do not need to break baseapp API. Users can still set a single AnteHandler if they choose.

Cons:

1. Decorator pattern may have a deeply nested structure that is hard to understand.
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved
2. Does not make use of the ModuleManager design. Since this is already being used for BeginBlocker/EndBlocker, this proposal seems unaligned with that design pattern.

## Status

> Proposed
Expand Down