-
-
Notifications
You must be signed in to change notification settings - Fork 268
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
v2.11.0: feat(): improved type resolution for order submit/amend methods & add "submitNewOrderList" method and related types #422
Conversation
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.
Looks great to me! Especially the response type mapping. Could you also increase the version number in the package.json please? Thanks for the PR!
@tiagosiebler, I will surely increase the version number. I will try to find a way to implement the generic type to the
|
@tiagosiebler. I also made const responseA = await client.submitNewOrder({ ...args, type: "MARKET" }); // FULL
const responseB = await client.submitNewOrder({ ...args, type: "MARKET", newOrderRespType: "ACK" }); // ACK
const responseC = await client.submitNewOrder({ ...args, type: "STOP_LOSS" }); // ACK
const responseD = await client.submitNewOrder({ ...args, type: "STOP_LOSS", newOrderRespType: "FULL" }); // FULL |
@tiagosiebler, I'm updating the first message in this PR when I update the PR, so you can use it for documentation purposes when you merge it. |
@tiagosiebler, could you please review the PR? It's getting bigger. I want to continue adding further fixes in separate PRs, but they may depend on this PR. |
Hey, I will need some time to review (currently travelling). Will review it soon. In the meantime, I suggest making another branch in your fork based on the branch you're currently working on. This way you're building on top of this PR without directly adding more commits to this PR/branch. |
I wish you a nice trip. FYI, I'm using the version with PR applied in a small personal project, no problem so far. |
src/websocket-client.ts
Outdated
@@ -57,6 +57,7 @@ export interface WSClientConfigurableOptions { | |||
agent?: any; | |||
}; | |||
wsUrl?: string; | |||
baseUrl?: string; |
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.
Where did this come from? Don't think this applies for the WS client
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.
Please see #414. WithoutbaseUrl
, you cannot subscribe SpotUserDataStream
on Testnet.
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.
This isn't the correct way of doing this unfortunately. Take a look at how testnet is initiated with the other embedded REST clients. I would strongly prefer if you undo this and we look at doing this properly in a separate PR (although I appreciate you looking to fix this too). Will check the rest in the meantime.
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.
As you asked, I pushed a new commit to undo this change.
Take a look at how testnet is initiated with the other embedded REST clients.
getSpotRestClient()
function in line 756 of websocket-client.js
file creates a new Rest Client. Here on the pinned issue, you described that to connect TestNet with Rest Client, we must provide baseURL
.
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.
Ah I see. Admittedly that's more for the REST client, but since then other contributors have added a simpler way to configure testnet on some of the REST clients. We'll get the spot testnet working properly for the websocket client in a separate PR - most likely similar to how it's working for futures:
https://github.com/tiagosiebler/binance/blob/master/src/websocket-client.ts#L958-L963
But that's for a separate PR to get this one finished & merged. Will review the rest of this.
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.
Thanks, @tiagosiebler; I hope you'll provide a solution sooner because, currently, it is impossible to use WebSocket subscriptions on TestNet.
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'll keep a note high up in my list to get that prioritised
src/types/spot.ts
Outdated
@@ -404,29 +408,40 @@ export interface ExchangeInfoParams { | |||
symbols?: string[]; | |||
} | |||
|
|||
export interface NewSpotOrderParams { | |||
export interface NewSpotOrderParams< | |||
T extends OrderType, |
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.
Any idea how we can make TypeScript automatically infer the order type from usage? For example, this is complaining because the first parameter of the generic type is missing:
However, it was never required before and would be a slight breaking change to require it now. It also seems redundant to force this:
const order: NewSpotOrderParams<'MARKET'> = {
symbol,
side: 'BUY',
type: 'MARKET',
quantity: 0.1,
};
When the value in the object already says exactly what the order type is. Any idea how to make TypeScript automatically infer TOrderType in a scenario like this? It's common to generate a request param (like an order object) before actually calling the function.
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 pushed a new commit to make the generic optional.
If a generic is not provided, it will act as before because plain objects cannot infer generics from attribute values.
If a generic is provided, or a plain object is written directly in a function call, it would be inferred.
const order: NewSpotOrderParams = {
symbol,
side: 'BUY',
type: 'MARKET',
quantity: 0.1,
};
// As before, cannot be inferred because of TypeScript limitation
const result = await submitNewOrder(order);
// Inferred
const result2 = await submitNewOrder({
symbol,
side: 'BUY',
type: 'MARKET',
quantity: 0.1,
});
// Inferred with optional generic
const orderX: NewSpotOrderParams<"MARKET"> = {
symbol,
side: 'BUY',
type: 'MARKET',
quantity: 0.1,
};
const resultX = await submitNewOrder(orderX);
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.
This is much better, thanks! This way there's no impact for anyone upgrading, just improvements/more flexibility.
Can you check if any other interfaces you've updated need to be updated as well? Or is this the only one?
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.
There were four more, and I fixed them in the last commit.
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.
Amazing work, looks great to me! Thanks for all the time you spent in improving this!
Happy to contribute, and thank you for publishing this great library. |
Summary
submitNewOrderList()
becauseNew OCO
is deprecated on Binance API.sumbitNewOrder()
andreplaceOrder()
generic to have correct report type for the response.workingTime
,selfTradePreventionMode
andtransactTime
.W
andV
to theexecutionReport
.OrderResponse
type for convenience.OrderResponseTypeFor<T>
for a better developer experience. Please take a look at the example below.any
.baseUrl
to theWSClientConfigurableOptions
, so it can be used to subscribe to SpotUserDataStream on Testnet. Closes Cannot connect/subscribe to SpotUserDataStream on Testnet #414CancelRestrictions
,CancelReplaceMode
SelfTradePreventionMode
Example
Thanks to the new generic type, the new
submitNewOrderList
method returns a response with the exact TypeScript type based on thenewOrderRespType
parameter. IfnewOrderRespType
is not provided, it defaults to the minimumACK
, not to mislead the developer.NOTE: I could have divided the features into multiple PRs, but since some of the changes modified the same files, I didn't want to mess with Git conflicts, so I divided them into multiple commits.