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

Why are we using double for BaseCurrencyVolume and QuoteCurrencyVolume in MarketCandle? #836

Closed
BZ-CO opened this issue Jul 1, 2024 · 4 comments · Fixed by #837
Closed

Comments

@BZ-CO
Copy link
Contributor

BZ-CO commented Jul 1, 2024

Why are we using double for BaseCurrencyVolume and QuoteCurrencyVolume in MarketCandle?
Shouldn't we use decimal instead?

I've noticed that casting baseVolume and convertVolume to double the precision is lost.

candle.BaseCurrencyVolume = (double)baseVolume;

candle.QuoteCurrencyVolume = (double)convertVolume;

debug

@vslee
Copy link
Collaborator

vslee commented Jul 2, 2024

There are some tradeoffs between decimal and double. It is true that double does lose some precision. However, decimal is more limited in range.
Since the volume of the candles is rounded at the exchange, there is no need for high precision at the client. Rather, high range is more important.

@BZ-CO
Copy link
Contributor Author

BZ-CO commented Jul 2, 2024

I've compiled a short list of why I think decimal would be a better choice for representing volume data.

  1. We are casting to double from decimal so range is already lost.
  2. I've aggregated yearly volume (both base and quote) for all symbols on Binance. The highest value is 12123500811184481.00. It is 6535089471943.0177759626357213 times lower than decimal.MaxValue.
  3. If we want to create custom candle interval, let's say 10 days and we aggregate the daily candle data we would get a volume value, which does not correspond to the real one.
  4. Other major libraries are using decimal for representing the volume.

https://github.com/sonvister/Binance/blob/f3d5751bf67b492e98fc242c5dfb637e329ce6c9/src/Binance/Market/Candlestick.cs#L52

https://github.com/JKorf/Binance.Net/blob/6128368fdc23d8f86ff3d0b906cbb50026fcd0e0/Binance.Net/Objects/Models/BinanceKlineBase.cs#L39

@vslee
Copy link
Collaborator

vslee commented Jul 2, 2024

Okay, you've convinced me.
If you could send a PR, we can make the change.

@BZ-CO
Copy link
Contributor Author

BZ-CO commented Jul 2, 2024

Great.
Will submit a PR soon.

@vslee vslee linked a pull request Jul 2, 2024 that will close this issue
@vslee vslee closed this as completed in #837 Jul 2, 2024
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 a pull request may close this issue.

2 participants