[TRA-617] Fix calculation for atomic resolution #2360
Conversation
Signed-off-by: Shrenuj Bansal <[email protected]>
Signed-off-by: Shrenuj Bansal <[email protected]>
Signed-off-by: Shrenuj Bansal <[email protected]>
Signed-off-by: Shrenuj Bansal <[email protected]>
Signed-off-by: Shrenuj Bansal <[email protected]>
Signed-off-by: Shrenuj Bansal <[email protected]>
Signed-off-by: Shrenuj Bansal <[email protected]>
Signed-off-by: Shrenuj Bansal <[email protected]>
Signed-off-by: Shrenuj Bansal <[email protected]>
Signed-off-by: Shrenuj Bansal <[email protected]>
Signed-off-by: Shrenuj Bansal <[email protected]>
Signed-off-by: Shrenuj Bansal <[email protected]>
|
Warning Rate limit exceeded@shrenujb has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 8 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes introduce several enhancements to the protocol's listing and vault management functionalities. Key modifications include the addition of a new branch trigger in the GitHub Actions workflow, updates to 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: 12
🧹 Outside diff range and nitpick comments (11)
protocol/x/listing/types/genesis.go (1)
Line range hint
1-16: Overall concerns about the changes and validationThe changes made in this file seem to diverge from the stated PR objective of fixing the calculation for atomic resolution. Instead, we see significant modifications to the default genesis state that could have far-reaching implications for the system.
Additionally, I noticed that the
Validatefunction remains unchanged and always returns nil. Given the new parameters added to the genesis state, it might be prudent to implement some validation logic.Consider the following:
- Align the changes with the PR objective or update the PR description to accurately reflect these modifications.
- Implement validation logic in the
Validatefunction to ensure the integrity of the new and modified fields in the genesis state.- Document the rationale behind these changes and their expected impact on the system.
Here's a suggestion for improving the
Validatefunction:func (gs GenesisState) Validate() error { if gs.HardCapForMarkets <= 0 { return fmt.Errorf("HardCapForMarkets must be positive, got %d", gs.HardCapForMarkets) } // Add validation for ListingVaultDepositParams if necessary return gs.ListingVaultDepositParams.Validate() }Please review and adjust as needed based on the specific requirements of your system.
protocol/x/listing/types/params.go (2)
Line range hint
14-27: Consider adding an upper limit check in theValidate()function.While the current validation logic correctly checks for non-negative values, it might be beneficial to add a sanity check for extremely large values to prevent potential overflow issues, especially considering the significant increase in the
NewVaultDepositAmount.Consider adding an upper limit check in the
Validate()function. For example:func (p ListingVaultDepositParams) Validate() error { // if any of the deposit params are negative, return an error - if p.NewVaultDepositAmount.Sign() <= 0 || p.MainVaultDepositAmount.Sign() < 0 { + if p.NewVaultDepositAmount.Sign() <= 0 || p.MainVaultDepositAmount.Sign() < 0 || + p.NewVaultDepositAmount.GT(dtypes.NewIntFromUint64(1_000_000_000_000)) || // 1 trillion + p.MainVaultDepositAmount.GT(dtypes.NewIntFromUint64(1_000_000_000_000)) { // 1 trillion return ErrInvalidDepositAmount } // if the number of blocks to lock shares is negative, return an error if p.NumBlocksToLockShares <= 0 { return ErrInvalidNumBlocksToLockShares } return nil }This change adds an upper limit of 1 trillion for both
NewVaultDepositAmountandMainVaultDepositAmount. Adjust the limit as appropriate for your use case.
Line range hint
1-27: Summary of changes and potential impactThe primary change in this file is a significant increase in the
NewVaultDepositAmountfrom 10,000 to 10,000,000,000 USDC. This 1000x increase could have substantial implications on the system's behavior, particularly in areas related to vault deposits and possibly token economics.Key points to consider:
- The change is correctly implemented, but the accompanying comment needs to be updated.
- The
Validate()function still works correctly with this new value, but adding an upper limit check could provide additional safety.- This change might require updates to other parts of the codebase, particularly test cases and any logic that depends on this default value.
- Documentation should be updated to reflect this change, especially if it affects user-facing aspects of the system.
Given the magnitude of this change, please ensure that:
- The rationale for this increase is well-documented.
- All relevant stakeholders are aware of this change and its potential impacts.
- Thorough testing is conducted, including integration and system-wide tests, to verify that this change doesn't introduce unexpected behaviors.
- Consider a phased rollout or feature flag if possible, to mitigate risks associated with such a significant change.
protocol/x/listing/keeper/msg_create_market_permissionless.go (1)
40-44: LGTM: New DepositToMegavaultforPML operation with proper error handling.The addition of the
DepositToMegavaultforPMLoperation enhances the functionality of the market creation process. The error handling is consistent with the existing pattern in the function.Consider adding more context to the error log by including the
clobPairIdandSubaccountId:- k.Logger(ctx).Error("failed to deposit to megavault for PML market", "error", err) + k.Logger(ctx).Error("failed to deposit to megavault for PML market", "error", err, "clobPairId", clobPairId, "subaccountId", msg.SubaccountId)This will provide more detailed information for debugging purposes.
protocol/x/listing/keeper/keeper.go (1)
35-35: Approved. Consider adding a comment for the new parameter.The update to
NewKeeperfunction is correct and consistent with theKeeperstruct changes.For improved readability, consider adding a comment for the new
vaultKeeperparameter:func NewKeeper( cdc codec.BinaryCodec, storeKey storetypes.StoreKey, authorities []string, pricesKeeper types.PricesKeeper, clobKeeper types.ClobKeeper, marketMapKeeper types.MarketMapKeeper, perpetualsKeeper types.PerpetualsKeeper, - vaultKeeper types.VaultKeeper, + vaultKeeper types.VaultKeeper, // Keeper for managing vault-related operations ) *Keeper { // ... }Also applies to: 45-45
protocol/x/listing/module.go (1)
Line range hint
1-163: Summary of changes and recommendationsThe changes to
protocol/x/listing/module.gointroduce a newvaultKeeperfield to theAppModulestruct and update theNewAppModuleconstructor accordingly. These modifications appear to be implemented correctly.However, to ensure the completeness and correctness of this change, please consider the following:
- Verify the necessity of adding
VaultKeeperto the listing module and its impact on the module's responsibilities.- Ensure all
NewAppModulecalls across the codebase have been updated to include thevaultKeeperargument.- Check for any additional files that might require updates due to this change (e.g., tests, dependency injections, or module initialization code).
- Update relevant documentation and comments to reflect the new dependency on
VaultKeeper.Consider documenting the reason for adding
VaultKeeperto the listing module in a comment or in the PR description. This will help future maintainers understand the design decision and the relationship between the listing and vault modules.protocol/x/clob/keeper/clob_pair.go (1)
168-172: LGTM! Consider clarifying the comment.The renaming of this function to
CreateClobPairStructuresand the addition of error return improve clarity and error handling. This change aligns well with the separation of concerns observed earlier.Consider updating the comment to more accurately reflect the function's new name and purpose. For example:
-// CreateClobPair performs all non stateful operations to create a CLOB pair. +// CreateClobPairStructures performs all non-stateful operations to set up the structures for a CLOB pair.protocol/testing/genesis.sh (2)
Line range hint
1-2260: Review ofedit_genesisfunctionThe
edit_genesisfunction is quite long and complex. While it seems to cover all necessary aspects of genesis configuration, there are some potential improvements and concerns:
Function length: The function is over 2000 lines long, which makes it hard to maintain and understand. Consider breaking it down into smaller, more focused functions.
Hardcoded values: There are many hardcoded values throughout the function. It might be beneficial to move these to configuration files or environment variables for easier management and flexibility.
Error handling: The script doesn't have explicit error handling. Consider adding checks and error messages for critical operations.
Comments: While there are some comments, more detailed explanations for each section would improve readability and maintainability.
Consistency: The script uses a mix of
daselcommands and direct file writes. Consider standardizing the approach for consistency.Security: Ensure that all the parameters set, especially those related to economics (like fees, limits, and tiers), have been thoroughly reviewed and tested.
Testability: Consider adding unit tests for critical parts of the script to ensure it behaves as expected under different scenarios.
To improve maintainability and readability, consider refactoring the
edit_genesisfunction into smaller, more focused functions. For example:function edit_genesis() { update_consensus_params update_crisis_module update_gov_module update_staking_module update_assets_module update_bridge_module update_ibc_module update_perpetuals_module # ... and so on for each major section }This approach would make the script easier to understand, maintain, and test.
Also applies to: 2268-2779
Line range hint
1-2779: Consistency and error handling improvementsThroughout the script, I noticed some areas where consistency and error handling could be improved:
- Error handling: Many commands assume success without checking. Consider adding error checks, especially for critical operations. For example:
if ! dasel put -t string -f "$GENESIS" '.some.path' -v "some_value"; then echo "Error: Failed to set .some.path in genesis file" exit 1 fi
Variable usage: Some parts of the script use hardcoded values where variables are defined earlier. Ensure consistent use of variables throughout the script.
Function naming: Consider adopting a consistent naming convention for functions. Currently, there's a mix of snake_case and camelCase.
Comments: While some sections are well-commented, others lack explanation. Consider adding more inline comments to explain the purpose of each section.
To improve consistency and error handling, consider implementing a wrapper function for
daselcommands:function set_genesis_value() { local path=$1 local type=$2 local value=$3 if ! dasel put -t "$type" -f "$GENESIS" "$path" -v "$value"; then echo "Error: Failed to set $path in genesis file" exit 1 fi } # Usage set_genesis_value '.app_state.gov.params.voting_period' 'string' '120s'This approach would centralize error handling and make the script more consistent and robust.
protocol/x/listing/types/expected_keepers.go (1)
69-90: Suggestion: Add method documentation forVaultKeeperinterface.For better readability and maintenance, consider adding comments to each method in the
VaultKeeperinterface to explain their purpose, parameters, and return values. This will help other developers understand the intended functionality.protocol/x/listing/keeper/listing.go (1)
205-208: Function name should follow Go naming conventions.The function name
DepositToMegavaultforPMLshould beDepositToMegavaultForPML, capitalizing 'For' to adhere to Go naming conventions for exported functions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
🔇 Files ignored due to path filters (1)
protocol/x/listing/types/genesis.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (21)
- .github/workflows/protocol-build-and-push.yml (1 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/listing/genesis.ts (6 hunks)
- proto/dydxprotocol/listing/genesis.proto (1 hunks)
- protocol/app/app.go (2 hunks)
- protocol/app/testdata/default_genesis_state.json (1 hunks)
- protocol/scripts/genesis/sample_pregenesis.json (1 hunks)
- protocol/testing/genesis.sh (1 hunks)
- protocol/testutil/constants/genesis.go (1 hunks)
- protocol/testutil/keeper/listing.go (4 hunks)
- protocol/x/clob/keeper/clob_pair.go (4 hunks)
- protocol/x/listing/genesis.go (1 hunks)
- protocol/x/listing/keeper/keeper.go (3 hunks)
- protocol/x/listing/keeper/listing.go (4 hunks)
- protocol/x/listing/keeper/listing_test.go (3 hunks)
- protocol/x/listing/keeper/msg_create_market_permissionless.go (1 hunks)
- protocol/x/listing/keeper/msg_create_market_permissionless_test.go (2 hunks)
- protocol/x/listing/module.go (3 hunks)
- protocol/x/listing/types/expected_keepers.go (3 hunks)
- protocol/x/listing/types/genesis.go (1 hunks)
- protocol/x/listing/types/params.go (1 hunks)
- protocol/x/vault/keeper/params.go (1 hunks)
🧰 Additional context used
buf
proto/dydxprotocol/listing/genesis.proto
4-4: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
🔇 Additional comments not posted (43)
protocol/x/listing/types/genesis.go (2)
7-7: NewListingVaultDepositParamsfield requires explanationA new field
ListingVaultDepositParamshas been added to the genesis state, initialized withDefaultParams(). This addition could have significant implications for the system's initialization and operation.Please provide more information:
- What is the purpose of
ListingVaultDepositParams?- How does this relate to the PR objective of fixing the calculation for atomic resolution?
- Are there corresponding changes in other parts of the codebase to handle this new parameter?
Let's verify the implementation and usage of
DefaultParams():✅ Verification successful
Addition of
ListingVaultDepositParamsVerifiedThe new
ListingVaultDepositParamsfield has been correctly added and initialized withDefaultParams(), consistent with patterns in other modules. This addition aligns with the module's parameter initialization and does not impact the PR objective negatively.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the definition and usage of DefaultParams rg --type go "func DefaultParams" -C 5 rg --type go "DefaultParams\(\)" -C 3Length of output: 9700
6-6: Significant change inHardCapForMarketsrequires explanationThe
HardCapForMarketsvalue has been changed from 0 to 500. This is a substantial modification to the default genesis state that could significantly impact the system's behavior and capacity.Could you please provide more context for this change?
- What is the rationale behind setting a hard cap of 500 markets?
- How does this relate to the PR objective of fixing the calculation for atomic resolution?
- Have potential impacts on system performance and scalability been considered?
Additionally, let's verify if this change is consistent across the codebase:
✅ Verification successful
Change to
HardCapForMarketsVerified as IsolatedThe modification of
HardCapForMarketsfrom0to500has been thoroughly verified and is confined toprotocol/x/listing/types/genesis.go. No other instances ofHardCapForMarketswere found in the codebase, ensuring that this change does not affect other modules or functionalities.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other occurrences of HardCapForMarkets rg --type go "HardCapForMarkets" -C 3Length of output: 9700
proto/dydxprotocol/listing/genesis.proto (1)
15-17: LGTM! New field added correctly. Please provide more context.The new
listing_vault_deposit_paramsfield is correctly added to theGenesisStatemessage with proper formatting and numbering. The non-nullable option is appropriate for ensuring this field is always present in the genesis state.Could you please provide more context about the
ListingVaultDepositParamstype and how it relates to the atomic resolution calculation mentioned in the PR objective? This would help ensure the change aligns with the intended purpose of the PR.protocol/x/listing/genesis.go (1)
17-19: LGTM, but verify new function and consider error handling.The addition of
SetListingVaultDepositParamsis consistent with the existing code structure and initializes an important parameter for the module. However, please ensure the following:
- Verify that the
SetListingVaultDepositParamsfunction is correctly implemented in thekeeperpackage.- Consider if panicking is the most appropriate error handling strategy for genesis initialization. While it's consistent with the existing code, it might be worth discussing if a more graceful error handling approach could be beneficial.
To verify the existence and correct implementation of the
SetListingVaultDepositParamsfunction, please run the following script:✅ Verification successful
Verification Successful:
SetListingVaultDepositParamsis correctly implemented.The
SetListingVaultDepositParamsfunction exists in thekeeperpackage and is properly integrated with the genesis state. However, consider reviewing the error handling strategy to determine if usingpanicis the most appropriate approach for genesis initialization.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of SetListingVaultDepositParams function # Test: Search for the SetListingVaultDepositParams function definition ast-grep --lang go --pattern $'func (k Keeper) SetListingVaultDepositParams(ctx sdk.Context, listingVaultDepositParams types.ListingVaultDepositParams) error { $$$ }' # Test: Check for any references or usages of ListingVaultDepositParams rg --type go "ListingVaultDepositParams"Length of output: 27699
protocol/x/listing/keeper/msg_create_market_permissionless.go (1)
34-38: LGTM: Proper capture and error handling of CreateClobPair return value.The modification to capture the
clobPairIdreturned byCreateClobPairis correct and necessary for the subsequent operation. The error handling remains consistent with the existing pattern in the function.protocol/x/listing/keeper/keeper.go (2)
23-23: Approved. Please clarify the purpose of adding VaultKeeper.The addition of
VaultKeeperto theKeeperstruct is structurally correct. However, could you please elaborate on how this addition relates to fixing the calculation for atomic resolution? This context would help in understanding the broader implications of this change.To ensure the
VaultKeepertype is properly defined, let's verify its existence:
23-23: ```shell
#!/bin/bashDescription: Search for the definition of the VaultKeeper interface
find . -type f -name ".go" ! -path "./**/_test.go" -exec grep -H "type VaultKeeper interface" {} ;
```shell #!/bin/bash # Description: Find implementations and usages of VaultKeeper related to atomic resolution # Search for methods of VaultKeeper that mention atomic resolution rg --type go 'VaultKeeper\..*atomicResolution' . # Additionally, search for atomic resolution calculations interacting with VaultKeeper rg --type go 'atomicResolution.*VaultKeeper' .indexer/packages/v4-protos/src/codegen/dydxprotocol/listing/genesis.ts (6)
1-1: LGTM: Import statement added correctly.The new import for
ListingVaultDepositParamsandListingVaultDepositParamsSDKTypeis correctly added and necessary for the new property in theGenesisStateinterface.
12-14: LGTM: New property added to GenesisState interface.The
listingVaultDepositParamsproperty is correctly added to theGenesisStateinterface with the appropriate type and optionality. The comment provides a clear description of its purpose.
24-26: LGTM: New property added to GenesisStateSDKType interface.The
listing_vault_deposit_paramsproperty is correctly added to theGenesisStateSDKTypeinterface with the appropriate type, optionality, and naming convention (snake_case). The comment provides a clear description of its purpose.
31-32: LGTM: New property correctly initialized in createBaseGenesisState.The
listingVaultDepositParamsproperty is correctly initialized asundefinedin thecreateBaseGenesisStatefunction, which is consistent with its optional nature in the interface definition.
42-44: LGTM: GenesisState object methods updated correctly.The
encode,decode, andfromPartialmethods of theGenesisStateobject have been correctly updated to handle the newlistingVaultDepositParamsproperty:
- The
encodemethod correctly checks iflistingVaultDepositParamsis defined before encoding.- The
decodemethod correctly decodes the new property.- The
fromPartialmethod correctly handles the new property, including cases where it might be undefined or null.These changes ensure proper serialization, deserialization, and partial object creation for the updated
GenesisStatestructure.Also applies to: 62-64, 78-78
Line range hint
1-82: Summary: New property added for PML megavault deposits.The changes in this file introduce a new optional property
listingVaultDepositParamsto theGenesisStateandGenesisStateSDKTypeinterfaces. This property is related to PML megavault deposits and could be part of the atomic resolution calculation mentioned in the PR objectives. The changes are consistently implemented throughout the file, including imports, interface definitions, and related methods.These modifications appear to enhance the functionality related to listing parameters, potentially improving the atomic resolution calculation as intended by the PR.
To ensure consistency across the codebase, please run the following verification script:
This script will help identify any other locations in the codebase where similar updates might be necessary or where the new
ListingVaultDepositParamsis being used.protocol/testutil/keeper/listing.go (4)
17-17: LGTM: New import for vaultkeeper added correctly.The new import for
vaultkeeperis properly formatted and consistent with the existing import style and project structure.
169-169: LGTM: vaultKeeper parameter added to createListingKeeper call.The
vaultKeeperparameter is correctly added to thecreateListingKeeperfunction call, maintaining consistency with the updated function signature.
Line range hint
1-210: Summary: vaultkeeper integration with Listing keeperThe changes in this file consistently introduce the
vaultkeeperas a new dependency for the Listing keeper. This modification appears to be part of a larger refactoring or feature addition involving the Vault keeper.Consider the following points:
- Ensure that all relevant tests have been updated to include the
vaultkeeperwhen creating a Listing keeper.- Review the actual Listing keeper implementation to verify that the
vaultkeeperis being used appropriately.- Check for any potential circular dependencies between the Listing and Vault keepers.
- Update relevant documentation to reflect this new dependency in the Listing keeper's architecture.
To ensure comprehensive test coverage, run the following script to check for test files that might need updating:
#!/bin/bash # Description: Find test files that might need updating due to the new vaultkeeper dependency echo "Searching for test files that create a Listing keeper:" rg --type go -l 'func TestListing' | xargs rg 'createListingKeeper' echo "Searching for mock Listing keepers that might need updating:" rg --type go -l 'mock.ListingKeeper'
187-187: LGTM: vaultkeeper parameter added to createListingKeeper and passed to keeper.NewKeeper.The
vaultkeeperparameter is correctly added to thecreateListingKeeperfunction signature and passed to thekeeper.NewKeeperfunction call. This change is consistent with the modifications in theListingKeepersfunction.Please ensure that the
keeper.NewKeeperfunction in the actual implementation file has been updated to accept and utilize thevaultkeeperparameter. Run the following script to verify:Also applies to: 207-207
protocol/x/vault/keeper/params.go (1)
Line range hint
1-132: Overall assessment of the changesThe addition of the
SetVaultStatusmethod to theKeeperstruct is a positive enhancement to the vault management functionality. It provides a dedicated method for updating the vault status, which can improve code organization and readability.Key points:
- The new method integrates well with existing code.
- It leverages existing methods (
GetVaultParamsandSetVaultParams) for consistency.- The change is focused and doesn't introduce unnecessary complexity.
To ensure the full benefit of this addition:
- Implement the suggested improvements in the method implementation.
- Verify its usage across the codebase as suggested in the previous comment.
- Consider updating relevant documentation or comments in other parts of the code that deal with vault status changes.
protocol/x/listing/module.go (2)
116-116: LGTM. Please verify allNewAppModulecalls have been updated.The addition of the
vaultKeeperparameter to theNewAppModuleconstructor and its initialization in the returnedAppModulestruct are implemented correctly. However, please ensure that:
- All calls to
NewAppModulethroughout the codebase have been updated to include thevaultKeeperargument.- The order of parameters in the constructor remains consistent with other similar constructors in the codebase.
To help verify that all
NewAppModulecalls have been updated, please run the following script:This script will help identify all calls to
NewAppModuleand highlight any that might be missing thevaultKeeperargument, ensuring consistency across the codebase.Also applies to: 125-125
106-106: LGTM. Please verify the necessity and impacts of addingvaultKeeper.The addition of the
vaultKeeperfield to theAppModulestruct is syntactically correct. However, please ensure that:
- The
VaultKeeperis necessary for the listing module's functionality.- The addition of this dependency doesn't introduce circular dependencies.
- The impact on the module's responsibilities and separation of concerns has been considered.
To help verify the necessity of this change, please run the following script:
This script will help identify where and how the
VaultKeeperis being used in the listing module, providing context for its addition..github/workflows/protocol-build-and-push.yml (1)
6-6: Approve the addition of 'tra617' branch trigger with a suggestion.The addition of the 'tra617' branch to the workflow triggers is approved. This change allows for continuous integration and deployment specifically for this feature branch, which is beneficial for testing the current PR (TRA-617).
However, please consider the following:
- Remove this branch trigger after merging the PR to prevent unnecessary workflow runs in the future.
- Verify if adding feature branch triggers to the main workflow file aligns with your team's practices. If not, consider using a separate workflow file for feature-specific CI/CD.
To verify the impact of this change, you can run the following script:
This script will help you identify any other feature branch triggers that might have been left in the workflow file and show recent workflow runs, which can help in understanding the impact of this change.
✅ Verification successful
The 'tra617' branch trigger has been successfully verified.
- Only the 'tra617' branch trigger has been added, with no additional feature branch triggers present.
- Recent workflow runs are limited to 'main', 'release/protocol/v6.x', and 'tra617', all functioning as expected.
Please ensure to:
- Remove the 'tra617' branch trigger after merging the PR to avoid unnecessary future workflow runs.
- Confirm that adding feature branch triggers aligns with your team's CI/CD practices.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other feature branch triggers and recent workflow runs # Check for other potential feature branch triggers echo "Checking for other feature branch triggers:" grep -nE '^\s+- [a-z0-9]+$' .github/workflows/protocol-build-and-push.yml | grep -vE '(main|release|tra617)' # Check recent workflow runs (requires GitHub CLI) echo "Recent workflow runs:" gh run list --workflow=protocol-build-and-push.yml --limit=5 --json event,headBranch,conclusion --jq '.[] | "\(.event) on \(.headBranch): \(.conclusion)"'Length of output: 539
protocol/app/testdata/default_genesis_state.json (1)
356-361: Please clarify the implications of the new listing parametersThe changes to the
listingsection introduce new parameters that may have significant impacts on the system's behavior:
The
hard_cap_for_marketsis set to 500. Can you confirm if this aligns with the intended design and explain its purpose?The new
listing_vault_deposit_paramsobject introduces three parameters:
new_vault_deposit_amountis set to a large value (10 billion in the smallest unit). What are the economic implications of this amount?main_vault_deposit_amountis set to 0. Does this mean main vault deposits are not required or handled differently?num_blocks_to_lock_sharesis set to 2,592,000 (approximately 30 days worth of blocks). What's the rationale behind this lock period?Could you provide more context on how these parameters will be used and their potential impact on the system?
To ensure these changes align with the PR objectives, let's verify the PR description:
Consider documenting these new parameters and their implications in the project's technical documentation to ensure clarity for future development and maintenance.
protocol/x/clob/keeper/clob_pair.go (2)
212-213: LGTM, but consider the implications of making this function public.The renaming of
setClobPairtoSetClobPairmakes the function public, which is consistent with the new architecture. However, this change could have implications for the module's API and potential misuse.Please run the following script to check for any external usage of this newly public function:
#!/bin/bash # Description: Check for any external usage of the newly public SetClobPair function # Search for imports of the clob keeper package echo "Packages importing clob keeper:" rg "\"github.com/dydxprotocol/v4-chain/protocol/x/clob/keeper\"" --type go echo "\nUsage of SetClobPair outside the clob package:" rg "keeper\.SetClobPair\s*\(" --type go | grep -v "protocol/x/clob"If there are any results from the second search, please review them carefully to ensure that the public access is intentional and secure.
69-72: LGTM! Consider verifying the order of operations.The separation of
SetClobPairandCreateClobPairStructuresis a good practice for modularity. However, please ensure that this order of operations doesn't introduce any potential race conditions or inconsistencies in the system state.To verify the correctness of this change, please run the following script:
protocol/app/app.go (3)
1209-1209: Summary of VaultKeeper addition to ListingKeeperThe changes introduce
VaultKeeperas a new dependency forListingKeeper. While the modification is straightforward, it has potential implications for the application's architecture and module interactions.Key points to consider:
- The change increases coupling between the
listingandvaultmodules.- It may affect module initialization order and overall application architecture.
- There might be a need for additional documentation and test updates.
The changes appear to be intentional and likely necessary for new functionality. However, to ensure a smooth integration, please:
- Review and update relevant documentation, including architecture diagrams if they exist.
- Adjust any affected tests, particularly those involving
ListingKeeper.- Verify that this change doesn't introduce any unintended circular dependencies.
- Consider adding comments in the code to explain why
VaultKeeperis now required byListingKeeper.These steps will help maintain the clarity and maintainability of the codebase as it evolves.
Also applies to: 1218-1218
1209-1209: Consider architectural implications of VaultKeeper dependencyThe addition of
VaultKeepertoListingKeeperintroduces a new dependency between these modules. This change may have broader architectural implications:
- It increases the coupling between the
listingandvaultmodules.- It may affect the module initialization order in the application.
Please consider the following actions:
- Review the
listingmodule's responsibilities and ensure that this new dependency aligns with its intended purpose.- Update the overall application architecture documentation to reflect this new relationship between modules.
- Consider if this change impacts the application's modularity or if it introduces any circular dependencies.
- Ensure that the
VaultKeepermethods used byListingKeeperare well-defined and properly encapsulated to maintain clear module boundaries.#!/bin/bash # Check for potential circular dependencies and module documentation # Look for imports between listing and vault modules rg --type go "import\s+\([^)]*vault[^)]*listing[^)]*\)" ./protocol/x rg --type go "import\s+\([^)]*listing[^)]*vault[^)]*\)" ./protocol/x # Check for updates in module documentation rg --type md "VaultKeeper" ./protocol/x/listing/specAlso applies to: 1218-1218
1209-1209: Vault Keeper added to Listing KeeperThe
VaultKeeperhas been added as a dependency to theListingKeeper. This change suggests that the listing functionality now requires access to vault-related operations or data.To ensure this change is properly integrated, please verify the following:
- The
VaultKeeperis initialized before theListingKeeperin the application setup.- The
listingmodule's documentation is updated to reflect this new dependency.- Any relevant unit or integration tests are updated to include mock
VaultKeeperwhen testingListingKeeper.Also applies to: 1218-1218
protocol/scripts/genesis/sample_pregenesis.json (2)
872-877: Review of changes in the "listing" moduleThe changes introduce new configuration parameters for the "listing" module:
hard_cap_for_markets: Set to 500, which likely represents a limit on the number of markets that can be listed.listing_vault_deposit_params: A new object with the following properties:
main_vault_deposit_amount: Set to "0"new_vault_deposit_amount: Set to "10000000000"num_blocks_to_lock_shares: Set to 2592000These changes appear to introduce a cap on the number of markets and define parameters for vault deposits related to listings.
The changes seem reasonable, but I have a few points to consider:
- The
hard_cap_for_marketsvalue of 500 seems appropriate, but it's worth confirming if this aligns with the intended scalability of the system.- The
main_vault_deposit_amountis set to "0". Is this intentional, or should it have a non-zero value?- The
num_blocks_to_lock_sharesis set to 2592000, which likely represents a significant time period. It would be helpful to have a comment explaining the duration this represents (e.g., in days or hours) for better readability.Consider adding comments to explain the purpose and implications of these new parameters, especially for future maintainers of the configuration.
Line range hint
1-3844: Overall assessment of the configuration fileThe changes to the
sample_pregenesis.jsonfile are focused on the "listing" module and introduce new parameters for market caps and vault deposits. These changes appear to be self-contained and don't seem to directly impact other modules in the configuration.The modifications seem appropriate for their intended purpose. However, to enhance the maintainability and clarity of the configuration:
- Consider adding comments throughout the file to explain the purpose and implications of key configuration parameters, especially for complex or critical settings.
- Ensure that the new
listingmodule parameters are consistent with the overall system design and performance goals.- It may be beneficial to review other modules to see if any adjustments are needed in response to these changes, particularly if there are any cross-module dependencies related to market listings or vault operations.
Overall, the changes appear to be a positive addition to the configuration, providing more control over market listings and associated deposits.
protocol/testutil/constants/genesis.go (3)
876-881: New listing parameters added. Please clarify their purpose and impact.The changes introduce new parameters in the
listingsection:
hard_cap_for_markets: 500listing_vault_deposit_paramswith fields for new and main vault deposit amounts, and number of blocks to lock shares.These additions seem to be related to market listings and vault deposit parameters. Could you please provide more context on:
- The purpose of the
hard_cap_for_marketsand its implications?- The significance of the
listing_vault_deposit_paramsand how they affect the system?- How these changes relate to fixing the calculation for atomic resolution, as mentioned in the PR objectives?
876-881: Summary of changes and clarification requestThe changes in this PR are focused on the
listingsection of theGenesisStateconstant, introducing new parameters for market cap and vault deposits. While these changes appear to be well-structured and self-contained, it's not immediately clear how they address the PR objective of "fixing calculation for atomic resolution."Could you please provide more context on:
- How these new parameters in the
listingsection contribute to fixing the calculation for atomic resolution?- Are there any other changes or considerations outside this file that work in conjunction with these modifications to achieve the PR's goal?
This additional information will help ensure that the changes fully address the intended objectives and maintain the overall integrity of the system.
876-881: Verify potential impacts on other sectionsThe changes to the
listingsection appear to be self-contained. However, it's important to ensure that these new parameters don't require corresponding changes in other sections of the file, such asassetsorperpetuals. Could you please confirm that:
- The
hard_cap_for_marketsdoesn't need to be referenced or checked in any other part of the configuration?- The
listing_vault_deposit_paramsdon't affect any existing vault or asset configurations?✅ Verification successful
Verification Successful: Changes are Self-Contained
The new parameters in the
listingsection ofprotocol/testutil/constants/genesis.goare only referenced within lines 876-880. No impacts detected on other sections of the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for potential references to the new parameters in other parts of the file echo "Searching for potential references to hard_cap_for_markets:" grep -n "hard_cap_for_markets" protocol/testutil/constants/genesis.go echo "Searching for potential references to listing_vault_deposit_params:" grep -n "listing_vault_deposit_params" protocol/testutil/constants/genesis.go echo "Searching for potential references to new_vault_deposit_amount:" grep -n "new_vault_deposit_amount" protocol/testutil/constants/genesis.go echo "Searching for potential references to main_vault_deposit_amount:" grep -n "main_vault_deposit_amount" protocol/testutil/constants/genesis.go echo "Searching for potential references to num_blocks_to_lock_shares:" grep -n "num_blocks_to_lock_shares" protocol/testutil/constants/genesis.goLength of output: 1293
protocol/testing/genesis.sh (2)
2261-2267: New listing functionality addedThe changes introduce new listing functionality to the genesis configuration:
- A hard cap for markets is set to 500.
- Default listing vault deposit parameters are established:
- New vault deposit amount: 10,000 USDC
- Main vault deposit amount: 0 USDC
- Number of blocks to lock shares: 2,592,000 (equivalent to 30 days)
These changes seem reasonable, but consider the following:
- The hard cap of 500 markets might need adjustment in the future. Consider making this configurable or document the process for changing it.
- The main vault deposit amount is set to 0. Ensure this is intentional and doesn't pose any security risks.
- The 30-day lock period for shares is a significant duration. Make sure this aligns with your tokenomics and governance model.
Line range hint
1-2779: Security considerationsWhile reviewing the entire script, I noticed some potential security considerations:
Sensitive data: The script handles sensitive economic parameters. Ensure that the values set are thoroughly reviewed and approved by the economic design team.
Input validation: There's limited input validation for the parameters passed to the script. Consider adding more robust checks to prevent potential misconfigurations.
Permissions: Ensure that this script is only executable by authorized personnel, as it has the power to significantly alter the network's economic parameters.
Audit trail: Consider adding logging functionality to keep track of who ran the script and what changes were made.
To verify the security of the economic parameters, you could run the following script:
This script would help ensure that critical economic parameters are set within expected ranges.
protocol/x/listing/types/expected_keepers.go (1)
4-7: Approval: Necessary imports added.The imports for
"math/big"andvaulttypesare added appropriately to support the new functionalities introduced in the code.protocol/x/listing/keeper/msg_create_market_permissionless_test.go (4)
4-5: Importing "math/big" for big integer computationsThe addition of
"math/big"import is appropriate for handling large integer values required for thebalancefield in the test cases.
27-27: *Adding 'balance big.Int' enhances test flexibilityIntroducing the
balancefield in the test case struct allows each test case to specify its own initial balance. This improves the configurability and coverage of the tests.
34-55: Setting 'balance' in test cases ensures accurate test scenariosProviding specific
balancevalues in each test case ensures that the subaccounts are funded appropriately for the test scenarios. This change enhances the validity of the test outcomes.
63-94: Initializing genesis state with subaccounts and specified balancesThe genesis state is correctly updated to include subaccounts with asset positions based on
tc.balance. This ensures that each test case starts with the intended initial state, enhancing test reliability.protocol/x/listing/keeper/listing.go (3)
5-6: Imports added are appropriate.The addition of
"math/big"is necessary for handling big integer operations used in the deposit calculations.
7-9: New imports for vault and subaccount types are correctly added.The imports of
vaulttypesandsatypesare needed for interacting with vault functionalities and subaccount identifiers.
109-109: Properly setting the ClobPair before creating structures.Adding
k.ClobKeeper.SetClobPair(ctx, clobPair)ensures that the clob pair is stored in the keeper before creating associated structures. This is important for maintaining consistency within the clob module.protocol/x/listing/keeper/listing_test.go (1)
281-295: LGTM!The added assertions correctly verify the clob pair's properties when in deliverTx mode.
Signed-off-by: Shrenuj Bansal <[email protected]>
Signed-off-by: Shrenuj Bansal <[email protected]>
Co-authored-by: vincentwschau <[email protected]>
|
@Mergifyio backport release/protocol/v7.x |
✅ Backports have been created
|
Signed-off-by: Shrenuj Bansal <[email protected]> (cherry picked from commit 5d082e2)
Co-authored-by: shrenujb <[email protected]>
Signed-off-by: Shrenuj Bansal <[email protected]>
Changelist
The atomic resolution calculation did not account for decimal offset of the reference prices from x/marketmap. This change makes this adjustment
Test Plan
Existing 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
Release Notes
New Features
listingVaultDepositParamsfor enhanced vault deposit configurations.Bug Fixes
Tests