feat(x/mint): Add max supply param #19896
Conversation
WalkthroughThe changes introduce a new Changes
Assessment against linked issues
Possibly related issues
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
This comment has been minimized.
This comment has been minimized.
julienrbrt
left a comment
julienrbrt
left a comment
There was a problem hiding this comment.
I think we need to bump consensus version + migration.
We are missing a changelog in x/mint/changelog as well and an update to x/mint/readme.md
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (9)
- api/cosmos/mint/v1beta1/mint.pulsar.go (16 hunks)
- tests/e2e/mint/grpc.go (1 hunks)
- x/mint/keeper/abci.go (1 hunks)
- x/mint/keeper/genesis_test.go (1 hunks)
- x/mint/keeper/msg_server_test.go (1 hunks)
- x/mint/proto/cosmos/mint/v1beta1/mint.proto (1 hunks)
- x/mint/simulation/genesis.go (1 hunks)
- x/mint/types/mint.pb.go (5 hunks)
- x/mint/types/params.go (4 hunks)
Path instructions used (8)
tests/e2e/mint/grpc.go (2)
**/*.go
Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
tests/**/*
Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull requestx/mint/keeper/msg_server_test.go (2)
**/*.go
Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
**/*_test.go
Assess the unit test code assessing sufficient code coverage for the changes associated in the pull requestx/mint/simulation/genesis.go (1)
**/*.go
Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/mint/keeper/abci.go (1)
**/*.go
Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/mint/keeper/genesis_test.go (2)
**/*.go
Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
**/*_test.go
Assess the unit test code assessing sufficient code coverage for the changes associated in the pull requestx/mint/types/params.go (1)
**/*.go
Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/mint/types/mint.pb.go (1)
**/*.go
Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.api/cosmos/mint/v1beta1/mint.pulsar.go (1)
**/*.go
Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments (17)
tests/e2e/mint/grpc.go (1)
- 32-32: The addition of
math.ZeroInt()tominttypes.QueryParamsResponsein the test case correctly reflects the changes made to the mint module to support amax supplyparameter. This ensures the test remains valid and accurately represents the module's updated behavior.x/mint/keeper/msg_server_test.go (1)
- 56-56: The addition of
MaxSupplywithsdkmath.ZeroInt()in the test case correctly reflects the changes made to the mint module to support amax supplyparameter, including scenarios where an infinite supply is a valid configuration. This ensures the test remains valid and accurately represents the module's updated behavior.x/mint/proto/cosmos/mint/v1beta1/mint.proto (1)
- 62-67: The addition of the
max_supplyfield to theParamsmessage with the typecosmossdk.io/math.Intand marked as non-nullable correctly reflects the changes made to the mint module to support amax supplyparameter. This ensures the protobuf definition remains valid and accurately represents the module's updated behavior.x/mint/simulation/genesis.go (1)
- 69-69: The addition of
math.ZeroInt()to theNewParamsfunction call in the simulation function correctly reflects the changes made to the mint module to support amax supplyparameter, including scenarios where an infinite supply is a valid configuration. This ensures the simulation remains valid and accurately represents the module's updated behavior.x/mint/types/mint.pb.go (3)
- 84-85: The addition of the
MaxSupplyfield to theParamsstruct is correctly implemented. This field is essential for enforcing the maximum supply limit for the token as described in the PR objectives.- 238-247: The serialization code for the
MaxSupplyfield in theParamsstruct is correctly implemented. This ensures that theMaxSupplyvalue is correctly serialized into the binary format for storage or network transmission.- 693-726: The deserialization code for the
MaxSupplyfield in theParamsstruct is correctly implemented. This ensures that theMaxSupplyvalue is correctly read from the binary format into the struct.api/cosmos/mint/v1beta1/mint.pulsar.go (10)
- 507-513: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [510-522]
The addition of the
MaxSupplyfield to theParamsstruct is correctly implemented in the protobuf descriptor. This change aligns with the PR objectives to introduce a maximum supply limit for tokens.
- 626-631: The
Rangemethod has been updated to include theMaxSupplyfield. This ensures that the new field is considered during reflection operations, which is necessary for correct serialization and deserialization.- 659-660: The
Hasmethod correctly checks for the presence of theMaxSupplyfield. This is important for determining if the field is set, especially since protobuf uses default values for unset fields.- 689-690: The
Clearmethod has been updated to allow clearing theMaxSupplyfield. This is a necessary part of the protobuf message interface, allowing fields to be programmatically unset.- 725-727: The
Getmethod correctly retrieves the value of theMaxSupplyfield. This method is crucial for accessing field values in a type-safe manner.- 760-761: The
Setmethod has been appropriately updated to allow setting theMaxSupplyfield. This change is necessary for the mutability of the protobuf message.- 794-795: The
Mutablemethod correctly panics when attempting to mutate theMaxSupplyfield, as expected for scalar fields in protobuf messages. This behavior is consistent with the immutability of scalar fields in protobuf.- 821-822: The
NewFieldmethod has been updated to return a default value for theMaxSupplyfield when creating a new field instance. This ensures that new fields are initialized with appropriate default values.- 1430-1442: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1367-1438]
The
Paramsstruct now includes theMaxSupplyfield with appropriate protobuf field annotations. This addition is crucial for the feature's goal to introduce a maximum supply limit for tokens. The protobuf field number (7) is correctly sequenced after the existing fields.
- 1491-1496: The protobuf field definition for
MaxSupplycorrectly specifies the field type asstringand includes a field number. The use of a string type forMaxSupplyis consistent with other numeric fields in the Cosmos SDK that are represented as strings to accommodate large values. However, it's important to ensure that the application logic properly handles the conversion and validation of this string to a numeric type, especially considering the context of token supply where numeric operations are common.Verification successful
The search results indicate that
MaxSupplyis handled in the application logic through the use of custom types (e.g.,cosmossdk.io/math.Int) and validation logic, suggesting an indirect but appropriate approach to managing its conversion and validation as a numeric value. This approach is consistent with the need to accommodate large values and perform numeric operations onMaxSupply, aligning with the practices observed in the Cosmos SDK. Therefore, the review comment's concerns are addressed by the existing implementation patterns.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that application logic correctly handles string to numeric conversion for MaxSupply grep -R "MaxSupply" . --include=*.go | grep -E "Parse|Convert"Length of output: 64
Script:
#!/bin/bash # Broaden the search to understand how MaxSupply is handled in the application logic grep -R "MaxSupply" . --include=*.goLength of output: 2486
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: .coderabbit.yml
Files selected for processing (4)
- x/mint/CHANGELOG.md (1 hunks)
- x/mint/README.md (3 hunks)
- x/mint/keeper/migrator.go (1 hunks)
- x/mint/module.go (2 hunks)
Path instructions used (4)
x/mint/keeper/migrator.go (1)
**/*.go
Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/mint/CHANGELOG.md (1)
**/*.md
Assess the documentation for misspellings, grammatical errors, missing documentation and correctnessx/mint/module.go (1)
**/*.go
Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/mint/README.md (1)
**/*.md
Assess the documentation for misspellings, grammatical errors, missing documentation and correctness
Additional comments (4)
x/mint/CHANGELOG.md (1)
- 30-30: Ensure that the changelog entry for the new
max supply genesis paramis clear and accurately describes the change. Consider adding more detail about how this parameter affects the minting process or its significance.x/mint/module.go (2)
- 24-24: The update of
ConsensusVersionto 3 is appropriate given the introduction of a new feature that likely affects consensus. Ensure that all dependent modules or components are aware of this version change.Verification successful
The verification process did not reveal any direct dependencies on the mint module's
ConsensusVersionfrom other modules. Each module, including the mint module, appears to manage its own consensus version independently. This suggests that the update of theConsensusVersionto 3 in the mint module is self-contained and should not directly affect other modules based on the information provided. However, it's crucial for all related migrations and version dependencies within the system to be thoroughly reviewed to ensure compatibility.* 112-114: Registration of the migration from version 2 to 3 in `RegisterMigrations` is correctly implemented. Ensure that the migration logic in `Migrate2to3` is completed and tested to avoid runtime errors.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify if any module or component specifically checks for the mint module's consensus version. rg "ConsensusVersion" --type goLength of output: 11596
x/mint/README.md (1)
- 148-148: The addition of the
MaxSupplyparameter is correctly documented in the parameters table. Ensure that the typestring (math.Int)and the example value"0"are accurate and consistent with the implementation. If"0"signifies no limit, consider clarifying this in the documentation.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (1)
- x/mint/keeper/migrator.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/mint/keeper/migrator.go
x/mint/keeper/abci.go
Outdated
| if !maxSupply.IsZero() { | ||
| if totalStakingSupply.Add(mintedCoins.AmountOf(params.MintDenom)).GT(maxSupply) { | ||
| // calculate the difference between maxSupply and totalStakingSupply | ||
| diff := maxSupply.Sub(totalStakingSupply) |
There was a problem hiding this comment.
We should also consider the burned tokens, correct?
There was a problem hiding this comment.
now that we use total supply from bank, supply is updated every time right?
There was a problem hiding this comment.
Supply doesn't track the burned tokens I guess. If it's not handled, we should track them separately?
There was a problem hiding this comment.
but how do we get burned tokens ?
There was a problem hiding this comment.
There was a problem hiding this comment.
burned tokens are deducted from the total supply in bank, no need to handle anything here
There was a problem hiding this comment.
That's the issue right. Let's say if the max limit is m, x tokens are already burned, the new max-tokens-limit should be m-x since x were minted some of time in the history. Otherwise burning tokens doesn't make sense in this case
There was a problem hiding this comment.
I always thought of it differently. That if you reduce the token then it gives you more to mint unless you reduce the total supply. I'm not sure how we will track this otherwise and on existing chains. Is there a need for this?
There was a problem hiding this comment.
We can just have 0 address for burning tokens. That way it will not be reduced from Total supply directly but we can have tally of burned tokens. For existing chains, if they are maintaining the data of burned tokens somewhere, they can just include in the upgrade handler to mint those tokens for 0 address. We need to change the burncoins functionality to send it to this 0 address
There was a problem hiding this comment.
Hmm I tend to disagree this is an overall change. It's worth talking to users first instead of making the change blindly.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // calculate the difference between maxSupply and totalStakingSupply | ||
| diff := maxSupply.Sub(totalStakingSupply) | ||
| // mint the difference | ||
| diffCoin := sdk.NewCoin(params.MintDenom, diff) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods
| diff := maxSupply.Sub(totalStakingSupply) | ||
| // mint the difference | ||
| diffCoin := sdk.NewCoin(params.MintDenom, diff) | ||
| diffCoins := sdk.NewCoins(diffCoin) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods
| // mint coins if maxSupply is infinite or total staking supply is less than maxSupply | ||
| if maxSupply.IsZero() || totalStakingSupply.LT(maxSupply) { | ||
| // mint coins | ||
| if err := k.MintCoins(ctx, mintedCoins); err != nil { |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: .coderabbit.yml
Files selected for processing (3)
- x/mint/keeper/abci.go (2 hunks)
- x/mint/testutil/expected_keepers_mocks.go (1 hunks)
- x/mint/types/expected_keepers.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/mint/keeper/abci.go
Additional Context Used
Path-based Instructions (2)
x/mint/types/expected_keepers.go (1)
**/*.go:
Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/mint/testutil/expected_keepers_mocks.go (1)
**/*.go:
Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (3)
x/mint/types/expected_keepers.go (1)
34-34: The addition of theGetSupplymethod to theBankKeeperinterface is a logical extension for managing token supplies. However, it's recommended to add documentation comments to clarify the purpose and expected behavior of this method, especially regarding how it interacts with different denominations.x/mint/testutil/expected_keepers_mocks.go (2)
170-176: The implementation of theGetSupplymethod in theMockBankKeeperstruct correctly follows the mocking pattern expected for interfaces in Go, using MockGen. This addition will facilitate testing scenarios involving the supply of tokens.
178-182: The recorder method forGetSupplyin theMockBankKeeperMockRecorderstruct is correctly implemented, following the standard mocking conventions in Go. This will enable developers to set up detailed expectations and behaviors for theGetSupplymethod in their tests.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: .coderabbit.yml
Files selected for processing (1)
- x/mint/keeper/abci.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/mint/keeper/abci.go
Additional Context Used
| if err != nil { | ||
| return err | ||
| maxSupply := params.MaxSupply | ||
| totalSupply := k.bankKeeper.GetSupply(ctx, params.MintDenom).Amount // fetch total supply from the bank module |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods
|
|
||
| // if maxSupply is not infinite, check against max_supply parameter | ||
| if !maxSupply.IsZero() { | ||
| if totalSupply.Add(mintedCoins.AmountOf(params.MintDenom)).GT(maxSupply) { |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods
|
|
||
| // if maxSupply is not infinite, check against max_supply parameter | ||
| if !maxSupply.IsZero() { | ||
| if totalSupply.Add(mintedCoins.AmountOf(params.MintDenom)).GT(maxSupply) { |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods
| if !maxSupply.IsZero() { | ||
| if totalSupply.Add(mintedCoins.AmountOf(params.MintDenom)).GT(maxSupply) { | ||
| // calculate the difference between maxSupply and totalSupply | ||
| diff := maxSupply.Sub(totalSupply) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods
| diffCoins := sdk.NewCoins(diffCoin) | ||
|
|
||
| // mint coins | ||
| if err := k.MintCoins(ctx, diffCoins); err != nil { |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods
| } | ||
|
|
||
| // mint coins if maxSupply is infinite or total staking supply is less than maxSupply | ||
| if maxSupply.IsZero() || totalSupply.Add(mintedCoins.AmountOf(params.MintDenom)).LT(maxSupply) { |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods
| } | ||
|
|
||
| // mint coins if maxSupply is infinite or total staking supply is less than maxSupply | ||
| if maxSupply.IsZero() || totalSupply.Add(mintedCoins.AmountOf(params.MintDenom)).LT(maxSupply) { |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods
tac0turtle
left a comment
tac0turtle
left a comment
There was a problem hiding this comment.
utACK, we should a sentence or two to the readme about this change.
There was a problem hiding this comment.
Actionable comments posted: 1
Review Status
Configuration used: .coderabbit.yml
Files selected for processing (1)
- x/mint/README.md (4 hunks)
Additional Context Used
Path-based Instructions (1)
x/mint/README.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
Additional comments not posted (2)
x/mint/README.md (2)
66-67: The explanation of theMaxSupplyparameter is clear and concise. However, it would enhance clarity to explicitly mention that settingMaxSupplyto "0" means there is no limit to the token supply, as suggested by a previous comment.Consider adding a sentence to explicitly state that a
MaxSupplyvalue of "0" signifies an unlimited token supply, which aligns with the suggestion from the previous review.
313-314: In the JSON example under theParamsgRPC endpoint section, themaxSupplyfield is correctly updated to reflect the new parameter. However, ensure consistency in the documentation by capitalizing the first letter of sentences and correcting verb agreement in various sections as indicated by static analysis hints.Consider reviewing the document for grammatical consistency, particularly in sections detailing CLI, gRPC, and REST commands, to ensure that sentences start with uppercase letters and verbs agree with their subjects.
|
Blocking this PR for now |
|
since this is an optional thing, i think we should get the feature in. mint needs other changes as well as the current design is not sufficient for different use cases. Also the burn/total supply has nothing to do with mint, it assumes the external module handles this. If we document this then users could look into how things should work. |
|
should we get this merged ? |
Description
Closes: #13308
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.
I have...
Summary by CodeRabbit
New Features
max supply genesis paramto control the maximum supply of minted coins.Documentation
MaxSupplyparameter, its type, and usage examples.Refactor
maxSupplylimit during coin minting operations.Tests
maxSupplyfeature.