[CT-1307] Make pnl ticks computation consistent within a transaction. #2548
Conversation
WalkthroughThe changes in this pull request focus on modifying the Changes
Possibly related PRs
Suggested labels
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: 2
🧹 Outside diff range and nitpick comments (2)
indexer/services/roundtable/src/tasks/create-pnl-ticks.ts (2)
Line range hint 61-69: Consider updating error logging to reflect actual function parameters.
The error logging still includes latestBlockHeight and latestBlockTime even though these are no longer passed to getPnlTicksCreateObjects. Consider updating the logging to be consistent with the new implementation.
logger.error({
at: 'create-pnl-ticks#runTask',
message: 'Error when getting pnl ticks',
error,
- latestBlockHeight,
- latestBlockTime,
txId,
});Transaction isolation level should be explicitly set to prevent race conditions
The code starts transactions but doesn't set an isolation level explicitly. While the default PostgreSQL isolation level (READ COMMITTED) prevents dirty reads, for operations involving PNL calculations that need consistent reads across tables, you should explicitly set REPEATABLE READ or SERIALIZABLE isolation level.
- In
create-pnl-ticks.ts, afterTransaction.start(), addTransaction.setIsolationLevel(txId, IsolationLevel.REPEATABLE_READ)to ensure consistent reads across the transaction. - Add the same for the transaction in the chunked ticks processing loop.
🔗 Analysis chain
Line range hint 57-74: Verify transaction isolation level for race condition prevention.
The code uses transactions to ensure consistency, which is good. However, to fully prevent race conditions, we should verify that the appropriate transaction isolation level is being used.
Let me gather more information about how the transaction is being used in the PNL ticks creation.
Let me check one more thing to understand the default isolation level in the database configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for transaction isolation level configuration
rg -A 5 "isolation.*level"
# Also check the Transaction class implementation
ast-grep --pattern 'class Transaction {
$$$
}'
Length of output: 3713
Script:
#!/bin/bash
# Search for transaction start and isolation level usage in create-pnl-ticks
rg -B 5 -A 5 "Transaction\.(start|setIsolationLevel)" indexer/services/roundtable/src/tasks/create-pnl-ticks.ts
# Check if IsolationLevel is imported and used
rg -B 2 -A 2 "IsolationLevel" indexer/services/roundtable/src/tasks/create-pnl-ticks.ts
# Look for the getPnlTicksCreateObjects implementation to understand transaction usage
ast-grep --pattern 'async function getPnlTicksCreateObjects($_) {
$$$
}'
Length of output: 1141
Script:
#!/bin/bash
# Check database configuration files for default isolation level
rg -B 3 -A 3 "isolation" indexer/packages/postgres/src/config.ts
rg -B 3 -A 3 "isolation" indexer/packages/postgres/src/db.ts
# Check if there are any other files setting transaction isolation
rg -g "!*.md" -g "!*.test.*" -B 3 -A 3 "SET TRANSACTION ISOLATION LEVEL"
Length of output: 895
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- indexer/services/roundtable/tests/helpers/pnl-ticks-helper.test.ts (3 hunks)
- indexer/services/roundtable/src/helpers/pnl-ticks-helper.ts (2 hunks)
- indexer/services/roundtable/src/tasks/create-pnl-ticks.ts (1 hunks)
🔇 Additional comments (5)
indexer/services/roundtable/src/tasks/create-pnl-ticks.ts (1)
58-58: LGTM! The change aligns with making block data fetching consistent.
The modification to only pass txId to getPnlTicksCreateObjects is a good improvement as it ensures the block data is fetched within the same transaction context, preventing potential race conditions.
indexer/services/roundtable/src/helpers/pnl-ticks-helper.ts (2)
4-5: LGTM: Block fetching encapsulation improves consistency
The changes to fetch the latest block within the transaction context and removal of blockHeight/blockTime parameters from the function signature help prevent potential race conditions, making the PnL ticks computation more consistent.
Also applies to: 58-63
58-63: Verify impact on dependent systems
The change in how block data is obtained might affect systems that depend on this function. Please ensure that all dependent systems are aware of this change and can handle potential errors from block fetching.
Let's verify the usage of this function across the codebase:
✅ Verification successful
No impact on dependent systems - change is properly handled
The verification shows that the change in block data retrieval is safe and properly handled:
-
The function is only used in two places:
create-pnl-ticks.ts: Has proper error handling with try-catch and transaction rollbackpnl-instrumentation.ts: Only imports a different function from the helper
-
The test file confirms:
- All database operations are wrapped in transactions
- Error cases are properly tested
- Block data is consistently used across the function
The change maintains transactional consistency and error handling, with no breaking changes to the interface or behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct calls to getPnlTicksCreateObjects
echo "Searching for direct function calls..."
rg "getPnlTicksCreateObjects\(" -A 2
# Search for any error handling patterns around this function
echo "Checking error handling patterns..."
rg "getPnlTicksCreateObjects.*catch" -A 5
Length of output: 1484
Script:
#!/bin/bash
# Let's examine the error handling in create-pnl-ticks.ts where the function is called
echo "Checking error handling in create-pnl-ticks.ts..."
cat indexer/services/roundtable/src/tasks/create-pnl-ticks.ts
# Let's also check the test file to understand the expected behavior
echo "Checking test file..."
cat indexer/services/roundtable/__tests__/helpers/pnl-ticks-helper.test.ts
# Let's check if there are any other files importing this helper
echo "Checking imports of pnl-ticks-helper..."
rg "from ['\"]\.\./helpers/pnl-ticks-helper" --type ts
Length of output: 22885
indexer/services/roundtable/__tests__/helpers/pnl-ticks-helper.test.ts (2)
480-481: LGTM!
The block data setup is correct and properly formatted for test verification.
500-500: LGTM!
The updated function call correctly reflects the new signature that only requires txId, aligning with the PR's objective of making PnL ticks computation consistent within a transaction.
… (backport #2548) (#2549) Co-authored-by: vincentwschau <[email protected]>
Changelist
Fetch the latest block within the same transaction as other PnL tick data to avoid race conditions.
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