tiagosiebler / binance

Node.js & JavaScript SDK for Binance REST APIs & WebSockets, with TypeScript & browser support, integration tests, beautification & more.
MIT License
728 stars 264 forks source link

input params for subscribeDiffBookDepth() are not right #402

Closed Simon1093 closed 4 months ago

Simon1093 commented 4 months ago

Hi! In docs it should be 100, 250 and 500 milliseconds. But in the code its only 100 and 1000 milliseconds.

https://binance-docs.github.io/apidocs/futures/en/#diff-book-depth-streams

https://github.com/tiagosiebler/binance/blob/0d3c20b01f91bc6fa37f94dff9be94773d6229e6/src/websocket-client.ts#L1598

tiagosiebler commented 4 months ago

Ah this method was originally for spot markets, which have 1000ms or 100ms: https://binance-docs.github.io/apidocs/spot/en/#diff-depth-stream

The comment on the method also specifies spot. However, you are correct that the update speed param is no longer correct since this method also supports usdm futures. Will get this corrected. Thanks for letting me know!

tiagosiebler commented 4 months ago

Fixed via #403 - will be on npm shortly from v2.10.0.

Simon1093 commented 4 months ago

Fixed via #403 - will be on npm shortly from v2.10.0.

Hi! Wouldn't

const updateMsSuffx = updateMs === 100 ? `@${updateMs}ms` : ''; 

allow to pass only 100ms parameter?

public subscribeDiffBookDepth(
    symbol: string,
    updateMs: 100 | 250 | 500 | 1000 = 100,
    market: 'spot' | 'usdm' | 'coinm',
    forceNewConnection?: boolean,
  ): WebSocket {
    const lowerCaseSymbol = symbol.toLowerCase();
    const streamName = 'depth';
    const wsKey = getWsKeyWithContext(
      market,
      'diffBookDepth',
      lowerCaseSymbol,
      String(updateMs),
    );
    const updateMsSuffx = updateMs === 100 ? `@${updateMs}ms` : '';
    return this.connectToWsUrl(
      this.getWsBaseUrl(market, wsKey) +
        `/ws/${lowerCaseSymbol}@${streamName}${updateMsSuffx}`,
      wsKey,
      forceNewConnection,
    );
  }
tiagosiebler commented 4 months ago

Well, that is really silly of me 🤦 not sure why I hardcoded it back then and completely missed it now... will get that corrected too. Another method has the same mistake.

tiagosiebler commented 4 months ago

Released. Let me know if you notice anything else. Thanks!