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

Issue with ToCandlestickInterval() method #86

Closed
sguryev opened this issue Apr 22, 2018 · 6 comments
Closed

Issue with ToCandlestickInterval() method #86

sguryev opened this issue Apr 22, 2018 · 6 comments
Assignees
Labels

Comments

@sguryev
Copy link

sguryev commented Apr 22, 2018

Hi @sonvister ,

I worked around the ToCandlestickInterval extension and have noticed this line:

case "1M": return CandlestickInterval.Month;

I didn't test but I'm 99% sure that after making s.Trim().ToLower() you will not me able get case "1M" and the main problem - it will go to the case "1m". What do you think?

@sguryev
Copy link
Author

sguryev commented Apr 22, 2018

Hmmmm, not sure if it affects the subscriptions. I remember the #82 issue related to the subscription and 6h interval. So I guess that it should affect it. Does it mean that on May 1st it should return 1M candle with monthly values but to the 1m interval?

@sonvister
Copy link
Owner

@sguryev, you are correct. The ToCandlestickInterval method ToLower() would prevent the "1M" match. 🤦‍♂️

@sonvister sonvister self-assigned this Apr 22, 2018
@sonvister sonvister added the bug label Apr 22, 2018
sonvister added a commit that referenced this issue Apr 23, 2018
@sguryev
Copy link
Author

sguryev commented Apr 23, 2018

What about my assumption about affected logic?

@sonvister
Copy link
Owner

The #82 issue was just the missing "6h" case. Here, the ToLower() was added only so the method would be case insensitive (obviously overlooking the "1M" at the time). Could you elaborate on 'affected logic'?

@sguryev
Copy link
Author

sguryev commented Apr 23, 2018

I'm collecting the candles using WebSocket to the custom store. I'm concerned about the data messing which this bug can lead to. My assumption that 1M subscription will be proceeded as 1m with this bug. And 1M candle will be recognized as 1m. Am I correct?

@sonvister
Copy link
Owner

sonvister commented Apr 23, 2018

Before 0.2.0-beta3, using ToCandlestickInterval() with "1M" would return CandlestickInterval.Minute instead of CandlestickInterval.Month and return the wrong data regardless (so, @sguryev, you likely have the wrong monthly data).

If subscribing or polling with CandlestickInterval values (not converting from string) the data would be as expected, but each returned Candlestick would have the wrong CandlestickInterval for monthly data (since ToCandlestickInterval() is used when deserializing candlestick data).

If using the IBinanceApi.GetCandlesticksAsync() extension with string parameter you will also get the wrong data for "1M" because of the conversion from string to CandlestickInterval.

With 0.2.0-beta3, anyone previously using uppercase strings with ToCandlestickInterval() or GetCandlesticksAsync() (low probability scenario) will now get errors or CandlestickInterval.Month instead of CandlestickInterval.Minute.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants