StratisBitcoinFullNode icon indicating copy to clipboard operation
StratisBitcoinFullNode copied to clipboard

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

Open quantumagi opened this issue 6 years ago • 3 comments

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:

  • Don't use decimal.ToString. Use Money.ToString instead.
  • Allow strings with scientific notation to be passed to Money.TryParse (for external callers).

quantumagi avatar Nov 19 '19 07:11 quantumagi

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?

codingupastorm avatar Nov 20 '19 05:11 codingupastorm

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

  • scientific notation being passed to us by external callers
  • consistency in how we ourselves convert money values to strings.

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 avatar Nov 20 '19 05:11 quantumagi

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

quantumagi avatar Nov 20 '19 07:11 quantumagi