-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Move x/feegrant/types to x/feegrant #9273
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9273 +/- ##
==========================================
+ Coverage 60.18% 60.20% +0.01%
==========================================
Files 593 591 -2
Lines 37063 37017 -46
==========================================
- Hits 22306 22285 -21
+ Misses 12782 12758 -24
+ Partials 1975 1974 -1
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, could you also update the code snippets in specs
?
@aleem1314 Done, I've also updated the naming in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, just a small doubt.
simapp/app.go
Outdated
feegrantkeeper "github.com/cosmos/cosmos-sdk/x/feegrant/keeper" | ||
feegranttypes "github.com/cosmos/cosmos-sdk/x/feegrant/types" | ||
feegrant_m "github.com/cosmos/cosmos-sdk/x/feegrant/module" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this package name okay? shouldn't we consider feegrantmodule
here? I can see a name feegrantkeeper
which is similar to this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just wanted to make it consistent with @robert-zaremba's PR on authz updates #9042 but I'm open to change it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would vote for feegrantmodule
even though it's longer because it's easier to read.
* Move x/feegrant/types to x/feegrant * Update spec * Use feegrantmodule
Description
closes: #9115
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.
docs/
) or specification (x/<module>/spec/
)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorerCodecov Report
in the comment section below once CI passes