-
Notifications
You must be signed in to change notification settings - Fork 54
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
[RestExchange] add option to edit_order_by_replacing #820
base: master
Are you sure you want to change the base?
[RestExchange] add option to edit_order_by_replacing #820
Conversation
) | ||
) | ||
|
||
replaced_order = await self.create_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.
shouldn't we cancel the order first to free funds ? We should also wait for the order to be completely cancel as is trader.cancel_order() I think
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.
It would have worked with some order types on some exchanges, but I reversed it anyway.
I didnt had a chance to test this code yet tho. I will let you know once I tested it.
8961844
to
6ad9c27
Compare
33e3677
to
109e215
Compare
): | ||
# Can be used if api doesnt have an endpoint to edit orders | ||
# see binance USD m as an example | ||
order_to_cancle = ( |
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.
order_to_cancle = ( | |
order_to_cancel = ( |
@@ -163,6 +163,45 @@ async def _edit_order(self, order_id: str, order_type: enums.TraderOrderType, sy | |||
quantity, price, stop_price, side, | |||
current_price, params) | |||
|
|||
async def edit_order_by_replacing( |
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.
Can you add a test for it please?
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.
Not sure how to test that, ss there somewhere a similar example test for a real exchange trading test?
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.
A test like https://github.com/Drakkar-Software/OctoBot-Trading/blob/master/tests/exchanges/implementations/test_default_rest_exchange.py#L46 to ensure that the new order is properly createdand to also test that the portfolio values are properly updated (available and total).
You can use the simulated exchange
if replaced_order: | ||
return replaced_order | ||
else: | ||
raise RuntimeError( |
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 think we should prefer something like a
raise RuntimeError( | |
raise errors.FailedRequest( |
109e215
to
8e59be1
Compare
1c9aa5b
to
9f5b1e9
Compare
9f5b1e9
to
70dfa9a
Compare
see binance usdm as an example usage