started on crypto futures #426
Conversation
|
My review is in progress 📖 - I will have feedback for you in a few minutes! |
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
I have reviewed your code and found 4 potential issues. To discuss my individual comments that I have added, tag me in replies using @korbit-ai-mentor.
Please react with a 👍 to my comments that you find helpful and a 👎 to those you find unhelpful - this will help me learn and improve as we collaborate.
| order = Order( | ||
| strategy_name, | ||
| Asset( | ||
| symbol=pair[0], | ||
| asset_type="crypto", | ||
| asset_type="crypto", # TODO: Check if it is a Future |
There was a problem hiding this comment.
The current implementation does not differentiate between Futures and regular crypto assets. It's crucial to implement this distinction to handle the unique characteristics of Futures, such as leverage and expiration dates. This should be prioritized to ensure the system can accurately manage and represent positions in Futures.
| orders = self.get_orders() | ||
| # TODO: Check if the orders are futures | ||
|
|
||
| positions = self.get_positions() | ||
| # TODO: Check if the positions are futures |
There was a problem hiding this comment.
The code includes TODO comments to check if the orders and positions are futures. This indicates that the functionality for handling futures is not yet fully implemented. It's important to complete this functionality to ensure the system behaves correctly when dealing with futures.
| base = Asset( | ||
| symbol="BTC/USDT", | ||
| asset_type=Asset.AssetType.FUTURE | ||
| ) | ||
| quote = Asset( | ||
| symbol="USDT", | ||
| asset_type=Asset.AssetType.CRYPTO | ||
| ) | ||
|
|
||
| # Market Order for 0.1 BTC/USDT | ||
| mkt_order = self.create_order(base, 0.1, "buy", quote=quote) | ||
| self.submit_order(mkt_order) |
There was a problem hiding this comment.
The code for placing a Crypto Futures Order is almost identical to the code for placing a regular order. This is a violation of the DRY (Don't Repeat Yourself) principle. Consider refactoring the code to avoid duplication. You could create a function that takes the asset type as a parameter and then use this function for both regular and futures orders.
| @@ -191,7 +191,7 @@ def _parse_broker_position(self, position, strategy, orders=None): | |||
|
|
|||
| asset = Asset( | |||
| symbol=symbol, | |||
| asset_type="crypto", | |||
| asset_type="crypto", # TODO: Make this check if it is a Future or regular crypto | |||
There was a problem hiding this comment.
While adding TODO comments is a good reminder, it's important to create a more actionable item such as a ticket in the project's issue tracking system or a more detailed comment explaining the necessary steps to implement the feature. This ensures that the task is not forgotten and provides more context for future development.
Additional changes: - Added some possible TODOs in other parts of the project. - Added sample strategy to test crypto perpetual futures feature
Add settlement parameters to get_last_price
No description provided.