feat(x/gov): extend governance config #18428
Conversation
WalkthroughWalkthroughThe changes primarily focus on introducing length constraints for proposal titles, metadata, and summaries in the Cosmos SDK governance module. This is achieved by modifying the Changes
TipsChat with CodeRabbit Bot (
|
![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 UI
Files selected for processing (3)
- x/gov/keeper/keeper.go (1 hunks)
- x/gov/keeper/proposal.go (1 hunks)
- x/gov/types/config.go (1 hunks)
Files skipped from review due to trivial changes (1)
- x/gov/keeper/keeper.go
Additional comments: 3
x/gov/keeper/proposal.go (1)
- 19-41: The checks for metadata, summary, and title lengths are correctly implemented. Ensure that the
assertMetadataLength,assertSummaryLength, andassertTitleLengthfunctions handle edge cases correctly, such as empty strings or strings consisting only of whitespace.x/gov/types/config.go (2)
2-11: The changes to the
Configstruct are well-documented and the purpose of each field is clear. Ensure that the new fields are properly used throughout the codebase.13-20: The
DefaultConfigfunction has been updated to initialize the new fields with default values. Ensure that these default values are appropriate for your use case.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional comments: 1
CHANGELOG.md (1)
- 53-59: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [56-60]
The changes in the changelog are well documented and provide clear information about the improvements made in the pull request.
emidev98
force-pushed
the
feat/gov/config
branch
from
cb771a4
to
7328d03
Compare
There was a problem hiding this comment.
I would say this isn't a bug but as you have said in your changelog an improvement.
Currently title and summary are indeed scaled on the length of the metadata, but that isn't a bug.
If you want to add those extra configs you have more to modify: module options for instance in gov module.proto and set them in module.go. Additionally we should add some extra tests cases for those errors.
Additionally we should make this non breaking and keep devalue of title equal to the max metadata length.
This way we will be able to backport this improvement.
EDIT: thanks for having edited your PR description.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files selected for processing (8)
- CHANGELOG.md (1 hunks)
- api/cosmos/gov/module/v1/module.pulsar.go (15 hunks)
- proto/cosmos/gov/module/v1/module.proto (1 hunks)
- x/gov/keeper/keeper.go (2 hunks)
- x/gov/keeper/msg_server_test.go (3 hunks)
- x/gov/keeper/proposal.go (1 hunks)
- x/gov/module.go (1 hunks)
- x/gov/types/config.go (1 hunks)
Additional comments: 24
CHANGELOG.md (1)
- 53-59: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [56-60]
The changes are well documented and provide clear links to the corresponding pull requests for further details. No issues found.
proto/cosmos/gov/module/v1/module.proto (1)
- 10-27: The changes to the
Modulemessage look good. The addition ofmax_title_len,max_metadata_len, andmax_summary_lenfields with appropriate comments and default values is clear and understandable. The change in theauthorityfield fromstringtostringwith a default value of 4 seems to be a mistake as the type hasn't changed and the default value for a string field doesn't make sense. Please verify this.x/gov/types/config.go (2)
2-11: The changes to the
Configstruct are well-documented and the new fieldsMaxTitleLen,MaxMetadataLen, andMaxSummaryLenare self-explanatory. Ensure that the new fields are used correctly throughout the codebase.13-20: The
DefaultConfigfunction has been updated to initialize the new fields with default values. Ensure that these default values are appropriate for your use case.x/gov/keeper/msg_server_test.go (2)
135-149: This test case checks if the function correctly handles a title that is too long. The test case is well written and covers an important edge case.
150-156: This test case checks if the function correctly handles metadata that is too long. The test case is well written and covers an important edge case.
x/gov/keeper/keeper.go (4)
91-106: The code checks if the
MaxTitleLen,MaxMetadataLen, andMaxSummaryLenare set by the app developer and if not, sets them to default values. This is a good practice to ensure that the configuration is always initialized with valid values.196-198: The
assertTitleLengthfunction checks if the length of the title is greater thanMaxTitleLenand returns an error if it is. This is a good practice to ensure that the title length does not exceed the maximum allowed length.205-207: The
assertMetadataLengthfunction checks if the length of the metadata is greater thanMaxMetadataLenand returns an error if it is. This is a good practice to ensure that the metadata length does not exceed the maximum allowed length.214-216: The
assertSummaryLengthfunction checks if the length of the summary is greater thanMaxSummaryLenand returns an error if it is. This is a good practice to ensure that the summary length does not exceed the maximum allowed length.x/gov/module.go (1)
- 183-193: The code checks if the configuration values for
MaxTitleLen,MaxMetadataLen, andMaxSummaryLenare non-zero before assigning them todefaultConfig. This is a good practice as it prevents overwriting of default values with zero if the configuration is not properly set.api/cosmos/gov/module/v1/module.pulsar.go (13)
18-22: The new field descriptors for
max_title_len,max_metadata_len, andmax_summary_lenare correctly initialized in theinitfunction.98-115: The
Rangefunction correctly checks if the fields are not zero before calling the provided functionf. This is a good practice to avoid unnecessary function calls.137-142: The
Hasfunction correctly checks if the fields are not zero. This is a good practice to avoid unnecessary operations on zero values.161-166: The
Clearfunction correctly sets the fields to zero. This is a good practice to ensure that the fields are properly reset.185-193: The
Getfunction correctly returns the value of the fields. This is a good practice to ensure that the fields are properly accessed.217-222: The
Setfunction correctly sets the value of the fields. This is a good practice to ensure that the fields are properly updated.245-250: The
Mutablefunction correctly panics when trying to mutate the fields. This is a good practice to prevent unexpected behavior.266-271: The
NewFieldfunction correctly returns a new, empty value for the fields. This is a good practice to ensure that the fields are properly initialized.343-351: The size calculation in the
Sizefunction correctly includes the size of the fields. This is a good practice to ensure that the size is accurately calculated.390-404: The
MarshalToSizedBufferfunction correctly encodes the fields into the buffer. This is a good practice to ensure that the fields are properly serialized.457-474: The
Unmarshalfunction correctly decodes the fields from the input data. This is a good practice to ensure that the fields are properly deserialized.599-609: The
Modulestruct correctly defines the fields with the appropriate protobuf tags. This is a good practice to ensure that the fields are properly serialized and deserialized.632-651: The getter functions correctly return the value of the fields. This is a good practice to ensure that the fields are properly accessed.
|
Hello @julienrbrt , thanks for the review, I have applied your suggestion. Would it be okay if is a breaking change for the seek of usint int64 instead of uint64 values? For some reason the proto files are not generated as expected which causes the tests to fail with the following error (screenshot). |
There was a problem hiding this comment.
Could you precise those changes in the docs:
Line 311 in b668f4f
You have changed it from |
julienrbrt
changed the title
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (8)
- api/cosmos/gov/module/v1/module.pulsar.go (14 hunks)
- proto/cosmos/gov/module/v1/module.proto (1 hunks)
- x/gov/README.md (1 hunks)
- x/gov/keeper/keeper.go (3 hunks)
- x/gov/keeper/msg_server_test.go (3 hunks)
- x/gov/keeper/proposal.go (1 hunks)
- x/gov/module.go (1 hunks)
- x/gov/types/config.go (1 hunks)
Additional comments: 26
x/gov/module.go (1)
- 183-190: The function
ProvideModulehas been updated to set the maximum lengths for title, metadata, and summary based on the input configuration. It also sets the default authority to the governance module if not provided. Ensure that the input configuration is correctly set in all calls to this function.x/gov/README.md (1)
- 311-312: The changes to the maximum lengths of the fields are well documented. Ensure that the documentation is updated in all relevant places, including API documentation and user guides.
proto/cosmos/gov/module/v1/module.proto (1)
- 16-27: The new fields
max_title_lenandmax_summary_lenhave been added correctly to theModulemessage. Ensure that these fields are being used correctly in the rest of the codebase.x/gov/types/config.go (2)
2-11: The type of
MaxTitleLen,MaxMetadataLen, andMaxSummaryLenhas been changed fromuint64toint. Ensure that this change doesn't introduce any issues when these fields are used elsewhere in the codebase.13-20: The
DefaultConfigfunction has been updated to provide default values forMaxTitleLenandMaxSummaryLen. Ensure that these default values are appropriate and that they don't cause any issues when used in the application.x/gov/keeper/keeper.go (5)
91-106: The default values for
MaxTitleLen,MaxMetadataLen, andMaxSummaryLenare set correctly if not provided by the app developer. Ensure that these values are not set to zero elsewhere in the codebase.193-204: The
validateProposalLengthsfunction correctly checks the lengths ofmetadata,title, andsummaryagainst the maximum lengths defined in the configuration.206-213: The
assertTitleLengthfunction correctly checks the length oftitleagainstMaxTitleLenfrom the configuration.190-217: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [215-221]
The
assertMetadataLengthfunction correctly checks the length ofmetadataagainstMaxMetadataLenfrom the configuration.
- 224-230: The
assertSummaryLengthfunction correctly checks the length ofsummaryagainstMaxSummaryLenfrom the configuration.x/gov/keeper/msg_server_test.go (3)
51-51: The test case is now clear from the context of the pull request summary. It is testing the scenario where the metadata string length is 100 characters. The expected outcome is that the proposal submission should be successful as the length is within the new maximum limit.
135-149: This new test case is testing the scenario where the title length exceeds the maximum limit. The expected outcome is that the proposal submission should fail with the error message "title too long". This is a good addition to the test suite as it verifies the new validation logic for title length.
173-173: The test case is now clear from the context of the pull request summary. It is testing the scenario where the metadata string length is 10201 characters. The expected outcome is that the proposal submission should fail as the length exceeds the new maximum limit.
api/cosmos/gov/module/v1/module.pulsar.go (13)
20-30: The new fields
MaxTitleLenandMaxSummaryLenhave been added to theModulestruct and are being initialized in theinitfunction. Ensure that these fields are being used correctly throughout the codebase.110-121: The
Rangefunction has been updated to handle the new fieldsMaxTitleLenandMaxSummaryLen. This function is used to iterate over all populated fields of the message in an undefined order.141-144: The
Hasfunction has been updated to handle the new fieldsMaxTitleLenandMaxSummaryLen. This function is used to check if a field is populated.165-168: The
Clearfunction has been updated to handle the new fieldsMaxTitleLenandMaxSummaryLen. This function is used to clear the value of a field, making it non-present.191-196: The
Getfunction has been updated to handle the new fieldsMaxTitleLenandMaxSummaryLen. This function is used to get the value of a field.221-224: The
Setfunction has been updated to handle the new fieldsMaxTitleLenandMaxSummaryLen. This function is used to set the value of a field.249-252: The
Mutablefunction has been updated to handle the new fieldsMaxTitleLenandMaxSummaryLen. This function is used to check if a field is mutable.270-273: The
Newfunction has been updated to handle the new fieldsMaxTitleLenandMaxSummaryLen. This function is used to create a new value for a field.350-355: The
Sizefunction has been updated to handle the new fieldsMaxTitleLenandMaxSummaryLen. This function is used to compute the size of the message.385-394: The
Marshalfunction has been updated to handle the new fieldsMaxTitleLenandMaxSummaryLen. This function is used to serialize the message.507-544: The
Unmarshalfunction has been updated to handle the new fieldsMaxTitleLenandMaxSummaryLen. This function is used to deserialize the message.604-609: The
Modulestruct has been updated to include the new fieldsMaxTitleLenandMaxSummaryLen. Ensure that these fields are being used correctly throughout the codebase.646-658: Getter methods for the new fields
MaxTitleLenandMaxSummaryLenhave been added. Ensure that these methods are being used correctly throughout the codebase.
x/gov/keeper/keeper.go
Outdated
| // is greater than a pre-defined MaxTitleLen. | ||
| func (k Keeper) assertTitleLength(title string) error { | ||
| if title != "" && uint64(len(title)) > k.config.MaxTitleLen { | ||
| return types.ErrMetadataTooLong.Wrapf("got title with length %d", len(title)) |
There was a problem hiding this comment.
Note to myself, in the backport PR, make this message identical as the metadata message.
There was a problem hiding this comment.
Nit, for main let's create a new error for the title being too long. I'll revert in the backport.
There was a problem hiding this comment.
Right, thanks for that one
julienrbrt
added
the
backport/v0.50.x
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- x/gov/README.md (1 hunks)
- x/gov/keeper/keeper.go (3 hunks)
- x/gov/keeper/proposal.go (1 hunks)
- x/gov/module.go (1 hunks)
Files skipped from review due to trivial changes (1)
- x/gov/README.md
Additional comments: 7
x/gov/keeper/proposal.go (1)
- 19-28: The
SubmitProposalfunction now includes a call tovalidateProposalLengthsto check the lengths ofmetadata,title, andsummary. Ensure that this function correctly validates the lengths and handles any errors appropriately.x/gov/module.go (1)
- 182-193: The changes in this hunk allow for customization of maximum title, metadata, and summary lengths when creating a module. This is a good practice as it allows for flexibility and adaptability of the system. However, it's important to ensure that these lengths are not set to extremely low values that could hinder functionality. The previous comments on this hunk are still valid and should be considered.
x/gov/keeper/keeper.go (5)
91-106: The default values for
MaxTitleLen,MaxMetadataLen, andMaxSummaryLenare set correctly if not provided by the app developer.193-206: The
validateProposalLengthsfunction correctly checks the lengths of the metadata, title, and summary.209-215: The
assertTitleLengthfunction correctly checks the length of the title and returns an error if it exceedsMaxTitleLen.190-219: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [217-224]
The
assertMetadataLengthfunction correctly checks the length of the metadata and returns an error if it exceedsMaxMetadataLen.
- 226-232: The
assertSummaryLengthfunction correctly checks the length of the summary and returns an error if it exceedsMaxSummaryLen.
There was a problem hiding this comment.
lgtm!
Can you please fix the tests (you first need to add replace cosmossdk.io/api => ../../api in x/gov/go.mod).
One nit: #18428 (comment)
|
Thanks for the feedback! Very much appreciated! |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- x/gov/go.mod
- x/gov/go.sum
Files selected for processing (2)
- x/gov/keeper/keeper.go (3 hunks)
- x/gov/types/errors.go (1 hunks)
Additional comments: 6
x/gov/types/errors.go (1)
- 25-29: The new error
ErrTitleTooLongis correctly registered and follows the existing pattern. Ensure that this error is properly handled in the code where proposal titles are processed.x/gov/keeper/keeper.go (5)
91-106: The default values for
MaxTitleLen,MaxMetadataLen, andMaxSummaryLenare set if not provided by the app developer. This is a good practice as it ensures that these values are always defined.194-205: The
validateProposalLengthsfunction checks the lengths of the proposal's metadata, summary, and title against the configured maximum lengths. This is a good practice as it ensures that the lengths of these fields are within the expected limits.210-214: The
assertTitleLengthfunction checks the length of the title against the configured maximum length and returns an error if it exceeds the limit. This is a good practice as it ensures that the length of the title is within the expected limit.190-219: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [217-222]
The
assertMetadataLengthfunction checks the length of the metadata against the configured maximum length and returns an error if it exceeds the limit. This is a good practice as it ensures that the length of the metadata is within the expected limit.
- 226-232: The
assertSummaryLengthfunction checks the length of the summary against the configured maximum length and returns an error if it exceeds the limit. This is a good practice as it ensures that the length of the summary is within the expected limit.
|
Are you up to do the exact same changes in a follow-up PR for x/group? |
|
Sounds perfect to me, now that know how the x/gov works I assume that x/group should be similar, over the weekend I'll submit a PR there! Thanks for the reviews and sorry for this long back an forth, quiet a lot of things changed from 0.47 to 0.50! 🥂 |
(cherry picked from commit 89296cc) # Conflicts: # CHANGELOG.md # api/cosmos/gov/module/v1/module.pulsar.go # proto/cosmos/gov/module/v1/module.proto # x/gov/go.mod # x/gov/go.sum # x/gov/keeper/keeper.go
Description
This pull request extend the governance configuration adding a few more parameters that works independently and allow a different length for each of the proposals' text parameters:
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...
!to the type prefix if API or client breaking changeCHANGELOG.mdmake lintandmake testReviewers 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...
!in the type prefix if API or client breaking changeSummary by CodeRabbit