add megavault operator params and update message #2259
Conversation
WalkthroughThe changes introduce enhancements to the vault system's management of operator parameters. New interfaces and methods are added to facilitate the encoding, decoding, and updating of operator parameters within the genesis state and transaction messages. Additionally, JSON structures are updated to accommodate these new parameters, supporting future extensibility in operator configurations. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MsgClientImpl
participant Keeper
participant Vault
User->>MsgClientImpl: updateOperatorParams(request)
MsgClientImpl->>Keeper: SetOperatorParams(params)
Keeper-->>Vault: Update operator parameters
Vault-->>Keeper: Confirmation
Keeper-->>MsgClientImpl: Success response
MsgClientImpl-->>User: Confirmation of update
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (2)
Files skipped from review as they are similar to previous changes (1)
Additional comments not posted (1)
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: 2
Outside diff range and nitpick comments (1)
proto/dydxprotocol/vault/genesis.proto (1)
24-25: LGTM!The addition of the
OperatorParamsfield to theGenesisStatemessage aligns with the PR objective of introducing new components related to the megavault functionality. The non-nullable annotation ensures that the operator parameters are always present in the genesis state.To ensure backward compatibility, consider the following:
- Provide a default value for the
OperatorParamsfield in case it is not explicitly set during the genesis state initialization.- Implement a migration strategy to handle the transition from the old genesis state format to the new one that includes the
OperatorParamsfield.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (3)
protocol/x/vault/types/genesis.pb.gois excluded by!**/*.pb.goprotocol/x/vault/types/params.pb.gois excluded by!**/*.pb.goprotocol/x/vault/types/tx.pb.gois excluded by!**/*.pb.go
Files selected for processing (20)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/genesis.ts (7 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/params.ts (2 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.rpc.msg.ts (4 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.ts (3 hunks)
- proto/dydxprotocol/vault/genesis.proto (1 hunks)
- proto/dydxprotocol/vault/params.proto (2 hunks)
- proto/dydxprotocol/vault/tx.proto (2 hunks)
- protocol/app/testdata/default_genesis_state.json (1 hunks)
- protocol/scripts/genesis/sample_pregenesis.json (1 hunks)
- protocol/testutil/constants/genesis.go (1 hunks)
- protocol/x/vault/genesis.go (3 hunks)
- protocol/x/vault/keeper/msg_server_update_operator_params.go (1 hunks)
- protocol/x/vault/keeper/msg_server_update_operator_params_test.go (1 hunks)
- protocol/x/vault/keeper/params.go (1 hunks)
- protocol/x/vault/keeper/params_test.go (1 hunks)
- protocol/x/vault/types/errors.go (1 hunks)
- protocol/x/vault/types/keys.go (1 hunks)
- protocol/x/vault/types/keys_test.go (1 hunks)
- protocol/x/vault/types/params.go (1 hunks)
- protocol/x/vault/types/params_test.go (1 hunks)
Files skipped from review due to trivial changes (2)
- protocol/app/testdata/default_genesis_state.json
- protocol/testutil/constants/genesis.go
Additional context used
buf
proto/dydxprotocol/vault/params.proto
4-4: import "cosmos_proto/cosmos.proto": file does not exist
(COMPILE)
Biome
indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.ts
[error] 177-177: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
[error] 180-180: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
Additional comments not posted (27)
protocol/x/vault/keeper/msg_server_update_operator_params.go (1)
14-32: LGTM!The
UpdateOperatorParamsfunction is implemented correctly and follows best practices:
- It checks the authority of the message sender to ensure only authorized users can update the operator parameters.
- It handles errors appropriately by returning the corresponding error if the authority check fails or if setting the operator parameters fails.
- It is modular as it delegates the actual setting of operator parameters to another function
SetOperatorParams.- It returns an empty response on success, which is a common pattern in gRPC services.
The code is secure, robust, and modular.
protocol/x/vault/types/keys_test.go (1)
23-23: LGTM!The new assertion in
TestStateKeyscorrectly verifies thattypes.OperatorParamsKeyis defined as "OperatorParams". This enhances the test coverage and ensures the integrity of the key without altering any existing functionality.protocol/x/vault/types/keys.go (1)
39-41: LGTM!The addition of the
OperatorParamsKeyconstant is a clear and appropriate way to introduce a specific identifier for accessing operator-related configurations or settings. This aligns with the PR objective of enhancing the megavault functionality and improves the clarity and organization of the codebase.protocol/x/vault/types/params.go (1)
65-72: LGTM!The
Validatefunction correctly checks if theOperatorfield is non-empty and returns an appropriate error if it's empty. This validation logic enhances the robustness of the code by ensuring that an essential parameter is always provided.protocol/x/vault/keeper/msg_server_update_operator_params_test.go (1)
16-75: Excellent test coverage for theUpdateOperatorParamsfunction!The test function
TestMsgUpdateOperatorParamsis well-structured and comprehensive. It covers both positive and negative scenarios, ensuring that theUpdateOperatorParamsfunction behaves as expected.Some positive aspects of the test function:
- Clear and descriptive naming of test cases, enhancing readability and maintainability.
- Utilization of the
testapppackage to create a controlled testing environment.- Assertions using the
requirepackage, following Go testing best practices.- Checking for expected error messages in failure scenarios, providing thorough test coverage.
The test function not only improves the reliability of the codebase but also serves as valuable documentation for the expected behavior of the
UpdateOperatorParamsfunction.Great job on the test implementation!
protocol/x/vault/genesis.go (3)
44-51: LGTM!The changes ensure that the operator address is always set during the genesis initialization process. If the operator address is not provided, it defaults to the governance module's address, which is a sensible fallback mechanism. The operator parameters are then persisted in the keeper for future use.
The changes are well-structured and follow the existing coding patterns in the file.
5-5: LGTM!The import statement for the
libpackage is correctly placed along with other import statements at the top of the file. This import is necessary to access theGovModuleAddressconstant used in theInitGenesisfunction.
65-65: LGTM!The change to include the operator parameters in the exported genesis state is necessary and aligns with the changes made in the
InitGenesisfunction. By exporting the operator parameters, the module ensures that the operator information is preserved when the genesis state is exported.The
GetOperatorParamsmethod is correctly used to retrieve the operator parameters from the keeper.protocol/x/vault/types/errors.go (1)
124-128: LGTM!The new error variable
ErrEmptyOperatoris registered correctly with a unique error code and a descriptive message. It enhances the error handling capabilities of the module by providing a specific error case for an empty operator address.proto/dydxprotocol/vault/params.proto (1)
52-55: LGTM!The new message type
OperatorParamsand its fieldoperatorare well-defined and serve the purpose of storing parameters related to the megavault operator. The use of the custom scalar typecosmos.AddressStringensures that theoperatorfield adheres to the expected address format.This addition enhances the protocol's capability to manage operator-specific configurations within the vault parameters, aligning with the PR objectives.
indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.rpc.msg.ts (3)
3-3: LGTM!The import statement correctly adds the new message types related to updating operator parameters, which aligns with the PR objective.
12-14: LGTM!The
updateOperatorParamsmethod is correctly added to theMsginterface, using the new message types for the request and response. This aligns with the PR objective.
49-53: LGTM!The
updateOperatorParamsmethod is correctly implemented in theMsgClientImplclass, following the same pattern as other methods. The RPC method name, request encoding, and response decoding are all handled correctly.protocol/x/vault/keeper/params.go (2)
126-136: LGTM!The function logic is correct, and the implementation follows best practices:
- It uses a specific key (
OperatorParamsKey) to retrieve the value from the store, avoiding potential key collisions.- It uses
k.cdc.MustUnmarshalto unmarshal the retrieved value, which will panic if the unmarshal fails, indicating a critical issue with the stored data that needs immediate attention.
138-153: LGTM!The function logic is correct, and the implementation follows best practices:
- It validates the
paramsusing theparams.Validate()method before setting them in the store, ensuring data integrity.- It uses a specific key (
OperatorParamsKey) to set the value in the store, avoiding potential key collisions.- It uses
k.cdc.MustMarshalto marshal theparams, which will panic if the marshal fails, indicating a critical issue with the data being set that needs immediate attention.protocol/x/vault/types/params_test.go (1)
136-169: LGTM!The
TestValidateOperatorParamsfunction is well-structured and follows the table-driven testing approach. It covers the important scenarios for validating theOperatorParamsstructure, including success cases for a valid operator (Alice's address) and the governance module account, as well as a failure case for an empty operator string.The test function uses appropriate assertions to verify the expected behavior and enhances the test coverage for the operator parameter validation logic.
proto/dydxprotocol/vault/tx.proto (3)
23-25: LGTM!The new RPC method
UpdateOperatorParamsis declared correctly with the appropriate input and output message types. The comment provides a clear description of the purpose of the method, which aligns with the PR objective of introducing new components related to the megavault functionality.
118-127: LGTM!The new message type
MsgUpdateOperatorParamsis defined correctly with the appropriate fields and annotations. Theauthorityfield ensures that only the authorized address can update the operator parameters, and the non-nullableparamsfield ensures that all the required parameters are provided. The message type aligns with the PR objective of introducing new components related to the megavault functionality.
129-130: LGTM!The new message type
MsgUpdateOperatorParamsResponseis defined correctly as an empty message. It serves as the response type for theUpdateOperatorParamsRPC method, indicating that no specific data needs to be returned upon successful execution of the method.protocol/x/vault/keeper/params_test.go (1)
169-199: LGTM!The
TestGetSetOperatorParamsfunction is a well-structured test that comprehensively validates the functionality of getting and setting operator parameters within the VaultKeeper. It covers the following scenarios:
- Checking the default operator at genesis (governance module account).
- Setting a new operator to a specific account (Alice) and verifying the change.
- Attempting to set an invalid operator (empty string) and expecting an error.
- Ensuring that the previously set operator remains unchanged after the invalid operator attempt.
The test enhances the test coverage for operator parameter management and helps maintain the integrity of the functionality by handling both valid and invalid scenarios correctly.
indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/genesis.ts (2)
23-25: LGTM!The addition of the
operatorParamsproperty to theGenesisStateinterface is consistent with the PR objective of introducing new components related to the megavault functionality. It enhances the configuration capabilities of the vault system by including parameters related to the megavault operator in the genesis state.
44-46: LGTM!The addition of the
operator_paramsproperty to theGenesisStateSDKTypeinterface is consistent with the changes made to theGenesisStateinterface. The property name follows the snake_case convention, which is consistent with the naming convention used for the other properties in the interface.indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/params.ts (2)
94-103: LGTM!The new
OperatorParamsandOperatorParamsSDKTypeinterfaces provide a structured way to manage operator-specific data within the megavault parameters. Theoperatorproperty is appropriately typed as a string to represent the operator's address or identifier.
343-386: Looks good!The implementation of the
encode,decode, andfromPartialmethods for theOperatorParamsobject follows the established protobuf encoding/decoding pattern in the codebase. These methods enhance the handling of operator parameters by allowing for serialization and deserialization, which is crucial for data interchange in applications that utilize these parameters.indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.ts (2)
161-166: LGTM!The
MsgUpdateOperatorParamsinterface is well-structured and aligns with the purpose of updating operator parameters. Theauthorityfield represents the authorized account, and the optionalparamsfield allows specifying the updated parameters.
169-174: LGTM!The
MsgUpdateOperatorParamsSDKTypeinterface correctly mirrors the structure ofMsgUpdateOperatorParamsfor SDK compatibility. The fields are consistent, and the interface is appropriately named with theSDKTypesuffix.protocol/scripts/genesis/sample_pregenesis.json (1)
3939-3939: The addition ofoperator_paramsis a reasonable change that enables future extensibility.Initializing
operator_paramsas an empty object provides a placeholder for storing operator-specific configuration parameters in the future, without impacting the existing codebase. This change allows the configuration structure to be extended gracefully as needed.
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (3)
protocol/x/vault/types/genesis.pb.gois excluded by!**/*.pb.goprotocol/x/vault/types/params.pb.gois excluded by!**/*.pb.goprotocol/x/vault/types/tx.pb.gois excluded by!**/*.pb.go
Files selected for processing (23)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/genesis.ts (7 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/params.ts (2 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.rpc.msg.ts (4 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.ts (3 hunks)
- proto/dydxprotocol/vault/genesis.proto (1 hunks)
- proto/dydxprotocol/vault/params.proto (2 hunks)
- proto/dydxprotocol/vault/tx.proto (2 hunks)
- protocol/app/msgs/all_msgs.go (1 hunks)
- protocol/app/msgs/internal_msgs.go (1 hunks)
- protocol/app/msgs/internal_msgs_test.go (1 hunks)
- protocol/app/testdata/default_genesis_state.json (1 hunks)
- protocol/scripts/genesis/sample_pregenesis.json (1 hunks)
- protocol/testutil/constants/genesis.go (1 hunks)
- protocol/x/vault/genesis.go (3 hunks)
- protocol/x/vault/keeper/msg_server_update_operator_params.go (1 hunks)
- protocol/x/vault/keeper/msg_server_update_operator_params_test.go (1 hunks)
- protocol/x/vault/keeper/params.go (1 hunks)
- protocol/x/vault/keeper/params_test.go (1 hunks)
- protocol/x/vault/types/errors.go (1 hunks)
- protocol/x/vault/types/keys.go (1 hunks)
- protocol/x/vault/types/keys_test.go (1 hunks)
- protocol/x/vault/types/params.go (1 hunks)
- protocol/x/vault/types/params_test.go (1 hunks)
Files skipped from review due to trivial changes (2)
- protocol/scripts/genesis/sample_pregenesis.json
- protocol/testutil/constants/genesis.go
Files skipped from review as they are similar to previous changes (14)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/genesis.ts
- indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.rpc.msg.ts
- proto/dydxprotocol/vault/genesis.proto
- proto/dydxprotocol/vault/tx.proto
- protocol/app/testdata/default_genesis_state.json
- protocol/x/vault/genesis.go
- protocol/x/vault/keeper/msg_server_update_operator_params.go
- protocol/x/vault/keeper/msg_server_update_operator_params_test.go
- protocol/x/vault/keeper/params_test.go
- protocol/x/vault/types/errors.go
- protocol/x/vault/types/keys.go
- protocol/x/vault/types/keys_test.go
- protocol/x/vault/types/params.go
- protocol/x/vault/types/params_test.go
Additional context used
buf
proto/dydxprotocol/vault/params.proto
4-4: import "cosmos_proto/cosmos.proto": file does not exist
(COMPILE)
Biome
indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.ts
[error] 177-177: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
[error] 180-180: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
Additional comments not posted (13)
proto/dydxprotocol/vault/params.proto (1)
52-55: LGTM!The introduction of the
OperatorParamsmessage type and itsoperatorfield is a valuable addition to the protocol. It enhances the management of operator-specific configurations within the vault parameters.The use of the
cosmos.AddressStringscalar type for theoperatorfield ensures type safety and validation, preventing invalid Cosmos addresses from being assigned.Overall, these changes expand the protocol's functionality to accommodate operator-related data effectively.
protocol/x/vault/keeper/params.go (2)
126-136: LGTM!The function correctly retrieves the
OperatorParamsfrom the state using theOperatorParamsKeyand unmarshals the data into theparamsvariable. The implementation looks good.
138-153: LGTM!The function correctly validates the
paramsbefore marshaling and storing them in the key-value store using theOperatorParamsKey. Returning an error on validation failure ensures data integrity. The implementation looks good.protocol/app/msgs/internal_msgs_test.go (1)
161-162: LGTM!The addition of the
MsgUpdateOperatorParamsandMsgUpdateOperatorParamsResponsemessage types to the test suite is a positive change. It ensures that the new functionality for updating operator parameters through governance proposals is properly tested and integrated into the application.This change enhances the flexibility and configurability of the vault system while maintaining the robustness and reliability of the codebase.
protocol/app/msgs/internal_msgs.go (2)
205-205: LGTM!The addition of the new message type
MsgUpdateOperatorParamsto theInternalMsgSamplesDydxCustommap is consistent with the existing pattern in the file. The key and value are correctly specified, and the message type is defined in the importedvaultpackage.
206-206: Looks good!The addition of the response type entry for
MsgUpdateOperatorParamsfollows the established pattern in the file. The key is correctly specified, and setting the value tonilis consistent with the handling of unused response types.indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/params.ts (2)
94-103: LGTM!The new
OperatorParamsinterface and its corresponding SDK type are correctly defined with the appropriate property type. The naming convention follows the existing pattern in the file, and the code segment is well-structured and readable.
343-386: LGTM!The functions associated with the
OperatorParamsinterface are correctly defined and follow the existing pattern in the file. Theencode,decode, andfromPartialfunctions handle theoperatorproperty appropriately, ensuring proper serialization and deserialization ofOperatorParamsinstances.protocol/app/msgs/all_msgs.go (1)
260-261: New message types for updating vault operator parametersThe new message types
MsgUpdateOperatorParamsandMsgUpdateOperatorParamsResponsefollow the established naming conventions and are correctly scoped under thevaultmodule.The
MsgUpdateOperatorParamsmessage type suggests the existence of a corresponding handler function to process updates to operator parameters.Please verify if the handler function for processing
MsgUpdateOperatorParamsis implemented and registered correctly. The handler should validate the input, perform necessary state updates, and return an instance ofMsgUpdateOperatorParamsResponse.To verify the existence and correctness of the
MsgUpdateOperatorParamshandler function:indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.ts (4)
161-166: LGTM!The
MsgUpdateOperatorParamsinterface is well-defined and follows the naming convention for request types. Theauthorityfield is mandatory, while theparamsfield is optional, allowing for flexibility in updating operator parameters.
169-174: LGTM!The
MsgUpdateOperatorParamsSDKTypeinterface is well-defined and follows the naming convention for SDK-compatible types. It mirrors the structure of theMsgUpdateOperatorParamsinterface, ensuring consistency between the request type and its SDK counterpart.
625-630: LGTM!The
createBaseMsgUpdateOperatorParamsfunction correctly initializes a baseMsgUpdateOperatorParamsobject with default values. Theauthorityfield is set to an empty string, and theparamsfield is set toundefined, aligning with the optional nature of theparamsfield in the interface.
632-678: LGTM!The
MsgUpdateOperatorParamsobject provides the necessary encoding, decoding, and partial object creation functionality for theMsgUpdateOperatorParamstype. Theencode,decode, andfromPartialmethods are implemented correctly, handling theauthorityandparamsfields as expected.
| } | ||
| /** MsgUpdateVaultParamsResponse is the Msg/UpdateOperatorParams response type. */ | ||
|
|
||
| export interface MsgUpdateOperatorParamsResponse {} |
There was a problem hiding this comment.
Use a type alias instead of an empty interface.
The MsgUpdateOperatorParamsResponse interface is currently empty, which provides no type safety. As suggested by the static analysis hint, consider using a type alias instead:
type MsgUpdateOperatorParamsResponse = Record<string, never>;This provides a clearer intention and better type safety.
Tools
Biome
[error] 177-177: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
| export interface MsgUpdateOperatorParamsResponse {} | ||
| /** MsgUpdateVaultParamsResponse is the Msg/UpdateOperatorParams response type. */ | ||
|
|
||
| export interface MsgUpdateOperatorParamsResponseSDKType {} |
There was a problem hiding this comment.
Use a type alias instead of an empty interface.
Similar to the MsgUpdateOperatorParamsResponse interface, the MsgUpdateOperatorParamsResponseSDKType interface is currently empty. Consider using a type alias instead:
type MsgUpdateOperatorParamsResponseSDKType = Record<string, never>;This provides a clearer intention and better type safety for the SDK-compatible response type.
Tools
Biome
[error] 180-180: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
| @@ -3936,6 +3936,9 @@ | |||
| "spread_buffer_ppm": 1500, | |||
| "spread_min_ppm": 3000 | |||
| }, | |||
| "operator_params": { | |||
| "operator": "" | |||
There was a problem hiding this comment.
nit: set as gov address?
protocol/x/vault/genesis.go
Outdated
| @@ -40,6 +41,14 @@ func InitGenesis(ctx sdk.Context, k keeper.Keeper, genState types.GenesisState) | |||
| k.SetMostRecentClientIds(ctx, vault.VaultId, vault.MostRecentClientIds) | |||
| k.AddVaultToAddressStore(ctx, vault.VaultId) | |||
| } | |||
|
|
|||
| // Set operator, which defaults to gov module account. | |||
There was a problem hiding this comment.
nit: Just set as w/e is in the genesis?
Changelist
add
OperatorParamsandMsgUpdateOperatorParamsTest Plan
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
Release Notes
New Features
operatorParamsfor enhanced vault configuration.Improvements
Testing Enhancements
These updates improve the flexibility and robustness of vault management, allowing for better operator configuration and error handling.