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

refactor!: BaseApp {Check,Deliver}Tx with middleware design #9920

Merged
merged 39 commits into from
Aug 25, 2021

Conversation

amaury1093
Copy link
Contributor

@amaury1093 amaury1093 commented Aug 12, 2021

Description

ref: #9585

This PR if the 1st step (out of 2) in the #9585 refactor. It transforms baseapp's ABCI {Check,Deliver}Tx into middleware stacks. A middleware is defined by the following interfaces:

// types/tx package

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

This Pr doesn't migrate antehandlers, but only baseapp's runTx and runMsgs as middlewares. It bundles antehandlers into one single middleware (for now).

More specifically, it introduces the 5 following middlewares:

Middleware Description
RunMsgsTxMiddleware This middleware replaces the old baseapp's runMsgs.
LegacyAnteMiddleware This middleware is temporary, it bundles all antehandlers into one middleware. It's only created for the purpose of breaking the refactor into 2 pieces. It will be removed in Part 2, and each antehandler will be replaced by 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 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.

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@orijbot
Copy link

orijbot commented Aug 12, 2021

@cosmos cosmos deleted a comment from lgtm-com bot Aug 12, 2021
@cosmos cosmos deleted a comment from lgtm-com bot Aug 13, 2021
types/tx/middleware.go Outdated Show resolved Hide resolved
types/tx/middleware.go Outdated Show resolved Hide resolved
types/tx/middleware.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Aug 23, 2021

Codecov Report

Merging #9920 (0bb9c48) into master (147d798) will decrease coverage by 0.13%.
The diff coverage is 61.38%.

❗ Current head 0bb9c48 differs from pull request most recent head 880ff6b. Consider uploading reports for the commit 880ff6b to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9920      +/-   ##
==========================================
- Coverage   63.75%   63.62%   -0.14%     
==========================================
  Files         566      571       +5     
  Lines       53402    53498      +96     
==========================================
- Hits        34048    34039       -9     
- Misses      17435    17525      +90     
- Partials     1919     1934      +15     
Impacted Files Coverage Δ
simapp/test_helpers.go 0.69% <0.00%> (ø)
x/auth/ante/ante.go 60.00% <ø> (-1.30%) ⬇️
x/simulation/util.go 0.00% <0.00%> (ø)
x/auth/middleware/legacy_ante.go 28.16% <28.16%> (ø)
x/auth/middleware/index_events.go 39.28% <39.28%> (ø)
x/auth/middleware/run_msgs.go 58.03% <58.03%> (ø)
x/auth/middleware/recovery.go 58.82% <58.82%> (ø)
x/auth/middleware/gas.go 64.70% <64.70%> (ø)
baseapp/test_helpers.go 63.26% <70.00%> (-2.12%) ⬇️
baseapp/abci.go 60.76% <78.57%> (-1.26%) ⬇️
... and 22 more

Copy link
Collaborator

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

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

Preapproving - let's resolve the expected order in CombineMiddleware function.

Also, let's create followup tasks (checkbox items in an issue) or issues for followups.

@amaury1093
Copy link
Contributor Author

@alexanderbez regarding #9920 (review), your bullet points are totally on point. I'll try to point them where in the PR they happen, but would appreciate another pair of eyes on the logic, given that it's one of the most crucial bits of the SDK

Events are propogated not only from tx execution, but also from the ante-handler

Antehandlers and tx execution both call ctx.EventManager().EmitEvents, and at the end the runMsgsTxHandler populates the Check/DeliverTx response field. I believe #9920 (comment) tests this.

Tx simulation works as expected

Multiple functions have a simulate bool flag, I tried to be careful to set to false for Check/DeliverTx and true for SimulateTx. #9920 (comment) should test both gas and result for the gprc simulation endpoint.

Tx cache/store writes work as expected, i.e. writes are reverted on failure, etc...

Yeah I tried to keep the old design's exact same logic, but would like someone double-check it. It happens mainly in the antehandlers branching and then runMsgs branching.

@amaury1093
Copy link
Contributor Author

@marbar3778 @alexanderbez any further comments on this PR? I'm going on vacation Friday, would like to get it merged before that.

I also started #9996, will make it R4R soon, and we can discuss more in details there?

baseapp/test_helpers.go Outdated Show resolved Hide resolved
s.Require().Equal(len(res.GetResult().GetEvents()), 6) // 1 coin recv 1 coin spent, 1 transfer, 3 messages.
s.Require().True(res.GetGasInfo().GetGasUsed() > 0) // Gas used sometimes change, just check it's not empty.
//
// The 13 events are:
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! Guess we fixed it...somehow :p

x/auth/middleware/run_msgs.go Outdated Show resolved Hide resolved
amaury1093 and others added 2 commits August 25, 2021 15:28
@amaury1093 amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Aug 25, 2021
@mergify mergify bot merged commit a15d7e3 into master Aug 25, 2021
@mergify mergify bot deleted the am/middleware branch August 25, 2021 14:40
larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
)

<!--
The default pull request template is for types feat, fix, or refactor.
For other templates, add one of the following parameters to the url:
- template=docs.md
- template=other.md
-->

## Description

ref: cosmos#9585 

This PR if the 1st step (out of 2) in the cosmos#9585 refactor. It transforms baseapp's ABCI {Check,Deliver}Tx into middleware stacks. A middleware is defined by the following interfaces:

```go
// types/tx package

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
```

This Pr doesn't migrate antehandlers, but only baseapp's runTx and runMsgs as middlewares. It bundles antehandlers into one single middleware (for now).

More specifically, it introduces the 5 following middlewares:

| Middleware              | Description                                                                                                                                                                                                                                                                                                                                                                                            |
| ----------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
| RunMsgsTxMiddleware     | This middleware replaces the old baseapp's `runMsgs`.                                                                                                                                                                                                                                                                                                                                                  |
| LegacyAnteMiddleware    | This middleware is temporary, it bundles all antehandlers into one middleware. It's only created for the purpose of breaking the refactor into 2 pieces. **It will be removed in Part 2**, and each antehandler will be replaced by 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/master/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. |

---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [x] added `!` to the type prefix if API or client breaking change
- [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [x] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [x] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [x] added a changelog entry to `CHANGELOG.md`
- [x] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [x] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed 
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants