[CT-913] Fix issue where pnl wasn't being calculated for some subaccounts #1662
Conversation
WalkthroughThe changes enhance the Changes
Sequence Diagram(s)sequenceDiagram
participant TaskRunner
participant PnlTicksTable
participant Logger
TaskRunner->>PnlTicksTable: findLatestProcessedBlocktimeAndCount()
PnlTicksTable-->>TaskRunner: { maxBlockTime, count }
TaskRunner->>Logger: Log { maxBlockTime, count }
alt If conditions met
TaskRunner->>PnlTicksTable: Process PNL Ticks
else If conditions not met
TaskRunner->>Logger: Skip processing
end
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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: 0
Outside diff range and nitpick comments (1)
indexer/services/roundtable/src/tasks/create-pnl-ticks.ts (1)
Line range hint
1-87: Overall, therunTaskfunction appears well-structured and the changes align with the PR's objectives. However, consider adding error handling for theawaitcalls within thePromise.allto prevent potential unhandled promise rejections.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- indexer/packages/postgres/tests/stores/pnl-ticks-table.test.ts (1 hunks)
- indexer/packages/postgres/src/stores/pnl-ticks-table.ts (1 hunks)
- indexer/services/roundtable/tests/tasks/create-pnl-ticks.test.ts (2 hunks)
- indexer/services/roundtable/src/tasks/create-pnl-ticks.ts (1 hunks)
Additional comments not posted (6)
indexer/services/roundtable/src/tasks/create-pnl-ticks.ts (3)
20-29: The destructuring assignment has been updated to accommodate the new methodfindLatestProcessedBlocktimeAndCount. Ensure that all usages ofmaxBlockTimeandcountare consistent throughout the codebase.
39-40: The condition for skipping a run has been updated to include a check on the count of accounts processed. This is a logical change given the PR's objectives. Ensure that the values forPNL_TICK_UPDATE_INTERVAL_MSandPNL_TICK_MAX_ACCOUNTS_PER_RUNare configured appropriately in the environment.
44-45: The log message has been updated to include information about subaccounts processing. This enhances transparency and debugging capabilities. Ensure that the log levels are appropriately set for production environments.indexer/packages/postgres/src/stores/pnl-ticks-table.ts (1)
215-232: The implementation offindLatestProcessedBlocktimeAndCountcorrectly fetches the maximum block time and count from thepnl_tickstable. Ensure that the SQL query is optimized for performance, especially if thepnl_tickstable grows significantly in size.indexer/packages/postgres/__tests__/stores/pnl-ticks-table.test.ts (1)
187-207: The test cases forfindLatestProcessedBlocktimeAndCountcorrectly assert themaxBlockTimeandcountvalues. Ensure that edge cases, such as when thepnl_tickstable is empty, are also covered to prevent potential issues in production.indexer/services/roundtable/__tests__/tasks/create-pnl-ticks.test.ts (1)
Line range hint
280-315: The test case for handling thePNL_TICK_UPDATE_INTERVAL_MScondition correctly simulates the scenario where the update interval has not been reached. Ensure that the mock implementations forDate.nowandDateTime.utcare correctly reset after each test to prevent side effects in other tests.
dydxwill
changed the title
| @@ -305,7 +312,7 @@ 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', | |||
There was a problem hiding this comment.
nit: create separate test to check the count condition
| export async function findLatestProcessedBlocktime(): Promise<string> { | ||
| export async function findLatestProcessedBlocktimeAndCount(): Promise<{ | ||
| maxBlockTime: string, | ||
| count: number, |
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
were we already checking the pnl timestamps fo subaccounts individually and reprocessing data? I don't see that logic
There was a problem hiding this comment.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- indexer/services/roundtable/tests/tasks/create-pnl-ticks.test.ts (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- indexer/services/roundtable/tests/tasks/create-pnl-ticks.test.ts
|
@Mergifyio backport release/indexer/v5.x |
✅ Backports have been created
|
…unts (backport #1662) (#1800) Co-authored-by: dydxwill <[email protected]>
Changelist
Fix issue where pnl wasn't being calculated for some subaccounts
Test Plan
Tested in env
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
Refactor
maxBlockTimeandcountfor better data insights.Tests