Include getting main subaccount equity / pnl for megavault PnL query. #2376
Conversation
WalkthroughThe changes introduce new constants related to the megavault functionality in the codebase, specifically Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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: 5
🧹 Outside diff range and nitpick comments (4)
indexer/packages/postgres/src/constants.ts (2)
134-135: LGTM:MEGAVAULT_MODULE_ADDRESSconstant.The constant is correctly defined and exported. The comment provides valuable context about the source of this constant.
Consider updating the comment to include the specific version of the protocol, as it currently references a development branch:
-// From https://github.com/dydxprotocol/v4-chain/blob/protocol/v7.0.0-dev0/protocol/app/module_accounts_test.go#L41 +// From https://github.com/dydxprotocol/v4-chain/blob/main/protocol/app/module_accounts_test.go#L41This will ensure the reference remains valid even if the development branch is deleted or renamed.
136-136: LGTM:MEGAVAULT_SUBACCOUNT_IDconstant.The constant is correctly defined using the
uuidfunction andMEGAVAULT_MODULE_ADDRESS.Consider adding a brief comment to explain the significance of the
0passed to theuuidfunction:-export const MEGAVAULT_SUBACCOUNT_ID: string = uuid(MEGAVAULT_MODULE_ADDRESS, 0); +// Generate the subaccount ID for the main (0th) subaccount of the megavault module +export const MEGAVAULT_SUBACCOUNT_ID: string = uuid(MEGAVAULT_MODULE_ADDRESS, 0);This will provide context for future developers about why
0is used here.indexer/services/comlink/__tests__/controllers/api/v4/vault-controller.test.ts (2)
Line range hint
158-182: Fix the typo in the comment for clarityThere's a minor typo in the comment when calling
createPnlTicks:-true, // createMainSubaccounPnlTicks +true, // createMainSubaccountPnlTicksCorrecting the typo improves code readability and prevents confusion.
Line range hint
467-548: RefactorcreatePnlTicksto reduce code duplicationThe code for creating PnL ticks for different subaccounts is repetitive. Consider refactoring to parameterize the subaccount ID and shared properties.
Here's how you can refactor the function:
async function createPnlTicks( createMainSubaccountPnlTicks: boolean = false, ): Promise<PnlTicksFromDatabase[]> { const createdTicks: PnlTicksFromDatabase[] = []; const subaccounts = [ testConstants.defaultSubaccountId, testConstants.vaultSubaccountId, ]; if (createMainSubaccountPnlTicks) { subaccounts.push(MEGAVAULT_SUBACCOUNT_ID); } for (const subaccountId of subaccounts) { const ticks = await Promise.all([ PnlTicksTable.create({ ...testConstants.defaultPnlTick, subaccountId, }), PnlTicksTable.create({ ...testConstants.defaultPnlTick, subaccountId, blockTime: twoDaysAgo.toISO(), createdAt: twoDaysAgo.toISO(), blockHeight: twoDayBlockHeight, }), PnlTicksTable.create({ ...testConstants.defaultPnlTick, subaccountId, blockTime: twoHoursAgo.toISO(), createdAt: twoHoursAgo.toISO(), blockHeight: twoHourBlockHeight, }), PnlTicksTable.create({ ...testConstants.defaultPnlTick, subaccountId, blockTime: currentTime.toISO(), createdAt: currentTime.toISO(), blockHeight: currentBlockHeight, }), ]); createdTicks.push(...ticks); } return createdTicks; }This refactoring improves maintainability by reducing duplication and making it easier to add or remove subaccounts in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- indexer/packages/postgres/src/constants.ts (2 hunks)
- indexer/services/comlink/tests/controllers/api/v4/vault-controller.test.ts (7 hunks)
- indexer/services/comlink/src/controllers/api/v4/vault-controller.ts (6 hunks)
🔇 Additional comments (9)
indexer/packages/postgres/src/constants.ts (2)
26-26: LGTM: Import statement foruuidfunction.The import statement for the
uuidfunction from the local module is correct and necessary for the new constantMEGAVAULT_SUBACCOUNT_ID.
26-26: Summary: Changes align with PR objectives.The introduced changes (new import and two new constants) are consistent with the PR objectives of enhancing functionality related to the main subaccount's PnL calculations within the megavault system. The
MEGAVAULT_MODULE_ADDRESSandMEGAVAULT_SUBACCOUNT_IDconstants provide necessary identifiers for the megavault functionality, which will be crucial for accurate equity calculations as mentioned in the PR summary.To ensure these constants are used appropriately in the codebase, run the following script:
This will help confirm that the new constants are being utilized as intended across the project.
Also applies to: 134-136
✅ Verification successful
Verified: Constants are being used appropriately across the codebase.
The constants
MEGAVAULT_MODULE_ADDRESSandMEGAVAULT_SUBaccount_IDare correctly imported and utilized in the relevant controller and test files, ensuring they align with the PR objectives to enhance megavault functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of new megavault constants in the codebase. echo "Searching for MEGAVAULT_MODULE_ADDRESS usage:" rg --type typescript "MEGAVAULT_MODULE_ADDRESS" echo "\nSearching for MEGAVAULT_SUBACCOUNT_ID usage:" rg --type typescript "MEGAVAULT_SUBACCOUNT_ID"Length of output: 368
Script:
#!/bin/bash # Description: Verify the usage of new megavault constants in the codebase. echo "Searching for MEGAVAULT_MODULE_ADDRESS usage:" rg "MEGAVAULT_MODULE_ADDRESS" --type ts echo "\nSearching for MEGAVAULT_SUBACCOUNT_ID usage:" rg "MEGAVAULT_SUBACCOUNT_ID" --type tsLength of output: 2084
indexer/services/comlink/src/controllers/api/v4/vault-controller.ts (4)
27-27: Approved: ImportingMEGAVAULT_SUBACCOUNT_IDThe addition of
MEGAVAULT_SUBACCOUNT_IDto the import statement ensures that the constant is available for use in the module.
110-110: Approved: Sorting PnL ticks by block timeThe use of
_.sortByto sortpnlTicksWithCurrentTickbyblockTimeensures that the PnL ticks are chronologically ordered, which is essential for accurate historical data representation.
88-91: Ensure consistent inclusion of the main subaccountWhen calling
getVaultSubaccountPnlTicks, verify that all necessary subaccount IDs, including the main subaccount, are included in other parts of the code where applicable.Run the following script to check for other invocations:
446-453: Verify the uniqueness of the main subaccount's USDC balance retrievalEnsure that the
findUsdcPositionForSubaccountsmethod correctly retrieves the USDC balance for the main subaccount and that there are no discrepancies in the data.Run the following script to validate the USDC balance retrieval:
Expected result: Confirm that the method is called with the correct subaccount ID and that the returned balance is as expected.
✅ Verification successful
USDC balance retrieval is correctly implemented and uniquely defined.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the USDC balance for the main subaccount is retrieved correctly. # Test: Check for multiple entries or missing entries for the main subaccount. ast-grep --lang typescript --pattern $'AssetPositionTable.findUsdcPositionForSubaccounts($_)'Length of output: 537
Script:
#!/bin/bash # Description: Verify the uniqueness and correct usage of MEGAVAULT_SUBACCOUNT_ID and findUsdcPositionForSubaccounts. # Check all definitions of MEGAVAULT_SUBACCOUNT_ID ast-grep --lang typescript --pattern $'const MEGAVAULT_SUBACCOUNT_ID = $_' # Check all usages of findUsdcPositionForSubaccounts to ensure it's only called with MEGAVAULT_SUBACCOUNT_ID ast-grep --lang typescript --pattern $'AssetPositionTable.findUsdcPositionForSubaccounts($_)' | grep 'MEGAVAULT_SUBACCOUNT_ID'Length of output: 306
indexer/services/comlink/__tests__/controllers/api/v4/vault-controller.test.ts (3)
16-17: Imports added correctlyThe addition of
MEGAVAULT_MODULE_ADDRESSandMEGAVAULT_SUBACCOUNT_IDto the imports is appropriate and necessary for the subsequent usage in the test cases.
37-37: Declaration ofmainVaultEquityis appropriateThe declaration of
mainVaultEquitywith a value of10000accurately reflects the equity for the main vault in the test scenarios.
75-80: Creating the main vault subaccount correctlyThe creation of the main vault subaccount with the
MEGAVAULT_MODULE_ADDRESSandsubaccountNumber: 0is correctly implemented and aligns with the test requirements.
indexer/services/comlink/__tests__/controllers/api/v4/vault-controller.test.ts
Show resolved
Hide resolved
vincentwschau
deleted the
vincentc/inlude-main-vault-subaccount-for-equity-pnl
branch
… (backport #2376) (#2378) Co-authored-by: vincentwschau <[email protected]>
Changelist
Get main subaccount PnL ticks + main subaccount USDC balance for equity calculations.
Misc.
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
MEGAVAULT_MODULE_ADDRESSandMEGAVAULT_SUBACCOUNT_IDto enhance vault functionality.Bug Fixes
Refactor