Fix bug with PnL aggregation. (backport #2446) (#2451)

Co-authored-by: vincentwschau <[email protected]>
dydxprotocol · Oct 3, 2024 · c182cd9 · c182cd9
1 parent 0b9e8a2
commit c182cd9
Show file tree

Hide file tree

Showing 4 changed files with 90 additions and 74 deletions.
diff --git a/indexer/services/comlink/__tests__/lib/helpers.test.ts b/indexer/services/comlink/__tests__/lib/helpers.test.ts
@@ -44,7 +44,7 @@ import {
   getPerpetualPositionsWithUpdatedFunding,
   initializePerpetualPositionsWithFunding,
   getChildSubaccountNums,
-  aggregatePnlTicks,
+  aggregateHourlyPnlTicks,
   getSubaccountResponse,
 } from '../../src/lib/helpers';
 import _ from 'lodash';
@@ -60,6 +60,7 @@ import {
 } from '@dydxprotocol-indexer/postgres/build/__tests__/helpers/constants';
 import { AssetPositionsMap, PerpetualPositionWithFunding, SubaccountResponseObject } from '../../src/types';
 import { ZERO, ZERO_USDC_POSITION } from '../../src/lib/constants';
+import { DateTime } from 'luxon';
 
 describe('helpers', () => {
   afterEach(async () => {
@@ -833,7 +834,7 @@ describe('helpers', () => {
     });
   });
 
-  describe('aggregatePnlTicks', () => {
+  describe('aggregateHourlyPnlTicks', () => {
     it('aggregates single pnl tick', () => {
       const pnlTick: PnlTicksFromDatabase = {
         ...testConstants.defaultPnlTick,
@@ -843,10 +844,12 @@ describe('helpers', () => {
         ),
       };
 
-      const aggregatedPnlTicks: Map<number, PnlTicksFromDatabase> = aggregatePnlTicks([pnlTick]);
+      const aggregatedPnlTicks: PnlTicksFromDatabase[] = aggregateHourlyPnlTicks([pnlTick]);
       expect(
-        aggregatedPnlTicks.get(parseInt(pnlTick.blockHeight, 10)),
-      ).toEqual(expect.objectContaining({ ...testConstants.defaultPnlTick }));
+        aggregatedPnlTicks,
+      ).toEqual(
+        [expect.objectContaining({ ...testConstants.defaultPnlTick })],
+      );
     });
 
     it('aggregates multiple pnl ticks same height', () => {
@@ -865,38 +868,58 @@ describe('helpers', () => {
         ),
       };
       const blockHeight2: string = '80';
+      const blockTime2: string = DateTime.fromISO(pnlTick.createdAt).plus({ hour: 1 }).toISO();
       const pnlTick3: PnlTicksFromDatabase = {
         ...testConstants.defaultPnlTick,
         id: PnlTicksTable.uuid(
           testConstants.defaultPnlTick.subaccountId,
-          testConstants.defaultPnlTick.createdAt,
+          blockTime2,
         ),
         blockHeight: blockHeight2,
+        blockTime: blockTime2,
+        createdAt: blockTime2,
+      };
+      const blockHeight3: string = '81';
+      const blockTime3: string = DateTime.fromISO(pnlTick.createdAt).plus({ minute: 61 }).toISO();
+      const pnlTick4: PnlTicksFromDatabase = {
+        ...testConstants.defaultPnlTick,
+        id: PnlTicksTable.uuid(
+          testConstants.defaultPnlTick.subaccountId,
+          blockTime3,
+        ),
+        equity: '1',
+        totalPnl: '2',
+        netTransfers: '3',
+        blockHeight: blockHeight3,
+        blockTime: blockTime3,
+        createdAt: blockTime3,
       };
 
-      const aggregatedPnlTicks: Map<number, PnlTicksFromDatabase> = aggregatePnlTicks(
-        [pnlTick, pnlTick2, pnlTick3],
+      const aggregatedPnlTicks: PnlTicksFromDatabase[] = aggregateHourlyPnlTicks(
+        [pnlTick, pnlTick2, pnlTick3, pnlTick4],
       );
-      // Combined pnl tick at initial block height.
-      expect(
-        aggregatedPnlTicks.get(parseInt(pnlTick.blockHeight, 10)),
-      ).toEqual(expect.objectContaining({
-        equity: (parseFloat(testConstants.defaultPnlTick.equity) +
+      expect(aggregatedPnlTicks).toEqual(
+        expect.arrayContaining([
+          // Combined pnl tick at initial hour
+          expect.objectContaining({
+            equity: (parseFloat(testConstants.defaultPnlTick.equity) +
             parseFloat(pnlTick2.equity)).toString(),
-        totalPnl: (parseFloat(testConstants.defaultPnlTick.totalPnl) +
-            parseFloat(pnlTick2.totalPnl)).toString(),
-        netTransfers: (parseFloat(testConstants.defaultPnlTick.netTransfers) +
-            parseFloat(pnlTick2.netTransfers)).toString(),
-        createdAt: testConstants.defaultPnlTick.createdAt,
-        blockHeight: testConstants.defaultPnlTick.blockHeight,
-        blockTime: testConstants.defaultPnlTick.blockTime,
-      }));
-      // Single pnl tick at second block height.
-      expect(
-        aggregatedPnlTicks.get(parseInt(blockHeight2, 10)),
-      ).toEqual(expect.objectContaining({
-        ...pnlTick3,
-      }));
+            totalPnl: (parseFloat(testConstants.defaultPnlTick.totalPnl) +
+                parseFloat(pnlTick2.totalPnl)).toString(),
+            netTransfers: (parseFloat(testConstants.defaultPnlTick.netTransfers) +
+                parseFloat(pnlTick2.netTransfers)).toString(),
+          }),
+          // Combined pnl tick at initial hour + 1 hour and initial hour + 1 hour, 1 minute
+          expect.objectContaining({
+            equity: (parseFloat(pnlTick3.equity) +
+            parseFloat(pnlTick4.equity)).toString(),
+            totalPnl: (parseFloat(pnlTick3.totalPnl) +
+                parseFloat(pnlTick4.totalPnl)).toString(),
+            netTransfers: (parseFloat(pnlTick3.netTransfers) +
+                parseFloat(pnlTick4.netTransfers)).toString(),
+          }),
+        ]),
+      );
     });
   });
 });
diff --git a/indexer/services/comlink/src/controllers/api/v4/historical-pnl-controller.ts b/indexer/services/comlink/src/controllers/api/v4/historical-pnl-controller.ts
@@ -19,7 +19,7 @@ import { getReqRateLimiter } from '../../../caches/rate-limiters';
 import config from '../../../config';
 import { complianceAndGeoCheck } from '../../../lib/compliance-and-geo-check';
 import { NotFoundError } from '../../../lib/errors';
-import { aggregatePnlTicks, getChildSubaccountIds, handleControllerError } from '../../../lib/helpers';
+import { aggregateHourlyPnlTicks, getChildSubaccountIds, handleControllerError } from '../../../lib/helpers';
 import { rateLimiterMiddleware } from '../../../lib/rate-limit';
 import {
   CheckLimitAndCreatedBeforeOrAtAndOnOrAfterSchema,
@@ -156,10 +156,10 @@ class HistoricalPnlController extends Controller {
     }
 
     // aggregate pnlTicks for all subaccounts grouped by blockHeight
-    const aggregatedPnlTicks: Map<number, PnlTicksFromDatabase> = aggregatePnlTicks(pnlTicks);
+    const aggregatedPnlTicks: PnlTicksFromDatabase[] = aggregateHourlyPnlTicks(pnlTicks);
 
     return {
-      historicalPnl: Array.from(aggregatedPnlTicks.values()).map(
+      historicalPnl: aggregatedPnlTicks.map(
         (pnlTick: PnlTicksFromDatabase) => {
           return pnlTicksToResponseObject(pnlTick);
         }),

diff --git a/indexer/services/comlink/src/controllers/api/v4/vault-controller.ts b/indexer/services/comlink/src/controllers/api/v4/vault-controller.ts
@@ -38,7 +38,7 @@ import {
 import { getReqRateLimiter } from '../../../caches/rate-limiters';
 import config from '../../../config';
 import {
-  aggregatePnlTicks,
+  aggregateHourlyPnlTicks,
   getSubaccountResponse,
   handleControllerError,
 } from '../../../lib/helpers';
@@ -93,7 +93,7 @@ class VaultController extends Controller {
     ]);
 
     // aggregate pnlTicks for all vault subaccounts grouped by blockHeight
-    const aggregatedPnlTicks: Map<number, PnlTicksFromDatabase> = aggregatePnlTicks(vaultPnlTicks);
+    const aggregatedPnlTicks: PnlTicksFromDatabase[] = aggregateHourlyPnlTicks(vaultPnlTicks);
 
     const currentEquity: string = Array.from(vaultPositions.values())
       .map((position: VaultPosition): string => {
@@ -473,46 +473,34 @@ function getPnlTicksWithCurrentTick(
 }
 
 /**
- * Takes in a map of block heights to PnlTicks and filters out the closest pnl tick per interval.
- * @param pnlTicksByBlock Map of block number to pnl tick.
+ * Takes in an array of PnlTicks and filters out the closest pnl tick per interval.
+ * @param pnlTicks Array of pnl ticks.
  * @param resolution Resolution of interval.
  * @returns Array of PnlTicksFromDatabase, one per interval.
  */
 function filterOutIntervalTicks(
-  pnlTicksByBlock: Map<number, PnlTicksFromDatabase>,
+  pnlTicks: PnlTicksFromDatabase[],
   resolution: PnlTickInterval,
 ): PnlTicksFromDatabase[] {
-  // Track block to block time.
-  const blockToBlockTime: Map<number, DateTime> = new Map();
-  // Track start of days to closest block by block time.
-  const blocksPerInterval: Map<string, number> = new Map();
-  // Track start of days to closest Pnl tick.
+  // Track start of intervals to closest Pnl tick.
   const ticksPerInterval: Map<string, PnlTicksFromDatabase> = new Map();
-  pnlTicksByBlock.forEach((pnlTick: PnlTicksFromDatabase, block: number): void => {
+  pnlTicks.forEach((pnlTick: PnlTicksFromDatabase): void => {
     const blockTime: DateTime = DateTime.fromISO(pnlTick.blockTime).toUTC();
-    blockToBlockTime.set(block, blockTime);
 
     const startOfInterval: DateTime = blockTime.toUTC().startOf(resolution);
     const startOfIntervalStr: string = startOfInterval.toISO();
-    const startOfIntervalBlock: number | undefined = blocksPerInterval.get(startOfIntervalStr);
-    // No block for the start of interval, set this block as the block for the interval.
-    if (startOfIntervalBlock === undefined) {
-      blocksPerInterval.set(startOfIntervalStr, block);
-      ticksPerInterval.set(startOfIntervalStr, pnlTick);
-      return;
-    }
-
-    const startOfDayBlockTime: DateTime | undefined = blockToBlockTime.get(startOfIntervalBlock);
-    // Invalid block set as start of day block, set this block as the block for the day.
-    if (startOfDayBlockTime === undefined) {
-      blocksPerInterval.set(startOfIntervalStr, block);
+    const tickForInterval: PnlTicksFromDatabase | undefined = ticksPerInterval.get(
+      startOfIntervalStr,
+    );
+    // No tick for the start of interval, set this tick as the block for the interval.
+    if (tickForInterval === undefined) {
       ticksPerInterval.set(startOfIntervalStr, pnlTick);
       return;
     }
+    const tickPerIntervalBlockTime: DateTime = DateTime.fromISO(tickForInterval.blockTime);
 
-    // This block is closer to the start of the day, set it as the block for the day.
-    if (blockTime.diff(startOfInterval) < startOfDayBlockTime.diff(startOfInterval)) {
-      blocksPerInterval.set(startOfIntervalStr, block);
+    // This tick is closer to the start of the interval, set it as the tick for the interval.
+    if (blockTime.diff(startOfInterval) < tickPerIntervalBlockTime.diff(startOfInterval)) {
       ticksPerInterval.set(startOfIntervalStr, pnlTick);
     }
   });

diff --git a/indexer/services/comlink/src/lib/helpers.ts b/indexer/services/comlink/src/lib/helpers.ts
@@ -28,6 +28,7 @@ import {
 import Big from 'big.js';
 import express from 'express';
 import _ from 'lodash';
+import { DateTime } from 'luxon';
 
 import config from '../config';
 import {
@@ -672,32 +673,36 @@ export function getSubaccountResponse(
 /* ------- PNL HELPERS ------- */
 
 /**
- * Aggregates a list of PnL ticks, combining any PnL ticks for the same blockheight by summing
+ * Aggregates a list of PnL ticks, combining any PnL ticks for the same hour by summing
  * the equity, totalPnl, and net transfers.
  * Returns a map of block height to the resulting PnL tick.
  * @param pnlTicks
  * @returns
  */
-export function aggregatePnlTicks(
+export function aggregateHourlyPnlTicks(
   pnlTicks: PnlTicksFromDatabase[],
-): Map<number, PnlTicksFromDatabase> {
-  const aggregatedPnlTicks: Map<number, PnlTicksFromDatabase> = new Map();
+): PnlTicksFromDatabase[] {
+  const hourlyPnlTicks: Map<string, PnlTicksFromDatabase> = new Map();
   for (const pnlTick of pnlTicks) {
-    const blockHeight: number = parseInt(pnlTick.blockHeight, 10);
-    if (aggregatedPnlTicks.has(blockHeight)) {
-      const currentPnlTick: PnlTicksFromDatabase = aggregatedPnlTicks.get(
-        blockHeight,
+    const truncatedTime: string = DateTime.fromISO(pnlTick.createdAt).startOf('hour').toISO();
+    if (hourlyPnlTicks.has(truncatedTime)) {
+      const aggregatedTick: PnlTicksFromDatabase = hourlyPnlTicks.get(
+        truncatedTime,
       ) as PnlTicksFromDatabase;
-      aggregatedPnlTicks.set(blockHeight, {
-        ...currentPnlTick,
-        equity: (parseFloat(currentPnlTick.equity) + parseFloat(pnlTick.equity)).toString(),
-        totalPnl: (parseFloat(currentPnlTick.totalPnl) + parseFloat(pnlTick.totalPnl)).toString(),
-        netTransfers: (parseFloat(currentPnlTick.netTransfers) +
-            parseFloat(pnlTick.netTransfers)).toString(),
-      });
+      hourlyPnlTicks.set(
+        truncatedTime,
+        {
+          ...aggregatedTick,
+          equity: (parseFloat(aggregatedTick.equity) + parseFloat(pnlTick.equity)).toString(),
+          totalPnl: (parseFloat(aggregatedTick.totalPnl) + parseFloat(pnlTick.totalPnl)).toString(),
+          netTransfers: (
+            parseFloat(aggregatedTick.netTransfers) + parseFloat(pnlTick.netTransfers)
+          ).toString(),
+        },
+      );
     } else {
-      aggregatedPnlTicks.set(blockHeight, pnlTick);
+      hourlyPnlTicks.set(truncatedTime, pnlTick);
     }
   }
-  return aggregatedPnlTicks;
+  return Array.from(hourlyPnlTicks.values());
 }