Skip to content
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

Application Updates #12

Merged
merged 2 commits into from
Jul 19, 2020
Merged

Application Updates #12

merged 2 commits into from
Jul 19, 2020

Conversation

DakotaLarson
Copy link
Contributor

Hi,
I have created a PR to pass along some (what I would consider) important updates to this package.

Current Issues:

  • With the HTTP API, Auth headers are not passed reducing the quantity of requests I can make to unauthenticated endpoints by half.

  • Additionally, upon inspection of the code, I found the use of setTimeout before a request is invoked. This will delay any request by 10ms minimum and doesn't always happen.

  • I have still received 429s using this package. Therefore, I am unsure if the timeout functionality is working correctly, however, that is not addressed in this PR.

  • With the WS API, it is a known issue that BitMEX will close websockets when many trades take place. There is no listener appended to the websocket to detect when the connection is closed.

Fixes:

  • Always pass auth headers if they are present.
  • Only use setTimeout if necessary
  • Allow users to pass custom delay for ping
  • Use more efficient refresh() method for ping timeout
  • Allow users to pass callback listener for websocket close event

I tested the code, but didn't create any tests since none existed previously. I figure that falls outside the scope of this PR.

Pass Auth headers when possible and more efficiently send requests.
Make ping time configurable, add WS close callback, and add method to close socket.
This was referenced Jul 18, 2020
@IlanFrumer IlanFrumer merged commit 2ce6343 into IlanFrumer:master Jul 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants