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 8 commits
27494e8
e9d7c1d
dd4603f
0d771e4
3954247
a16eea6
bad6264
321ea18
246e4e1
870debd
b55219b
ab8fc1d
f21026b
dbde391
3154eb4
3d0fe07
ace9fd4
da93369
c74d409
1b006bf
File 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.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: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
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:
SetMintFn
implementations to ensure it's correctly integrated.SetMintFn
is 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
mintFn
is nil, the default minting logic is used." However, theSetMintFn
method preventsmintFn
from being nil by returning an error if it is. This might cause confusion. Please update the comment to accurately reflect thatmintFn
must 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
MintFn
method callsk.mintFn(...)
directly without verifying ifk.mintFn
is nil. Even thoughSetMintFn
enforcesmintFn
to be non-nil, adding a nil check here enhances robustness and prevents possible runtime panics ifSetMintFn
was not called.Apply this diff to add a nil check:
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
mintFn
with a default mint function duringKeeper
construction. This provides a safe fallback and ensures that minting operations have valid logic even ifSetMintFn
is not called.Apply this refactor to set a default
mintFn
:This refactor requires passing
staking
toNewKeeper
and setsmintFn
to 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
SetMintFn
has 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
MaxSupply
andBlocksPerYear
, 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
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 ofnewMinter
andminter
to confirm the expected changes occurred due to theBeginBlocker
execution.