-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
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 (
|
…ita/mint-max-supply
This comment has been minimized.
This comment has been minimized.
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.
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
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.
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.QueryParamsResponse
in the test case correctly reflects the changes made to the mint module to support amax supply
parameter. 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
MaxSupply
withsdkmath.ZeroInt()
in the test case correctly reflects the changes made to the mint module to support amax supply
parameter, 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_supply
field to theParams
message with the typecosmossdk.io/math.Int
and marked as non-nullable correctly reflects the changes made to the mint module to support amax supply
parameter. 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 theNewParams
function call in the simulation function correctly reflects the changes made to the mint module to support amax supply
parameter, 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
MaxSupply
field to theParams
struct 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
MaxSupply
field in theParams
struct is correctly implemented. This ensures that theMaxSupply
value is correctly serialized into the binary format for storage or network transmission.- 693-726: The deserialization code for the
MaxSupply
field in theParams
struct is correctly implemented. This ensures that theMaxSupply
value 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
MaxSupply
field to theParams
struct 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
Range
method has been updated to include theMaxSupply
field. This ensures that the new field is considered during reflection operations, which is necessary for correct serialization and deserialization.- 659-660: The
Has
method correctly checks for the presence of theMaxSupply
field. This is important for determining if the field is set, especially since protobuf uses default values for unset fields.- 689-690: The
Clear
method has been updated to allow clearing theMaxSupply
field. This is a necessary part of the protobuf message interface, allowing fields to be programmatically unset.- 725-727: The
Get
method correctly retrieves the value of theMaxSupply
field. This method is crucial for accessing field values in a type-safe manner.- 760-761: The
Set
method has been appropriately updated to allow setting theMaxSupply
field. This change is necessary for the mutability of the protobuf message.- 794-795: The
Mutable
method correctly panics when attempting to mutate theMaxSupply
field, as expected for scalar fields in protobuf messages. This behavior is consistent with the immutability of scalar fields in protobuf.- 821-822: The
NewField
method has been updated to return a default value for theMaxSupply
field 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
Params
struct now includes theMaxSupply
field 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
MaxSupply
correctly specifies the field type asstring
and includes a field number. The use of a string type forMaxSupply
is 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
MaxSupply
is 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 param
is 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
ConsensusVersion
to 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
ConsensusVersion
from other modules. Each module, including the mint module, appears to manage its own consensus version independently. This suggests that the update of theConsensusVersion
to 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
MaxSupply
parameter 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also consider the burned tokens, correct?
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.
now that we use total supply from bank, supply is updated every time right?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but how do we get burned tokens ?
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.
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.
burned tokens are deducted from the total supply in bank, no need to handle anything here
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I tend to disagree this is an overall change. It's worth talking to users first instead of making the change blindly.
…ita/mint-max-supply
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.
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 theGetSupply
method to theBankKeeper
interface 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 theGetSupply
method in theMockBankKeeper
struct 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 forGetSupply
in theMockBankKeeperMockRecorder
struct is correctly implemented, following the standard mocking conventions in Go. This will enable developers to set up detailed expectations and behaviors for theGetSupply
method in their tests.
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.
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
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.
utACK, we should a sentence or two to the readme about this change.
…ita/mint-max-supply
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.
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 theMaxSupply
parameter is clear and concise. However, it would enhance clarity to explicitly mention that settingMaxSupply
to "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
MaxSupply
value of "0" signifies an unlimited token supply, which aligns with the suggestion from the previous review.
313-314
: In the JSON example under theParams
gRPC endpoint section, themaxSupply
field 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. |
…ita/mint-max-supply
should we get this merged ? |
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.
utACK!
…ita/mint-max-supply
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.md
Reviewers 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 param
to control the maximum supply of minted coins.Documentation
MaxSupply
parameter, its type, and usage examples.Refactor
maxSupply
limit during coin minting operations.Tests
maxSupply
feature.