refactor: remove staking as a require module from mint #21858
Conversation
WalkthroughWalkthroughThe changes primarily involve modifications to the minting process within the Cosmos SDK's mint module. Key updates include the removal of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
tac0turtle
requested review from
lucaslopezf,
facundomedica,
a team and
julienrbrt
as code owners
| func (k Keeper) GetAuthority() string { | ||
| return k.authority | ||
| } | ||
| // SetMintFn sets the mint function. |
There was a problem hiding this comment.
That's meh, why not add this in the constructor?
There was a problem hiding this comment.
then the mint module would take the staking keeper in the constructor, i wanted to avoid the staking keeper being required in the constructor to avoid optional parameters in the constructor. Also we dont know if the mintFn will require the mint module before its past to the constructor. It would be a weird look.
There was a problem hiding this comment.
Still feels like a code smell. types.MintFn doesn't look like it requires the staking module.
That function only has access to state via the environment, how would you use staking or mint in there? Via the router? Then you already don't need the keepers.
There was a problem hiding this comment.
the default function requires staking and mint keeper context is passed to them. that is the caveat
cosmos-sdk/x/mint/keeper/keeper.go
Line 121 in a16eea6
There was a problem hiding this comment.
I see, I think we can still make it okay with depinject. Posted a comment below
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Outside diff range and nitpick comments (6)
x/mint/epoch_hooks.go (1)
20-20: LGTM! Consider enhancing error handling.The change from
am.mintFntoam.keeper.MintFnaligns well with the PR objective of removing staking as a required module for minting. This refactoring improves encapsulation by moving the minting function into the keeper.Consider wrapping the error returned from
am.keeper.MintFnwith additional context to aid in debugging:-err = am.keeper.MintFn(ctx, &minter, epochIdentifier, epochNumber) +err = am.keeper.MintFn(ctx, &minter, epochIdentifier, epochNumber) +if err != nil { + return fmt.Errorf("failed to execute MintFn: %w", err) +}This change would provide more context in case of an error, making it easier to trace the source of the problem.
x/mint/keeper/abci.go (1)
12-12: Approve function signature change and suggest documentation updateThe removal of the
mintFnparameter from theBeginBlockerfunction signature aligns with the PR objective of decoupling the mint module from the staking module. This change improves encapsulation by making the minting function an internal part of the Keeper.Consider updating the function's documentation to reflect this change in behavior, explaining that the minting function is now internal to the Keeper.
x/mint/depinject.go (1)
Line range hint
1-85: Overall assessment of changes in x/mint/depinject.goThe changes in this file successfully remove the dependency on the staking module for minting, aligning with the PR objectives. The modifications simplify the minting logic and make it more flexible by requiring a non-nil
MintFnto be provided.Key points:
- The addition of a nil check for
MintFnensures that a minting function is always provided, though the error handling could be improved.- The direct setting of
MintFnwithout a fallback mechanism simplifies the logic.- The
NewAppModuleinstantiation has been updated to reflect these changes.These changes enhance the modularity of the minting functionality, allowing for greater flexibility in how minting is implemented. However, consider improving the error handling as suggested in the earlier comment.
x/mint/module_test.go (2)
67-67: LGTM! Consider adding a test forSetMintFn.The addition of
SetMintFnaligns with the PR objective of making the staking module optional for minting. This change enhances flexibility in setting the mint function.To improve test coverage, consider adding a specific test case for
SetMintFnto ensure it correctly sets the minting function.
78-78: LGTM! Consider adding a test for the newNewAppModulesignature.The simplification of
NewAppModuleinitialization by removing themintFnparameter aligns with the PR objective of decoupling the mint module from the staking module.To ensure comprehensive test coverage, consider adding a specific test case for the new
NewAppModulefunction signature to verify that it correctly initializes theAppModulewithout themintFnparameter.x/mint/keeper/genesis_test.go (1)
Line range hint
1-101: Improve test coverage for new functionalityWhile the existing tests cover the basic genesis functionality, they don't adequately test the new
SetMintFnfeature or its impact on the minting process. Consider adding the following test cases:
- Test different
SetMintFnimplementations and their effect on minting.- Verify that genesis import/export correctly handles the new minting function.
- Test error scenarios, such as setting an invalid minting function.
Example additional test:
func (s *GenesisTestSuite) TestGenesisWithCustomMintFn() { customMintFn := func(ctx context.Context, env appmodule.Environment, minter *types.Minter, epochId string, epochNumber int64) error { // Custom minting logic return nil } err := s.keeper.SetMintFn(customMintFn) s.NoError(err) genesisState := types.DefaultGenesisState() err = s.keeper.InitGenesis(s.sdkCtx, s.accountKeeper, genesisState) s.NoError(err) // Verify that the custom mint function is used during minting operations // ... exportedState, err := s.keeper.ExportGenesis(s.sdkCtx) s.NoError(err) // Verify that the exported state reflects the use of the custom mint function // ... }These additional tests will help ensure that the new
SetMintFnfunctionality is working as expected and properly integrated with the existing genesis operations.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (11)
- simapp/app.go (2 hunks)
- x/mint/depinject.go (1 hunks)
- x/mint/epoch_hooks.go (1 hunks)
- x/mint/keeper/abci.go (2 hunks)
- x/mint/keeper/genesis.go (1 hunks)
- x/mint/keeper/genesis_test.go (2 hunks)
- x/mint/keeper/grpc_query_test.go (0 hunks)
- x/mint/keeper/keeper.go (4 hunks)
- x/mint/keeper/keeper_test.go (5 hunks)
- x/mint/module.go (3 hunks)
- x/mint/module_test.go (2 hunks)
Files not reviewed due to no reviewable changes (1)
- x/mint/keeper/grpc_query_test.go
Additional context used
Path-based instructions (10)
simapp/app.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/mint/depinject.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/mint/epoch_hooks.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/mint/keeper/abci.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/mint/keeper/genesis.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/mint/keeper/genesis_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/mint/keeper/keeper.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/mint/keeper/keeper_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/mint/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/mint/module_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
Additional comments not posted (17)
x/mint/epoch_hooks.go (1)
Line range hint
35-37: Verify the intended behavior of AfterEpochEndThe
AfterEpochEndmethod is currently a no-op. While this might be intentional, it's worth confirming that no actions are required after an epoch ends for the mint module.Could you please confirm if this is the intended behavior? If actions are needed after an epoch ends, we should implement them here.
x/mint/keeper/abci.go (2)
Line range hint
1-34: Summary of changes and their impactThe modifications to
x/mint/keeper/abci.gosuccessfully remove the dependency on an external minting function, aligning with the PR objective of decoupling the mint module from the staking module. The changes are minimal, focused, and maintain the overall structure and logic of theBeginBlockerfunction.Key points:
- The function signature has been updated to remove the
mintFnparameter.- The internal logic now uses
k.MintFninstead of the passedmintFn.- These changes improve encapsulation while potentially allowing for more flexibility in minting function implementation.
The code adheres to the Uber Golang style guide, and there are no apparent issues with naming conventions or code structure.
25-25: Approve internal logic change and suggest verificationThe replacement of
mintFnwithk.MintFnis consistent with the function signature change and maintains the existing behavior. This modification enhances the modularity of the mint module.Please ensure that
k.MintFnis properly implemented within the Keeper struct and provides the same functionality as the previousmintFn. You can verify this by running the following command:x/mint/depinject.go (2)
80-80: Simplified MintFn setting looks goodThe direct setting of
MintFnwithout a fallback mechanism aligns well with the new requirement ofMintFnbeing non-nil. This change simplifies the logic and makes the code more straightforward.
82-82: Modified NewAppModule instantiation looks correctThe removal of the
MintFnparameter from theNewAppModulecall is consistent with the earlier changes and reflects the removal of the default minting function logic.To ensure this change is consistent throughout the codebase, please run the following command:
This will help verify that all
NewAppModulecalls have been updated correctly.Verification successful
Verification successful: No remaining
NewAppModulecalls withMintFnparameter found.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining NewAppModule calls with MintFn parameter rg --type go 'NewAppModule.*MintFn'Length of output: 37
Script:
#!/bin/bash # Enhanced search for NewAppModule calls with MintFn parameter rg --type go 'NewAppModule\s*\([^)]*MintFn[^)]*\)'Length of output: 52
x/mint/module_test.go (1)
Line range hint
1-96: Enhance test coverage for the new changes.While the existing tests provide good coverage for the basic functionality of the mint module, they don't specifically target the changes introduced in this PR. To ensure the robustness of the new implementation:
- Add test cases that verify the mint module's behavior without the staking module.
- Include tests that cover different scenarios of setting custom mint functions using
SetMintFn.- Ensure that the
TestEpochHooksfunction adequately tests the module's behavior with the new changes.These additions will help maintain high code quality and prevent potential regressions.
To assist in identifying areas that may need additional test coverage, you can run the following command:
This will generate a coverage report, helping to identify specific areas that may need more test cases.
x/mint/keeper/genesis_test.go (1)
62-62: LGTM: Keeper initialization updatedThe
NewKeeperfunction call has been updated to remove thestakingKeeper, which aligns with the PR objective of removing staking as a required module for minting.x/mint/module.go (4)
14-14: LGTM: Import statement added correctly.The new import statement for
mintKeeperis necessary and correctly aligns with the changes made to thekeeperfield type in theAppModulestruct.
43-43: LGTM: AppModule struct updated correctly.The
keeperfield type has been appropriately updated tomintKeeper.Keeper, which is consistent with the new import statement and the overall refactoring goal. The removal of themintFnfield aligns with the PR objective of decoupling the mint module from the staking module.
151-151: LGTM: BeginBlock method updated correctly.The
BeginBlockmethod has been appropriately updated to callam.keeper.BeginBlocker(ctx)without passingam.mintFn, which is consistent with the removal of themintFnfield from theAppModulestruct. This change aligns with the PR objective of decoupling the mint module from the staking module.Please verify that the
BeginBlockermethod in thekeeperpackage has been updated to handle this change:#!/bin/bash # Description: Verify that the BeginBlocker method in the keeper package has been updated. rg --type go -A 10 'func \(k Keeper\) BeginBlocker\(' x/mint/keeper/
51-51: LGTM: NewAppModule function signature updated correctly.The
keeperparameter type has been appropriately updated tomintKeeper.Keeper, which is consistent with the changes in theAppModulestruct. The removal of themintFnparameter aligns with the PR objective of decoupling the mint module from the staking module.Please verify that the function body has been updated to reflect these changes:
Verification successful
Verified: NewAppModule function has been correctly updated.
The
keeperparameter type is nowmintKeeper.Keeper, and themintFnparameter has been successfully removed. The function body reflects these changes appropriately.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the NewAppModule function body has been updated. rg --type go -A 10 'func NewAppModule\(' x/mint/module.goLength of output: 598
Script:
#!/bin/bash # Description: Verify that the NewAppModule function body has been updated. rg --type go -A 10 'func NewAppModule\(' x/mint/module.goLength of output: 244
simapp/app.go (2)
375-376: LGTM! Decoupling of MintKeeper from StakingKeeperThe changes successfully remove the direct dependency of MintKeeper on StakingKeeper during initialization, aligning with the PR objective. The new
SetMintFnmethod provides a flexible way to set the minting function, allowing for easier customization and potential future changes to the minting logic.
471-471: LGTM! Simplified mint AppModule initializationThe removal of the StakingKeeper parameter (previously passed as nil) from the
mint.NewAppModulefunction call is consistent with the decoupling of the mint module from the staking module. This change simplifies the AppModule initialization and reduces unnecessary dependencies.To ensure this change is consistent with the mint module's internal implementation, please run the following command:
Verification successful
Verified: StakingKeeper parameter successfully removed
The
NewAppModulefunction inx/mint/module.gono longer includes theStakingKeeperparameter. This confirms the decoupling of the mint module from the staking module, simplifying the AppModule initialization as intended.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the mint module no longer requires StakingKeeper in its NewAppModule function rg --type go 'func NewAppModule' x/mint/Length of output: 78
Script:
#!/bin/bash # Check if NewAppModule function includes StakingKeeper as a parameter rg --type go 'func NewAppModule\(.*StakingKeeper' x/mint/module.goLength of output: 68
Script:
#!/bin/bash # Retrieve the NewAppModule function signature to verify its parameters rg --type go 'func NewAppModule' x/mint/module.go -A 5Length of output: 160
x/mint/keeper/keeper.go (2)
73-81: SetMintFn method correctly enforces non-nil 'mintFn'The
SetMintFnmethod appropriately checks for a nilmintFnand ensures that theKeeperhas a valid mint function set, preventing potential runtime panics.
120-125: Error handling is properly implemented when retrieving staking dataThe retrieval of
stakingTokenSupplyandbondedRatioincludes appropriate error checks, ensuring that issues are caught and handled effectively.x/mint/keeper/keeper_test.go (2)
125-126: Consistent Initialization of Mint FunctionSetting the Mint function at the start of
TestBeginBlockeris a good practice. This ensures that the test uses the correct Mint function and reduces the risk of unintended side effects from previous tests.
140-145: Confirm Minter State Remains Unchanged with No-op Mint FunctionYou've set a Mint function that performs no operation. It's good that you retrieve
unchangedMinterafter callingBeginBlocker. Ensure that your assertions.Equal(newMinter, unchangedMinter)verifies that the minter's state remains unchanged, confirming the no-op Mint function behaves as intended.
| if keeper.mintFn == nil { | ||
| panic("mintFn cannot be nil") | ||
| } | ||
|
|
There was a problem hiding this comment.
Approve the nil check for mintFn, but consider moving it.
The addition of a nil check for keeper.mintFn is a good safeguard against uninitialized minting functions. This aligns with the principle of failing fast and explicitly.
However, consider moving this check to the beginning of the function. This would prevent unnecessary operations if mintFn is nil and follows the pattern of validating inputs early. Here's a suggested refactor:
func (keeper Keeper) InitGenesis(ctx context.Context, ak types.AccountKeeper, data *types.GenesisState) error {
+ if keeper.mintFn == nil {
+ panic("mintFn cannot be nil")
+ }
+
if err := keeper.Minter.Set(ctx, data.Minter); err != nil {
return err
}
if err := keeper.Params.Set(ctx, data.Params); err != nil {
return err
}
ak.GetModuleAccount(ctx, types.ModuleName)
- if keeper.mintFn == nil {
- panic("mintFn cannot be nil")
- }
return nil
}This change would improve the function's efficiency and readability.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if keeper.mintFn == nil { | |
| panic("mintFn cannot be nil") | |
| } | |
| func (keeper Keeper) InitGenesis(ctx context.Context, ak types.AccountKeeper, data *types.GenesisState) error { | |
| if keeper.mintFn == nil { | |
| panic("mintFn cannot be nil") | |
| } | |
| if err := keeper.Minter.Set(ctx, data.Minter); err != nil { | |
| return err | |
| } | |
| if err := keeper.Params.Set(ctx, data.Params); err != nil { | |
| return err | |
| } | |
| ak.GetModuleAccount(ctx, types.ModuleName) | |
| return nil | |
| } |
| err := s.keeper.SetMintFn(func(ctx context.Context, env appmodule.Environment, minter *types.Minter, epochId string, epochNumber int64) error { | ||
| return nil | ||
| }) | ||
| s.NoError(err) |
There was a problem hiding this comment.
Enhance test coverage for SetMintFn
While the SetMintFn is being set, the current implementation is a no-op function. Consider adding more comprehensive tests to verify the behavior of this new functionality.
Suggestions:
- Add test cases that use different
SetMintFnimplementations to ensure it's correctly integrated. - Verify that the
SetMintFnis called during relevant mint operations in other test cases.
Example test case:
func (s *GenesisTestSuite) TestSetMintFn() {
called := false
err := s.keeper.SetMintFn(func(ctx context.Context, env appmodule.Environment, minter *types.Minter, epochId string, epochNumber int64) error {
called = true
return nil
})
s.NoError(err)
// Trigger a mint operation
// ...
s.True(called, "SetMintFn was not called during minting operation")
}| // mintFn is used to mint new coins during BeginBlock. This function is in charge of | ||
| // minting new coins based on arbitrary logic, previously done through InflationCalculationFn. | ||
| // If mintFn is nil, the default minting logic is used. | ||
| mintFn types.MintFn |
There was a problem hiding this comment.
Clarify the behavior when 'mintFn' is nil to avoid inconsistency
The comment states: "If mintFn is nil, the default minting logic is used." However, the SetMintFn method prevents mintFn from being nil by returning an error if it is. This might cause confusion. Please update the comment to accurately reflect that mintFn must be set and cannot be nil.
| // DefaultMintFn returns a default mint function. It requires the Staking module and the mint keeper. | ||
| // The default Mintfn has a requirement on staking as it uses bond to calculate inflation. | ||
| func DefaultMintFn(ic types.InflationCalculationFn, staking types.StakingKeeper, k Keeper) types.MintFn { |
There was a problem hiding this comment.
Initialize 'mintFn' with default logic to improve safety
Consider initializing mintFn with a default mint function during Keeper construction. This provides a safe fallback and ensures that minting operations have valid logic even if SetMintFn is not called.
Apply this refactor to set a default mintFn:
func NewKeeper(
cdc codec.BinaryCodec,
env appmodule.Environment,
ak types.AccountKeeper,
bk types.BankKeeper,
feeCollectorName string,
authority string,
+ staking types.StakingKeeper,
) Keeper {
// ensure mint module account is set
if addr := ak.GetModuleAddress(types.ModuleName); addr == nil {
panic(fmt.Sprintf("the x/%s module account has not been set", types.ModuleName))
}
sb := collections.NewSchemaBuilder(env.KVStoreService)
k := Keeper{
Environment: env,
cdc: cdc,
bankKeeper: bk,
feeCollectorName: feeCollectorName,
authority: authority,
Params: collections.NewItem(sb, types.ParamsKey, "params", codec.CollValue[types.Params](cdc)),
Minter: collections.NewItem(sb, types.MinterKey, "minter", codec.CollValue[types.Minter](cdc)),
+ mintFn: DefaultMintFn(DefaultInflationCalculationFn, staking, nil),
}
// [existing code]
return k
}This refactor requires passing staking to NewKeeper and sets mintFn to the default implementation if no custom function is provided.
Committable suggestion was skipped due to low confidence.
| err := s.mintKeeper.SetMintFn(keeper.DefaultMintFn(types.DefaultInflationCalculationFn, s.stakingKeeper, s.mintKeeper)) | ||
| s.NoError(err) |
There was a problem hiding this comment.
Add Assertions to Confirm Mint Function Is Set Correctly
While you are checking for errors after setting the Mint function, consider adding assertions to verify that the Mint function has been set as intended. This will ensure that SetMintFn has properly initialized the Mint function before proceeding with the test.
|
|
||
| minter, err := s.mintKeeper.Minter.Get(s.ctx) | ||
| s.NoError(err) | ||
|
|
||
| err = s.mintKeeper.DefaultMintFn(types.DefaultInflationCalculationFn)(s.ctx, s.mintKeeper.Environment, &minter, "block", 0) | ||
| err = s.mintKeeper.MintFn(s.ctx, &minter, "block", 0) |
There was a problem hiding this comment.
Enhance Test by Verifying Mint Operation Results
After calling s.mintKeeper.MintFn(s.ctx, &minter, "block", 0), it's beneficial to add assertions to verify that the expected state changes have occurred. For example, you can check if the total supply has increased appropriately or if the minter's properties have been updated. This will strengthen the test by confirming the Mint function behaves as expected.
| @@ -118,7 +96,7 @@ | |||
| err = s.mintKeeper.Params.Set(s.ctx, params) | |||
| s.NoError(err) | |||
|
|
|||
| err = s.mintKeeper.DefaultMintFn(types.DefaultInflationCalculationFn)(s.ctx, s.mintKeeper.Environment, &minter, "block", 0) | |||
| err = s.mintKeeper.MintFn(s.ctx, &minter, "block", 0) | |||
There was a problem hiding this comment.
Verify Behavior When Total Supply Exceeds Max Supply
In this scenario where the total supply exceeds the max supply, consider adding assertions to check that no additional coins are minted and that the minter's state reflects this condition. Confirming this behavior in your test ensures that the Mint function correctly handles cases where minting should be limited.
| @@ -134,7 +112,7 @@ | |||
| s.bankKeeper.EXPECT().MintCoins(s.ctx, types.ModuleName, sdk.NewCoins(sdk.NewCoin("stake", math.NewInt(2000)))).Return(nil) | |||
| s.bankKeeper.EXPECT().SendCoinsFromModuleToModule(s.ctx, types.ModuleName, authtypes.FeeCollectorName, gomock.Any()).Return(nil) | |||
|
|
|||
| err = s.mintKeeper.DefaultMintFn(types.DefaultInflationCalculationFn)(s.ctx, s.mintKeeper.Environment, &minter, "block", 0) | |||
| err = s.mintKeeper.MintFn(s.ctx, &minter, "block", 0) | |||
There was a problem hiding this comment.
Assert Minted Amount Matches Adjusted Parameters
Given the adjustments to MaxSupply and BlocksPerYear, after calling s.mintKeeper.MintFn(s.ctx, &minter, "block", 0), add assertions to verify that the minted amount is as expected (e.g., 2000 stake). This ensures the Mint function correctly calculates the mint amount based on the new parameters.
| // get minter (it should get modified afterwards) | ||
| minter, err := s.mintKeeper.Minter.Get(s.ctx) | ||
| s.NoError(err) | ||
|
|
||
| err = s.mintKeeper.BeginBlocker(s.ctx, s.mintKeeper.DefaultMintFn(types.DefaultInflationCalculationFn)) | ||
| err = s.mintKeeper.BeginBlocker(s.ctx) |
There was a problem hiding this comment.
Enhance Test by Asserting State Changes After BeginBlocker
After invoking s.mintKeeper.BeginBlocker(s.ctx), consider adding assertions to verify that the minter state has been updated appropriately. For instance, you can compare specific fields of newMinter and minter to confirm the expected changes occurred due to the BeginBlocker execution.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -159,7 +148,7 @@ | |||
|
|
|||
| // BeginBlock returns the begin blocker for the mint module. | |||
| func (am AppModule) BeginBlock(ctx context.Context) error { | |||
| return am.keeper.BeginBlocker(ctx, am.mintFn) | |||
| return am.keeper.BeginBlocker(ctx) | |||
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| @@ -64,31 +64,22 @@ func ProvideModule(in ModuleInputs) ModuleOutputs { | |||
| panic(err) | |||
| } | |||
|
|
|||
| if in.MintFn == nil { | |||
There was a problem hiding this comment.
should not be marked as optional then.
| func (k Keeper) GetAuthority() string { | ||
| return k.authority | ||
| } | ||
| // SetMintFn sets the mint function. |
There was a problem hiding this comment.
Still feels like a code smell. types.MintFn doesn't look like it requires the staking module.
That function only has access to state via the environment, how would you use staking or mint in there? Via the router? Then you already don't need the keepers.
| in.AccountKeeper, | ||
| in.BankKeeper, | ||
| feeCollectorName, | ||
| as, | ||
| ) | ||
|
|
||
| if in.MintFn != nil && in.InflationCalculationFn != nil { |
There was a problem hiding this comment.
I am not fan of that setter, but we can get an OK UX with depinject.
IMHO we should keep most of this logic and just make the stakingkeeper optional.
If StakingKeeper && MintFn is nil, then we panic as well.
The rest of this can be brought back.
| func (k Keeper) GetAuthority() string { | ||
| return k.authority | ||
| } | ||
| // SetMintFn sets the mint function. |
There was a problem hiding this comment.
I see, I think we can still make it okay with depinject. Posted a comment below
| authKeeper types.AccountKeeper | ||
|
|
||
| // mintFn is used to mint new coins during BeginBlock. This function is in charge of |
There was a problem hiding this comment.
All of this go doc should be brought on SetMintFn
| @@ -11,6 +11,7 @@ import ( | |||
| "cosmossdk.io/core/appmodule" | |||
| "cosmossdk.io/core/registry" | |||
| "cosmossdk.io/x/mint/keeper" | |||
| mintKeeper "cosmossdk.io/x/mint/keeper" | |||
There was a problem hiding this comment.
can we just say keeper? we always do that for the keeper of the module we are in.
| @@ -39,34 +40,22 @@ var ( | |||
| // AppModule implements an application module for the mint module. | |||
| type AppModule struct { | |||
| cdc codec.Codec | |||
| keeper keeper.Keeper | |||
| keeper mintKeeper.Keeper | |||
There was a problem hiding this comment.
this could be k if the package shadowing was an issue
Description
this pr is part of the staking requirements in the repo. Reducing it allows more people to use the module without needing to use the default staking module
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...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers 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.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
SetMintFnto define the minting function for enhanced flexibility.MintKeeperandAppModuleto simplify setup.Bug Fixes
Refactor