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: Revert middlewares to antehandler (part 2/2: posthandler) #11985

Merged
merged 34 commits into from
May 23, 2022

Conversation

amaury1093
Copy link
Contributor

@amaury1093 amaury1093 commented May 18, 2022

Description

We decided to remove middlewares, and revert to antehandlers (exactly like in v045) for this release. A better middleware solution will be implemented after v046.

ref: #11955

This PR is part 2 of 2:

  • part 1: Revert baseapp and middlewares to v0.45.4
  • part 2: Add posthandler, tips, priority

Depends on:


Suggestion for reviewers:

  • Apart from correctness, I would also like someone to review exhaustiveness. I.e. all changes we made in v046 into the middleware folder are reflected in this PR, and that I didn't forget anything. I found the following ones:
    • add a TxFeeChecker in DeductFee
    • add a ExtensionChecker in ExtCheckerDecorator
    • add a TipDecorator

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)

@amaury1093 amaury1093 mentioned this pull request May 18, 2022
56 tasks
Base automatically changed from am/revert-045-baseapp to main May 20, 2022 09:27
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

LGTM, but I don't see anywhere where we use ctx.Priority for CheckTx?

baseapp/baseapp.go Show resolved Hide resolved
baseapp/baseapp.go Outdated Show resolved Hide resolved
amaury1093 and others added 3 commits May 20, 2022 17:16
Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
@amaury1093
Copy link
Contributor Author

LGTM, but I don't see anywhere where we use ctx.Priority for CheckTx?

Oh yeah good catch, see 35866ff.

@faddat
Copy link
Contributor

faddat commented May 23, 2022

hey, if it is possible to "lgtm" that would be super because this PR is now blocking the refactors we need to do for craft. Thanks guys.

// needs a coordinated upgrade.
//
// Uncomment to enable postHandlers:
// app.setPostHandler()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any opinions if we should enable Tips by default on simapp (note: all chains that will copy-paste simapp might also enable them)? Is it a use case that's generic enough to be enabled by default on all cosmos chains?

I'd personally say no, and it's more a specific use-case, but would like to hear other opinions.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would go with a yes, when we built craft, one of the discoveries that I think that we had is that an application template with all of the features turned on would be useful. Also, if we have this turned on in simapp, it gets tested

Copy link
Contributor

Choose a reason for hiding this comment

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

But if it’s there and come and to down at least it can be used. Just so that you know, craft began as an implementation of simapp.

basically I’d like to see a Nonsimapp app template in the sdk

I’ve been calling that tiny.

Copy link
Contributor

Choose a reason for hiding this comment

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

For various reasons Gaia is no longer a great template. Unfortunately the same can be said of starport.

Copy link
Contributor

Choose a reason for hiding this comment

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

Like yourself I think it’s not too much of a big deal in either direction if this is present directly in the simulation app or not but I would lean toward including it

@amaury1093
Copy link
Contributor Author

Going to put automerge on, to unblock downstream work.

Two decisions still need to be made:

  1. Should posthandlers run in their own store branch? refactor: Revert middlewares to antehandler (part 2/2: posthandler) #11985 (comment). Currently: no, they run in the same one as runMsgs
  2. Should tips be included in simapp by default? refactor: Revert middlewares to antehandler (part 2/2: posthandler) #11985 (comment). Currently: no, it's an opt-in feature.

I can create a follow-up PR if any decision changes on the two items above.

@amaury1093 amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label May 23, 2022
@faddat
Copy link
Contributor

faddat commented May 23, 2022

thanks @AmauryM :)

@codecov
Copy link

codecov bot commented May 23, 2022

Codecov Report

Merging #11985 (92c1f0b) into main (b2af716) will decrease coverage by 0.02%.
The diff coverage is 78.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #11985      +/-   ##
==========================================
- Coverage   66.02%   66.00%   -0.03%     
==========================================
  Files         672      671       -1     
  Lines       71232    70959     -273     
==========================================
- Hits        47032    46834     -198     
+ Misses      21523    21470      -53     
+ Partials     2677     2655      -22     
Impacted Files Coverage Δ
baseapp/options.go 66.03% <0.00%> (-2.59%) ⬇️
types/context.go 88.40% <0.00%> (-2.64%) ⬇️
x/auth/ante/sigverify.go 67.41% <0.00%> (-0.66%) ⬇️
x/auth/client/testutil/suite.go 94.28% <68.75%> (+0.02%) ⬆️
simapp/app.go 81.05% <70.00%> (-0.95%) ⬇️
baseapp/baseapp.go 79.63% <77.77%> (-0.89%) ⬇️
x/auth/ante/fee.go 82.35% <87.50%> (ø)
x/auth/ante/ext.go 90.00% <89.47%> (-10.00%) ⬇️
x/auth/ante/validator_tx_fee.go 92.30% <92.30%> (ø)
baseapp/abci.go 64.05% <100.00%> (+0.06%) ⬆️
... and 8 more

@mergify mergify bot merged commit d416ee8 into main May 23, 2022
@mergify mergify bot deleted the am/revert-045-part2 branch May 23, 2022 10:32
mergify bot pushed a commit that referenced this pull request May 23, 2022
## Description

ref: #11985 (comment)



---

### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] 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](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)
@nghuyenthevinh2000 nghuyenthevinh2000 mentioned this pull request Aug 19, 2022
12 tasks
larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
…osmos#11985)

## Description

We decided to remove middlewares, and revert to antehandlers (exactly like in v045) for this release. A better middleware solution will be implemented after v046.

ref: cosmos#11955

This PR is part 2 of 2:

- part 1: Revert baseapp and middlewares to v0.45.4
- part 2: Add posthandler, tips, priority

Depends on:
- [x] cosmos#11979 

---

Suggestion for reviewers:

- Apart from correctness, I would also like someone to review **exhaustiveness**. I.e. all changes we made in v046 into the [middleware folder](https://github.com/cosmos/cosmos-sdk/tree/v0.46.0-beta2/x/auth/middleware) are reflected in this PR, and that I didn't forget anything. I found the following ones:
  - add a TxFeeChecker in DeductFee
  - add a ExtensionChecker in ExtCheckerDecorator
  - add a TipDecorator



---

### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] 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](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
Labels
A:automerge Automatically merge PR once all prerequisites pass. C:CLI C:x/auth
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants