Implement spot test network

[SebastianBoehler]

Summary

Based on #409 just added the type as requested in the discussion to not have breaking changes

[tiagosiebler]

Thank you for picking this up! Minor request, to encapsulate all configurable parameters for the REST client into the existing RestClientOptions class, where other configurable options live too. The rest looks great to me.

[tiagosiebler]

Can you move this into RestClientOptions?

[tiagosiebler]

I'm also wondering about the WS client. Anything where there's a boolean for testnet or the user has to choose spottest instead of spot, maybe it's simpler to have a useTestnet?: boolean in WSClientConfigurableOptions.

The rest of the logic in the WS client could then automatically know whether to use testnet or not, and the user just says "spot" instead of needing to know "spottest" is an option. Simpler that way perhaps...less for anyone using the SDK to think about - thoughts? @SebastianBoehler

[SebastianBoehler]

Moved it into the RestClientOptions, havent touched ws client tho would be breaking changes there as it is pased as param in some public methods

[tiagosiebler]

I'm not expecting any breaking changes from the WS update, lemme comment in-line, one sec

[SebastianBoehler]

https://github.com/tiagosiebler/binance/blob/master/src/websocket-client.ts?plain=1#L1940 subscribeUsdFuturesUserDataStream would be breaking change if isTestnet would be removed since used in the constructor then. Or we use it in the WSOptions and keep it in such places

[tiagosiebler]

ah man, you're completely right, and these methods are inconsistent with the other subscribe*UserDataStream methods (the others don't have that param at the start). Seems these are the only two methods that expose this param, unless I missed anything else.

I think I'd prefer to implement this in the WS options but keep it in these places for now, so avoiding the breaking change for now but not making it worse.

In a future breaking change / major release I'll want to consolidate these to the unified standard. Would you mind working through that as part of this PR?

[SebastianBoehler]

Yeah had to double check everything as well until I got through all.

Lets keep this PR open then and I might work through all of this on the weekend. Can we merge the other one then in the meantime? #419

[tiagosiebler]

Fair enough, no rush here - very much appreciate the help. Let me take another look at #419 in the meantime!

[tiagosiebler]

So with my proposed change of having the same useTestnet boolean in the ws configuration object, any fn like this would basically remain as before:

  public subscribeEndpoint(
    endpoint: string,
    market: 'spot' | 'usdm' | 'coinm',
    forceNewConnection?: boolean,
  ): WebSocket {

The user wouldn't need to say in the function to use spotTestnet instead of spot, they would just say "spot" and the SDK automatically knows to use testnet, because that's in the WS client's config in the constructor. Not a breaking change, since "spotTestnet" was never a market choice in the function call anyway, and this way it's less for the user to think about while working similar to how it works in the REST client with the change you've just added.

The only caveat is each line like this:

    const wsKey = getWsKeyWithContext(market, endpoint);

might need to become something like this (open to cleaner ideas):

    const resolvedMarket = this.options.useTestnet ? market + 'Testnet' : market;
    const wsKey = getWsKeyWithContext(resolvedMarket, endpoint);

So this way the user only cares about testnet when instancing the WS client, and the rest of the function calls are the same for live vs testnet. It also makes the new "isTestnet?: boolean," param in some of the functions further down, redundant. Let me know your thoughts! The less a user has to worry about internal plumbing, the less mistakes they make -> less confusion -> easier usage all around :)