move feegrant ante to auth ante #8682

atheeshp · 2021-02-24T10:52:52Z

Description

closes: #8611

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

codecov · 2021-02-24T11:04:36Z

Codecov Report

Merging #8682 (7ca23a6) into master (e30959c) will increase coverage by 0.31%.
The diff coverage is 72.91%.

❗ Current head 7ca23a6 differs from pull request most recent head 148294a. Consider uploading reports for the commit 148294a to get more accurate results

@@            Coverage Diff             @@
##           master    #8682      +/-   ##
==========================================
+ Coverage   58.91%   59.22%   +0.31%     
==========================================
  Files         574      568       -6     
  Lines       32122    31804     -318     
==========================================
- Hits        18924    18837      -87     
+ Misses      10979    10760     -219     
+ Partials     2219     2207      -12

Impacted Files	Coverage Δ
x/feegrant/keeper/keeper.go	`78.65% <ø> (ø)`
x/auth/ante/ante.go	`66.66% <57.89%> (-33.34%)`	⬇️
simapp/app.go	`82.84% <81.81%> (-0.50%)`	⬇️
x/auth/ante/fee.go	`78.43% <83.33%> (+4.07%)`	⬆️
x/bank/keeper/genesis.go	`68.42% <0.00%> (-10.53%)`	⬇️
x/bank/keeper/send.go	`75.00% <0.00%> (-8.63%)`	⬇️
x/bank/types/genesis.go	`52.00% <0.00%> (-5.58%)`	⬇️
x/gov/legacy/v043/store.go	`84.61% <0.00%> (-1.10%)`	⬇️
x/staking/types/msg.go	`54.92% <0.00%> (-0.63%)`	⬇️
x/genutil/client/cli/migrate.go	`68.42% <0.00%> (ø)`
... and 4 more

x/auth/ante/ante.go

…ve_feegrant_ante

…-sdk into atheesh/move_feegrant_ante

amaury1093

lgtm

x/auth/ante/ante.go

…ve_feegrant_ante

amaury1093

So reading this PR again, I still think it's quite confusing.

I believe we initially wanted separate decorators, because feegrant seemed like an optional module, which apps could choose to include or not.

TBH right now I really wish to see it as a core module that will be in most/all chains, so I propose @aaronc's (a) solution: to have one single decorator in x/auth that handles everything related to fees.

So:

we remove RejectFeeGranterDecorator and DeductGrantedFeeDecorator
we put all their logic in the single DeductFeeDecorator (it takes a nullable FeeGrantKeeper as argument)

It will also be easier to understand the fee deduction logic in this case.

Sorry @atheeshp again for not having thought this through at first.

atheeshp · 2021-03-05T11:28:10Z

So reading this PR again, I still think it's quite confusing.

I believe we initially wanted separate decorators, because feegrant seemed like an optional module, which apps could choose to include or not.

TBH right now I really wish to see it as a core module that will be in most/all chains, so I propose @aaronc's (a) solution: to have one single decorator in x/auth that handles everything related to fees.

So:
* we remove RejectFeeGranterDecorator and DeductGrantedFeeDecorator

* we put all their logic in the single DeductFeeDecorator (it takes a nullable FeeGrantKeeper as argument)
It will also be easier to understand the fee deduction logic in this case.

Sorry @atheeshp again for not having thought this through at first.

Ah, yes I think I misunderstood what @aaronc is saying here, thanks for making it clear @AmauryM .

…ve_feegrant_ante

orijbot · 2021-03-18T12:02:01Z

Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=576a93a1b0ef31b733eac3950d7fefac38b2011e

orijbot · 2021-03-18T13:18:39Z

Benchmark finished. See the result: https://github.orijtech.com/benchmark/result?id=e7adba9d3c064ffc8537c4231f4aa2e8

aaronc · 2021-03-18T16:24:23Z

simapp/app.go

+	anteHandler, err := ante.NewAnteHandler(
+		ante.HandlerOptions{
+			AccountKeeper:   app.AccountKeeper,
+			BankKeeper:      app.BankKeeper,
+			SignModeHandler: encodingConfig.TxConfig.SignModeHandler(),
+			FeegrantKeeper:  app.FeeGrantKeeper,
+			SigGasConsumer:  ante.DefaultSigVerificationGasConsumer,
+		},


@alessio @jgimeno can someone at your team take a look at this? Just want to make sure you're all aware and onboard with us moving positional parameters into an options struct here.

@fdymylja @gsora @sahith-narahari

Verbal ACK from @alexanderbez on our SDK call today

Change requests have been addressed

alexanderbez

Looks great!

x/auth/ante/ante.go

x/auth/ante/fee.go

…ve_feegrant_ante

…-sdk into atheesh/move_feegrant_ante

fdymylja

LGTM, can we add a changelog entry?

orijbot · 2021-03-20T07:55:54Z

Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=bb91e5b3f9b9b51b3781e790d3960297d61317cc

alessio

ACK'd - pending changelog entry.

alessio · 2021-03-20T23:43:10Z

x/auth/ante/feegrant_test.go

-func TestAnteTestSuite(t *testing.T) {
-	suite.Run(t, new(AnteTestSuite))
-}


If you remove this, the test cases in AnteTestSuite won't run.

This suite is no longer required, since all tests which are in x/feegrant/ante moved to x/auth/ante and using the test suite from the same package(x/auth/ante).

…ve_feegrant_ante

…-sdk into atheesh/move_feegrant_ante

amaury1093

changelog added, I'm putting automerge on!

* move feegrant ante to auth ante * update ante builder * remove commented code * review changes * fix lint * review changes * review changes * review changes * review changes * fix ante builder * review changes * update ante builder with options struct * review changes * review changes * add changelog Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com> Co-authored-by: Aaron Craelius <aaron@regen.network>

move feegrant ante to auth ante

d7b83ba

atheeshp added 2 commits February 24, 2021 16:37

Merge branch 'master' into atheesh/move_feegrant_ante

967dd28

Merge branch 'master' into atheesh/move_feegrant_ante

d692f10

aaronc reviewed Feb 25, 2021

View reviewed changes

x/auth/ante/ante.go Outdated Show resolved Hide resolved

atheeshp added 4 commits February 26, 2021 12:50

update ante builder

b229812

Merge branch 'master' of github.com:cosmos/cosmos-sdk into atheesh/mo…

7eb8897

…ve_feegrant_ante

Merge branch 'atheesh/move_feegrant_ante' of github.com:cosmos/cosmos…

f3874a7

…-sdk into atheesh/move_feegrant_ante

remove commented code

c8b38fa

atheeshp marked this pull request as ready for review February 26, 2021 07:28

atheeshp requested review from alessio and alexanderbez as code owners February 26, 2021 07:28

atheeshp requested review from aaronc, robert-zaremba, aleem1314, amaury1093, anilcse, blushi and clevinson February 26, 2021 07:28

amaury1093 approved these changes Mar 1, 2021

View reviewed changes

aaronc reviewed Mar 2, 2021

View reviewed changes

x/auth/ante/ante.go Outdated Show resolved Hide resolved

x/auth/ante/ante.go Outdated Show resolved Hide resolved

atheeshp added 3 commits March 3, 2021 16:04

Merge branch 'master' of github.com:cosmos/cosmos-sdk into atheesh/mo…

d1d74b8

…ve_feegrant_ante

review changes

8bd1cfe

Merge branch 'master' of github.com:cosmos/cosmos-sdk into atheesh/mo…

b282167

…ve_feegrant_ante

atheeshp requested a review from aaronc March 5, 2021 09:31

fix lint

5e2c190

amaury1093 suggested changes Mar 5, 2021

View reviewed changes

atheeshp added 2 commits March 5, 2021 22:46

review changes

3c6c8a1

Merge branch 'master' of github.com:cosmos/cosmos-sdk into atheesh/mo…

3d633df

…ve_feegrant_ante

aaronc reviewed Mar 18, 2021

View reviewed changes

alexanderbez approved these changes Mar 19, 2021

View reviewed changes

x/auth/ante/ante.go Outdated Show resolved Hide resolved

x/auth/ante/ante.go Outdated Show resolved Hide resolved

x/auth/ante/ante.go Outdated Show resolved Hide resolved

x/auth/ante/ante.go Outdated Show resolved Hide resolved

x/auth/ante/fee.go Outdated Show resolved Hide resolved

atheeshp added 3 commits March 19, 2021 21:42

Merge branch 'master' of github.com:cosmos/cosmos-sdk into atheesh/mo…

6ac8011

…ve_feegrant_ante

review changes

943190c

Merge branch 'atheesh/move_feegrant_ante' of github.com:cosmos/cosmos…

3e4649c

…-sdk into atheesh/move_feegrant_ante

fdymylja self-requested a review March 19, 2021 17:01

fdymylja approved these changes Mar 19, 2021

View reviewed changes

Merge branch 'master' into atheesh/move_feegrant_ante

bb91e5b

alessio approved these changes Mar 20, 2021

View reviewed changes

alessio reviewed Mar 20, 2021

View reviewed changes

atheeshp added 3 commits March 22, 2021 11:45

Merge branch 'master' of github.com:cosmos/cosmos-sdk into atheesh/mo…

7c08eba

…ve_feegrant_ante

add changelog

e852d05

Merge branch 'atheesh/move_feegrant_ante' of github.com:cosmos/cosmos…

7ca23a6

…-sdk into atheesh/move_feegrant_ante

amaury1093 approved these changes Mar 22, 2021

View reviewed changes

amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Mar 22, 2021

amaury1093 and others added 3 commits March 22, 2021 10:36

Merge branch 'master' into atheesh/move_feegrant_ante

dcfdf08

Merge branch 'master' into atheesh/move_feegrant_ante

4a6199f

Merge branch 'master' into atheesh/move_feegrant_ante

148294a

mergify bot merged commit 025d226 into master Mar 23, 2021

mergify bot deleted the atheesh/move_feegrant_ante branch March 23, 2021 07:31

aleem1314 mentioned this pull request Apr 2, 2021

x/feegrant API audit #9028

Closed

3 tasks

atheeshp mentioned this pull request Apr 9, 2021

feegrant filtered msgs #8604

Merged

9 tasks

This was referenced Apr 20, 2021

x/feegrant Updates #9115

Closed

x/feegrant API Audit changes #9194

Merged

technicallyty mentioned this pull request Apr 29, 2021

Update x/auth AnteHandler Spec and Documentation #9232

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move feegrant ante to auth ante #8682

move feegrant ante to auth ante #8682

atheeshp commented Feb 24, 2021

codecov bot commented Feb 24, 2021 •

edited

Loading

amaury1093 left a comment

amaury1093 left a comment •

edited

Loading

atheeshp commented Mar 5, 2021

orijbot commented Mar 18, 2021

orijbot commented Mar 18, 2021

aaronc Mar 18, 2021

alessio Mar 18, 2021

clevinson Mar 19, 2021

alexanderbez left a comment

fdymylja left a comment

orijbot commented Mar 20, 2021

alessio left a comment

alessio Mar 20, 2021

atheeshp Mar 22, 2021

amaury1093 left a comment

move feegrant ante to auth ante #8682

move feegrant ante to auth ante #8682

Conversation

atheeshp commented Feb 24, 2021

Description

codecov bot commented Feb 24, 2021 • edited Loading

Codecov Report

amaury1093 left a comment

Choose a reason for hiding this comment

amaury1093 left a comment • edited Loading

Choose a reason for hiding this comment

atheeshp commented Mar 5, 2021

orijbot commented Mar 18, 2021

orijbot commented Mar 18, 2021

aaronc Mar 18, 2021

Choose a reason for hiding this comment

alessio Mar 18, 2021

Choose a reason for hiding this comment

clevinson Mar 19, 2021

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

fdymylja left a comment

Choose a reason for hiding this comment

orijbot commented Mar 20, 2021

alessio left a comment

Choose a reason for hiding this comment

alessio Mar 20, 2021

Choose a reason for hiding this comment

atheeshp Mar 22, 2021

Choose a reason for hiding this comment

amaury1093 left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 24, 2021 •

edited

Loading

amaury1093 left a comment •

edited

Loading