feat(spot): add cl_ord_id to create_order and edit_order endpoints#416
feat(spot): add cl_ord_id to create_order and edit_order endpoints#416claudiapacini wants to merge 1 commit intobtschwertfeger:masterfrom
Conversation
btschwertfeger
left a comment
There was a problem hiding this comment.
Hey @claudiapacini, thanks for bringing this up!
Initially, I wanted to reject your issue and PR as the idea does not align with the decision on discontinuing the specialized Market, User, Trade, etc clients as stated here. The recommended functions to achieve your goal (e.g. for Spot trading) are the SpotClient.request and SpotAsyncClient.request methods. It also introduces a breaking change to the amend_order function that relied on the extra_params usage and now requires setting txid or cl_ord_id.
But after going through your changes, I see the benefit in applying them especially considering that the cancel_order function currently does not support client order IDs. While I still don't have the capacity to continuously track Kraken API changes, it makes sense to keep these specialized clients reasonably up to date with the help of contributors like you. This is particularly important not only from a functional perspective, but also because many users are likely still relying on the legacy client interfaces. Since the parameters are mostly optional, the risk of introducing future breaking changes also remains relatively low. So I'm happy to accept your proposed changes once you address my remaining comments.
| def amend_order( # pylint: disable=too-many-arguments # noqa: PLR0913, PLR0917 | ||
| self: Trade, | ||
| txid: str | None = None, | ||
| cl_ord_id: str | None = None, | ||
| order_qty: str | float | None = None, | ||
| display_qty: str | float | None = None, | ||
| limit_price: str | float | None = None, | ||
| trigger_price: str | float | None = None, | ||
| post_only: bool | None = None, | ||
| userref: int | None = None, | ||
| validate: bool = False, |
There was a problem hiding this comment.
Some parameters from the list are still missing https://docs.kraken.com/api/docs/rest-api/amend-order/.
There was a problem hiding this comment.
i've double checked these. They should be ok now.
There was a problem hiding this comment.
I don't see a validate parameter in the docs but it seems to work here as well.
Lets not add it to the function signature to avoid breakage in case they remove the support for this undocumented parameter. Users can still set it via extra_params.
|
@claudiapacini, are you planning to address the created comments and suggestions? |
b1a630f to
8329e36
Compare
8329e36 to
86f7d4f
Compare
|
I have removed the breaking changes in amend_order and cancel_order. These were not required. |
sorry for the delay |
| :type validate: bool, optional | ||
| :param userref: User reference id for example to group orders | ||
| :type userref: int, optional | ||
| :param cl_ord_id: Client order id (optional) |
There was a problem hiding this comment.
The optional is given by the type declaration and not necessary at this place.
(Similar to other places; please address)
| :param cl_ord_id: Client order id (optional) | |
| :param cl_ord_id: Client order id |
| def amend_order( # pylint: disable=too-many-arguments # noqa: PLR0913, PLR0917 | ||
| self: Trade, | ||
| txid: str | None = None, | ||
| cl_ord_id: str | None = None, | ||
| order_qty: str | float | None = None, | ||
| display_qty: str | float | None = None, | ||
| limit_price: str | float | None = None, | ||
| trigger_price: str | float | None = None, | ||
| post_only: bool | None = None, | ||
| userref: int | None = None, | ||
| validate: bool = False, |
There was a problem hiding this comment.
I don't see a validate parameter in the docs but it seems to work here as well.
Lets not add it to the function signature to avoid breakage in case they remove the support for this undocumented parameter. Users can still set it via extra_params.
|
|
||
| :param txid: The txid of the order to edit | ||
| :type txid: str, optional | ||
| :param cl_ord_id: Client order id (optional) |
There was a problem hiding this comment.
| :param cl_ord_id: Client order id (optional) | |
| :param cl_ord_id: Client order id |
| :type nonce: int, optional | ||
| :param validate: Validate the order without placing on the market (default: ``False``) | ||
| :type validate: bool, optional | ||
| :raises ValueError: If both ``txid`` and ``cl_ord_id`` are not set |
There was a problem hiding this comment.
Not having txid or cl_ord_id results in kraken.exceptions.KrakenInvalidArgumentsError.
| :raises ValueError: If both ``txid`` and ``cl_ord_id`` are not set | |
| :raises kraken.exceptions.KrakenInvalidArgumentsError: If neither ``txid`` or ``cl_ord_id`` is set |
| @@ -632,21 +697,24 @@ def edit_order( # pylint: disable=too-many-arguments # noqa: PLR0913, PLR0917 | |||
| @ensure_string("txid") | |||
There was a problem hiding this comment.
I looked up the docs and it seems like txid can be an integer as well, but cl_ord_id must be a string.
So lets swap these out:
| @ensure_string("txid") | |
| @ensure_string("cl_ord_id") |
| with pytest.raises(ValueError, match=r"Either txid or cl_ord_id must be set!"): | ||
| spot_auth_trade.cancel_order() |
There was a problem hiding this comment.
I do get kraken.exceptions.KrakenInvalidArgumentsError here.
Same here, busy times. (: |
Summary
Adds the cl_ord_id for create order and edit order endpoints as per the docs:
Trade endpoints
https://docs.kraken.com/api/docs/rest-api/add-order
https://docs.kraken.com/api/docs/rest-api/amend-order
https://docs.kraken.com/api/docs/rest-api/cancel-order
User endpoints
https://docs.kraken.com/api/docs/rest-api/get-open-orders
https://docs.kraken.com/api/docs/rest-api/get-closed-orders
https://docs.kraken.com/api/docs/rest-api/get-orders-info
Closes the open issue:
python-kraken-sdk/issues/415.