tiagosiebler / binance

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

Add 10 new typeguards and a small fix in WsFormattedMessage type hierarchy #277

Closed jule64 closed 1 year ago

jule64 commented 1 year ago

Hi, I'd like to submit this pull request.

Contains two commits, more info provided in commit details

Cheers

jule64 commented 1 year ago

Pushed new change. I can remove the class comment if it is not helpful ( about the file being organised by Typeguards message types) let me know

jule64 commented 1 year ago

Also as a general observation/question, I wanted to create unit tests for these typeguards but I can't find a clean way to do that. The main issue I found is there does not seem to be a way to "bind" a typeguards with the types that they are supposed to guard for. For example, I can't see a good way to encode that isWsFormattedFuturesUserDataAccountUpdate is "type guarding" WsMessageFuturesUserDataAccountUpdateFormatted. If that makes sense..

tiagosiebler commented 1 year ago

Pushed new change. I can remove the class comment if it is not helpful ( about the file being organised by Typeguards message types) let me know

I think this is a helpful comment

tiagosiebler commented 1 year ago

Also as a general observation/question, I wanted to create unit tests for these typeguards but I can't find a clean way to do that. The main issue I found is there does not seem to be a way to "bind" a typeguards with the types that they are supposed to guard for. For example, I can't see a good way to encode that isWsFormattedFuturesUserDataAccountUpdate is "type guarding" WsMessageFuturesUserDataAccountUpdateFormatted. If that makes sense..

Regarding tests, I have mixed feelings here because these functions are not that complex. The main concern is functions used in many places (where breaking those functions will break other type guards as a side effect). I'd be tempted to throw a basic test at them...but at the same time, functions like isWsFormattedUserDataEvent aren't necessarily doing much either..

The other option is looking at integration tests rather than unit tests on the type guard itself. For example isWsFormattedUserDataEvent depends on the wsKey being provided by the ws client when it consumes and emits user data events. It'll be trickier to implement, but it would make this more robust (since the wsKey essentially must always contain that else the type guard is no longer valid).

The other option is end to end tests on these websockets. I've added the first generation of these to the bybit connector, although I'm not completely happy yet with how the test works it seems to add some extra safety around the ws client (at least more than without tests). Look at the ws*.test.ts files here if you're curious: https://github.com/tiagosiebler/bybit-api/tree/master/test/usdc/perpetual

End to end tests are still insufficient in my binance connector in my opinion, compared to my other connectors that have +100 of these making real api calls. It's unlikely to break unless I start doing low-level refactoring, but it's an extra safety net too (and I wouldn't start refactoring without them).

This is complicated a bit more too by the fact that binance blocked US IPs from making API calls (and CircleCI, used by this connector, is using US IPs). Not sure if github actions run on US IPs too (my other connectors use github actions), I did want to transition this connector to using github actions for tests too. Or I spin up a proxy and use the underrated proxying capabilities of axios via this connector. A few different approaches going forward that are currently on my mind...

tiagosiebler commented 1 year ago

Let me know once you've addressed the linting request (& if there's anything else you wanted to slip into this PR related to guards), otherwise I'm happy to merge after the linting. Thanks!

jule64 commented 1 year ago

Thanks for the comments and pointers. I have pushed the suggested changes now. I did not know that Binance blocked US IPs, is it recent? And is it complicated to migrate to github actions?

tiagosiebler commented 1 year ago

Thanks for the comments and pointers. I have pushed the suggested changes now. I did not know that Binance blocked US IPs, is it recent? And is it complicated to migrate to github actions?

It happened 1-2 weeks ago that they started blocking API calls from IPs in the US, quite frustrating. Migrating to github actions should be relatively easy, just need to move any secrets across and copy one of the build & test actions from my other connectors. Doesn't seem like it'll fix the US IP problem though without using a self-hosted runner or a proxy. Tempting to spin up a proxy (this would have the extra benefit of also verifying the proxying functionality is robust), but that's all for another PR.

jule64 commented 1 year ago

LGTM, thanks!

Thanks for the merge!