Add cumulativeEntryPrice migration #2534
Conversation
WalkthroughThe changes in this pull request introduce a new property, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 1
🧹 Outside diff range and nitpick comments (4)
indexer/packages/postgres/src/db/migrations/migration_files/20241022151729_add_perpetual_positions_cumulative_entry_price.ts (1)
3-9: Consider specifying precision and scale for the decimal column.The migration looks good, but since this column will be used for financial calculations (PnL), consider explicitly specifying precision and scale to ensure sufficient decimal places for crypto price calculations.
- table.decimal('cumulativeEntryPrice', null).defaultTo(0).notNullable(); + table.decimal('cumulativeEntryPrice', 32, 18).defaultTo(0).notNullable();indexer/packages/postgres/src/db/helpers.ts (1)
Line range hint
76-95: Documentation needs to be updated to reflect the new PnL calculation method.The function's JSDoc comment should be updated to explain:
- The use of
cumulativeEntryPriceinstead ofentryPrice- How this affects PnL calculation
- Any specific scenarios or edge cases to be aware of
Consider adding this to the existing documentation:
* Get unrealized pnl for a perpetual position. If the perpetual market is not found in the * markets map or the oracle price is not found in the market, return 0. + * + * The PnL is calculated using the position's cumulative entry price, which represents the + * weighted average entry price across all modifications to the position. * * @param position Perpetual position object from the database, or the updated * perpetual position subaccountKafkaObject.indexer/packages/postgres/src/models/perpetual-position-model.ts (1)
174-175: Consider adding JSDoc documentation for the new property.While the property is correctly declared, adding documentation would help explain its purpose and relationship with the existing
entryPricefield.+ /** Cumulative sum of entry prices for the position. Used for PnL calculations. */ cumulativeEntryPrice!: string;indexer/packages/postgres/src/types/websocket-message-types.ts (1)
59-59: Add JSDoc documentation for the new field.Please add documentation to explain the purpose and usage of
cumulativeEntryPrice. This will help other developers understand how this field is used in perpetual position calculations.Apply this diff:
+ /** The cumulative entry price for the perpetual position. */ cumulativeEntryPrice: string,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- indexer/packages/postgres/tests/db/helpers.test.ts (1 hunks)
- indexer/packages/postgres/tests/helpers/constants.ts (2 hunks)
- indexer/packages/postgres/tests/stores/perpetual-position-table.test.ts (1 hunks)
- indexer/packages/postgres/src/db/helpers.ts (1 hunks)
- indexer/packages/postgres/src/db/migrations/migration_files/20241022151729_add_perpetual_positions_cumulative_entry_price.ts (1 hunks)
- indexer/packages/postgres/src/models/perpetual-position-model.ts (4 hunks)
- indexer/packages/postgres/src/types/db-model-types.ts (1 hunks)
- indexer/packages/postgres/src/types/perpetual-position-types.ts (4 hunks)
- indexer/packages/postgres/src/types/websocket-message-types.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (12)
indexer/packages/postgres/src/db/migrations/migration_files/20241022151729_add_perpetual_positions_cumulative_entry_price.ts (1)
11-17: LGTM!The down migration correctly removes the column, providing a clean rollback path.
indexer/packages/postgres/src/types/perpetual-position-types.ts (2)
37-37: LGTM on type consistency and structural changesThe changes maintain consistency by:
- Using
stringtype across all interfaces- Adding the field to all relevant interfaces
- Including the field in
PerpetualPositionColumnsenum for database mappingAlso applies to: 57-57, 98-98, 129-129
37-37: Verify the optionality ofcumulativeEntryPriceacross interfacesThere's an inconsistency in how
cumulativeEntryPriceis defined across interfaces:
- Optional in
PerpetualPositionCreateObjectandPerpetualPositionUpdateObject- Required in
UpdatedPerpetualPositionSubaccountKafkaObjectPlease verify if this is intentional. If the field is meant to be required for Kafka messages, ensure there's proper validation when creating/updating positions to guarantee the field is set before publishing to Kafka.
Also applies to: 57-57, 98-98
indexer/packages/postgres/src/db/helpers.ts (1)
93-93: Verify the mathematical correctness of using cumulativeEntryPrice for PnL calculation.The change from
entryPricetocumulativeEntryPricerepresents a fundamental shift in PnL calculation. Please ensure this aligns with the expected trading behavior and position management requirements.Let's verify the usage and implementation:
✅ Verification successful
Let's try a different approach to gather more context about this change and its implications.
The previous search might have been too specific. This broader search should help us understand:
- How the PnL calculation is tested
- The complete implementation of getUnrealizedPnl
- Other usages of cumulativeEntryPrice in the codebase
- Any related schema changes that introduced this field
Based on the gathered information, I can now provide a final response about the change from
entryPricetocumulativeEntryPrice.The change to use cumulativeEntryPrice for PnL calculation is correct and properly implemented
The evidence shows this is a well-structured change:
- A migration file
20241022151729_add_perpetual_positions_cumulative_entry_price.tsproperly adds the new column- The field is properly typed in multiple type definitions with appropriate validation
- Tests in
helpers.test.tsvalidate the PnL calculations for both long and short positions- The implementation maintains the same mathematical formula, just using the new price field
- The change is part of a broader, intentional modification to the position tracking system
The tests specifically verify:
- Long position PnL calculation expecting
-50000- Short position PnL calculation expecting
50000- Both cases use the new
cumulativeEntryPricefield🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for tests validating PnL calculations with cumulativeEntryPrice rg -l "getUnrealizedPnl.*cumulativeEntryPrice" --type ts # Look for related documentation or specs fd -e md -e txt . | xargs rg -l "cumulative.*entry.*price" # Check for other functions using cumulativeEntryPrice ast-grep --pattern 'cumulativeEntryPrice'Length of output: 161
Script:
#!/bin/bash # Find the test file for helpers.ts fd "helpers.test" --type f --exec rg -l "getUnrealizedPnl" {} # Look for the implementation of getUnrealizedPnl function rg -A 10 "getUnrealizedPnl" --type ts # Search for any references to cumulativeEntryPrice in the codebase rg "cumulativeEntryPrice" --type ts -A 2 -B 2 # Look for any migration files or schema changes related to this field fd "migration|schema" --type f --exec rg -l "cumulativeEntryPrice" {}Length of output: 16251
indexer/packages/postgres/src/models/perpetual-position-model.ts (3)
82-82: LGTM: Schema definition for cumulativeEntryPrice is well-structured.The property is correctly defined with appropriate constraints:
- Required field in the schema
- Uses NonNegativeNumericPattern which is suitable for price data
- Has a sensible default value of '0'
Also applies to: 101-101
133-133: LGTM: SQL to JSON conversion is properly mapped.The conversion type 'string' is consistent with other numeric fields in the model.
82-82: Verify the migration path for existing records.The schema changes look correct, but we should ensure there's a proper migration strategy for existing records since this is a required field.
Also applies to: 101-101, 133-133, 175-175
indexer/packages/postgres/__tests__/db/helpers.test.ts (1)
35-35: Add test cases for cumulativeEntryPrice field.While the property has been added to match the type changes, there are no test cases that validate how this new field affects position calculations. Consider adding test cases that verify:
- The impact of cumulativeEntryPrice on funding calculations
- Edge cases (zero, negative, very large values)
- Precision handling for decimal values
Let's check if there are any other test files that might cover these scenarios:
indexer/packages/postgres/src/types/websocket-message-types.ts (1)
59-59: Verify consistent implementation across related types.The addition of
cumulativeEntryPricelooks correct. Let's verify it's consistently implemented across related types and models.indexer/packages/postgres/src/types/db-model-types.ts (1)
55-55: LGTM! Verify migration coverage.The new
cumulativeEntryPricefield follows consistent typing patterns with other price fields in the interface.Let's verify the migration coverage:
✅ Verification successful
Let me examine the migration file and test coverage in more detail to ensure proper implementation.
Based on the gathered information, I can now provide the final response:
Migration and implementation coverage verified ✅
The
cumulativeEntryPricefield is properly implemented with:
- A migration file that adds the column with appropriate constraints (non-null, default 0)
- Model schema validation with correct type patterns
- Integration in position calculations within helpers
- Proper type definitions across related files
The implementation follows best practices with:
- Proper up/down migrations
- Consistent decimal type in database
- Non-negative numeric pattern validation
- Default value handling
- Usage in PnL calculations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for migration files and related changes # Test 1: Look for migration files echo "Checking for migration files..." fd -e sql -e ts migration # Test 2: Check for related model changes echo "Checking for related model changes..." rg -l "cumulativeEntryPrice" --type ts # Test 3: Check for test coverage echo "Checking for test coverage..." fd -e test.ts | xargs rg "cumulativeEntryPrice"Length of output: 1261
Script:
#!/bin/bash # Check migration implementation and test coverage # Examine the migration file echo "Migration file contents:" cat indexer/packages/postgres/src/db/migrations/migration_files/20241022151729_add_perpetual_positions_cumulative_entry_price.ts # Check migration test coverage echo -e "\nMigration test coverage:" cat indexer/packages/postgres/__tests__/db/migrations.test.ts | rg -A 10 -B 10 "cumulativeEntryPrice" # Check model implementation echo -e "\nPerpetual position model implementation:" cat indexer/packages/postgres/src/models/perpetual-position-model.ts | rg -A 5 -B 5 "cumulativeEntryPrice" # Check helpers implementation echo -e "\nHelpers implementation:" cat indexer/packages/postgres/src/db/helpers.ts | rg -A 5 -B 5 "cumulativeEntryPrice"Length of output: 2517
indexer/packages/postgres/__tests__/helpers/constants.ts (2)
521-521: LGTM: Added cumulativeEntryPrice to defaultPerpetualPositionThe addition of
cumulativeEntryPricewith value '20000' matches theentryPricefield, which is appropriate for test fixtures.
544-544: LGTM: Added cumulativeEntryPrice to isolatedPerpetualPositionThe addition of
cumulativeEntryPricewith value '1.5' matches theentryPricefield, maintaining consistency with the isolated position's price scale.
| @@ -419,6 +419,7 @@ describe('PerpetualPosition store', () => { | |||
| maxSize: tinyMaxSize, | |||
| sumOpen: tinySize, | |||
| entryPrice: tinyPrice, | |||
| cumulativeEntryPrice: tinyPrice, | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure assertions cover cumulativeEntryPrice.
You've added cumulativeEntryPrice to the test data, but there are no assertions verifying its correctness. Consider adding assertions to ensure that cumulativeEntryPrice is being calculated and stored as expected.
Changelist
[Describe or list the changes made in this PR]
Test Plan
[Describe how this PR was tested (if applicable)]
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
Release Notes
New Features
cumulativeEntryPrice, to enhance tracking of entry prices in perpetual positions across various interfaces and models.Bug Fixes
Tests
Chores
cumulativeEntryPricecolumn in the database, ensuring data integrity and structure.