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

Add fee grant module #8061

Merged
merged 240 commits into from
Jan 29, 2021
Merged

Add fee grant module #8061

merged 240 commits into from
Jan 29, 2021

Conversation

atheeshp
Copy link
Contributor

@atheeshp atheeshp commented Dec 2, 2020

ref: #7106


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

@atheeshp
Copy link
Contributor Author

@amaurymartiny can you have a look again. Thanks!

docs/core/proto-docs.md Outdated Show resolved Hide resolved
Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

lgtm, thanks @atheeshp !

Still not sure about having 2 antehandlers in the SDK (one normal, one normal+feegrant), but I don't want to block this PR on that.

x/feegrant/types/key.go Outdated Show resolved Hide resolved
x/feegrant/ante/fee_test.go Show resolved Hide resolved
@atheeshp
Copy link
Contributor Author

@anilcse , @blushi , @robert-zaremba can you guys have a look again

Copy link
Contributor

@blushi blushi 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 it'd be good to add some more docs in proto definitions.

proto/cosmos/feegrant/v1beta1/feegrant.proto Outdated Show resolved Hide resolved
proto/cosmos/feegrant/v1beta1/feegrant.proto Outdated Show resolved Hide resolved
@atheeshp atheeshp requested a review from blushi January 28, 2021 16:01
Copy link
Contributor

@blushi blushi left a comment

Choose a reason for hiding this comment

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

LGTM

@amaury1093 amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Jan 29, 2021
@aaronc
Copy link
Member

aaronc commented Jan 29, 2021

@anilcse @robert-zaremba can you please re-review soon as your reviews are currently blocking this, or let us know you'd like your change request dismissed. Thanks 🙏

Copy link
Collaborator

@anilcse anilcse left a comment

Choose a reason for hiding this comment

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

lgtm

@@ -166,6 +167,13 @@ func (ctx Context) WithFromAddress(addr sdk.AccAddress) Context {
return ctx
}

// WithFeeGranterAddress returns a copy of the context with an updated fee granter account
// address.
func (ctx Context) WithFeeGranterAddress(addr sdk.AccAddress) Context {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
func (ctx Context) WithFeeGranterAddress(addr sdk.AccAddress) Context {
func (ctx Context) WithFeeGranter(addr sdk.AccAddress) Context {

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.

Approving - left one suggestion re HasDefinedTime

}

// HasDefinedTime returns true if `ExpiresAt` has valid time
func (e ExpiresAt) HasDefinedTime() bool {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have Undefined method, so for symmetry , how about calling this method IsDefined?

@mergify mergify bot merged commit d97e790 into master Jan 29, 2021
@mergify mergify bot deleted the atheesh/x-fee-grant branch January 29, 2021 19:54
@atheeshp atheeshp mentioned this pull request Feb 1, 2021
15 tasks
@aaronc aaronc removed the in progress label Feb 2, 2021
larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
* Add docs

* Add BasicFeeAllowance implementation

* Add expiration structs and complete basic fee

* Add delegation messages, add validation logic

* Add keeper and helper structs

* Add alias and handler to top level

* Add delegation module

* Add basic querier

* Add types tests

* Add types tests

* More internal test coverage

* Solid internal test coverage

* Expose Querier to top level module

* Add FeeAccount to auth/types, like StdTx, SignDoc

* Fix all tests in x/auth

* All tests pass

* Appease the Golang Linter

* Add fee-account command line flag

* Start on DelegatedDeductFeeDecorator

* Cleanup the Decorator

* Wire up delegation module in simapp

* add basic test for decorator (no delegation)

* Table tests for deduct fees

* Table tests over all conditions of delegated fee decorator

* Build full ante handler stack and test it

* Start genesis

* Implement Genesis

* Rename package delegation to subkeys

* Clarify antes test cases, handle empty account w/o fees

* Allow paying delegated fees with no account

* Pull mempool into delegated ante, for control on StdFee

* Use custom DelegatedTx, DelegatedFee for subkeys

* Revert all changes to x/auth.StdTx

* Appease scopelint

* Register DelegatedTx with codec

* Address PR comments

* Remove unnecessary DelegatedMempoolFeeDecorator

* Cleaned up errors in querier

* Clean up message sign bytes

* Minor PR comments

* Replace GetAllFees... with Iterator variants

* PrepareForExport adjusts grant expiration height

* Panic on de/serialization error in keeper

* Move custom ante handler chain to tests, update docs

* More cleanup

* More doc cleanup

* Renamed subkeys module to fee_grant

* Rename subkeys/delegation to fee grant in all strings

* Modify Msg and Keeper methods to use Grant not Delegate

* Add PeriodicFeeAllowance

* Update aliases

* Cover all accept cases for PeriodicFeeAllowance

* Et tu scopelint?

* Update docs as requested

* Remove error return from GetFeeGrant

* Code cleanup as requested by PR

* Updated all errors to use new sdk/errors package

* Use test suite for keeper tests

* Clean up alias.go file

* Define expected interfaces in exported, rather than importing from account

* Remove dependency on auth/ante

* Improve godoc, Logger

* Cleaned up ExpiresAt

* Improve error reporting with UseGrantedFee

* Enforce period limit subset of basic limit

* Add events

* Rename fee_grant to feegrant

* Ensure KeeperTestSuite actually runs

* Move types/tx to types

* Update alias file, include ante

* I do need nolint in alias.go

* Properly emit events in the handler. Use cosmos-sdk in amino types

* Update godoc

* Linting...

* Update errors

* Update pkg doc and fix ante-handler order

* Merge PR cosmos#5782: Migrate x/feegrant to proto

* fix errors

* proto changes

* proto changes

* fix errors

* fix errors

* genesis state changed to proto

* fix keeper tests

* fix test

* fixed tests

* fix tests

* updated expected keepers

* updated ante tests

* lint

* deleted alias.go

* tx updated to proto tx

* remove explicit signmode

* tests

* Added `cli/query.go`

* Added tx.go in cli

* updated `module.go`

* resolve errors in tx.go

* Add fee payer gentx func

* updated tx

* fixed error

* WIP: cli tests

* fix query error

* fix tests

* Unused types and funcs

* fix tests

* rename helper func to create tx

* remove unused

* update tx cfg

* fix cli tests

* added simulations

* Add `decoder.go`

* fix build fail

* added init genesis code

* update tx.go

* fixed LGTM alert

* modified cli

* remove gogoproto extensions

* change acc address type to string

* lint

* fix simulations

* Add gen simulations

* remove legacy querier

* remove legacy code

* add grpc queries tests

* fix simulations

* update module.go

* lint

* register feegrant NewSimulationManager

* fix sims

* fix sims

* add genesis test

* add periodic grant

* updated cmd

* changed times

* updated flags

* removed days as period clock

* added condition for period and exp

* add periodic fee cli tests

* udpated tests

* fix lint

* fix tests

* fix sims

* renaming to `fee_grant`

* review changes

* fix test

* add condition for duplicate grants

* fix tests

* add `genTxWithFeeGranter` in tests

* fix simulation

* one of changes & test fixes

* fix test

* fix lint

* changed package name `feegrant` to `fee_grant`

* review comments

* review changes

* review change

* review changes

* added fee-account in flags

* address review changes

* read fee granter from cli

* updated create account with mnemonic

* Address review comments

* move `simapp/ante` file to `feegrant/ante`

* update keeper logic to create account

* update docs

* fix tests

* update `serviceMsgClientConn` from `msgservice`

* review changes

* add test case for using more fees than allowed

* eliminate panic checks from keeper

* fix lint

* change store keys string to bytes

* fix tests

* review changes

* review changes

* udpate docs

* make spend limit optional

* fix tests

* fix tests

* review changes

* add norace tag

* proto-docs

* add docs

Co-authored-by: Ethan Frey <[email protected]>
Co-authored-by: Alexander Bezobchuk <[email protected]>
Co-authored-by: Aleksandr Bezobchuk <[email protected]>
Co-authored-by: SaReN <[email protected]>
Co-authored-by: aleem1413 <[email protected]>
Co-authored-by: MD Aleem <[email protected]>
Co-authored-by: Anil Kumar Kammari <[email protected]>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
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:x/feegrant
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants