refactor: Revert middlewares to antehandlers (part 1/2: baseapp) #11979

amaury1093 · 2022-05-17T14:00:40Z

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

Because this refactor is big, so I decided to cut it into two. This PR is part 1 of 2:

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

Suggestion for reviewers:

This PR might still be hard to review though. I think it's easier to actually review the diff between v0.45.4 and this PR:

git difftool -d v0.45.4..am/revert-045-baseapp baseapp
- most important parts to review: runTx, runMsgs
git difftool -d v0.45.4..am/revert-045-baseapp x/auth/ante
- only cosmetic changes
git difftool -d v0.45.4..am/revert-045-baseapp simapp

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

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)

julienrbrt · 2022-05-17T14:54:07Z

Note, when merged we should as well revert part of #9954 (if not done here)

codecov · 2022-05-17T17:42:02Z

Codecov Report

Merging #11979 (5829d9e) into main (cf750b8) will increase coverage by 0.05%.
The diff coverage is 84.81%.

@@            Coverage Diff             @@
##             main   #11979      +/-   ##
==========================================
+ Coverage   66.00%   66.05%   +0.05%     
==========================================
  Files         677      674       -3     
  Lines       71249    71130     -119     
==========================================
- Hits        47026    46987      -39     
+ Misses      21537    21478      -59     
+ Partials     2686     2665      -21

Impacted Files	Coverage Δ
types/handler.go	`100.00% <ø> (ø)`
x/auth/tx/builder.go	`80.98% <ø> (ø)`
x/feegrant/keeper/keeper.go	`80.50% <ø> (ø)`
x/gov/keeper/keeper.go	`76.82% <0.00%> (ø)`
server/mock/tx.go	`63.15% <33.33%> (+3.15%)`	⬆️
x/auth/ante/ante.go	`61.29% <61.29%> (ø)`
x/auth/ante/sigverify.go	`68.07% <77.51%> (ø)`
x/auth/ante/basic.go	`79.27% <79.27%> (ø)`
x/auth/ante/fee.go	`82.35% <82.35%> (ø)`
x/auth/ante/setup.go	`83.78% <83.78%> (ø)`
... and 27 more

amaury1093 · 2022-05-17T17:55:07Z

Marking this as r4r, cc @alexanderbez @aaronc.

I didn't find a way to make this PR smaller unfortunately. And it's not finished yet: tomorrow I'll work on part 2, which is adding a postHandler to baseapp, and adding tips there.

Note, when merged we should as well revert part of #9954 (if not done here)

@julienrbrt could you create an issue for this? I don't think I'll tackle it in these 2 refactor PRs.

alexanderbez · 2022-05-17T20:40:27Z

I tried using difftool (I've never used it before) and I'm not really understanding how to use it. Could you walk me through it? In the meantime, I've just started to review the diff in the PR as-is.

aaronc · 2022-05-18T00:06:06Z

What's the rationale for difftool?

alexanderbez · 2022-05-18T13:26:36Z

I used difftool directly and it was completely unintuitive...maybe I was just using it wrong. I will just review the diff here.

alexanderbez · 2022-05-18T14:29:41Z

I just reviewed using git diff instead and I didn't see anything abnormal in baseapp so I'm approving 👍

robert-zaremba · 2022-05-18T19:51:39Z

I have added more analysis of antehandlers vs middlewares in #11955 (comment) and would be great if we reconsider the revert.

faddat · 2022-05-18T22:11:33Z

I really hope that we reconsider the revert.

The majority of the work to make both ibc and cw work with 46, involves this change. The work is now completed and, I mean, yeah we could change, but that's a whole ton of code in the bin.

aaronc · 2022-05-19T00:03:00Z

@AmauryM @alexanderbez let's find some time to discuss tomorrow

alexanderbez · 2022-05-19T13:43:45Z

Well we either:

revert
keep middleware + introduce a fix + test/audit -> delays 0.46 further (which btw, I will not approve such PRs, but don't feel blocked on me)

…ert-045-baseapp

faddat · 2022-05-20T03:09:42Z

Ok, let's revert

…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: #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] #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)

…mos#11979) ## 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 Because this refactor is big, so I decided to cut it into two. This PR is part 1 of 2: - part 1: Revert baseapp and middlewares to v0.45.4 - part 2: Add posthandler, tips, priority --- Suggestion for reviewers: This PR might still be hard to review though. I think it's easier to actually review the diff between v0.45.4 and this PR: - `git difftool -d v0.45.4..am/revert-045-baseapp baseapp` - most important parts to review: runTx, runMsgs - `git difftool -d v0.45.4..am/revert-045-baseapp x/auth/ante` - only cosmetic changes - `git difftool -d v0.45.4..am/revert-045-baseapp simapp` --- ### 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)

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

amaury1093 added 2 commits May 17, 2022 15:30

Make baseapp changes

6389c92

Revert middlewares

8edac39

github-actions bot added C:x/auth C:x/authz C:x/feegrant C:x/gov C:x/group labels May 17, 2022

amaury1093 mentioned this pull request May 17, 2022

fix!: Branch antehandlers and commit to state before runMsgs #11942

Closed

19 tasks

amaury1093 added 2 commits May 17, 2022 16:34

Fix baseapp_test build

9455376

Fix one test

3e17c52

amaury1093 added 2 commits May 17, 2022 16:57

Fix simapp

99584a1

Fix more tests

6b34400

github-actions bot added the C:x/upgrade label May 17, 2022

amaury1093 added 4 commits May 17, 2022 18:07

Fix another one?

39dfcaa

Fix ante test build

f2747df

Fix amino tests

16cc4f6

Merge branch 'main' into am/revert-045-baseapp

bbf631d

amaury1093 changed the title ~~refactor: Revert middlewares to antehandlers~~ refactor: Revert middlewares to antehandlers (part 1/2) May 17, 2022

amaury1093 added 2 commits May 17, 2022 19:25

Fix test

e875c25

Fix block gas test

d444a2e

github-actions bot added C:CLI Type: ADR labels May 17, 2022

amaury1093 marked this pull request as ready for review May 17, 2022 17:55

amaury1093 requested a review from a team as a code owner May 17, 2022 17:55

Small changes

ab6365d

Better comments

a7d54a1

amaury1093 mentioned this pull request May 18, 2022

Investigate group invariants violation in CI tests #11984

Closed

4 tasks

amaury1093 changed the title ~~refactor: Revert middlewares to antehandlers (part 1/2)~~ refactor: Revert middlewares to antehandlers (part 1/2: baseapp) May 18, 2022

amaury1093 mentioned this pull request May 18, 2022

refactor: Revert middlewares to antehandler (part 2/2: posthandler) #11985

Merged

20 tasks

Small updates

cd66328

alexanderbez approved these changes May 18, 2022

View reviewed changes

amaury1093 assigned JeancarloBarrios and atheeshp May 18, 2022

Merge branch 'main' of ssh://github.com/cosmos/cosmos-sdk into am/rev…

c685015

…ert-045-baseapp

JeancarloBarrios approved these changes May 19, 2022

View reviewed changes

faddat mentioned this pull request May 20, 2022

Craft Middleware refactor notional-labs/craft#114

Closed

atheeshp approved these changes May 20, 2022

View reviewed changes

amaury1093 added 2 commits May 20, 2022 11:07

Fix test build

65f4d16

Merge branch 'main' into am/revert-045-baseapp

5829d9e

amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label May 20, 2022

github-actions bot added the C:Store label May 20, 2022

mergify bot merged commit 01832e6 into main May 20, 2022

mergify bot deleted the am/revert-045-baseapp branch May 20, 2022 09:27

amaury1093 mentioned this pull request May 23, 2022

refactor: Add Tips decorator in simapp #12016

Merged

19 tasks

yihuang mentioned this pull request Sep 20, 2022

Problem: state streamers are not integrated crypto-org-chain/cronos#702

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Revert middlewares to antehandlers (part 1/2: baseapp) #11979

refactor: Revert middlewares to antehandlers (part 1/2: baseapp) #11979

amaury1093 commented May 17, 2022 •

edited

Loading

julienrbrt commented May 17, 2022

codecov bot commented May 17, 2022 •

edited

Loading

amaury1093 commented May 17, 2022 •

edited

Loading

alexanderbez commented May 17, 2022 •

edited

Loading

aaronc commented May 18, 2022

alexanderbez commented May 18, 2022 •

edited

Loading

alexanderbez commented May 18, 2022

robert-zaremba commented May 18, 2022

faddat commented May 18, 2022 •

edited

Loading

aaronc commented May 19, 2022

alexanderbez commented May 19, 2022

faddat commented May 20, 2022

refactor: Revert middlewares to antehandlers (part 1/2: baseapp) #11979

refactor: Revert middlewares to antehandlers (part 1/2: baseapp) #11979

Conversation

amaury1093 commented May 17, 2022 • edited Loading

Description

Author Checklist

Reviewers Checklist

julienrbrt commented May 17, 2022

codecov bot commented May 17, 2022 • edited Loading

Codecov Report

amaury1093 commented May 17, 2022 • edited Loading

alexanderbez commented May 17, 2022 • edited Loading

aaronc commented May 18, 2022

alexanderbez commented May 18, 2022 • edited Loading

alexanderbez commented May 18, 2022

robert-zaremba commented May 18, 2022

faddat commented May 18, 2022 • edited Loading

aaronc commented May 19, 2022

alexanderbez commented May 19, 2022

faddat commented May 20, 2022

amaury1093 commented May 17, 2022 •

edited

Loading

codecov bot commented May 17, 2022 •

edited

Loading

amaury1093 commented May 17, 2022 •

edited

Loading

alexanderbez commented May 17, 2022 •

edited

Loading

alexanderbez commented May 18, 2022 •

edited

Loading

faddat commented May 18, 2022 •

edited

Loading