feat(x/group): extend group config and make it configurable #18448
Conversation
|
Warning Rate Limit Exceeded@emidev98 has exceeded the limit for the number of files or commits that can be reviewed per hour. Please wait 12 minutes and 24 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the @coderabbitai review command. Alternatively, push new commits to this PR. We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe changes involve extending the configuration of the Cosmos SDK's group module to include new fields for maximum proposal title and summary lengths. New validation functions and error messages have been added to enforce these constraints. The Changes
TipsChat with CodeRabbit Bot (
|
github-prbot
requested review from
a team,
kocubinski and
samricotta
and removed request for
a team
![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 ignored due to filter (1)
- x/group/go.mod
Files selected for processing (9)
- api/cosmos/group/module/v1/module.pulsar.go (15 hunks)
- proto/cosmos/group/module/v1/module.proto (1 hunks)
- x/group/errors/errors.go (1 hunks)
- x/group/keeper/keeper.go (4 hunks)
- x/group/keeper/msg_server.go (2 hunks)
- x/group/module/module.go (2 hunks)
- x/group/simulation/operations.go (3 hunks)
- x/group/types.go (4 hunks)
- x/group/types/config.go (1 hunks)
Files skipped from review due to trivial changes (1)
- x/group/types/config.go
Additional comments: 34
x/group/errors/errors.go (1)
- 6-20: The new error variables
ErrMetadataTooLong,ErrSummaryTooLong, andErrTitleTooLongare correctly registered with unique error codes and appropriate messages. Ensure that these errors are properly handled in the code where length constraints are enforced.proto/cosmos/group/module/v1/module.proto (4)
18-19: Ensure that the
max_execution_periodfield is being used correctly throughout the codebase.21-25: The comment for
max_metadata_lenis clear and the default value is set correctly.27-31: The comment for
max_proposal_title_lenis clear and the default value is set correctly.33-37: The comment for
max_proposal_summary_lenis clear and the default value is set correctly.x/group/simulation/operations.go (3)
13-13: The new import statement is correctly placed in alphabetical order.
426-429: The
Validatefunction now uses thegrouptypes.DefaultConfig(). Ensure that this change is reflected in all calls toValidate.894-897: The
Validatefunction now uses thegrouptypes.DefaultConfig(). Ensure that this change is reflected in all calls toValidate.x/group/types.go (4)
14-17: The new import statement for
grouptypeshas been added correctly.45-49: The
Validatefunction signatures of theThresholdDecisionPolicyandPercentageDecisionPolicystructs have been updated to usegrouptypes.Config. Ensure that all calls to these functions throughout the codebase have been updated to match the new signature.137-143: The
Validatefunction ofThresholdDecisionPolicystruct has been updated to usegrouptypes.Config. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.189-195: The
Validatefunction ofPercentageDecisionPolicystruct has been updated to usegrouptypes.Config. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.x/group/module/module.go (2)
18-24: The import of the
grouptypespackage is a new addition. Ensure that this package is available and accessible.207-234: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [209-235]
The
ProvideModulefunction has been updated to initializedefaultConfigusinggrouptypes.DefaultConfig(). It then conditionally updatesdefaultConfigbased onin.Configvalues. Thekeeper.NewKeeperfunction call has been modified to include thedefaultConfigparameter. Ensure that all calls toProvideModulehave been updated to match the new function signature and that thedefaultConfigvalues are being set correctly.- k := keeper.NewKeeper(in.Key, in.Cdc, in.MsgServiceRouter, in.AccountKeeper) + k := keeper.NewKeeper(in.Key, in.Cdc, in.MsgServiceRouter, in.AccountKeeper, defaultConfig)x/group/keeper/keeper.go (3)
78-84: The
Keeperstruct has been updated to include aconfigfield of typegrouptypes.Config. This is a good practice as it allows the configuration to be easily accessed and modified within theKeepermethods.87-108: The
NewKeeperfunction has been updated to accept agrouptypes.Configparameter and initialize theconfigfield of theKeeperstruct. It also sets default values forMaxMetadataLen,MaxProposalTitleLen, andMaxProposalSummaryLenif they are not provided. This is a good practice as it ensures that these values are always defined and prevents potential runtime errors.454-479: The new validation methods
assertMetadataLength,assertSummaryLength, andassertTitleLengthare introduced to enforce length constraints on metadata, proposal summary, and proposal title, respectively. These methods are crucial for maintaining data integrity and preventing potential errors or security vulnerabilities related to excessively long inputs.x/group/keeper/msg_server.go (3)
524-530: The
assertMetadataLengthandassertSummaryLengthfunctions have been removed. Ensure that the length validation formsg.Metadataandmsg.Summaryis now handled elsewhere in the codebase.532-532: The validation order of
msg.Metadataandmsg.Titlehas been swapped. Ensure that this change does not affect the overall logic and error handling.1060-1062: The
validateDecisionPoliciesfunction is not affected by the changes in this pull request. No action is required.api/cosmos/group/module/v1/module.pulsar.go (14)
20-33: The new fields
MaxProposalTitleLenandMaxProposalSummaryLenhave been added to theModulestruct and are being initialized in theinitfunction. Ensure that these fields are being used correctly throughout the codebase.113-124: The
MaxProposalTitleLenandMaxProposalSummaryLenfields are being checked for non-zero values. This is a good practice as it prevents unnecessary operations on zero values.144-147: The
Hasfunction has been updated to include the new fields. This is a good practice as it ensures that the function's behavior remains consistent with the updated struct.168-171: The
Clearfunction has been updated to clear the new fields. This is a good practice as it ensures that the function's behavior remains consistent with the updated struct.194-199: The
Getfunction has been updated to get the values of the new fields. This is a good practice as it ensures that the function's behavior remains consistent with the updated struct.224-227: The
Setfunction has been updated to set the values of the new fields. This is a good practice as it ensures that the function's behavior remains consistent with the updated struct.255-258: The
Mutablefunction has been updated to include the new fields. This is a good practice as it ensures that the function's behavior remains consistent with the updated struct.277-280: The
Newfunction has been updated to initialize the new fields. This is a good practice as it ensures that the function's behavior remains consistent with the updated struct.357-362: The
Sizefunction has been updated to include the new fields. This is a good practice as it ensures that the function's behavior remains consistent with the updated struct.392-401: The
MarshalToSizedBufferfunction has been updated to include the new fields. This is a good practice as it ensures that the function's behavior remains consistent with the updated struct.525-562: The
Unmarshalfunction has been updated to include the new fields. This is a good practice as it ensures that the function's behavior remains consistent with the updated struct.620-634: The protobuf definition has been updated to include the new fields. This is a good practice as it ensures that the protobuf definition remains consistent with the updated struct.
671-683: Getter methods for the new fields have been added. This is a good practice as it provides a safe way to access the fields of the struct.
704-733: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [698-730]
The protobuf raw descriptor has been updated to include the new fields. This is a good practice as it ensures that the protobuf raw descriptor remains consistent with the updated struct.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (6)
- x/group/keeper/genesis_test.go (2 hunks)
- x/group/keeper/grpc_query_test.go (2 hunks)
- x/group/keeper/keeper.go (4 hunks)
- x/group/keeper/keeper_test.go (4 hunks)
- x/group/types/config.go (1 hunks)
- x/group/types_test.go (2 hunks)
Additional comments: 12
x/group/types_test.go (2)
7-20: The import path and the function call to get the default configuration have been updated to reflect the new package structure and configuration setup. Ensure that the new package and function provide the same functionality as the old ones.
57-63: The import path and the function call to get the default configuration have been updated to reflect the new package structure and configuration setup. Ensure that the new package and function provide the same functionality as the old ones.
x/group/keeper/grpc_query_test.go (1)
- 68-71: The
groupKeeperinitialization has been updated to usegrouptypes.DefaultConfig()instead ofgroup.DefaultConfig(). Ensure that this change is consistent with the intended configuration for thegroupKeeperinstance.x/group/types/config.go (1)
- 1-35: The new
Configstruct andDefaultConfigfunction are well defined and follow the best practices. The comments are clear and provide a good understanding of the purpose of each field in theConfigstruct. The default values in theDefaultConfigfunction seem reasonable, but you might want to verify these values with your team or stakeholders.x/group/keeper/keeper_test.go (4)
9-12: The import of the
grouptypespackage is a necessary change due to the shift in configuration handling for the group module.74-80: The replacement of
group.DefaultConfig()withgrouptypes.DefaultConfig()is consistent with the changes in the configuration handling for the group module. Also, theKeeperstruct is now initialized with the newgrouptypes.Configtype.408-414: The calculation of
minExecutionPeriodhas been updated to usegrouptypes.DefaultConfig().MaxExecutionPeriod. This change is consistent with the shift in configuration handling for the group module.472-478: The calculation of
minExecutionPeriodhas been updated to usegrouptypes.DefaultConfig().MaxExecutionPeriod. This change is consistent with the shift in configuration handling for the group module.x/group/keeper/genesis_test.go (2)
6-12: The import statement has been updated to include the
grouptypespackage. This change is consistent with the pull request summary, which mentions a shift in the configuration handling for the group module.72-76: The
keeper.NewKeeperfunction is now being called withgrouptypes.DefaultConfig()instead ofgroup.DefaultConfig(). This change is consistent with the pull request summary, which mentions a shift in the configuration handling for the group module. Ensure that thegrouptypes.DefaultConfig()function is correctly defined and returns the expected default configuration.x/group/keeper/keeper.go (2)
78-93: The
Keeperstruct now includes aconfigfield of typegrouptypes.Config. This is a good change as it allows the module to have its own configuration, which can be easily extended in the future.95-107: The
NewKeeperfunction now accepts agrouptypes.Configparameter and sets default values for certain configuration parameters if they are not provided. This is a good practice as it provides sensible defaults and allows the caller to override them if necessary.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- x/group/go.sum
Files selected for processing (3)
- simapp/app.go (2 hunks)
- x/group/keeper/keeper.go (4 hunks)
- x/group/keeper/msg_server_test.go (11 hunks)
Additional comments: 8
simapp/app.go (2)
12-12: The import path has been updated to reflect the new location of the group module. Ensure that the new path is correct and accessible.
350-350: The
group.DefaultConfig()function has been replaced withgrouptypes.DefaultConfig(). Ensure that all instances ofgroup.DefaultConfig()have been updated throughout the codebase.x/group/keeper/keeper.go (4)
10-14: The import of the
grouptypespackage is a new addition. Ensure that this package is available and accessible.78-93: The
configfield in theKeeperstruct has been changed togrouptypes.Config. TheNewKeeperfunction has been updated to initializeconfigwith default values if they are not set by the app developer. This is a good practice as it provides sensible defaults and reduces the chance of misconfiguration.95-112: The
configfield is being initialized with default values if they are not set by the app developer. This is a good practice as it provides sensible defaults and reduces the chance of misconfiguration.459-484: Three new functions
assertMetadataLength,assertSummaryLength, andassertTitleLengthhave been added to validate metadata, summary, and title lengths based on the configuration. This is a good practice as it ensures that the lengths of these fields do not exceed the maximum allowed lengths, thereby preventing potential issues such as database errors or memory overflows.x/group/keeper/msg_server_test.go (2)
108-115: The test case checks if the error is thrown when the metadata length exceeds the limit. Ensure that the limit is set correctly in the configuration.
2322-2322: These test cases are checking if the error is thrown when the metadata length exceeds the limit. Ensure that the limit is set correctly in the configuration.
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)
- 56-59: The changes are well-documented and the links to the pull requests provide a good reference for further details. Ensure that the pull request numbers and the links are correct.
|
Tank you @odeke-em! Changes applied, let me know if this looks better to you |
CHANGELOG.md
Outdated
| @@ -53,6 +53,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ | |||
|
|
|||
| ### Improvements | |||
|
|
|||
| * (x/group) [#18448](https://github.com/cosmos/cosmos-sdk/pull/18448) Extend group config | |||
There was a problem hiding this comment.
Can we get this one and the gov one entry in respectively group/CHANGELOG.md and gov changelog instead of the main one?
x/group/types/config.go
Outdated
| @@ -0,0 +1,35 @@ | |||
| package types | |||
There was a problem hiding this comment.
Let's not create a type package. Group follows the latest module directory guidelines: https://docs.cosmos.network/main/build/building-modules/structure
| // MaxMetadataLen defines the max chars allowed in all | ||
| // messages that allows creating or updating a group | ||
| // with a metadata field | ||
| // Defaults to 140 if not explicitly set. |
There was a problem hiding this comment.
Thanks for pointing that out, consequences of copy paste
| // MaxProposalTitleLen defines the max chars allowed | ||
| // in string for the MsgSubmitProposal and Proposal | ||
| // summary field | ||
| // Defaults to 140 if not explicitly set. |
There was a problem hiding this comment.
Thanks for pointing that out, consequences of copy paste
|
Gentle bump :) |
|
I have not seen the comments, let me work these changes tomorrow and get back to you! |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- x/group/go.mod
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional comments: 1
CHANGELOG.md (1)
- 57-57:
The changelog entry for the group module should be moved togroup/CHANGELOG.mdas per the previous review comment.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files selected for processing (6)
- proto/cosmos/group/module/v1/module.proto (1 hunks)
- x/gov/CHANGELOG.md (1 hunks)
- x/group/CHANGELOG.md (1 hunks)
- x/group/config.go (1 hunks)
- x/group/keeper/keeper.go (3 hunks)
- x/group/module/module.go (1 hunks)
Files skipped from review due to trivial changes (1)
- x/gov/CHANGELOG.md
Additional comments: 2
x/group/CHANGELOG.md (1)
- 27-33:
The changelog entries for pull requests #18448 and #18286 are correctly listed under the "Improvements" section and are formatted properly according to the guidelines. No further action is required for this hunk.
x/group/keeper/keeper.go (1)
- 224-227:
The NewKeeper function correctly initializes the Keeper and panics on unrecoverable errors, which is appropriate for setup functions.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (3)
- x/group/config.go (1 hunks)
- x/group/keeper/keeper.go (3 hunks)
- x/group/module/module.go (1 hunks)
Additional comments: 2
x/group/config.go (1)
- 25-32:
x/group/keeper/keeper.go (1)
- 91-115:
The default configuration logic is correctly implemented, ensuring that the system has sensible defaults if not provided by the app developer. The use of panic is appropriate for initialization failures of critical components.
x/group/module/module.go
Outdated
| */ | ||
|
|
||
| k := keeper.NewKeeper(in.Key, in.Cdc, in.MsgServiceRouter, in.AccountKeeper, group.Config{MaxExecutionPeriod: in.Config.MaxExecutionPeriod.AsDuration(), MaxMetadataLen: in.Config.MaxMetadataLen}) | ||
| if in.Config.MaxMetadataLen != 0 { |
There was a problem hiding this comment.
This isn't necessary, let's revert this, it is already checked and updated in the keeper.
There was a problem hiding this comment.
For some reason when removing this code and I send the config itself to the module without doing checks I get the following errors:
No matters how many times I build the proto nor if I remove the modules cache and try again. Any idea why that could happen to me in local?
There was a problem hiding this comment.
My first guess was the missing API module replace but I do see it in the go.mod. 🤷🏾♂️, works for me locally.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (4)
- api/cosmos/group/module/v1/module.pulsar.go (15 hunks)
- simapp/app.go (2 hunks)
- x/group/keeper/keeper.go (3 hunks)
- x/group/module/module.go (1 hunks)
Additional comments: 17
api/cosmos/group/module/v1/module.pulsar.go (13)
19-25: The addition of new fields
MaxProposalTitleLenandMaxProposalSummaryLento theModulestruct is correctly reflected in the variable declarations.27-34: The
initfunction has been correctly updated to initialize the field descriptors for the new fieldsMaxProposalTitleLenandMaxProposalSummaryLen.110-125: The
Rangemethod has been correctly updated to iterate over the new fieldsMaxProposalTitleLenandMaxProposalSummaryLen.141-150: The
Hasmethod has been correctly updated to check the presence of the new fieldsMaxProposalTitleLenandMaxProposalSummaryLen.165-174: The
Clearmethod has been correctly updated to clear the new fieldsMaxProposalTitleLenandMaxProposalSummaryLen.191-202: The
Getmethod has been correctly updated to retrieve the values of the new fieldsMaxProposalTitleLenandMaxProposalSummaryLen.221-230: The
Setmethod has been correctly updated to set the values of the new fieldsMaxProposalTitleLenandMaxProposalSummaryLen.252-261: The
Mutablemethod correctly panics for the new fieldsMaxProposalTitleLenandMaxProposalSummaryLenas they are not mutable.274-283: The
NewFieldmethod has been correctly updated to return a new value for the new fieldsMaxProposalTitleLenandMaxProposalSummaryLen.354-365: The
ProtoMethodsmethod has been correctly updated to handle the size and marshaling of the new fieldsMaxProposalTitleLenandMaxProposalSummaryLen.522-565: The
ProtoMethodsmethod has been correctly updated to handle the unmarshaling of the new fieldsMaxProposalTitleLenandMaxProposalSummaryLen.617-635: The
Modulestruct has been correctly updated to include the new fieldsMaxProposalTitleLenandMaxProposalSummaryLenwith appropriate protobuf tags and comments.668-683: The getter methods
GetMaxProposalTitleLenandGetMaxProposalSummaryLenin theModulestruct are correctly implemented.simapp/app.go (1)
- 350-358: The group module configuration is provided as a commented-out example. Please verify that this is intentional and ensure that the actual configuration is set elsewhere in the codebase if necessary.
x/group/keeper/keeper.go (2)
91-119: The logic for setting default configuration values in the
NewKeeperfunction is correct and well-documented with examples. This ensures that the keeper is initialized with valid configuration values, even if the app developer does not provide them.466-491: The assert functions
assertMetadataLength,assertSummaryLength, andassertTitleLengthare correctly enforcing length constraints based on the module configuration. While additional documentation could be beneficial, the function names and parameters are self-explanatory, and the code is straightforward.x/group/module/module.go (1)
- 208-221: The changes to the
ProvideModulefunction correctly incorporate the new configuration parametersMaxProposalTitleLenandMaxProposalSummaryLen. Ensure that these parameters are validated appropriately elsewhere in the codebase.
Co-authored-by: Julien Robert <[email protected]>
Co-authored-by: Julien Robert <[email protected]>
Co-authored-by: Julien Robert <[email protected]>
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- x/group/module/module.go (1 hunks)
Additional comments: 1
x/group/module/module.go (1)
- 208-221: The changes to the
ProvideModulefunction correctly integrate the new configuration parametersMaxProposalTitleLenandMaxProposalSummaryLeninto thegroup.Configstruct. Ensure that these new parameters are validated appropriately elsewhere in the codebase, as the hunk does not show validation logic.
Description
This pull request extends group module config and allows setting a length for the Title, Summary and Metadata on the messsages.
Closes: #XXXX
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
New Features
Improvements
Bug Fixes
Documentation
Refactor
Style
Chores