DigitalRuby / ExchangeSharp

ExchangeSharp is a powerful, fast and easy to use .NET/C# API for interfacing with many crypto currency exchanges. REST and web sockets are supported.
https://www.digitalruby.com
MIT License
741 stars 374 forks source link

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

Closed BZ-CO closed 3 months ago

BZ-CO commented 3 months ago

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. https://github.com/DigitalRuby/ExchangeSharp/blob/1729aaf896979adaeb90e5eb413aee05fb8d65e1/src/ExchangeSharp/API/Exchanges/_Base/ExchangeAPIExtensions.cs#L1099 https://github.com/DigitalRuby/ExchangeSharp/blob/1729aaf896979adaeb90e5eb413aee05fb8d65e1/src/ExchangeSharp/API/Exchanges/_Base/ExchangeAPIExtensions.cs#L1100 debug

vslee commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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

BZ-CO commented 3 months ago

Great. Will submit a PR soon.