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

feat: add construct orderbook example using websocket #242

Closed
wants to merge 4 commits into from

Conversation

2hyunbin
Copy link

when i use a WebSocket to get the price, there was no properly documented example so i made it. For information on resolving the cross order book, i saw dydx docs (https://docs.dydx.exchange/api_integration-guides/how_to_uncross_orderbook).

@yogurtandjam yogurtandjam changed the title add construct orderbook example using websocket feat: add construct orderbook example using websocket Sep 18, 2024
@yogurtandjam
Copy link
Contributor

thank you! some general thoughts:

  • we've recently enforced using conventional commits for PR titles. i've updated this for you
  • i'm okay with stamping as is, but would prefer to encapsulate the example logic a bit better. it's fairly complex and gets a bit hard to follow as a single flat function.

if you feel like encapsulating will be more effort than it's worth, i'm happy to stamp as is. let me know what you think!

@2hyunbin
Copy link
Author

2hyunbin commented Sep 19, 2024

Thank you for applying the convention to the PR title. @yogurtandjam
Your feedback that the code should be encapsulated to make it easier to read is reasonable.
So I reflected the feedback.
Please let me know if there is anything else that needs to be improved!

  • When pushing orderbook data, it's better to insert it in the right place to keep the sorted orderbook list than to sort it at the end, but I chose a good read logic because the orderbook list is small and just an example.

@2hyunbin 2hyunbin closed this Sep 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants