bchavez / Coinbase

:moneybag: A .NET/C# implementation of the Coinbase API.
https://developers.coinbase.com/api/v2
MIT License
170 stars 92 forks source link

Money class only handles currency amounts that are in decimal #59

Closed astrohart closed 5 years ago

astrohart commented 5 years ago

Hi,

In the Money class, you put the type of the Amount property as Decimal. However, this is only valid for, e.g., USD, GBP, EUR etc. currencies. For Bitcoin, and other cryptos, the quantity of the currency you have is actually a floating-point number. You should change the type of this property to float or double.

bchavez commented 5 years ago

Maybe it might be better as a string, as the official doc examples seem to be using strings for "amount":

https://developers.coinbase.com/api/v2#fields

MONEY HASH

Money values are represented by a hash object which contains amount and currency fields. Amount is always returned as a string which you should be careful when parsing to have correct decimal precision. Bitcoin, Bitcoin Cash, Litecoin and Ethereum values will have 8 decimal points and fiat currencies will have two.

{
"amount": "39.59000000",
"currency": "BTC"
}

However, can you provide any examples where you find using decimal is actually a problem? Do you receive any HTTP error codes as a result of using decimal? If so, can you please provide some examples?

Thanks, Brian

Other official APIs using string: Ruby: https://github.com/coinbase/coinbase-ruby/search?q=amount&unscoped_q=amount

JavaScript: https://github.com/coinbase/coinbase-node/search?q=amount&unscoped_q=amount

Java (v1 API) https://github.com/coinbase/coinbase-java/blob/3e20c0bf2b0b84761b0ef8390ea437685d52647c/coinbase-java/src/main/java/com/coinbase/resources/transactions/MoneyHash.java#L26-L29

astrohart commented 5 years ago

As you probably are aware, decimal is a data type, according to the docs: Because the decimal type has more precision and a smaller range than both float and double, it's appropriate for financial and monetary calculations.

In principle, you would be correct to utilize decimal for the property. However, I don't think your example is very good, as far as BTC. BTC and other cryptocurrency amounts are actually real numbers (think back to high school math class) and are not just limited to two place values after the decimals like a lot of major world currencies such as dollars, euros, etc). They are real numbers due to the fact that division is utilized when computing how much BTC to give you for your USD.

For example, I have a quantity:

{
    "amount": "0.013838324958",
    "currency": "BTC"
}

Again, since money amounts probably don't need to be representable with as wide a range of values as would a double , I need to do math in a bot I am using and I find double easier to work with doing math such as division.

I think maybe string would be the best, since then callers can just convert it (using Convert.ToDouble(...) etc) as needed.

astrohart commented 5 years ago

And, as to your point about other APIs using string the examples you gave are almost all in languages (JavaScript, Python, Ruby) which are not as strongly-typed as C#.

bchavez commented 5 years ago

Hi @astrohart,

Thank you for providing more information.

Do you have any practical examples using the current API where decimal is a real problem? If you have examples, please provide the following:

  1. The HTTP request body
  2. The HTTP response body
  3. The C# code that generated the request.

I understand your bot is using double which is easier for you to work with. However, as far as communicating over the network to Coinbase's API servers, are you receiving any HTTP error status codes because of the fundamental conversion from double to decimal?

I'm trying to look at this from a practical perspective; whatever type we use for Money.Amount, these values generally result from money calculations. Be it BTC or USD, these are real money calculations. So, as you've indicated, in principle, utilizing decimal is the right kind of type for financial and monetary calculations.

From an API design perspective, using decimal for Money.Amount, encourages those calculations to happen in decimal; especially important for USD calculations. When performing calculations in double for BTC money, you're forced to make the conversion from double to decimal which forces the developer to think about those calculations a bit more; especially if they're derived from many calculations where rounding can become an issue.

I'm hesitant to make any changes here unless there's a convincing case that requires the need to change types from decimal to string. Also, changing decimal to string would be a huge breaking and inconvenient change for downstream consumers.

Thank you, Brian

astrohart commented 5 years ago

Don’t really have any practical examples. Just wanting to make the math easier.

I simply forked the code and made myself a double property instead of a decimal. I think that will do nicely for me.

-B

On Aug 18, 2019, at 12:26 AM, Brian Chavez notifications@github.com wrote:

Hi @astrohart,

Thank you for providing more information.

Do you have any practical examples using the current API where decimal is a real problem? If you have examples, please provide the following:

The HTTP request body The HTTP response body The C# code that generated the request. I understand your bot is using double which is easier for you to work with. However, as far as communicating over the network to Coinbase's API servers, are you receiving any HTTP error status codes because of the fundamental conversion from double to decimal?

I'm trying to look at this from a practical perspective; whatever type we use for Money.Amount, these values generally result from money calculations. Be it BTC or USD, these are real money calculations. So, as you've indicated, in principle, utilizing decimal is the right kind of type for financial and monetary calculations.

From an API design perspective, using decimal for Money.Amount, encourages those calculation to happen in decimal from an API perspective; especially important for USD calculations. When performing calculations in double for BTC money, you're forced to make the conversion from double to decimal which forces the developer to think about those calculations a bit more; especially if they're derived from many calculations where rounding can become an issue.

I'm hesitant to make any changes here unless there's a convincing use case that necessitates the need to change types from decimal to string. Also, changing decimal to string would be a huge breaking (and inconvenient) change for downstream consumers.

Thank you, Brian

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

bchavez commented 5 years ago

Hi @astrohart ,

Thanks for the reply. No problem. If you stumble on any use case where decimal causes some blocking issue with Coinbase's API servers or causes a specific issue in a cryptocurrency, please let me know and we'll consider changing types.

Also, I'll keep in mind if this issue comes up again for others.

For now, I'd like to keep decimal for the reasons outlined in my previous post.

Thanks, Brian