[TRA-446] include vault shares in genesis state #1774
Conversation
WalkthroughThe changes introduce and integrate a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GenesisState
participant Vault
participant Keeper
participant Store
User->>+GenesisState: Initialize GenesisState
GenesisState-->>-Vault: Create Vault Entities
GenesisState->>Keeper: Call InitGenesis
Keeper->>Store: Store Vaults and Shares
User->>Keeper: Retrieve All Vaults
Keeper->>Store: Retrieve Vaults and Shares
Store-->>Keeper: Return Vault and Shares
Keeper-->>User: Return All Vaults and Shares
User->>Keeper: Retrieve All Owner Shares
Keeper->>Store: Retrieve Owner Shares
Store-->>Keeper: Return Owner Shares
Keeper-->>User: Return All Owner Shares
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
protocol/x/vault/types/genesis.pb.gois excluded by!**/*.pb.go
Files selected for processing (8)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/genesis.ts (4 hunks)
- proto/dydxprotocol/vault/genesis.proto (1 hunks)
- protocol/x/vault/genesis.go (1 hunks)
- protocol/x/vault/keeper/shares.go (1 hunks)
- protocol/x/vault/keeper/shares_test.go (1 hunks)
- protocol/x/vault/keeper/vault.go (1 hunks)
- protocol/x/vault/types/errors.go (1 hunks)
- protocol/x/vault/types/genesis.go (1 hunks)
Additional comments not posted (13)
proto/dydxprotocol/vault/genesis.proto (2)
16-26: Review the newVaultmessage definition.The
Vaultmessage definition correctly captures the vault's ID, total shares, and owner shares as per the PR objectives. Ensure that the field numbers are consistent and do not overlap with existing fields in other messages within the same file to maintain protobuf backward compatibility.
6-7: Ensure proper usage of new imports.You've added imports for
query.protoandvault.proto. Verify that these are indeed necessary and correctly utilized in this file to avoid unnecessary dependencies.Verification successful
Ensure proper usage of new imports.
The imports for
query.protoandvault.protoare indeed utilized in thegenesis.protofile, as evidenced by the presence ofVaultandvaultsin the protobuf definitions.
vault.proto: The termVaultandvaultsare used in the protobuf definitions.No further action is needed regarding these imports.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the imported files are used in the protobuf definitions. # Test: Search for `query` and `vault` usage in the file. Expect: At least one usage of each. rg --type proto --files-with-matches $'query|vault' proto/dydxprotocol/vault/genesis.protoLength of output: 121
Script:
#!/bin/bash # Description: Verify that the imported files are used in the protobuf definitions. # Test: Search for `query` and `vault` usage in the file. Expect: At least one usage of each. rg 'query|vault' proto/dydxprotocol/vault/genesis.protoLength of output: 588
protocol/x/vault/genesis.go (1)
34-39: Review data export logic inExportGenesis.The logic to export vault parameters and vaults themselves in
ExportGenesisseems correct. However, ensure this exported data is in the expected format and that all necessary data fields are included.Verification successful
Review data export logic in
ExportGenesis.The
ExportGenesisfunction correctly retrieves and exports vault parameters and vaults. TheParamsandVaulttypes include all necessary fields to ensure the data is in the expected format.
Paramsinprotocol/x/vault/types/params.pb.goincludes fields likeLayers,SpreadMinPpm,SpreadBufferPpm,SkewFactorPpm,OrderSizePctPpm,OrderExpirationSeconds, andActivationThresholdQuoteQuantums.Vaultinprotocol/x/vault/types/genesis.pb.goincludes fields likeVaultId,TotalShares, andOwnerShares.No further issues were found with the export logic.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the exported genesis state format. # Test: Check the exported data format by invoking the function with a mock context. Expect: Correctly formatted output. echo "Mock test to verify exported genesis state format."Length of output: 110
Script:
#!/bin/bash # Search for the ExportGenesis function implementation in the codebase ast-grep --lang go --pattern 'func ExportGenesis($_, $_) $_'Length of output: 8117
Script:
#!/bin/bash # Search for the GetParams method implementation in the codebase ast-grep --lang go --pattern 'func (k Keeper) GetParams($_) $_' # Search for the GetAllVaults method implementation in the codebase ast-grep --lang go --pattern 'func (k Keeper) GetAllVaults($_) $_'Length of output: 4071
Script:
#!/bin/bash # Search for the Params type definition in the codebase ast-grep --lang go --pattern 'type Params struct $_' # Search for the Vault type definition in the codebase ast-grep --lang go --pattern 'type Vault struct $_'Length of output: 798
Script:
#!/bin/bash # Search for the Params type definition in the codebase ast-grep --lang go --pattern 'type Params struct {$$$}' # Search for the Vault type definition in the codebase ast-grep --lang go --pattern 'type Vault struct {$$$}'Length of output: 7633
protocol/x/vault/types/genesis.go (1)
18-39: Enhance validation logic inGenesisState.Validate.The validation logic correctly checks for non-negative shares and consistency between total and owner shares. However, consider adding more detailed comments to explain each validation step for better maintainability.
+ // Check if total shares are non-negative. + // Ensure each owner's shares are non-negative and sum up to total shares. + // Validate that the owner field is not empty.protocol/x/vault/types/errors.go (1)
78-87: Validate new error definitions.The new error definitions
ErrInvalidOwnerandErrMismatchedTotalAndOwnerSharesare appropriate and well-defined. Ensure that these errors are used consistently throughout the module wherever relevant conditions are checked.protocol/x/vault/keeper/vault.go (1)
103-126: Review modifications inGetAllVaults.The modifications to
GetAllVaultsare correctly implemented to include total shares and owner shares. Ensure that the function handles potential errors during data retrieval and aggregation, such as possible nil values or data inconsistencies.indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/genesis.ts (5)
13-13: Approved the addition ofvaultsfield in theGenesisStateinterfaces.The addition of the
vaultsfield in bothGenesisStateandGenesisStateSDKTypeinterfaces is consistent with the PR's objective to include vault shares in the genesis state. This change is crucial for initializing the network with predefined vault states.Also applies to: 22-22
26-46: Review of newly definedVaultandVaultSDKTypeinterfaces.The interfaces are well-defined with appropriate fields for vault ID, total shares, and owner shares. This aligns with the PR’s objectives and seems correctly implemented. However, consider adding comments for each field to enhance code readability and maintainability.
+ /** The unique identifier of the vault. */ vaultId?: VaultId; + /** The total number of shares available in the vault. */ totalShares?: NumShares; + /** The shares owned by each owner within the vault. */ ownerShares: OwnerShare[];
62-64: Review of encoding and decoding logic forVaultwithinGenesisState.The encoding and decoding functions for
VaultwithinGenesisStatehave been correctly implemented. These functions are critical for ensuring the correct serialization and deserialization of vault data in the genesis state.Also applies to: 82-84
98-99: Review offromPartialimplementation forGenesisState.The implementation of
fromPartialforGenesisStatecorrectly handles optional fields and ensures that thevaultsarray is populated correctly if provided. This method aids in creating partial instances ofGenesisState, which is useful in scenarios where not all data is available.
104-163: Review ofVaultutility functions and methods.The utility functions for creating base
Vaultinstances and encoding/decoding them are well-implemented. These functions are essential for handlingVaultdata throughout the application. The use of protobuf for serialization ensures consistency and reliability in data handling.protocol/x/vault/keeper/shares.go (1)
104-123: Review of theGetAllOwnerSharesfunction.The implementation of
GetAllOwnerSharesis robust, leveraging thegetVaultOwnerSharesStoreto fetch all owner shares associated with a given vault. The use of an iterator to traverse owner shares is efficient, and the proper handling of the iterator's closure via defer is commendable. This function is crucial for retrieving comprehensive ownership data, which aligns with the PR's objectives.protocol/x/vault/keeper/shares_test.go (1)
135-168: Review ofTestGetAllOwnerSharesunit test.The unit test
TestGetAllOwnerSharesis well-structured, covering scenarios with no owner shares and multiple owner shares. The use ofElementsMatchto verify the correctness of the retrieved data is appropriate, ensuring that the order of elements does not affect the test outcome. This test effectively validates the functionality of theGetAllOwnerSharesmethod.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
protocol/testutil/constants/genesis.go (1)
Line range hint
342-342: Potential security risk: Generic API key detected.A potential API key was detected in the file. Review this to ensure that no sensitive keys are hardcoded in the source code. If this is a placeholder or example, consider removing it or using environment variables to handle sensitive data securely.
- "eth_address": "0xEf01c3A30eB57c91c40C52E996d29c202ae72193" + "eth_address": "<REPLACE_WITH_ENV_VAR>"protocol/scripts/genesis/sample_pregenesis.json (1)
Line range hint
538-538: Potential Security Risk: Exposed Generic API KeyIt appears that an API key might be exposed in the configuration, which can lead to security vulnerabilities if this file is accessible publicly or within a broader internal network.
- "eth_address": "0xsampleaddress", + "eth_address": "<REDACTED>",Please ensure that sensitive keys are stored securely and not hard-coded in public or shared files.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- protocol/app/testdata/default_genesis_state.json (1 hunks)
- protocol/scripts/genesis/sample_pregenesis.json (1 hunks)
- protocol/testing/containertest/preupgrade_genesis.json (1 hunks)
- protocol/testutil/constants/genesis.go (1 hunks)
Files skipped from review due to trivial changes (1)
- protocol/app/testdata/default_genesis_state.json
Additional context used
Gitleaks
protocol/testutil/constants/genesis.go
342-342: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key)
protocol/scripts/genesis/sample_pregenesis.json
538-538: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key)
protocol/testing/containertest/preupgrade_genesis.json
675-675: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key)
Additional comments not posted (3)
protocol/testutil/constants/genesis.go (1)
1514-1515: Ensure proper initialization of thevaultsarray.The empty
vaultsarray has been added as per the PR's objective to include vault shares in the genesis state. Ensure that this array is properly populated during the initialization or migration scripts to reflect the intended functionality.protocol/scripts/genesis/sample_pregenesis.json (1)
1829-1829: Ensure initialization of vaults array for genesis state.The addition of an empty "vaults" array is consistent with the PR's objective to prepare the genesis state for including vault shares. This setup allows for easy addition of vault data in future configurations or during runtime initialization.
protocol/testing/containertest/preupgrade_genesis.json (1)
2319-2319: Ensure thevaultsarray initialization aligns with the intended genesis state configuration.The addition of an empty
vaultsarray is consistent with the PR's objective to include vault shares in the genesis state for testing purposes. This setup allows for future modifications to be made easily, such as adding specific vault data during tests.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- protocol/x/vault/keeper/shares_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/vault/keeper/shares_test.go
Changelist
include vault total / owner shares in genesis state
besides general benefits of exporting state, this enables a network to automatically start with existing vaults (e.g. for testing purposes)
Test Plan
unit tests
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
Vaultentity with fields for total shares, owner shares, and ID.Bug Fixes
Tests
Documentation
vaultsarray for clarity and initialization purposes.