Add current equity as a pnl tick. #2335
Conversation
WalkthroughThe changes introduce a new utility function for retrieving vault positions in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 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: 3
Outside diff range and nitpick comments (1)
indexer/services/comlink/src/controllers/api/v4/vault-controller.ts (1)
90-95: Simplify the calculation ofcurrentEquityYou can streamline the calculation of
currentEquityby directly reducing thevaultPositionswithout the intermediate mapping to strings.Here's a suggested refactor:
-const currentEquity: string = Array.from(vaultPositions.values()) - .map((position: VaultPosition): string => { - return position.equity; - }).reduce((acc: string, curr: string): string => { - return (Big(acc).add(Big(curr))).toFixed(); - }, '0'); +const currentEquity: string = Array.from(vaultPositions.values()) + .reduce((acc: Big, position: VaultPosition) => acc.add(Big(position.equity)), Big(0)) + .toFixed();This makes the code more concise and removes the unnecessary
.map()step.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- indexer/services/comlink/tests/controllers/api/v4/vault-controller.test.ts (8 hunks)
- indexer/services/comlink/src/controllers/api/v4/vault-controller.ts (6 hunks)
Additional comments not posted (6)
indexer/services/comlink/src/controllers/api/v4/vault-controller.ts (2)
26-26: Importing 'big.js' for precise decimal arithmeticThe import of
big.jsis appropriate and necessary for accurate calculation of large decimal numbers when summing equities.
72-85: Concurrent data fetching optimizes performanceUsing
Promise.allto fetchvaultPnlTicks,vaultPositions, andlatestBlockconcurrently improves performance by initiating multiple asynchronous operations simultaneously.indexer/services/comlink/__tests__/controllers/api/v4/vault-controller.test.ts (4)
21-21: Proper import of Big.js for precise calculationsThe import of
Bigfrom'big.js'is necessary for accurate decimal arithmetic in financial computations within the tests.
26-36: Initialization of test constants enhances readabilityThe constants for block heights, times, and vault equities are clearly defined and appropriately typed, improving the readability and maintainability of the test setup.
69-73: Adding the latest block toBlockTableThe inclusion of the latest block with correct time and block height ensures that the test cases consider the most recent state of the blockchain.
76-95: Seeding multiple tables with test dataThe batch seeding of
PerpetualPositionTable,AssetPositionTable, andFundingIndexUpdatesTableis correctly implemented, setting up comprehensive test data for various scenarios.
| ]: [ | ||
| SubaccountFromDatabase[], | ||
| AssetFromDatabase[], | ||
| PerpetualPositionFromDatabase[], | ||
| AssetPositionFromDatabase[], | ||
| MarketFromDatabase[], | ||
| BlockFromDatabase | undefined, | ||
| ] = await Promise.all([ |
There was a problem hiding this comment.
Potential null reference on latestBlock
getVaultPositions obtains latestBlock which may be undefined. Accessing latestBlock.blockHeight without checking if latestBlock is defined can lead to a runtime error.
Add a null check for latestBlock before using it:
if (!latestBlock) {
throw new Error('Failed to retrieve the latest block.');
}Alternatively, handle the case where latestBlock is undefined appropriately.
| const latestFundingIndexMap: FundingIndexMap = await FundingIndexUpdatesTable | ||
| .findFundingIndexMap( | ||
| latestBlock.blockHeight, | ||
| ); | ||
| const assetPositionsBySubaccount: | ||
| { [subaccountId: string]: AssetPositionFromDatabase[] } = _.groupBy( | ||
| assetPositions, | ||
| 'subaccountId', | ||
| ); | ||
| const openPerpetualPositionsBySubaccount: | ||
| { [subaccountId: string]: PerpetualPositionFromDatabase[] } = _.groupBy( | ||
| openPerpetualPositions, | ||
| 'subaccountId', | ||
| ); | ||
| const assetIdToAsset: AssetById = _.keyBy( | ||
| assets, | ||
| AssetColumns.id, | ||
| ); | ||
|
|
||
| const vaultPositionsAndSubaccountId: { | ||
| position: VaultPosition, | ||
| subaccountId: string, | ||
| }[] = await Promise.all( | ||
| subaccounts.map(async (subaccount: SubaccountFromDatabase) => { | ||
| const perpetualMarket: PerpetualMarketFromDatabase | undefined = perpetualMarketRefresher | ||
| .getPerpetualMarketFromClobPairId(vaultSubaccounts[subaccount.id]); | ||
| if (perpetualMarket === undefined) { | ||
| throw new Error( | ||
| `Vault clob pair id ${vaultSubaccounts[subaccount.id]} does not correspond to a ` + | ||
| 'perpetual market.'); | ||
| } | ||
| const lastUpdatedFundingIndexMap: FundingIndexMap = await FundingIndexUpdatesTable | ||
| .findFundingIndexMap( | ||
| subaccount.updatedAtHeight, | ||
| ); | ||
|
|
||
| const subaccountResponse: SubaccountResponseObject = getSubaccountResponse( | ||
| subaccount, | ||
| openPerpetualPositionsBySubaccount[subaccount.id] || [], | ||
| assetPositionsBySubaccount[subaccount.id] || [], | ||
| assets, | ||
| markets, | ||
| perpetualMarketRefresher.getPerpetualMarketsMap(), | ||
| latestBlock.blockHeight, | ||
| latestFundingIndexMap, | ||
| lastUpdatedFundingIndexMap, |
There was a problem hiding this comment.
Ensure latestBlock is defined before accessing its properties
In the getVaultPositions function, you are accessing latestBlock.blockHeight and passing latestBlock.blockHeight and latestBlock.time to other functions without verifying that latestBlock is not undefined.
Before using latestBlock, add a check to confirm it is defined:
if (!latestBlock) {
throw new Error('Failed to retrieve the latest block.');
}This will prevent potential runtime errors due to null or undefined reference.
| const finalTick: PnlTicksFromDatabase = { | ||
| ...createdPnlTicks[expectedTicksIndex[expectedTicksIndex.length - 1]], | ||
| equity: Big(vault1Equity).toFixed(), | ||
| blockHeight: latestBlockHeight, | ||
| blockTime: latestTime.toISO(), | ||
| createdAt: latestTime.toISO(), | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor repetitive finalTick object construction
The construction of finalTick objects is repeated across multiple test cases with similar logic. Consider refactoring this code into a helper function to reduce duplication and improve maintainability.
For example, you could create a helper function like:
function createFinalTick(
baseTick: PnlTicksFromDatabase,
equity: Big,
blockHeight: string,
blockTime: DateTime,
): PnlTicksFromDatabase {
return {
...baseTick,
equity: equity.toFixed(),
blockHeight,
blockTime: blockTime.toISO(),
createdAt: blockTime.toISO(),
};
}Then use it in your tests to create finalTick instances:
const finalTick = createFinalTick(
createdPnlTicks[expectedIndex],
Big(vault1Equity),
latestBlockHeight,
latestTime,
);This refactoring will make your tests cleaner and more maintainable.
Also applies to: 178-184, 223-229, 268-281
|
@Mergifyio backport release/indexer/v7.x |
✅ Backports have been created
|
(cherry picked from commit 78de680)
Co-authored-by: vincentwschau <[email protected]>
Changelist
Get current equity of vaults for megavault PnL.
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
Bug Fixes
Documentation