[CT-970] Add debug logs to roundtable task, fix case where marketId!=perpetualId #1786
Conversation
|
Warning Review failedThe pull request is closed. WalkthroughThis update introduces significant enhancements and new test scenarios for the Changes
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 (
|
dydxwill
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- .github/workflows/indexer-build-and-push-mainnet.yml (1 hunks)
- indexer/services/roundtable/tests/helpers/pnl-ticks-helper.test.ts (1 hunks)
- indexer/services/roundtable/src/helpers/pnl-ticks-helper.ts (7 hunks)
Additional comments not posted (4)
.github/workflows/indexer-build-and-push-mainnet.yml (1)
6-6: Confirm appropriateness of new branch trigger for workflow.The addition of the
'wl/pnldbg2'branch to the workflow triggers is noted. Please confirm that this branch is intended for changes that should trigger Mainnet deployments.indexer/services/roundtable/src/helpers/pnl-ticks-helper.ts (2)
76-101: Review added logging statements for clarity and necessity.Logging has been added to
getPnlTicksCreateObjectsto provide insights into the process of creating PNL ticks. Ensure that these logs provide clear, actionable information and do not expose sensitive data.
Line range hint
316-388: Validate new logging ingetNewPnlTickfor utility and security.The extensive logging in
getNewPnlTickis useful for debugging but ensure it does not lead to potential leakage of sensitive information. Validate that the logged data is necessary and properly sanitized.indexer/services/roundtable/__tests__/helpers/pnl-ticks-helper.test.ts (1)
351-381: Review new test case for handling missing market prices.The new test case
calculateEquity with missing priceis designed to test the equity calculation when a market price is missing. Ensure this test adequately covers all edge cases related to missing market prices.
| error, | ||
| account, | ||
| pnlTicksToBeCreatedAt, | ||
| blockHeight, | ||
| blockTime, | ||
| }); | ||
| } | ||
| }); | ||
| logger.info({ | ||
| at: 'pnl-ticks-helper#getPnlTicksCreateObjects', | ||
| message: 'New ticks to create', | ||
| subaccountIds: _.map(newTicksToCreate, 'subaccountId'), | ||
| }); |
There was a problem hiding this comment.
Ensure error handling includes necessary details for debugging.
The error logging within getPnlTicksCreateObjects should include all relevant information to diagnose issues effectively. Consider adding more context if needed.
- error,
- account,
- pnlTicksToBeCreatedAt,
- blockHeight,
- blockTime,
+ error: error.message,
+ account: account.id, # Assuming `account` is an object
+ pnlTicksToBeCreatedAt: pnlTicksToBeCreatedAt.toISO(),
+ blockHeight: blockHeight.toString(),
+ blockTime: blockTime.toString(),Committable suggestion was skipped due to low confidence.
dydxwill
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- indexer/services/roundtable/tests/helpers/pnl-ticks-helper.test.ts (4 hunks)
- indexer/services/roundtable/src/helpers/pnl-ticks-helper.ts (8 hunks)
- indexer/services/roundtable/src/tasks/create-pnl-ticks.ts (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- indexer/services/roundtable/src/helpers/pnl-ticks-helper.ts
Additional comments not posted (3)
indexer/services/roundtable/src/tasks/create-pnl-ticks.ts (1)
8-8: Approved addition ofperpetualMarketRefresherimport.The import and usage of
perpetualMarketRefresherare crucial for ensuring that market data is up-to-date before processing PnL ticks, aligning with the PR's objectives to fix related issues.indexer/services/roundtable/__tests__/helpers/pnl-ticks-helper.test.ts (2)
20-24: Approved new imports and test setup enhancements.The addition of new imports and the update call in the
beforeEachhook are appropriate for the new test scenarios introduced. Ensure thatupdatePerpetualMarkets()is effectively mocked or isolated during testing to prevent unintended side effects.
359-417: Approved new test case forperpetualId/marketIdmismatch scenario.The test case is well-structured and addresses a critical scenario. Ensure comprehensive validation of the mismatch handling to cover potential edge cases that might not be represented by the current test setup.
Verification successful
Approved new test case for
perpetualId/marketIdmismatch scenario.The test case is well-structured and addresses a critical scenario. The search results indicate that various edge cases involving
perpetualIdare covered in the tests. Ensure comprehensive validation of the mismatch handling to cover potential edge cases that might not be represented by the current test setup.
indexer/services/roundtable/__tests__/helpers/pnl-ticks-helper.test.tsindexer/services/roundtable/__tests__/tasks/market-updater.test.tsindexer/services/roundtable/__tests__/tasks/create-pnl-ticks.test.tsindexer/services/ender/__tests__/handlers/order-fills/liquidation-handler.test.tsindexer/services/ender/__tests__/handlers/funding-handler.test.tsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all edge cases for `perpetualId/marketId` mismatch are covered in tests. rg --type py "perpetualId" "*/__tests__/*"Length of output: 96
Script:
#!/bin/bash # Verify that all edge cases for `perpetualId/marketId` mismatch are covered in tests. rg --type ts "perpetualId" "*/__tests__/*"Length of output: 96
Script:
#!/bin/bash # Locate all test files in the repository and search for `perpetualId`. fd --type f --name "*.test.ts" | xargs rg "perpetualId"Length of output: 107018
| @@ -53,6 +54,7 @@ export default async function runTask(): Promise<void> { | |||
| const txId: number = await Transaction.start(); | |||
| let newTicksToCreate: PnlTicksCreateObject[] = []; | |||
| try { | |||
| await perpetualMarketRefresher.updatePerpetualMarkets(); | |||
There was a problem hiding this comment.
Tip
Codebase Verification
Ensure proper error handling and transaction management when updating perpetual markets.
The function updatePerpetualMarkets does not perform any write operations that could affect the transaction's atomicity. However, consider wrapping the call to updatePerpetualMarkets in a try-catch block to handle potential exceptions explicitly and maintain clean transaction boundaries.
- File:
indexer/services/roundtable/src/tasks/create-pnl-ticks.ts - Line: 57
Analysis chain
Ensure proper error handling and transaction management when updating perpetual markets.
The addition of perpetualMarketRefresher.updatePerpetualMarkets() within a transaction is a critical update. Ensure that this method does not have side effects that could affect the transaction's atomicity.
Consider wrapping the call to updatePerpetualMarkets() in a try-catch block to handle potential exceptions explicitly and maintain clean transaction boundaries.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that `updatePerpetualMarkets` does not perform any write operations that could affect the transaction's integrity.
ast-grep --lang typescript --pattern $'async function updatePerpetualMarkets() {
$$$
}'
Length of output: 90
Script:
#!/bin/bash
# Searching for the definition of `updatePerpetualMarkets` to inspect its implementation
rg 'updatePerpetualMarkets' --glob '*.{ts,js}'
Length of output: 9773
Script:
#!/bin/bash
# Extracting the implementation of `updatePerpetualMarkets` from the identified file
ast-grep --lang typescript --pattern $'export async function updatePerpetualMarkets($_): Promise<void> {
$$$
}' indexer/packages/postgres/src/loops/perpetual-market-refresher.ts
Length of output: 2064
| @@ -73,6 +80,7 @@ describe('pnl-ticks-helper', () => { | |||
|
|
|||
| beforeEach(async () => { | |||
| await testMocks.seedData(); | |||
| await perpetualMarketRefresher.updatePerpetualMarkets(); | |||
There was a problem hiding this comment.
Tip
Codebase Verification
Ensure isolation of market updates in test setup.
The updatePerpetualMarkets function is called in the test files but is not explicitly mocked. To prevent actual database or network interactions during tests, ensure that this function is properly mocked.
indexer/services/roundtable/__tests__/helpers/pnl-ticks-helper.test.tsindexer/services/roundtable/__tests__/tasks/remove-expired-orders.test.ts
Analysis chain
Ensure isolation of market updates in test setup.
Calling updatePerpetualMarkets() in the beforeEach hook is crucial for setting a consistent state. However, ensure this is properly mocked to prevent actual database or network interactions during tests.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that `updatePerpetualMarkets` is mocked in tests to prevent real interactions.
rg --type py "updatePerpetualMarkets" "*/__tests__/*"
Length of output: 107
Script:
#!/bin/bash
# Verify that `updatePerpetualMarkets` is mocked in tests to prevent real interactions.
rg "updatePerpetualMarkets" "*/__tests__/*"
Length of output: 97
Script:
#!/bin/bash
# Find all TypeScript test files and search for `updatePerpetualMarkets` to verify if it is mocked.
fd --type f --extension ts --search-path "indexer/services/roundtable/__tests__" | xargs rg "updatePerpetualMarkets"
Length of output: 509
Script:
#!/bin/bash
# Verify if `updatePerpetualMarkets` is mocked in the identified test files.
rg "updatePerpetualMarkets" --context 10 "indexer/services/roundtable/__tests__/tasks/remove-expired-orders.test.ts" "indexer/services/roundtable/__tests__/helpers/pnl-ticks-helper.test.ts"
Length of output: 6610
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/src/helpers/pnl-ticks-helper.ts (7 hunks)
Files skipped from review as they are similar to previous changes (1)
- indexer/services/roundtable/src/helpers/pnl-ticks-helper.ts
|
@Mergifyio backport release/indexer/v5.x |
✅ Backports have been created
|
Changelist
calculateEquity function was failing when marketId!=perpetualId, as is the case for dydx (perpetualId=63, marketId=1000001)
Test Plan
Tested in e2e 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
Bug Fixes
Logging Enhancements
Performance Improvements