Skip to content
This repository has been archived by the owner on Aug 16, 2021. It is now read-only.

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

Open
wants to merge 2 commits into
base: release/3.0.8.0
Choose a base branch
from

Conversation

quantumagi
Copy link
Contributor

@quantumagi quantumagi commented Nov 19, 2019

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 quantumagi changed the title Improve consistency when converting money to string values [3.0.8.0] Improve consistency when converting money to string values Nov 19, 2019
@quantumagi quantumagi requested a review from fassadlr November 19, 2019 11:21
@codingupastorm
Copy link
Collaborator

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
Copy link
Contributor Author

quantumagi commented Nov 20, 2019

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
Copy link
Contributor Author

quantumagi commented Nov 20, 2019

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 quantumagi changed the title [3.0.8.0] Improve consistency when converting money to string values [3.0.8.0] API models shouldn't use string to hold Value amounts Nov 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants