From a82fc61f7675f4a35c2e98360104752c0337e1c8 Mon Sep 17 00:00:00 2001 From: Amaury M <1293565+amaurym@users.noreply.github.com> Date: Fri, 20 Aug 2021 15:14:17 +0200 Subject: [PATCH 1/9] 1st draft --- .../adr-045-check-delivertx-middlewares.md | 55 +++++++++++++++++++ 1 file changed, 55 insertions(+) create mode 100644 docs/architecture/adr-045-check-delivertx-middlewares.md diff --git a/docs/architecture/adr-045-check-delivertx-middlewares.md b/docs/architecture/adr-045-check-delivertx-middlewares.md new file mode 100644 index 000000000000..5dd7de4381f9 --- /dev/null +++ b/docs/architecture/adr-045-check-delivertx-middlewares.md @@ -0,0 +1,55 @@ +# ADR 045: BaseApp `{Check,Deliver}Tx` as Middlewares + +## Changelog + +- 20.08.2021: Initial draft. + +## Status + +PROPOSED + +## Abstract + +This ADR replaces the current BaseApp `runTx` and antehandlers design (partially described in [ADR-10](./adr-010-modular-antehandler.md)) with a middleware-based design. + +## Context + +BaseApp's implementation of ABCI `{Check,Deliver}Tx()` and its own `Simulate()` method call the `runTx` method under the hood, which first runs antehandlers, then executes `Msg`s. However, the [transaction Tips](https://github.com/cosmos/cosmos-sdk/issues/9406) feature requires custom logic to be run after the `Msg`s execution. There is currently no way to achieve this. + +An naive solution would be to add post-`Msg` hooks to BaseApp. However, the SDK team thinks in parallel about the bigger picture of making app wiring simpler ([#9181](https://github.com/cosmos/cosmos-sdk/discussions/9182)), which includes making BaseApp more lightweight and modular. + +## Decision + +We decide to transform Baseapp's implementation of ABCI `{Check,Deliver}Tx()` and its own `Simulate()` method to use a middleware-based design. + +## Consequences + +> This section describes the resulting context, after applying the decision. All consequences should be listed here, not just the "positive" ones. A particular decision may have positive, negative, and neutral consequences, but all of them affect the team and project in the future. + +### Backwards Compatibility + +> All ADRs that introduce backwards incompatibilities must include a section describing these incompatibilities and their severity. The ADR must explain how the author proposes to deal with these incompatibilities. ADR submissions without a sufficient backwards compatibility treatise may be rejected outright. + +### Positive + +{positive consequences} + +### Negative + +{negative consequences} + +### Neutral + +{neutral consequences} + +## Further Discussions + +- [#9934](https://github.com/cosmos/cosmos-sdk/discussions/9934) Decomposing BaseApp's other ABCI methods into middlewares. + +## Test Cases [optional] + +Test cases for an implementation are mandatory for ADRs that are affecting consensus changes. Other ADRs can choose to include links to test cases if applicable. + +## References + +- {reference link} From 28d8b93066dfc47c44836d77539a9bc8ef9740ca Mon Sep 17 00:00:00 2001 From: Amaury M <1293565+amaurym@users.noreply.github.com> Date: Mon, 23 Aug 2021 17:14:23 +0200 Subject: [PATCH 2/9] WIP --- .../adr-045-check-delivertx-middlewares.md | 93 ++++++++++++++++++- 1 file changed, 92 insertions(+), 1 deletion(-) diff --git a/docs/architecture/adr-045-check-delivertx-middlewares.md b/docs/architecture/adr-045-check-delivertx-middlewares.md index 5dd7de4381f9..fdc2ea1de5a3 100644 --- a/docs/architecture/adr-045-check-delivertx-middlewares.md +++ b/docs/architecture/adr-045-check-delivertx-middlewares.md @@ -10,7 +10,7 @@ PROPOSED ## Abstract -This ADR replaces the current BaseApp `runTx` and antehandlers design (partially described in [ADR-10](./adr-010-modular-antehandler.md)) with a middleware-based design. +This ADR replaces the current BaseApp `runTx` and antehandlers design with a middleware-based design. ## Context @@ -22,6 +22,93 @@ An naive solution would be to add post-`Msg` hooks to BaseApp. However, the SDK We decide to transform Baseapp's implementation of ABCI `{Check,Deliver}Tx()` and its own `Simulate()` method to use a middleware-based design. +The two following interfaces are the base of the middleware design, and are defined in `types/tx`: + +```go +type Handler interface { + CheckTx(ctx context.Context, tx sdk.Tx, req abci.RequestCheckTx) (abci.ResponseCheckTx, error) + DeliverTx(ctx context.Context, tx sdk.Tx, req abci.RequestDeliverTx) (abci.ResponseDeliverTx, error) + SimulateTx(ctx context.Context, tx sdk.Tx, req RequestSimulateTx) (ResponseSimulateTx, error) +} + +type Middleware func(Handler) Handler +``` + +BaseApp holds a reference to a `tx.Handler`, and the ABCI `{Check,Deliver}Tx()` and `Simulate()` methods simply call `app.txHandler.{Check,Deliver,Simulate}Tx()` with the relevant arguments. For example, for `DeliverTx`: + +```go +func (app *BaseApp) DeliverTx(req abci.RequestDeliverTx) abci.ResponseDeliverTx { + tx, err := app.txDecoder(req.Tx) + if err != nil { + return sdkerrors.ResponseDeliverTx(err, 0, 0, app.trace) + } + + ctx := app.getContextForTx(runTxModeDeliver, req.Tx) + res, err := app.txHandler.DeliverTx(ctx, tx, req) + if err != nil { + return sdkerrors.ResponseDeliverTx(err, uint64(res.GasUsed), uint64(res.GasWanted), app.trace) + } + + return res +} +``` + +The implementations are similar for `BaseApp.CheckTx` and `BaseApp.Simulate`. + +### Composing Middlewares + +While BaseApp simply holds a reference to a `tx.Handler`, this `tx.Handler` itself is defined using a middleware stack. We define a base `tx.Handler` called `RunMsgsTxHandler`, which executes messages. + +Then, the app developer can compose multiple middlewares on top on the base `tx.Handler`. Each middleware can run pre-and-post-processing logic around its next middleware. Conceptually, as an example, given the middlewares `A`, `B`, and `C` and the base `tx.Handler` `H` the stack looks like: + +``` +A.pre + B.pre + C.pre + H // The base handler, usually `RunMsgsTxHandler` + C.post + B.post +A.post +``` + +We define a `ComposeMiddlewares` function for composing middlewares. It takes the base handler as first argument, and middlewares in the "inner to outer" order. For the above stack, the final `tx.Handler` is: + +```go +txHandler := middleware.ComposeMiddlewares(H, C, B, A) +``` + +The middleware is set in BaseApp via its `SetTxHandler` setter: + +```go +// simapp/app.go + +txHandler := middleware.ComposeMiddlewares(...) +app.SetTxHandler(txHandler) +``` + +Naturally, the app developer can define their own middlewares. + +### Middlewares Maintained by the SDK + +While the app developer can define and compose the middlewares of their choice, the SDK provides a set of middlewares that caters for the ecosystem's most common use cases. These middlewares are: + +| Middleware | Description | +| ----------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| RunMsgsTxHandler | This is the base `tx.Handler`. It replaces the old baseapp's `runMsgs`, and executes a transaction's `Msg`s. | +| {Antehandlers} | Each antehandler is converted to its own middleware. | +| IndexEventsTxMiddleware | This is a simple middleware that chooses which events to index in Tendermint. Replaces `baseapp.indexEvents` (which unfortunately still exists in baseapp too, because it's used to index Begin/EndBlock events) | +| RecoveryTxMiddleware | This index recovers from panics. It replaces baseapp.runTx's panic recovery. | +| GasTxMiddleware | This replaces the [`Setup`](https://github.com/cosmos/cosmos-sdk/blob/v0.43.0/x/auth/ante/setup.go) Antehandler. It sets a GasMeter on sdk.Context. Note that before, GasMeter was set on sdk.Context inside the antehandlers, and there was some mess around the fact that antehandlers had their own panic recovery system so that the GasMeter could be read by baseapp's recovery system. Now, this mess is all removed: one middleware sets GasMeter, another one handles recovery. | + +### Similarities and Differences between Antehandlers and Middlewares + +The middleware-based design builds upon the existing antehandlers design described in [ADR-010](./adr-010-modular-antehandler.md). + +#### Similarities + +- Both design are based on chaining/composing small modular pieces. +- + ## Consequences > This section describes the resulting context, after applying the decision. All consequences should be listed here, not just the "positive" ones. A particular decision may have positive, negative, and neutral consequences, but all of them affect the team and project in the future. @@ -53,3 +140,7 @@ Test cases for an implementation are mandatory for ADRs that are affecting conse ## References - {reference link} + +``` + +``` From 4e58e5ea3522d10a69773c03ea3b6efdbc033cfd Mon Sep 17 00:00:00 2001 From: Amaury M <1293565+amaurym@users.noreply.github.com> Date: Tue, 24 Aug 2021 16:57:34 +0200 Subject: [PATCH 3/9] WIP --- .../adr-045-check-delivertx-middlewares.md | 46 +++++++++++++++++-- 1 file changed, 41 insertions(+), 5 deletions(-) diff --git a/docs/architecture/adr-045-check-delivertx-middlewares.md b/docs/architecture/adr-045-check-delivertx-middlewares.md index fdc2ea1de5a3..9af16c1983bf 100644 --- a/docs/architecture/adr-045-check-delivertx-middlewares.md +++ b/docs/architecture/adr-045-check-delivertx-middlewares.md @@ -55,6 +55,36 @@ func (app *BaseApp) DeliverTx(req abci.RequestDeliverTx) abci.ResponseDeliverTx The implementations are similar for `BaseApp.CheckTx` and `BaseApp.Simulate`. +### Implementing a Middleware + +In practice, a middleware is a Go function that takes as arguments some parameters needed for the middleware, and returns a `tx.Middleware`. + +For example, for creating `MyMiddleware`, we can implement: + +```go +// myTxHandler is the +type myTxHandler struct { + // next is the next tx.Handler in the middleware stack. + next tx.Handler + // some other fields that are relevant to the middleware can be added here +} + +// NewMyMiddleware returns a middleware that does this and that. +func NewMyMiddleware(arg1, arg2) tx.Middleware { + return func (txh tx.Handler) tx.Handler { + return myTxHandler{ + next: txh, + // optionally, set arg1, arg2... if they are needed in the middleware + } + } +} + +// Assert myTxHandler is a tx.Handler. +var _ tx.Handler = myTxHandler{} + +func (txh myTxHandler) CheckTx(ctx context.Context, tx sdk.Tx, req abci.RequestCheckTx) (abci.ResponseCheckTx, error) { +``` + ### Composing Middlewares While BaseApp simply holds a reference to a `tx.Handler`, this `tx.Handler` itself is defined using a middleware stack. We define a base `tx.Handler` called `RunMsgsTxHandler`, which executes messages. @@ -71,10 +101,10 @@ A.pre A.post ``` -We define a `ComposeMiddlewares` function for composing middlewares. It takes the base handler as first argument, and middlewares in the "inner to outer" order. For the above stack, the final `tx.Handler` is: +We define a `ComposeMiddlewares` function for composing middlewares. It takes the base handler as first argument, and middlewares in the "outer to inner" order. For the above stack, the final `tx.Handler` is: ```go -txHandler := middleware.ComposeMiddlewares(H, C, B, A) +txHandler := middleware.ComposeMiddlewares(H, A, B, C) ``` The middleware is set in BaseApp via its `SetTxHandler` setter: @@ -102,12 +132,18 @@ While the app developer can define and compose the middlewares of their choice, ### Similarities and Differences between Antehandlers and Middlewares -The middleware-based design builds upon the existing antehandlers design described in [ADR-010](./adr-010-modular-antehandler.md). +The middleware-based design builds upon the existing antehandlers design described in [ADR-010](./adr-010-modular-antehandler.md). It is very similar to the [Decorator Pattern](./adr-010-modular-antehandler.md#decorator-pattern) described in that ADR and used in [weave](https://github.com/iov-one/weave). #### Similarities -- Both design are based on chaining/composing small modular pieces. -- +- Designed as chaining/composing small modular pieces. +- Allow code reuse for `{Check,Deliver}Tx` and for `Simulate`. +- Set up in `app.go`, and easily customizable by app developers. + +#### Differences + +- The middleware approach uses separate methods for `{Check,Deliver,Simulate}Tx`, whereas the antehandlers pass a `simulate bool` flag and uses the `sdkCtx.Is{Check,Recheck}Tx()` flags to determine how we are executing transactions. +- The middleware design lets each middleware hold a reference to the next middleware ## Consequences From c465d7bc52073f61d0d226d11c7f85978e5dc3ee Mon Sep 17 00:00:00 2001 From: Amaury M <1293565+amaurym@users.noreply.github.com> Date: Wed, 25 Aug 2021 13:00:38 +0200 Subject: [PATCH 4/9] WIP --- .../adr-045-check-delivertx-middlewares.md | 90 +++++++++++++------ 1 file changed, 65 insertions(+), 25 deletions(-) diff --git a/docs/architecture/adr-045-check-delivertx-middlewares.md b/docs/architecture/adr-045-check-delivertx-middlewares.md index 9af16c1983bf..6bf447f1f810 100644 --- a/docs/architecture/adr-045-check-delivertx-middlewares.md +++ b/docs/architecture/adr-045-check-delivertx-middlewares.md @@ -6,7 +6,7 @@ ## Status -PROPOSED +ACCEPTED ## Abstract @@ -14,13 +14,13 @@ This ADR replaces the current BaseApp `runTx` and antehandlers design with a mid ## Context -BaseApp's implementation of ABCI `{Check,Deliver}Tx()` and its own `Simulate()` method call the `runTx` method under the hood, which first runs antehandlers, then executes `Msg`s. However, the [transaction Tips](https://github.com/cosmos/cosmos-sdk/issues/9406) feature requires custom logic to be run after the `Msg`s execution. There is currently no way to achieve this. +BaseApp's implementation of ABCI `{Check,Deliver}Tx()` and its own `Simulate()` method call the `runTx` method under the hood, which first runs antehandlers, then executes `Msg`s. However, the [transaction Tips](https://github.com/cosmos/cosmos-sdk/issues/9406) and [refunding unused gas](https://github.com/cosmos/cosmos-sdk/issues/2150) use cases require custom logic to be run after the `Msg`s execution. There is currently no way to achieve this. An naive solution would be to add post-`Msg` hooks to BaseApp. However, the SDK team thinks in parallel about the bigger picture of making app wiring simpler ([#9181](https://github.com/cosmos/cosmos-sdk/discussions/9182)), which includes making BaseApp more lightweight and modular. ## Decision -We decide to transform Baseapp's implementation of ABCI `{Check,Deliver}Tx()` and its own `Simulate()` method to use a middleware-based design. +We decide to transform Baseapp's implementation of ABCI `{Check,Deliver}Tx` and its own `Simulate` methods to use a middleware-based design. The two following interfaces are the base of the middleware design, and are defined in `types/tx`: @@ -34,7 +34,16 @@ type Handler interface { type Middleware func(Handler) Handler ``` -BaseApp holds a reference to a `tx.Handler`, and the ABCI `{Check,Deliver}Tx()` and `Simulate()` methods simply call `app.txHandler.{Check,Deliver,Simulate}Tx()` with the relevant arguments. For example, for `DeliverTx`: +BaseApp holds a reference to a `tx.Handler`: + +```go +type BaseApp struct { + // other fields + txHandler tx.Handler +} +``` + +Baseapp's ABCI `{Check,Deliver}Tx()` and `Simulate()` methods simply call `app.txHandler.{Check,Deliver,Simulate}Tx()` with the relevant arguments. For example, for `DeliverTx`: ```go func (app *BaseApp) DeliverTx(req abci.RequestDeliverTx) abci.ResponseDeliverTx { @@ -55,14 +64,17 @@ func (app *BaseApp) DeliverTx(req abci.RequestDeliverTx) abci.ResponseDeliverTx The implementations are similar for `BaseApp.CheckTx` and `BaseApp.Simulate`. +`baseapp.txHandler`'s three methods' implementations can obviously be monolithic functions, but for modularity we propose a middleware composition design, where a middleware is simply a function that takes a `tx.Handler`, and returns another `tx.Handler` wrapped around the previous one. + ### Implementing a Middleware -In practice, a middleware is a Go function that takes as arguments some parameters needed for the middleware, and returns a `tx.Middleware`. +In practice, middlewares are created by Go function that takes as arguments some parameters needed for the middleware, and returns a `tx.Middleware`. -For example, for creating `MyMiddleware`, we can implement: +For example, for creating an arbitrary `MyMiddleware`, we can implement: ```go -// myTxHandler is the +// myTxHandler is the tx.Handler of this middleware. Note that it holds a +// reference to the next tx.Handler in the stack. type myTxHandler struct { // next is the next tx.Handler in the middleware stack. next tx.Handler @@ -83,19 +95,50 @@ func NewMyMiddleware(arg1, arg2) tx.Middleware { var _ tx.Handler = myTxHandler{} func (txh myTxHandler) CheckTx(ctx context.Context, tx sdk.Tx, req abci.RequestCheckTx) (abci.ResponseCheckTx, error) { + // CheckTx specific pre-processing logic + + // run the next middleware + res, err := txh.next.CheckTx(ctx, tx, req) + + // CheckTx specific post-processing logic + + return res, err +} + +func (txh myTxHandler) DeliverTx(ctx context.Context, tx sdk.Tx, req abci.RequestDeliverTx) (abci.ResponseDeliverTx, error) { + // DeliverTx specific pre-processing logic + + // run the next middleware + res, err := txh.next.DeliverTx(ctx, tx, req) + + // DeliverTx specific post-processing logic + + return res, err +} + +func (txh myTxHandler) SimulateTx(ctx context.Context, tx sdk.Tx, req tx.RequestSimulateTx) (abci.ResponseSimulateTx, error) { + // SimulateTx specific pre-processing logic + + // run the next middleware + res, err := txh.next.SimulateTx(ctx, tx, req) + + // SimulateTx specific post-processing logic + + return res, err +} ``` ### Composing Middlewares -While BaseApp simply holds a reference to a `tx.Handler`, this `tx.Handler` itself is defined using a middleware stack. We define a base `tx.Handler` called `RunMsgsTxHandler`, which executes messages. +While BaseApp simply holds a reference to a `tx.Handler`, this `tx.Handler` itself is defined using a middleware stack. The SDK exposes a base (i.e. innermost) `tx.Handler` called `RunMsgsTxHandler`, which executes messages. -Then, the app developer can compose multiple middlewares on top on the base `tx.Handler`. Each middleware can run pre-and-post-processing logic around its next middleware. Conceptually, as an example, given the middlewares `A`, `B`, and `C` and the base `tx.Handler` `H` the stack looks like: +Then, the app developer can compose multiple middlewares on top on the base `tx.Handler`. Each middleware can run pre-and-post-processing logic around its next middleware, as described in the section above. Conceptually, as an example, given the middlewares `A`, `B`, and `C` and the base `tx.Handler` `H` the stack looks like: ``` A.pre B.pre C.pre - H // The base handler, usually `RunMsgsTxHandler` + H # The base tx.handler, for example `RunMsgsTxHandler` C.post B.post A.post @@ -116,7 +159,7 @@ txHandler := middleware.ComposeMiddlewares(...) app.SetTxHandler(txHandler) ``` -Naturally, the app developer can define their own middlewares. +The app developer can define their own middlewares, or use the SDK's pre-defined middlewares. ### Middlewares Maintained by the SDK @@ -125,30 +168,30 @@ While the app developer can define and compose the middlewares of their choice, | Middleware | Description | | ----------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | RunMsgsTxHandler | This is the base `tx.Handler`. It replaces the old baseapp's `runMsgs`, and executes a transaction's `Msg`s. | -| {Antehandlers} | Each antehandler is converted to its own middleware. | +| {Antehandlers} | Each antehandler is converted to its own middleware. These middlewares perform signature verification, fee deductions and other validations on the incoming transaction. | | IndexEventsTxMiddleware | This is a simple middleware that chooses which events to index in Tendermint. Replaces `baseapp.indexEvents` (which unfortunately still exists in baseapp too, because it's used to index Begin/EndBlock events) | -| RecoveryTxMiddleware | This index recovers from panics. It replaces baseapp.runTx's panic recovery. | +| RecoveryTxMiddleware | This index recovers from panics. It replaces baseapp.runTx's panic recovery described in [ADR-022](./adr-022-custom-panic-handling.md). | | GasTxMiddleware | This replaces the [`Setup`](https://github.com/cosmos/cosmos-sdk/blob/v0.43.0/x/auth/ante/setup.go) Antehandler. It sets a GasMeter on sdk.Context. Note that before, GasMeter was set on sdk.Context inside the antehandlers, and there was some mess around the fact that antehandlers had their own panic recovery system so that the GasMeter could be read by baseapp's recovery system. Now, this mess is all removed: one middleware sets GasMeter, another one handles recovery. | ### Similarities and Differences between Antehandlers and Middlewares -The middleware-based design builds upon the existing antehandlers design described in [ADR-010](./adr-010-modular-antehandler.md). It is very similar to the [Decorator Pattern](./adr-010-modular-antehandler.md#decorator-pattern) described in that ADR and used in [weave](https://github.com/iov-one/weave). +The middleware-based design builds upon the existing antehandlers design described in [ADR-010](./adr-010-modular-antehandler.md). Even though the final decision of ADR-010 was to go with the "Simple Decorators" approach, the middleware design is actually very similar to the other [Decorator Pattern](./adr-010-modular-antehandler.md#decorator-pattern) proposal, also used in [weave](https://github.com/iov-one/weave). -#### Similarities +#### Similarities with Antehandlers - Designed as chaining/composing small modular pieces. - Allow code reuse for `{Check,Deliver}Tx` and for `Simulate`. - Set up in `app.go`, and easily customizable by app developers. -#### Differences +#### Differences with Antehandlers -- The middleware approach uses separate methods for `{Check,Deliver,Simulate}Tx`, whereas the antehandlers pass a `simulate bool` flag and uses the `sdkCtx.Is{Check,Recheck}Tx()` flags to determine how we are executing transactions. -- The middleware design lets each middleware hold a reference to the next middleware +- The Antehandlers are run before `Msg` execution, whereas middlewares can run before and after. +- The middleware approach uses separate methods for `{Check,Deliver,Simulate}Tx`, whereas the antehandlers pass a `simulate bool` flag and uses the `sdkCtx.Is{Check,Recheck}Tx()` flags to determine in which transaction mode we are. +- The middleware design lets each middleware hold a reference to the next middleware, whereas the antehandlers pass a `next` argument in the `AnteHandle` method. +- The middleware design use Go's standard `context.Context`, whereas the antehandlers use `sdk.Context`. ## Consequences -> This section describes the resulting context, after applying the decision. All consequences should be listed here, not just the "positive" ones. A particular decision may have positive, negative, and neutral consequences, but all of them affect the team and project in the future. - ### Backwards Compatibility > All ADRs that introduce backwards incompatibilities must include a section describing these incompatibilities and their severity. The ADR must explain how the author proposes to deal with these incompatibilities. ADR submissions without a sufficient backwards compatibility treatise may be rejected outright. @@ -175,8 +218,5 @@ Test cases for an implementation are mandatory for ADRs that are affecting conse ## References -- {reference link} - -``` - -``` +- Initial discussion: https://github.com/cosmos/cosmos-sdk/issues/9585 +- Implementation: https://github.com/cosmos/cosmos-sdk/pull/9920 From 830358063c1b0d238c6ebbb9eb6b30aab241f95c Mon Sep 17 00:00:00 2001 From: Amaury M <1293565+amaurym@users.noreply.github.com> Date: Wed, 25 Aug 2021 14:33:10 +0200 Subject: [PATCH 5/9] WIP --- .../adr-045-check-delivertx-middlewares.md | 40 ++++++++++++++++--- 1 file changed, 34 insertions(+), 6 deletions(-) diff --git a/docs/architecture/adr-045-check-delivertx-middlewares.md b/docs/architecture/adr-045-check-delivertx-middlewares.md index 6bf447f1f810..f5abbfbde1b7 100644 --- a/docs/architecture/adr-045-check-delivertx-middlewares.md +++ b/docs/architecture/adr-045-check-delivertx-middlewares.md @@ -26,9 +26,9 @@ The two following interfaces are the base of the middleware design, and are defi ```go type Handler interface { - CheckTx(ctx context.Context, tx sdk.Tx, req abci.RequestCheckTx) (abci.ResponseCheckTx, error) - DeliverTx(ctx context.Context, tx sdk.Tx, req abci.RequestDeliverTx) (abci.ResponseDeliverTx, error) - SimulateTx(ctx context.Context, tx sdk.Tx, req RequestSimulateTx) (ResponseSimulateTx, error) + CheckTx(ctx context.Context, tx sdk.Tx, req abci.RequestCheckTx) (abci.ResponseCheckTx, error) + DeliverTx(ctx context.Context, tx sdk.Tx, req abci.RequestDeliverTx) (abci.ResponseDeliverTx, error) + SimulateTx(ctx context.Context, tx sdk.Tx, req RequestSimulateTx) (ResponseSimulateTx, error) } type Middleware func(Handler) Handler @@ -194,7 +194,33 @@ The middleware-based design builds upon the existing antehandlers design describ ### Backwards Compatibility -> All ADRs that introduce backwards incompatibilities must include a section describing these incompatibilities and their severity. The ADR must explain how the author proposes to deal with these incompatibilities. ADR submissions without a sufficient backwards compatibility treatise may be rejected outright. +Since this refactor removes some logic away from BaseApp and into middlewares, it introduces API-breaking changes for app developers. Most notably, instead of creating an antehandler chain in `app.go`, app developers need to create a middleware stack: + +```diff +- anteHandler, err := ante.NewAnteHandler( +- ante.HandlerOptions{ +- AccountKeeper: app.AccountKeeper, +- BankKeeper: app.BankKeeper, +- SignModeHandler: encodingConfig.TxConfig.SignModeHandler(), +- FeegrantKeeper: app.FeeGrantKeeper, +- SigGasConsumer: ante.DefaultSigVerificationGasConsumer, +- }, +-) ++txHandler, err := authmiddleware.NewDefaultTxHandler(authmiddleware.TxHandlerOptions{ ++ Debug: app.Trace(), ++ IndexEvents: indexEvents, ++ LegacyRouter: app.legacyRouter, ++ MsgServiceRouter: app.msgSvcRouter, ++ LegacyAnteHandler: anteHandler, ++}) +if err != nil { + panic(err) +} +- app.SetAnteHandler(anteHandler) ++ app.SetTxHandler() +``` + +As usual, the SDK will provide a migration document for app developers. ### Positive @@ -212,9 +238,11 @@ The middleware-based design builds upon the existing antehandlers design describ - [#9934](https://github.com/cosmos/cosmos-sdk/discussions/9934) Decomposing BaseApp's other ABCI methods into middlewares. -## Test Cases [optional] +## Test Cases + +We update the existing baseapp and antehandlers tests to the new middleware API, but keep the same test cases, to avoid introducing regressions. Existing CLI tests will also not be touched. -Test cases for an implementation are mandatory for ADRs that are affecting consensus changes. Other ADRs can choose to include links to test cases if applicable. +For new middlewares, we introduce unit tests. Since middlewares are purposefully small, unit tests suit well. ## References From fc2107b0890b4c92b42ddbe4ea12dd05f6563bbe Mon Sep 17 00:00:00 2001 From: Amaury M <1293565+amaurym@users.noreply.github.com> Date: Wed, 25 Aug 2021 14:54:01 +0200 Subject: [PATCH 6/9] Update consequences --- .../adr-045-check-delivertx-middlewares.md | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/docs/architecture/adr-045-check-delivertx-middlewares.md b/docs/architecture/adr-045-check-delivertx-middlewares.md index f5abbfbde1b7..c92c3a63ee99 100644 --- a/docs/architecture/adr-045-check-delivertx-middlewares.md +++ b/docs/architecture/adr-045-check-delivertx-middlewares.md @@ -217,30 +217,36 @@ if err != nil { panic(err) } - app.SetAnteHandler(anteHandler) -+ app.SetTxHandler() ++ app.SetTxHandler(txHandler) ``` -As usual, the SDK will provide a migration document for app developers. +Other more minor API breaking changes will also be provided in the CHANGELOG. As usual, the SDK will provide a release migration document for app developers. + +This ADR does not introduce any state-machine-, client- or CLI-breaking changes. ### Positive -{positive consequences} +- Allow custom logic to be run before an after `Msg` execution. This enables the [tips](https://github.com/cosmos/cosmos-sdk/issues/9406) and [gas refund](https://github.com/cosmos/cosmos-sdk/issues/2150) uses cases, and possibly other ones. +- Make BaseApp more lightweight, and defer complex logic to small modular components. +- Separate paths for `{Check,Deliver,Simulate}Tx` with different returns types. This allows for improved readability (replace `if sdkCtx.IsRecheckTx() && !simulate {...}` with separate methods) and more flexibility (e.g. returning a `priority` in `abci.ResponseCheckTx`). ### Negative -{negative consequences} +- It is hard to understand at first glance the state updates that would occur after a middleware runs given the `sdk.Context` and `tx`. A middleware can have an arbitrary number of nested middleware being called within its function body, each possibly doing some pre- and post-processing before calling the next middleware on the chain. Thus to understand what a middleware is doing, one must also understand what every other middleware further along the chain is also doing. This can get quite complicated to understand. +- API-breaking changes for app developers. ### Neutral -{neutral consequences} +No neutral consequences. ## Further Discussions - [#9934](https://github.com/cosmos/cosmos-sdk/discussions/9934) Decomposing BaseApp's other ABCI methods into middlewares. +- Replace `sdk.Tx` interface with the concrete protobuf Tx type in the `tx.Handler` methods signature. ## Test Cases -We update the existing baseapp and antehandlers tests to the new middleware API, but keep the same test cases, to avoid introducing regressions. Existing CLI tests will also not be touched. +We update the existing baseapp and antehandlers tests to use the new middleware API, but keep the same test cases and logic, to avoid introducing regressions. Existing CLI tests will also be left untouched. For new middlewares, we introduce unit tests. Since middlewares are purposefully small, unit tests suit well. From 204f4f4b30515d935980d37bbae635d752a46a96 Mon Sep 17 00:00:00 2001 From: Amaury M <1293565+amaurym@users.noreply.github.com> Date: Mon, 13 Sep 2021 17:20:38 +0200 Subject: [PATCH 7/9] Address review --- docs/architecture/adr-045-check-delivertx-middlewares.md | 2 +- x/auth/middleware/middleware.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/architecture/adr-045-check-delivertx-middlewares.md b/docs/architecture/adr-045-check-delivertx-middlewares.md index c92c3a63ee99..c84a3ef00748 100644 --- a/docs/architecture/adr-045-check-delivertx-middlewares.md +++ b/docs/architecture/adr-045-check-delivertx-middlewares.md @@ -159,7 +159,7 @@ txHandler := middleware.ComposeMiddlewares(...) app.SetTxHandler(txHandler) ``` -The app developer can define their own middlewares, or use the SDK's pre-defined middlewares. +The app developer can define their own middlewares, or use the SDK's pre-defined middlewares from `middleware.NewDefaultTxHandler()`. ### Middlewares Maintained by the SDK diff --git a/x/auth/middleware/middleware.go b/x/auth/middleware/middleware.go index de9d986cb3fd..80ad606cd219 100644 --- a/x/auth/middleware/middleware.go +++ b/x/auth/middleware/middleware.go @@ -6,7 +6,7 @@ import ( ) // ComposeMiddlewares compose multiple middlewares on top of a tx.Handler. The -// middleware order in the variadic arguments is from inside to outside. +// middleware order in the variadic arguments is from outer to inner. // // Example: Given a base tx.Handler H, and two middlewares A and B, the // middleware stack: From bfbd552215ca00c4625c89b8f98aa391d33429e8 Mon Sep 17 00:00:00 2001 From: Amaury M <1293565+amaurym@users.noreply.github.com> Date: Tue, 14 Sep 2021 16:59:27 +0200 Subject: [PATCH 8/9] Deprecate ADR-010 and ADR-022 --- docs/architecture/PROCESS.md | 2 +- docs/architecture/adr-010-modular-antehandler.md | 5 +++++ docs/architecture/adr-022-custom-panic-handling.md | 5 +++++ 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/docs/architecture/PROCESS.md b/docs/architecture/PROCESS.md index 07008b9689a6..209beb0f72d1 100644 --- a/docs/architecture/PROCESS.md +++ b/docs/architecture/PROCESS.md @@ -35,7 +35,7 @@ IMPLEMENTATION STATUS is either `Implemented` or `Not Implemented`. #### Consensus Status ``` -DRAFT -> PROPOSED -> LAST CALL yyyy-mm-dd -> ACCEPTED | REJECTED -> SUPERSEEDED by ADR-xxx +DRAFT -> PROPOSED -> LAST CALL yyyy-mm-dd -> ACCEPTED | REJECTED -> SUPERSEDED by ADR-xxx \ | \ | v v diff --git a/docs/architecture/adr-010-modular-antehandler.md b/docs/architecture/adr-010-modular-antehandler.md index eb5e8145de33..4fdd2ebdfc6b 100644 --- a/docs/architecture/adr-010-modular-antehandler.md +++ b/docs/architecture/adr-010-modular-antehandler.md @@ -3,6 +3,11 @@ ## Changelog - 2019 Aug 31: Initial draft +- 2021 Sep 14: Superseded by ADR-045 + +## Status + +SUPERSEDED by ADR-045 ## Context diff --git a/docs/architecture/adr-022-custom-panic-handling.md b/docs/architecture/adr-022-custom-panic-handling.md index 228adeef2877..22dad4447df4 100644 --- a/docs/architecture/adr-022-custom-panic-handling.md +++ b/docs/architecture/adr-022-custom-panic-handling.md @@ -3,6 +3,11 @@ ## Changelog - 2020 Apr 24: Initial Draft +- 2021 Sep 14: Superseded by ADR-045 + +## Status + +SUPERSEDED by ADR-045 ## Context From e67ee9ea0e97cf3db7f6719d1efe52e96b6696e2 Mon Sep 17 00:00:00 2001 From: Amaury M <1293565+amaurym@users.noreply.github.com> Date: Tue, 14 Sep 2021 17:01:39 +0200 Subject: [PATCH 9/9] Add both implementation PRs --- docs/architecture/adr-045-check-delivertx-middlewares.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/adr-045-check-delivertx-middlewares.md b/docs/architecture/adr-045-check-delivertx-middlewares.md index c84a3ef00748..b66f410fec52 100644 --- a/docs/architecture/adr-045-check-delivertx-middlewares.md +++ b/docs/architecture/adr-045-check-delivertx-middlewares.md @@ -253,4 +253,4 @@ For new middlewares, we introduce unit tests. Since middlewares are purposefully ## References - Initial discussion: https://github.com/cosmos/cosmos-sdk/issues/9585 -- Implementation: https://github.com/cosmos/cosmos-sdk/pull/9920 +- Implementation: [#9920 BaseApp refactor](https://github.com/cosmos/cosmos-sdk/pull/9920) and [#10028 Antehandlers migration](https://github.com/cosmos/cosmos-sdk/pull/10028)