stratisproject / StratisBitcoinFullNode

Bitcoin full node in C#
https://stratisplatform.com
MIT License
787 stars 315 forks source link

[3.0.8.0] API models shouldn't use string to hold Value amounts #4087

Open quantumagi opened 4 years ago

quantumagi commented 4 years ago

https://stratisplatformuk.visualstudio.com/StratisBitcoinFullNode/_workitems/edit/3904/

Re: "... currently MoneyFormatAttribute is causing us problems because Money.TryParse doesn't understand scientific notation ..."

I suggest that we improve consistency without changing the APIs in any way. The root issue seems to be that we (or external callers) are not always using the ToString method of the Money class to convert money values to strings. As a result we end up with a string that is not guaranteed to be parsable by the Money class.

Changes in this PR:

codingupastorm commented 4 years ago

Code looks good, I don't really understand the issue it's fixing though? Unless we want sci notation to be allowed?

I think the issue is talking about the request models being passed to the controller?

quantumagi commented 4 years ago

The idea of this PR is to deal with the two issues (in a way that avoids changing the API):

As suggested by @codingupastorm , when we make the next version of the API, we can ensure that we use the same type for the same data - i.e. avoid passing money values as strings.

quantumagi commented 4 years ago

Alternatively adding these changes will give us backwards compatibility:

        public override object ReadJson(JsonReader reader, Type objectType, object existingValue,
            JsonSerializer serializer)
        {
            if (reader.Value is string strValue && Money.TryParse(strValue, out Money value))
                return value.ToDecimal(MoneyUnit.BTC);
            return (decimal)reader.Value;
        }
        public override bool CanRead => true;
        public override bool CanConvert(Type objectType) => objectType == typeof(decimal);

Adding that code will make BtcDecimalJsonConverter accept quoted or unquoted amounts - including scientific notation.

        [JsonConverter(typeof(BtcDecimalJsonConverter))]
        public decimal? OpReturnAmount { get; set; }

after this we won't need MoneyFormat