Add MsgUpdateParams #300
Add MsgUpdateParams #300
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces several modifications to the rollup module within the Cosmos SDK. Key changes include the addition of an Changes
Possibly related issues
Possibly related PRs
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
natebeauregard
force-pushed
the
nkb.update-params
branch
from
b1d665a to
42a9885
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (9)
proto/rollup/module/v1/module.proto (1)
15-16: Enhance documentation for authority fieldThe comment should be more specific about the governance module fallback mechanism. Consider clarifying:
- The exact governance parameter that defines the default authority
- How/when the fallback occurs
- // authority defines the custom module authority. If not set, defaults to the governance module. + // authority defines the custom module authority. If not set, defaults to the x/gov module's + // authority parameter. This address will have the ability to execute UpdateParams messages.x/rollup/types/errors.go (1)
22-22: Fix typo in error messageThe error message contains a grammatical error: "failed to updated params" should be "failed to update params"
Apply this diff to fix the typo:
- ErrUpdateParams = registerErr("failed to updated params") + ErrUpdateParams = registerErr("failed to update params")Otherwise, the addition of this error code aligns well with the PR objectives for supporting parameter updates in the rollup module.
x/rollup/keeper/keeper.go (2)
17-17: LGTM! Authority field follows Cosmos SDK patterns.The addition of
authority sdk.AccAddressaligns with standard Cosmos SDK patterns for authority management. This enables proper access control for parameter updates.Consider documenting the authority's capabilities and restrictions in the module's spec document, particularly noting that it defaults to the gov module's authority when unspecified.
26-26: Consider validating the authority address.While the parameter placement is correct, consider adding validation to ensure the authority address is not empty.
func NewKeeper( cdc codec.BinaryCodec, storeService store.KVStoreService, authority sdk.AccAddress, bankKeeper types.BankKeeper, accountKeeper types.AccountKeeper, ) *Keeper { + if len(authority) == 0 { + panic("authority address cannot be empty") + } return &Keeper{x/rollup/module.go (2)
63-67: Add authority address validationWhile the authority handling logic is correct, consider adding validation for the custom authority address format when specified. This helps prevent configuration errors early.
if in.Config.Authority != "" { + if _, err := sdk.AccAddressFromBech32(in.Config.Authority); err != nil { + panic(fmt.Sprintf("invalid authority address: %s", err)) + } authority = authtypes.NewModuleAddressOrBech32Address(in.Config.Authority) }
63-69: Add logging for authority configurationConsider adding logging statements to track which authority is being used. This improves operational visibility and debugging capabilities.
// default to governance authority if not provided authority := authtypes.NewModuleAddress(govtypes.ModuleName) +ctx := log.With(logger, "module", types.ModuleName) if in.Config.Authority != "" { authority = authtypes.NewModuleAddressOrBech32Address(in.Config.Authority) + ctx.Info("using custom authority", "address", in.Config.Authority) +} else { + ctx.Info("using default governance authority", "address", authority.String()) }x/rollup/keeper/msg_server.go (1)
150-169: Consider adding event emission and debug logging for parameter updates.While the implementation correctly handles parameter updates, consider these improvements for better observability:
- Emit an event when parameters are updated (Cosmos SDK convention)
- Add debug logging for successful updates
Here's a suggested implementation:
func (k *Keeper) UpdateParams( goCtx context.Context, msg *types.MsgUpdateParams, ) (*types.MsgUpdateParamsResponse, error) { if k.authority.String() != msg.Authority { return nil, types.WrapError(types.ErrUpdateParams, "invalid authority: expected %s, got %s", k.authority.String(), msg.Authority) } params := &msg.Params if err := params.Validate(); err != nil { return nil, types.WrapError(types.ErrUpdateParams, "validate params: %w", err) } ctx := sdk.UnwrapSDKContext(goCtx) if err := k.SetParams(ctx, &msg.Params); err != nil { return nil, types.WrapError(types.ErrUpdateParams, "set params: %w", err) } + ctx.Logger().Debug("Updated rollup parameters", "authority", msg.Authority) + + k.EmitEvents(ctx, sdk.Events{ + sdk.NewEvent( + types.EventTypeParamsUpdated, + sdk.NewAttribute(types.AttributeKeyAuthority, msg.Authority), + // Add relevant parameter attributes here + ), + }) return &types.MsgUpdateParamsResponse{}, nil }x/rollup/keeper/msg_server_test.go (1)
284-333: Enhance test coverage for UpdateParamsWhile the test structure is good, consider these improvements:
- Add expected error messages in the test cases:
"invalid authority": { authority: sdk.AccAddress("invalid_authority"), params: validParams, shouldError: true, + errorMsg: "unauthorized", }, "valid authority with invalid params": { authority: authority, params: invalidParams, shouldError: true, + errorMsg: "invalid params", },
- Add these test cases:
- Empty authority string
- Verify original params remain unchanged when update fails
- Nil params
Would you like me to provide the complete test cases implementation?
x/rollup/tests/integration/rollup_test.go (1)
131-152: Add negative test cases for params update.While the current test coverage is good, consider adding the following test cases:
- Attempt to update params with an invalid authority
- Attempt to update params with invalid values
- Verify that unauthorized accounts cannot update params
Example test case:
// attempt to update params with invalid authority _, err = integrationApp.RunMsg(&rolluptypes.MsgUpdateParams{ Authority: "invalid_authority", Params: updatedParams, }) require.Error(t, err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
gen/rollup/module/v1/module.pb.gois excluded by!**/*.pb.go,!**/gen/**go.work.sumis excluded by!**/*.sumx/rollup/types/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (10)
proto/rollup/module/v1/module.proto(1 hunks)proto/rollup/v1/tx.proto(3 hunks)x/rollup/keeper/grpc_query.go(2 hunks)x/rollup/keeper/keeper.go(3 hunks)x/rollup/keeper/keeper_test.go(2 hunks)x/rollup/keeper/msg_server.go(1 hunks)x/rollup/keeper/msg_server_test.go(2 hunks)x/rollup/module.go(3 hunks)x/rollup/tests/integration/rollup_test.go(8 hunks)x/rollup/types/errors.go(1 hunks)
🔇 Additional comments (16)
proto/rollup/module/v1/module.proto (1)
14-16: Verify backward compatibility of the Module message
Adding a new field with number 1 to an existing message could potentially break backward compatibility if other fields were previously defined. Let's verify this is the first field added to this message.
✅ Verification successful
No backward compatibility concerns with the Module message
The git history shows this is the first field added to the Module message, which was previously empty except for the cosmos.app.v1alpha1.module option. Adding field number 1 is safe and won't break backward compatibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for previous versions of the Module message definition
# Expected: Confirm this is the first field added to the message
# Search for previous definitions of the Module message
git log -p -- proto/rollup/module/v1/module.proto | grep -A 10 "message Module"
Length of output: 576
x/rollup/keeper/grpc_query.go (1)
19-21: Verify implications of Keeper value copying
The constructor dereferences and copies the entire Keeper struct. This could be inefficient or problematic if:
- The Keeper struct is large
- The Keeper contains sync primitives (e.g., mutexes)
- The Keeper has internal pointers that need to maintain specific relationships
Let's check the Keeper's structure:
#!/bin/bash
# Description: Analyze Keeper struct composition
# Find Keeper struct definition
ast-grep --pattern 'type Keeper struct {
$$$
}'
# Look for sync primitive usage
rg 'sync\.' -A 5x/rollup/keeper/keeper_test.go (2)
14-14: LGTM: Import aligns with authority integration
The addition of govtypes import is necessary for using the governance module as the default authority in the keeper tests.
55-55: LGTM: Proper authority setup in keeper initialization
The addition of authtypes.NewModuleAddress(govtypes.ModuleName) as the authority parameter correctly implements the default governance authority pattern for the rollup module.
Let's verify the keeper initialization pattern is consistent across the codebase:
✅ Verification successful
Let's gather more context about these keeper initializations to verify the authority parameter setup.
Authority parameter is consistently implemented across keeper initializations
The verification confirms that both test files (keeper_test.go and rollup_test.go) properly initialize the keeper with authtypes.NewModuleAddress(govtypes.ModuleName) as the authority parameter, maintaining a consistent pattern across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify keeper initialization pattern across tests
# Expected: All keeper initializations should include the authority parameter
# Search for keeper initialization patterns in test files
rg -t go "NewKeeper\(" --glob "*_test.go"
Length of output: 193
Script:
#!/bin/bash
# Get more context around keeper initializations
rg -t go "NewKeeper\(" --glob "*_test.go" -B 2 -A 5
Length of output: 1226
x/rollup/keeper/keeper.go (2)
37-37: LGTM! Proper authority field initialization.
The authority field is correctly initialized in the NewKeeper constructor.
Line range hint 17-37: Verify UpdateParams implementation uses authority check.
The authority field is properly added, but we should verify its usage in the UpdateParams method.
✅ Verification successful
The search results show that the authority check is implemented in UpdateParams method but uses string comparison instead of bytes.Equal. Let me verify if this is the recommended approach by checking other modules' implementations.
Let me check the Cosmos SDK's recommended patterns for authority checks.
Authority check implementation is correct and follows SDK patterns
The authority check in UpdateParams is properly implemented:
- Uses
sdk.AccAddresstype from cosmos-sdk for authority - Performs string comparison which is safe for address comparison in Cosmos SDK
- Includes proper error wrapping and descriptive messages
- Has test coverage including invalid authority cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for UpdateParams implementation to verify authority check
rg -A 10 "func.*UpdateParams" x/rollup/keeper/
# Search for any other authority checks
ast-grep --pattern 'if !bytes.Equal($authority, k.authority) {
$$$
}'
Length of output: 1606
Script:
#!/bin/bash
# Search for similar authority checks in other modules
rg -A 3 "authority.String\(\) !=" .
rg -A 3 "msg.Authority.*authority" .
Length of output: 384
Script:
#!/bin/bash
# Search for authority check patterns in the codebase
rg -A 3 "func.*UpdateParams.*authority" .
# Check imports to see if we're using cosmos-sdk
rg "github.com/cosmos/cosmos-sdk" x/rollup/keeper/msg_server.go
# Look for other authority validation patterns
rg -A 3 "invalid authority" .
Length of output: 803
proto/rollup/v1/tx.proto (4)
9-9: LGTM: Required import for Params type
The import is correctly added to support the Params type used in the new MsgUpdateParams message.
74-84: LGTM: MsgUpdateParams follows best practices
The message definition includes:
- Proper authority field with correct address scalar annotation
- Non-nullable params field with appropriate amino serialization
- Clear documentation indicating all parameters must be supplied
86-87: LGTM: Response message follows convention
The empty response message follows the standard Cosmos SDK pattern for parameter update responses.
25-26: LGTM: RPC method follows Cosmos SDK conventions
The UpdateParams RPC method is correctly defined and follows the standard Cosmos SDK pattern for parameter updates. The method signature aligns with the PR objectives for authority-based parameter management.
Let's verify this follows the standard pattern used in other Cosmos SDK modules:
✅ Verification successful
RPC method follows Cosmos SDK authority-based parameter update pattern
The verification confirms that the UpdateParams RPC method follows the correct pattern for authority-based parameter updates:
- Uses proper message structure with
authorityfield marked as signer - Includes cosmos address scalar for authority validation
- Has non-nullable params field with proper annotations
- Empty response message follows convention
- Proper documentation of the RPC method's purpose
The implementation aligns with Cosmos SDK's best practices for parameter management through governance/admin control.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if other Cosmos SDK modules follow the same pattern for UpdateParams
rg -U "rpc UpdateParams.*returns.*UpdateParamsResponse" --type proto
Length of output: 100
Script:
#!/bin/bash
# Search for proto files first to confirm file extension
fd -e proto
# Then search for UpdateParams pattern in all files
rg -U "rpc UpdateParams.*returns.*UpdateParamsResponse"
Length of output: 431
Script:
#!/bin/bash
# Let's search for any Msg service definitions to see the common patterns
rg -U "service Msg {" -A 10
# Also search for any params-related RPC methods to see variations
rg -U "rpc.*Params.*returns"
Length of output: 339
Script:
#!/bin/bash
# Search for service Msg definitions with proper regex
rg "service Msg \{" -A 10
# Search for the MsgUpdateParams message definition to verify its structure
rg -U "message MsgUpdateParams"
Length of output: 1793
Script:
#!/bin/bash
# Let's see the full MsgUpdateParams message definition
rg -U "message MsgUpdateParams" -A 10
# Also check for any other param-related messages to compare patterns
rg -U "message.*Params" -A 5
Length of output: 8062
x/rollup/module.go (2)
17-17: LGTM: Required imports for authority handling
The new imports for authtypes and govtypes are necessary for implementing the authority-based parameter updates.
Also applies to: 19-19
34-34: LGTM: Config field addition
The Config field is properly added to ModuleInputs struct, enabling the injection of module configuration including the authority setting.
x/rollup/keeper/msg_server.go (1)
154-156: Verify authority field initialization in the Keeper.
The authority check is implemented correctly, but we should verify that the authority field is properly initialized in the Keeper constructor.
✅ Verification successful
Authority field is properly initialized in the Keeper
The verification confirms that:
- The
authorityfield is correctly defined assdk.AccAddressin the Keeper struct - The
NewKeeperconstructor properly accepts theauthorityparameter and initializes it in the returned Keeper instance - The implementation follows the standard Cosmos SDK patterns for authority handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how the authority field is initialized in the Keeper
# Look for Keeper constructor or initialization
ast-grep --pattern 'type Keeper struct {
$$$
authority $$$
$$$
}'
# Look for NewKeeper function
ast-grep --pattern 'func NewKeeper($$$) *Keeper {
$$$
}'
Length of output: 1475
x/rollup/keeper/msg_server_test.go (1)
8-8: LGTM: Import addition is appropriate
The addition of the govtypes import is necessary for creating the governance module address used in testing the authority validation.
x/rollup/tests/integration/rollup_test.go (2)
39-40: LGTM! Clear and descriptive query client naming.
The separation of bank and rollup query clients improves code clarity and maintainability.
187-187: Verify authority configuration across the module.
The authority configuration and query server registration look correct. Let's verify the authority configuration is consistent across the module.
Also applies to: 214-214
✅ Verification successful
Let me gather more information about the authority usage in the module to ensure complete verification.
Let me check the message handling implementation to complete the verification.
Based on the gathered information, I can now provide the final response:
Authority configuration is properly implemented and consistent across the module
The authority setup is correctly implemented with the following verifications:
- Authority is properly initialized in the keeper constructor
- Module configuration allows for custom authority override via config
- Parameter updates are properly guarded with authority checks
- Test cases cover both valid and invalid authority scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check authority configuration consistency across the module
# Check for authority configuration in keeper and module files
rg -A 2 "NewKeeper.*authority" x/rollup/
rg -A 2 "authority.*govtypes.ModuleName" x/rollup/
Length of output: 751
Script:
#!/bin/bash
# Check for authority-related parameter updates and message handling
rg -A 3 "MsgUpdate.*Params" x/rollup/
rg -A 3 "authority" x/rollup/keeper/
rg -A 3 "GetAuthority" x/rollup/
Length of output: 13894
Script:
#!/bin/bash
# Check message server implementation and parameter validation
rg -A 5 "MsgServer" x/rollup/keeper/
ast-grep --pattern 'func (k Keeper) $method($ctx, $msg) ($resp, error) {
$$$
}'
Length of output: 662
| // default to governance authority if not provided | ||
| authority := authtypes.NewModuleAddress(govtypes.ModuleName) | ||
| if in.Config.Authority != "" { | ||
| authority = authtypes.NewModuleAddressOrBech32Address(in.Config.Authority) | ||
| } |
There was a problem hiding this comment.
Note for reviewer: this follows the same flow as the OOTB cosmos sdk modules.
MsgUpdateParams defines a Msg for updating the x/rollup module parameters.
natebeauregard
force-pushed
the
nkb.update-params
branch
from
b4c3c8e to
d3fc17f
Compare
|
Force-pushed to rebase on main |
Add
MsgUpdateParamsto enable an authority to update thex/rollupmodule-specific parameters. The authority will default to thex/govmodule if not explicitly specified forx/rollup.Summary by CodeRabbit
Release Notes
New Features
authorityfield in theModulemessage to define custom module authority.UpdateParamsmethod for updating parameters in the rollup module.Bug Fixes
ErrUpdateParams.Tests
UpdateParamsmethod, ensuring robust validation of authority and parameters.Documentation