-
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/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 (
|
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 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
, andassertTitleLength
functions 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
Config
struct 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
DefaultConfig
function 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.
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 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.
cb771a4
to
7328d03
Compare
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 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.
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: 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
Module
message look good. The addition ofmax_title_len
,max_metadata_len
, andmax_summary_len
fields with appropriate comments and default values is clear and understandable. The change in theauthority
field fromstring
tostring
with 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
Config
struct are well-documented and the new fieldsMaxTitleLen
,MaxMetadataLen
, andMaxSummaryLen
are self-explanatory. Ensure that the new fields are used correctly throughout the codebase.13-20: The
DefaultConfig
function 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
, andMaxSummaryLen
are 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
assertTitleLength
function checks if the length of the title is greater thanMaxTitleLen
and 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
assertMetadataLength
function checks if the length of the metadata is greater thanMaxMetadataLen
and 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
assertSummaryLength
function checks if the length of the summary is greater thanMaxSummaryLen
and 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
, andMaxSummaryLen
are 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_len
are correctly initialized in theinit
function.98-115: The
Range
function 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
Has
function correctly checks if the fields are not zero. This is a good practice to avoid unnecessary operations on zero values.161-166: The
Clear
function correctly sets the fields to zero. This is a good practice to ensure that the fields are properly reset.185-193: The
Get
function correctly returns the value of the fields. This is a good practice to ensure that the fields are properly accessed.217-222: The
Set
function correctly sets the value of the fields. This is a good practice to ensure that the fields are properly updated.245-250: The
Mutable
function correctly panics when trying to mutate the fields. This is a good practice to prevent unexpected behavior.266-271: The
NewField
function 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
Size
function correctly includes the size of the fields. This is a good practice to ensure that the size is accurately calculated.390-404: The
MarshalToSizedBuffer
function correctly encodes the fields into the buffer. This is a good practice to ensure that the fields are properly serialized.457-474: The
Unmarshal
function correctly decodes the fields from the input data. This is a good practice to ensure that the fields are properly deserialized.599-609: The
Module
struct 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you precise those changes in the docs:
Line 311 in b668f4f
The metadata has a maximum length that is chosen by the app developer, and |
You have changed it from |
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 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
ProvideModule
has 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_len
andmax_summary_len
have been added correctly to theModule
message. 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
, andMaxSummaryLen
has been changed fromuint64
toint
. Ensure that this change doesn't introduce any issues when these fields are used elsewhere in the codebase.13-20: The
DefaultConfig
function has been updated to provide default values forMaxTitleLen
andMaxSummaryLen
. 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
, andMaxSummaryLen
are 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
validateProposalLengths
function correctly checks the lengths ofmetadata
,title
, andsummary
against the maximum lengths defined in the configuration.206-213: The
assertTitleLength
function correctly checks the length oftitle
againstMaxTitleLen
from 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
assertMetadataLength
function correctly checks the length ofmetadata
againstMaxMetadataLen
from the configuration.
- 224-230: The
assertSummaryLength
function correctly checks the length ofsummary
againstMaxSummaryLen
from 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
MaxTitleLen
andMaxSummaryLen
have been added to theModule
struct and are being initialized in theinit
function. Ensure that these fields are being used correctly throughout the codebase.110-121: The
Range
function has been updated to handle the new fieldsMaxTitleLen
andMaxSummaryLen
. This function is used to iterate over all populated fields of the message in an undefined order.141-144: The
Has
function has been updated to handle the new fieldsMaxTitleLen
andMaxSummaryLen
. This function is used to check if a field is populated.165-168: The
Clear
function has been updated to handle the new fieldsMaxTitleLen
andMaxSummaryLen
. This function is used to clear the value of a field, making it non-present.191-196: The
Get
function has been updated to handle the new fieldsMaxTitleLen
andMaxSummaryLen
. This function is used to get the value of a field.221-224: The
Set
function has been updated to handle the new fieldsMaxTitleLen
andMaxSummaryLen
. This function is used to set the value of a field.249-252: The
Mutable
function has been updated to handle the new fieldsMaxTitleLen
andMaxSummaryLen
. This function is used to check if a field is mutable.270-273: The
New
function has been updated to handle the new fieldsMaxTitleLen
andMaxSummaryLen
. This function is used to create a new value for a field.350-355: The
Size
function has been updated to handle the new fieldsMaxTitleLen
andMaxSummaryLen
. This function is used to compute the size of the message.385-394: The
Marshal
function has been updated to handle the new fieldsMaxTitleLen
andMaxSummaryLen
. This function is used to serialize the message.507-544: The
Unmarshal
function has been updated to handle the new fieldsMaxTitleLen
andMaxSummaryLen
. This function is used to deserialize the message.604-609: The
Module
struct has been updated to include the new fieldsMaxTitleLen
andMaxSummaryLen
. Ensure that these fields are being used correctly throughout the codebase.646-658: Getter methods for the new fields
MaxTitleLen
andMaxSummaryLen
have 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to myself, in the backport PR, make this message identical as the metadata message.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, thanks for that one
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 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
SubmitProposal
function now includes a call tovalidateProposalLengths
to 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
, andMaxSummaryLen
are set correctly if not provided by the app developer.193-206: The
validateProposalLengths
function correctly checks the lengths of the metadata, title, and summary.209-215: The
assertTitleLength
function 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
assertMetadataLength
function correctly checks the length of the metadata and returns an error if it exceedsMaxMetadataLen
.
- 226-232: The
assertSummaryLength
function correctly checks the length of the summary and returns an error if it exceedsMaxSummaryLen
.
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.
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.
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 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
ErrTitleTooLong
is 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
, andMaxSummaryLen
are 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
validateProposalLengths
function 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
assertTitleLength
function 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
assertMetadataLength
function 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
assertSummaryLength
function 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.
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.
lgtm! thank you!
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.md
make lint
andmake test
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...
!
in the type prefix if API or client breaking changeSummary by CodeRabbit