-
Notifications
You must be signed in to change notification settings - Fork 104
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-629] Fix entryPrice calc #2455
base: main
Are you sure you want to change the base?
Conversation
This reverts commit a282777.
WalkthroughThe pull request introduces modifications to the test suites for the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
indexer/services/ender/src/scripts/helpers/dydx_update_perpetual_position_aggregate_fields.sql (1)
Line range hint
1-78
: Consider additional improvements and clarifications.While the main change has been addressed, here are some suggestions to enhance the overall function:
Error Handling: The function raises an exception when no existing perpetual position is found. Consider adding a more graceful error handling mechanism or documenting this behavior clearly in the function description.
Documentation: The function's description could be expanded to include information about the
dydx_get_weighted_average
,dydx_trim_scale
, anddydx_perpetual_position_and_order_side_matching
functions. This would provide better context for future maintainers.Performance: For frequently called functions, consider adding an index on (
subaccountId
,perpetualId
,openEventId
) if not already present, to optimize the initial SELECT query.Could you provide more context on:
- The frequency of calls to this function?
- The typical volume of data it processes?
- Any performance benchmarks or requirements for this function?
This information would help in determining if any further optimizations are necessary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- indexer/services/ender/tests/handlers/order-fills/liquidation-handler.test.ts (2 hunks)
- indexer/services/ender/tests/handlers/order-fills/order-handler.test.ts (2 hunks)
- indexer/services/ender/src/scripts/helpers/dydx_liquidation_fill_handler_per_order.sql (1 hunks)
- indexer/services/ender/src/scripts/helpers/dydx_update_perpetual_position_aggregate_fields.sql (1 hunks)
🔇 Additional comments (7)
indexer/services/ender/src/scripts/helpers/dydx_update_perpetual_position_aggregate_fields.sql (1)
47-47
: Approved: Entry price calculation reverted as intended.The change from
perpetual_position_record."sumOpen"
toperpetual_position_record."size"
in thedydx_get_weighted_average
function call aligns with the PR objective to revert the previous change in entry price calculation.To ensure the change doesn't introduce unintended consequences, please verify:
- The accuracy of entry price calculations, especially for positions that have been partially closed and reopened.
- The consistency of this change with other parts of the codebase that may rely on the entry price calculation.
Consider adding a comment explaining the rationale behind using
size
instead ofsumOpen
for future reference. For example:-- Using 'size' instead of 'sumOpen' for entry price calculation to accurately reflect the current position entry_price := dydx_get_weighted_average( perpetual_position_record."entryPrice", perpetual_position_record."size", price, size );indexer/services/ender/src/scripts/helpers/dydx_liquidation_fill_handler_per_order.sql (1)
203-203
: Approved: Correction to entry price calculationThis change reverts the entry price calculation to use
perpetual_position_record."size"
instead ofperpetual_position_record."sumOpen"
when calculating the weighted average. This modification aligns with the PR objective and likely provides a more accurate representation of the current position size for the entry price calculation.The impact of this change:
- It corrects the entry price calculation, addressing the issue introduced in PR Revert "[CT-629] Fix entryPrice calc" #2425.
- The resulting
entry_price
will now be based on the actual size of the position rather than the sum of open amounts.- This change ensures that the entry price more accurately reflects the current state of the perpetual position.
To ensure this change doesn't introduce any unintended side effects, please verify that:
- All other occurrences of
dydx_get_weighted_average
in the codebase use the correct size parameter.- The
perpetual_position_record."size"
is always up-to-date and accurately represents the current position size.You can use the following script to check for other occurrences:
indexer/services/ender/__tests__/handlers/order-fills/liquidation-handler.test.ts (3)
140-141
: Update in default perpetual position sizeThe
size
property ofdefaultPerpetualPosition
has been changed from '10' to '5'. This change affects the initial state of the perpetual position used in tests.This change is likely intentional and may be part of adjusting test scenarios to match updated business logic or requirements.
395-397
: Consistent updates to test expectationsThe test expectations for
sumOpen
and other calculations have been updated to reflect the change in the initial position size. This ensures consistency with the modifieddefaultPerpetualPosition
.These changes maintain the integrity of the test cases and accurately reflect the new initial conditions.
Line range hint
1-1180
: Summary of changesThe primary change in this file is the update to the initial perpetual position size from '10' to '5'. This change has been consistently applied throughout the test cases, with appropriate adjustments to calculations and expectations. The core functionality and structure of the tests remain unchanged.
These changes appear to be intentional and well-implemented, maintaining the integrity and purpose of the test suite while adjusting for new initial conditions.
indexer/services/ender/__tests__/handlers/order-fills/order-handler.test.ts (2)
442-442
: Approved: Consistent use ofsumOpen
for perpetual position calculationsThis change correctly updates the
expectPerpetualPosition
function to usedefaultPerpetualPosition.sumOpen
instead ofdefaultPerpetualPosition.size
when calculating the expectedsumOpen
value. This modification ensures consistency with the changes made to thedefaultPerpetualPosition
object and more accurately reflects the intended behavior of the perpetual position calculations.
141-142
: Verify the relationship betweensize
andsumOpen
The
size
property has been reduced from '10' to '5', but thesumOpen
property remains at '10'. This results insumOpen
being larger thansize
, which might be intentional but could also be an oversight. Please verify if this is the desired behavior for thedefaultPerpetualPosition
object.✅ Verification successful
No inconsistency found between
size
andsumOpen
fordefaultPerpetualPosition
.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for other occurrences of defaultPerpetualPosition to ensure consistency grep -rn "defaultPerpetualPosition" . | grep -v "__tests__"Length of output: 57
@@ -200,7 +200,7 @@ BEGIN | |||
perpetual_position_record."side", order_side) THEN | |||
sum_open = dydx_trim_scale(perpetual_position_record."sumOpen" + fill_amount); | |||
entry_price = dydx_get_weighted_average( | |||
perpetual_position_record."entryPrice", perpetual_position_record."sumOpen", | |||
perpetual_position_record."entryPrice", ABS(perpetual_position_record."size"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Possible to add a test-case where perpetual position size is = negative size of fill to test that the previous error case couldn't happen again?
Summary by CodeRabbit
Bug Fixes
LiquidationHandler
andOrderHandler
to reflect changes in thedefaultPerpetualPosition
size, ensuring accurate validation of liquidation orders and order fills.sumOpen
and position size.Chores