Refactor liquidation logic #8544
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JosueNina
force-pushed
the
bug-8467-liquidate-existing-holdings-flag
branch
from
9b0a315 to
8acef7f
Compare
- Implemented regression tests for order creation and liquidation scenarios - Removed LiquidateExistingHoldings method - Replaced LiquidateExistingHoldings with the general Liquidate method
JosueNina
force-pushed
the
bug-8467-liquidate-existing-holdings-flag
branch
from
8acef7f to
80b0647
Compare
JosueNina
force-pushed
the
bug-8467-liquidate-existing-holdings-flag
branch
from
bd8ebcd to
9bbba46
Compare
JosueNina
force-pushed
the
bug-8467-liquidate-existing-holdings-flag
branch
from
0a7a8f2 to
2fc8f99
Compare
Martin-Molinero
approved these changes
Jan 22, 2025
Martin-Molinero
approved these changes
Jan 22, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Removed LiquidateExistingHoldings and replaced it with the general Liquidate method. Created regression tests for order creation and liquidation to ensure correct order cancellation and portfolio holdings check.
Related Issue
Closes #8467
Motivation and Context
The LiquidateExistingHoldings method was redundant, as it only created a single order to liquidate existing holdings and didn't cancel orders. The new Liquidate method simplifies the process by handling cancellations and ensuring consistency
Requires Documentation Change
No.
How Has This Been Tested?
Regression tests confirmed correct order creation, limit orders, and liquidation logic through backtesting in the specified date range, ensuring no remaining holdings
Types of changes
Checklist:
bug-<issue#>-<description>orfeature-<issue#>-<description>