Skip to content

BL3P: added Orderbook implementation and tests (websockets) #470

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

Merged
merged 2 commits into from
Oct 31, 2019
Merged

BL3P: added Orderbook implementation and tests (websockets) #470

merged 2 commits into from
Oct 31, 2019

Conversation

johnnyasantoss
Copy link
Collaborator

Add Orderbook via WebSockets

Made OnGetMarketSymbolsMetadataAsync protected internal to be able to mock in tests

Add unit tests and timestamp

Rename folder and namespace

@johnnyasantoss johnnyasantoss marked this pull request as ready for review October 29, 2019 11:05
@johnnyasantoss johnnyasantoss changed the title [WIP] BL3P Websockets implementation and tests BL3P Websockets (Orderbook) implementation and tests Oct 29, 2019
@johnnyasantoss
Copy link
Collaborator Author

This implementation can be tested with the command dotnet run -- ws-orderbook -e bl3p -m btceur in the ExchangeSharpConsole folder.

@vslee
Copy link
Collaborator

vslee commented Oct 29, 2019

You can update the readme to show the new websockets feature

Johnny Santos added 2 commits October 29, 2019 16:28
Add Orderbook via WebSockets

Made OnGetMarketSymbolsMetadataAsync protected internal to be able to mock in tests

Add unit tests and timestamp

Rename folder and namespace
@johnnyasantoss
Copy link
Collaborator Author

Rebased and updated the readme 😃

@vslee
Copy link
Collaborator

vslee commented Oct 29, 2019

Somewhat off topic, but do you have sway with BL3P? I was looking at their websockets API and noticed that the trade stream doesn't include a trade ID (or order ID). Is that something you could have them add? Would be helpful in figuring out if there are missing/duplicate trades.
Also most exchanges give a side (Buy/Sell) for each trade. So that would be helpful to add as well (but less important).

@jjxtra
Copy link
Collaborator

jjxtra commented Oct 29, 2019

I'd still like the bl3p exchange specific model objects to be marked as internal instead of public, anyone have any strong objections?

In my ideal world they would simply be child objects inside the partial exchange class, that way we keep the namespace clean, but I am less picky about that.

@vslee
Copy link
Collaborator

vslee commented Oct 29, 2019

I can change it to private nested classes like the NDAX

@vslee vslee merged commit ebc014b into DigitalRuby:master Oct 31, 2019
@johnnyasantoss johnnyasantoss deleted the websockets-impl branch November 1, 2019 10:33
@johnnyasantoss
Copy link
Collaborator Author

johnnyasantoss commented Nov 1, 2019

I'd still like the bl3p exchange specific model objects to be marked as internal instead of public, anyone have any strong objections?

In my ideal world they would simply be child objects inside the partial exchange class, that way we keep the namespace clean, but I am less picky about that.

@jjxtra
I would prefer to keep them in separate files and follow the Microsoft Csharp standards. But I agree that there's no need for these classes to be public :) I'll fix it in my next PR.

@johnnyasantoss
Copy link
Collaborator Author

Somewhat off topic, but do you have sway with BL3P? I was looking at their websockets API and noticed that the trade stream doesn't include a trade ID (or order ID). Is that something you could have them add? Would be helpful in figuring out if there are missing/duplicate trades.
Also most exchanges give a side (Buy/Sell) for each trade. So that would be helpful to add as well (but less important).

I work at Bitonic (the company behind BL3P) and I can check with our team about those additional IDs :)

@johnnyasantoss
Copy link
Collaborator Author

I'd still like the bl3p exchange specific model objects to be marked as internal instead of public, anyone have any strong objections?
In my ideal world they would simply be child objects inside the partial exchange class, that way we keep the namespace clean, but I am less picky about that.

I would prefer to keep them in separate files and follow the Microsoft Csharp standards. But I agree that there's no need for these classes to be public :) I'll fix it in my next PR.

Regarding the PR #474 I would like to point out that using this strategy of using partial classes we are creating yet another indentation level and at the same time making the classes a new type of name space, which is bad for the type itself, because it gets polluted with symbols that could be hidden by access modifier.
Should we keep with the same strategy or start using private internal for model types and other helper types?

@vslee vslee changed the title BL3P Websockets (Orderbook) implementation and tests BL3P: added Orderbook implementation and tests (websockets) Nov 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants