-
Notifications
You must be signed in to change notification settings - Fork 592
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(x/mint): unexport keeper methods #2417
Conversation
// - amount is nil or zero. | ||
// - if ctx has block height greater than 0. | ||
// - developer vesting module account is already created prior to calling this method. | ||
func (k Keeper) CreateDeveloperVestingModuleAccount(ctx sdk.Context, amount sdk.Coin) error { |
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.
Note to reviewers: some methods in this file were shuffled around so that exported methods go first, and, then, unexported follow
None of the methods were actually removed
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.
LGTM. Is there any specific methodology you go through to determine which keeper methods should stay unexported? I think it may be helpful to document this somewhere so we can easily determine in the future (and possibly automate warnings!)
As a general rule, we should be hiding as many methods as possible without a specific use case outside of the internal logic. More on the principle. For example, with We should be encapsulating logic into stable interfaces and exposing them to the clients. In the context of |
Co-authored-by: Adam Tucker <adam@osmosis.team>
Co-authored-by: Adam Tucker <adam@osmosis.team>
Co-authored-by: Adam Tucker <adam@osmosis.team>
Co-authored-by: Adam Tucker <adam@osmosis.team>
* refactor(x/mint): unexport keeper methods * changelog * Update x/mint/keeper/keeper.go Co-authored-by: Adam Tucker <adam@osmosis.team> * Update x/mint/keeper/keeper.go Co-authored-by: Adam Tucker <adam@osmosis.team> * Update x/mint/keeper/keeper.go Co-authored-by: Adam Tucker <adam@osmosis.team> * Update x/mint/keeper/keeper.go Co-authored-by: Adam Tucker <adam@osmosis.team> Co-authored-by: Adam Tucker <adam@osmosis.team> (cherry picked from commit 4176b28) # Conflicts: # CHANGELOG.md # app/apptesting/test_suite.go # x/mint/keeper/genesis.go # x/mint/keeper/hooks.go # x/mint/keeper/keeper.go # x/mint/keeper/keeper_test.go
Closes: #XXX
What is the purpose of the change
Currently, a lot of the keeper methods are exported when they are only used inside the keeper package or in tests.
For the purposes of information hiding, we should unexport as many methods as possible to avoid misuse from incorrect locations.
There are no changes other than unexporting some methods and none of the methods were removed.
Brief Changelog
setLastReductionEpochNum
getLastReductionEpochNum
createDeveloperVestingModuleAccount
mintCoins
apptesting
for minting tokens in testskeeper.go
so that exported methods go first, then unexportedTesting and Verifying
This change is a trivial rework / code cleanup without any test coverage.
Documentation and Release Note
Unreleased
section inCHANGELOG.md
? yes