Feat/injective w batch update #302
Conversation
# Conflicts: # hummingbot/connector/gateway/amm/gateway_evm_amm.py # hummingbot/connector/test_support/exchange_connector_test.py
…date # Conflicts: # hummingbot/connector/gateway/clob_spot/data_sources/injective/injective_api_data_source.py
…inAlpha/hummingbot into feat/injective_w_batch_update
- Splits batch order updating into batch order creation and batch order cancelation methods. - Updates the rest of the `ExchangePyBase` class to take full advantage of the new methods. - Adds tests for the new functionality.
|
@CoinAlpha/qa-team, please note that Injective will be failing, as with the parent PR, so no need to test Injective for now, but the rest of the exchanges can be tested. |
|
Test runs Injective: On hold for now |
There was a problem hiding this comment.
@petioptrv let's coordinate a meeting to discuss the current approach.
| @dataclass | ||
| class Order: |
There was a problem hiding this comment.
Do we need yet a new order class? Why can't we use LimitOrder or OrderCandidate as the class to represent an order between the strategy and the connector?
|
|
||
| @dataclass | ||
| class PlaceOrderResult: | ||
| success: bool |
There was a problem hiding this comment.
I think success should not be a flag. It should be derived from the fact that there is or there is not an exception
| exchange_order_id: Optional[str] | ||
| trading_pair: str | ||
| misc_updates: Dict[str, Any] = field(default_factory=lambda: {}) |
There was a problem hiding this comment.
We should only disclose information from the connector to the strategy that does not expose internal connector implementation. I think exchange_order_id and misc_updates should not be here
| misc_updates: Dict[str, Any] = field(default_factory=lambda: {}) | ||
| not_found: bool = False |
There was a problem hiding this comment.
I don't think misc_updates should be disclosed to the strategy, at least not as the response to the cancel message (if we need any information in particular in the strategy should be included in the cancel event). The not_found condition shouldn't be here either (the strategy will receive an order failed event if the order is no longer valid)
| client_order_id: str | ||
| trading_pair: str |
There was a problem hiding this comment.
Maybe it would be better to just include here an order object (an instance of the class we finally decide to use to communicate strategies and connectors)
| @@ -1219,3 +1220,37 @@ async def clob_cancel_order( | |||
| "orderId": exchange_order_id, | |||
| } | |||
| return await self.api_request("delete", "clob/orders", request_payload) | |||
|
|
|||
| async def clob_batch_order_update( | |||
There was a problem hiding this comment.
Is it correct to call the method batch_order_update? In the client the "update" term is in general reserved for when we look for updates on the status of a currently existing order. I don't think that apply to order creation. And for order cancelation we are not trying to update our local status with the server status. We are requesting an action on the order.
| cancelation_results = [ | ||
| CancellationResult(order_id=cancel_order_result.client_order_id, success=cancel_order_result.success) | ||
| for cancel_order_result in cancel_order_results | ||
| ] | ||
| for cancel_order_result in cancel_order_results: | ||
| if cancel_order_result.success: |
There was a problem hiding this comment.
It is not a good idea to cycle the collection of cancel_order_results twice
|
Converting this PR to draft in favour of this PR, which will be submitted to foundation. |
|
The gateway side is in this PR: CoinAlpha/gateway#4 |
Before submitting this PR, please make sure:
A description of the changes proposed in the pull request:
This PR introduces batch order creation and cancelation for the client.
The functionality is implemented with backward compatibility.
The Injective connector is refactored to provide this enhanced order management.
Tests performed by the developer:
Added tests for the new functionality.
Tips for QA testing:
Please test that each exchange works with a normal PMM strategy, as well as with the new
batch_order_update.pyscript. The script strategy has four phases:The script begins by setting some global variables
The
ORDERS_INTERVALvariable is the number of ticks (seconds) between each of the script phases. ThePRICE_OFFSET_RATIOvariable sets the orders' prices at 10% from the current mid-market price.