Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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): remove staking as a required module #21858
refactor(x/mint): remove staking as a required module #21858
Changes from 6 commits
27494e8e9d7c1ddd4603f0d771e43954247a16eea6bad6264321ea18246e4e1870debdb55219bab8fc1df21026bdbde3913154eb43d0fe07ace9fd4da93369c74d4091b006bfFile filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Approve the nil check for
mintFn, but consider moving it.The addition of a nil check for
keeper.mintFnis 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
mintFnis 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
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.
Enhance test coverage for
SetMintFnWhile the
SetMintFnis being set, the current implementation is a no-op function. Consider adding more comprehensive tests to verify the behavior of this new functionality.Suggestions:
SetMintFnimplementations to ensure it's correctly integrated.SetMintFnis called during relevant mint operations in other test cases.Example test case:
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.
Clarify the behavior when 'mintFn' is nil to avoid inconsistency
The comment states: "If
mintFnis nil, the default minting logic is used." However, theSetMintFnmethod preventsmintFnfrom being nil by returning an error if it is. This might cause confusion. Please update the comment to accurately reflect thatmintFnmust be set and cannot be nil.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.
Add nil check in 'MintFn' method to prevent potential panics
The
MintFnmethod callsk.mintFn(...)directly without verifying ifk.mintFnis nil. Even thoughSetMintFnenforcesmintFnto be non-nil, adding a nil check here enhances robustness and prevents possible runtime panics ifSetMintFnwas not called.Apply this diff to add a nil check:
func (k Keeper) MintFn(ctx context.Context, minter *types.Minter, epochId string, epochNumber int64) error { + if k.mintFn == nil { + return errors.New("mintFn is not set; please ensure SetMintFn is called") + } return k.mintFn(ctx, k.Environment, minter, epochId, epochNumber) }Committable suggestion
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.
Initialize 'mintFn' with default logic to improve safety
Consider initializing
mintFnwith a default mint function duringKeeperconstruction. This provides a safe fallback and ensures that minting operations have valid logic even ifSetMintFnis 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
stakingtoNewKeeperand setsmintFnto the default implementation if no custom function is provided.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.
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
SetMintFnhas properly initialized the Mint function before proceeding with the test.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.
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.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.
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.
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.
Assert Minted Amount Matches Adjusted Parameters
Given the adjustments to
MaxSupplyandBlocksPerYear, after callings.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.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.
Enhance Test by Asserting State Changes After
BeginBlockerAfter 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 ofnewMinterandminterto confirm the expected changes occurred due to theBeginBlockerexecution.