allow megavault operator to set default quoting params #2620
Conversation
WalkthroughThe changes in this pull request focus on enhancing the authority validation logic within the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
protocol/x/vault/keeper/msg_server_update_default_quoting_params.go (1)
17-26: Consider adding events and improving documentationA few suggestions to enhance the implementation:
- Emit an event when parameters are updated for better observability
- Add documentation about the expected format and constraints for the Authority field
Example implementation:
func (k msgServer) UpdateDefaultQuotingParams( goCtx context.Context, msg *types.MsgUpdateDefaultQuotingParams, ) (*types.MsgUpdateDefaultQuotingParamsResponse, error) { ctx := lib.UnwrapSDKContext(goCtx, types.ModuleName) operator := k.GetOperatorParams(ctx).Operator // Check if authority is valid (must be a module authority or operator). if !k.HasAuthority(msg.Authority) && msg.Authority != operator { return nil, errorsmod.Wrapf( types.ErrInvalidAuthority, "invalid authority %s", msg.Authority, ) } if err := k.SetDefaultQuotingParams(ctx, msg.DefaultQuotingParams); err != nil { return nil, err } + + // Emit event for parameter update + ctx.EventManager().EmitEvent( + sdk.NewEvent( + types.EventTypeDefaultQuotingParamsUpdate, + sdk.NewAttribute("authority", msg.Authority), + sdk.NewAttribute("params", msg.DefaultQuotingParams.String()), + ), + ) return &types.MsgUpdateDefaultQuotingParamsResponse{}, nil }protocol/x/vault/keeper/msg_server_update_default_quoting_params_test.go (3)
Line range hint
21-79: Consider adding edge cases to strengthen test coverage.The test cases effectively cover the main scenarios, but consider adding these edge cases:
- Empty operator address in genesis
- Empty authority in message
- Maximum/minimum bounds for QuotingParams values
Would you like me to help generate these additional test cases?
84-96: LGTM! Genesis setup is well-structured.The genesis configuration correctly sets up the operator params for testing different authority scenarios.
Consider improving readability with consistent indentation:
tApp := testapp.NewTestAppBuilder(t).WithGenesisDocFn(func() (genesis types.GenesisDoc) { genesis = testapp.DefaultGenesis() // Set megavault operator. testapp.UpdateGenesisDocWithAppStateForModule( &genesis, func(genesisState *vaulttypes.GenesisState) { genesisState.OperatorParams = vaulttypes.OperatorParams{ Operator: tc.operator, } }, ) return genesis }).Build()
Line range hint
104-108: Consider strengthening error validation.While the current assertions verify the behavior correctly, consider using
errors.Is()instead ofErrorContains()for more precise error type checking:-require.ErrorContains(t, err, tc.expectedErr) +require.True(t, errors.Is(err, vaulttypes.ErrInvalidAuthority))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
protocol/x/vault/keeper/msg_server_update_default_quoting_params.go(1 hunks)protocol/x/vault/keeper/msg_server_update_default_quoting_params_test.go(3 hunks)
🔇 Additional comments (3)
protocol/x/vault/keeper/msg_server_update_default_quoting_params.go (2)
17-19: LGTM: Context handling and operator retrieval look good
The context unwrapping and operator parameter retrieval are implemented correctly.
20-26: Authority validation logic looks secure, but needs verification
The implementation correctly validates both module authorities and operator authority, with proper error handling.
Let's verify two critical aspects:
✅ Verification successful
Authority validation implementation is secure and properly validated
The implementation is secure for the following reasons:
- The
HasAuthorityimplementation properly checks against a pre-initialized map of valid authorities - The operator address is validated through
OperatorParams.Validate()which ensures it's non-empty - The operator is initialized securely in genesis with the governance module account as default
- Changes to operator require proper authority validation through both
HasAuthoritycheck and operator match
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify operator validation and HasAuthority implementation
# Check if operator address is validated when set
ast-grep --pattern 'SetOperatorParams($$$)'
# Check HasAuthority implementation
ast-grep --pattern 'func ($_) HasAuthority($_) bool {
$$$
}'
# Look for any tests validating operator address
rg -l "TestSetOperatorParams|TestValidateOperatorAddress" --type go
Length of output: 76625
Script:
#!/bin/bash
# Let's verify the operator validation and initialization
# Check how operator is set and validated
rg -A 5 "SetOperator" --type go
# Check for operator validation in tests
rg -A 5 "TestSetOperator" --type go
# Look for operator initialization in keeper
rg -A 5 "NewKeeper.*operator" --type go
Length of output: 2742
Script:
#!/bin/bash
# Let's verify the operator validation implementation
# Check operator validation in types
rg -A 5 "func.*Validate.*Operator" --type go
# Check operator params validation
cat protocol/x/vault/types/params.go
# Check operator params initialization in genesis
cat protocol/x/vault/types/genesis.go
Length of output: 4718
protocol/x/vault/keeper/msg_server_update_default_quoting_params_test.go (1)
12-14: LGTM! Import changes are appropriate.
The imports are correctly updated to support the new operator validation functionality and package renaming.
Changelist
[Describe or list the changes made in this PR]
Test Plan
[Describe how this PR was tested (if applicable)]
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
New Features
Bug Fixes
Documentation