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

Add Candles (OHLC) websocket support in ExchangeAPI #596

Merged
merged 2 commits into from
Jun 5, 2021

Conversation

vslee
Copy link
Collaborator

@vslee vslee commented Jun 1, 2021

@jdx-john
Copy link
Contributor

jdx-john commented Jun 3, 2021

I don't think I'm placed to review but this seemed a good place to ask a question, as I am interested to provide a Kraken implementation.

Why are the method signatures not consistent?

OnGetCandlesWebSocketAsync(Action<IReadOnlyCollection<MarketCandle>> callback, params string[] marketSymbols)
OnGetTickersWebSocketAsync(Action<IReadOnlyCollection<KeyValuePair<string, ExchangeTicker>>> tickers, params string[] marketSymbols)
OnGetTradesWebSocketAsync(Func<KeyValuePair<string, ExchangeTrade>, Task> callback, params string[] marketSymbols) 

  1. Trades passes a Func rather than Action but looking at the console they are used exactly the same
  2. Why do we have return type of <string, ...> on Tickers & Trades but not Candles? I think the string holds the pair-name, but isn't that returned inside the objects anyway?

Doesn't stop me working on a Kraken implementation in the meantime locally.

@vslee
Copy link
Collaborator Author

vslee commented Jun 3, 2021

There was not an API design process in the beginning. Instead, it has evolved over the ears. Various contributors have added bits and pieces. That is why there are inconsistencies. Perhaps one day someone will want to go through and clean up the API. In the meantime, we can do what makes the most sense for any new additions.
You're right in that returning a string for the symbol is superfluous, since the symbol name already exists inside MarketCandle, so that's why I have left it off. The reason that Trades passes a Func instead of Action is that this allows the callback itself to be an async method. Come to think of it, this would be useful for the candles callback so I will push a commit that changes this. Eventually, the rest of the methods can be updated to support this as well, but for now we can just update this one.

@vslee vslee requested a review from jjxtra June 3, 2021 21:33
@jdx-john
Copy link
Contributor

jdx-john commented Jun 3, 2021

Thanks @vslee it's good to know I wasn't missing anything major with it being a new codebase to me. I hadn't thought of the async callback detail, though. That's a good point.

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