Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CT-913] Fix issue where pnl wasn't being calculated for some subaccounts #1662

Merged
merged 2 commits into from
Jun 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -184,14 +184,27 @@ describe('PnlTicks store', () => {
}),
]);

const latestBlocktime: string = await PnlTicksTable.findLatestProcessedBlocktime();
const {
maxBlockTime, count,
}: {
maxBlockTime: string,
count: number
} = await PnlTicksTable.findLatestProcessedBlocktimeAndCount();

expect(latestBlocktime).toEqual(blockTime);
expect(maxBlockTime).toEqual(blockTime);
expect(count).toEqual(2);
});

it('Successfully finds latest block time without any pnl ticks', async () => {
const latestBlocktime: string = await PnlTicksTable.findLatestProcessedBlocktime();
expect(latestBlocktime).toEqual(ZERO_TIME_ISO_8601);
const {
maxBlockTime, count,
}: {
maxBlockTime: string,
count: number
} = await PnlTicksTable.findLatestProcessedBlocktimeAndCount();

expect(maxBlockTime).toEqual(ZERO_TIME_ISO_8601);
expect(count).toEqual(0);
});

it('createMany PnlTicks, find most recent pnl ticks for each account', async () => {
Expand Down
17 changes: 12 additions & 5 deletions indexer/packages/postgres/src/stores/pnl-ticks-table.ts
Original file line number Diff line number Diff line change
Expand Up @@ -212,17 +212,24 @@ function convertPnlTicksFromDatabaseToPnlTicksCreateObject(
return _.omit(pnlTicksFromDatabase, PnlTicksColumns.id);
}

export async function findLatestProcessedBlocktime(): Promise<string> {
export async function findLatestProcessedBlocktimeAndCount(): Promise<{
maxBlockTime: string,
count: number,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

im a little confused, why are we counting the number or rows in pnl_ticks? How does that relate to "all subaccounts have been processed"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is used to determine whether or not to run the pnl task.

Previously, we were looking at the last block time in the pnl table to determine whether or not to run the pnl task. This worked, but then we imposed a limit on the number of subaccounts/run to compute pnl for. What happened was we started to skip computing pnl on certain subaccounts because we were hitting the limit/run. So, this PR changes this check to see if the last run computed pnl for LIMIT # of subaccounts, and if so, proceed with the task & check the pnl timestamps of the subaccounts individually.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

were we already checking the pnl timestamps fo subaccounts individually and reprocessing data? I don't see that logic

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...getAccountsToUpdate(accountToLastUpdatedBlockTime, blockTime),

}> {
const result: {
rows: [{ max: string }]
rows: [{ max: string, count: number }]
} = await knexReadReplica.getConnection().raw(
`
SELECT MAX("blockTime")
SELECT MAX("blockTime") as max, COUNT(*) as count
FROM "pnl_ticks"
`
,
) as unknown as { rows: [{ max: string }] };
return result.rows[0].max || ZERO_TIME_ISO_8601;
) as unknown as { rows: [{ max: string, count: number }] };

return {
maxBlockTime: result.rows[0].max || ZERO_TIME_ISO_8601,
count: Number(result.rows[0].count) || 0,
};
}

export async function findMostRecentPnlTickForEachAccount(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -277,9 +277,16 @@ describe('create-pnl-ticks', () => {
...testConstants.defaultPnlTick,
blockTime: testConstants.defaultBlock.time,
});
const blockTimeIsoString: string = await PnlTicksTable.findLatestProcessedBlocktime();
const {
maxBlockTime,
// eslint-disable-next-line @typescript-eslint/no-unused-vars
count,
}: {
maxBlockTime: string,
count: number,
} = await PnlTicksTable.findLatestProcessedBlocktimeAndCount();

const date: number = Date.parse(blockTimeIsoString).valueOf();
const date: number = Date.parse(maxBlockTime).valueOf();
jest.spyOn(Date, 'now').mockImplementation(() => date);
jest.spyOn(DateTime, 'utc').mockImplementation(() => dateTime);
jest.spyOn(logger, 'info');
Expand All @@ -305,8 +312,48 @@ describe('create-pnl-ticks', () => {
expect(pnlTicks.length).toEqual(1);
expect(logger.info).toHaveBeenCalledWith(
expect.objectContaining({
message: 'Skipping run because update interval has not been reached',
message: 'Skipping run because update interval has not been reached and all subaccounts have been processed',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: create separate test to check the count condition

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

}),
);
});

it(
'calculates pnl per subaccount if last run hit subaccount limit',
async () => {
const pnlTicksHelper = require('../../src/helpers/pnl-ticks-helper');
const getPnlTicksCreateObjectsSpy = jest.spyOn(pnlTicksHelper, 'getPnlTicksCreateObjects');
config.PNL_TICK_UPDATE_INTERVAL_MS = 3_600_000;
config.PNL_TICK_MAX_ACCOUNTS_PER_RUN = 1;
await PnlTicksTable.create({
...testConstants.defaultPnlTick,
blockTime: testConstants.defaultBlock.time,
});
const {
maxBlockTime,
// eslint-disable-next-line @typescript-eslint/no-unused-vars
count,
}: {
maxBlockTime: string,
count: number,
} = await PnlTicksTable.findLatestProcessedBlocktimeAndCount();

const date: number = Date.parse(maxBlockTime).valueOf();
jest.spyOn(Date, 'now').mockImplementation(() => date);
jest.spyOn(DateTime, 'utc').mockImplementation(() => dateTime);
jest.spyOn(logger, 'info');
await LatestAccountPnlTicksCache.set(
pnlTickForSubaccounts,
redisClient,
);
await Promise.all([
PerpetualPositionTable.create(testConstants.defaultPerpetualPosition),
PerpetualPositionTable.create({
...testConstants.defaultPerpetualPosition,
perpetualId: testConstants.defaultPerpetualMarket2.id,
openEventId: testConstants.defaultTendermintEventId2,
}),
]);
await createPnlTicksTask();
expect(getPnlTicksCreateObjectsSpy).toHaveBeenCalledTimes(1);
});
});
20 changes: 13 additions & 7 deletions indexer/services/roundtable/src/tasks/create-pnl-ticks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,26 +17,32 @@ export default async function runTask(): Promise<void> {
const startGetNewTicks: number = Date.now();
const [
block,
pnlTickLatestBlocktime,
{
maxBlockTime,
count,
},
]: [
BlockFromDatabase,
string,
{
maxBlockTime: string,
count: number,
},
] = await Promise.all([
BlockTable.getLatest({ readReplica: true }),
PnlTicksTable.findLatestProcessedBlocktime(),
PnlTicksTable.findLatestProcessedBlocktimeAndCount(),
]);
const latestBlockTime: string = block.time;
const latestBlockHeight: string = block.blockHeight;
// Check that the latest block time is within PNL_TICK_UPDATE_INTERVAL_MS of the last computed
// PNL tick block time.
if (
Date.parse(latestBlockTime) - normalizeStartTime(new Date(pnlTickLatestBlocktime)).getTime() <
config.PNL_TICK_UPDATE_INTERVAL_MS
Date.parse(latestBlockTime) - normalizeStartTime(new Date(maxBlockTime)).getTime() <
config.PNL_TICK_UPDATE_INTERVAL_MS && count < config.PNL_TICK_MAX_ACCOUNTS_PER_RUN
) {
logger.info({
at: 'create-pnl-ticks#runTask',
message: 'Skipping run because update interval has not been reached',
pnlTickLatestBlocktime,
message: 'Skipping run because update interval has not been reached and all subaccounts have been processed',
pnlTickLatestBlocktime: maxBlockTime,
latestBlockTime,
threshold: config.PNL_TICK_UPDATE_INTERVAL_MS,
});
Expand Down
Loading