-
Notifications
You must be signed in to change notification settings - Fork 152
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
started on crypto futures #426
base: dev
Are you sure you want to change the base?
Conversation
My review is in progress 📖 - I will have feedback for you in a few minutes! |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.