Fix #657: Format Operation FormData Amount Attribute

[banterCZ]

No description provided.

[banterCZ]

👀

[banterCZ]

👀

[romanstrobl]

Good progress. We can add additional formatting options later, if required.

[zcgandcomp]

Please let's use the currency code, not the symbol. It is a compatible change without formating issues on the frontend.

[zcgandcomp]

Looks OK. Thank you.

[romanstrobl]

In which universe does this code make sense?

                .sourceCurrency(sourceCurrencyRaw)
                .sourceCurrencyFormatted(sourceCurrencyRaw)

...

                .targetCurrency(targetCurrencyRaw)
                .targetCurrencyFormatted(targetCurrencyRaw)

This is clearly a bad API design. We are sending sending values which do not correspond to the names of the variables.

I see following options:

1. (preferred) fix the issue properly, which means sending existing values as they should be (raw and formatted) and pass another value which contains fully formatted value (containing both the amount and currency). For example:

• amount: 1000000
• amountFormatted: 1,000,000
• currency: USD
• currencyFormatted: $
• amountAndCurrencyFormatted: US$1,000,000

Mobile token uses the best formatted value and amount, which is US$1,000,000, the other fields stay for compatibility reasons and the app will continue to support them.

This option requires a change in the mobile token application to support such formatting.

2. (not ideal, but works without app changes) fix the issue partially, which means sending the formatted value correctly, for example:

• amount: 1000000
• amountFormatted: 1,000,000
• currency: USD
• currencyFormatted: $

Mobile token uses the best formatted value and amount, which is 1,000,000 $. This is not ideal, but it will not mean that we are sending values in the REST API which do not correspond to the variable names. It will work for most currencies, just not for the dollar-based ones.

3. (worst option) the current proposal, which means that we are passing non-formatted currency into the formatted currency parameter and the mobile app considers this currency as formatted from backend and displays it in the formatted currency field. Yes, it sort of works, but it is a hack. Example:

• amount: 1000000
• amountFormatted: 1,000,000
• currency: USD
• currencyFormatted: USD

How would you explain such design to a developer from another company who wants to publish the operation list endpoint with such an API? Do we really want to tell them they need to put the unformatted value into the currencyFormatted field, like we do?

[petrdvorak]

Hello, here is a nice article on the subject:

https://medium.com/wultra-blog/decoding-money-from-json-in-swift-d61a3fcf6404#833e

I think we already solved this issue years ago, and the correct approach is what Roman suggests as "option 2". Basically, it makes sense to put the values to attributes that correspond to the values. The client app can then decide what to do.

Btw the reason we introduced the *Formatted values in API is explained in the article. Swift had trouble working with double values in JSON. As a result, the mobile front-end needed a hint. The app can decide which values to pick up for the purposes of display. For example, it can use *AmountFormatted and *Currency to format the values on the client side. Also, if Swift fixes the JSON deserialization, it can work with *Amount and *Currency...

[petrdvorak]

This is how "Blockchain" app shows the amounts:

This is what "Wise" does:

I think neither is problematic from the user's perspective. We can keep the API semantics clean.

[romanstrobl]

This is how "Blockchain" app shows the amounts:

I suggest we investigate whether we can do the same formatting. We would use option 2) and just instead of $ use the value US$ which solves the dollar issue.

[banterCZ]

I would like to highlight that we may fix currencyFormatted to use symbols and do not afraid of backward compatibility. Because as far as I know, no customer uses the Amount attribute in the operation list provided by enrollment-server yet.

[banterCZ]

This is how "Blockchain" app shows the amounts:

I suggest we investigate whether we can do the same formatting. We would use option 2) and just instead of $ use the value US$ which solves the dollar issue.

I played a little bit with test cases and it seems that $ is always USD and other dollars or pesos are safe to distinguish.

@ParameterizedTest
@CsvSource({
        "CZK, en, CZK",
        "CZK, cs, Kč",
        "USD, en, $",
        "USD, cs, US$",
        "UYU, cs, UYU",
        "UYU, en, UYU",
        "UYU, uy, UYU",
        "CAD, cs, CA$",
        "CAD, ca, CAD",
        "CAD, en, CA$",
        "NZD, en, NZ$",
        "NZD, cs, NZ$",
        "NZD, nz, NZ$",
        "BTC, cs, BTC"
})
void testFormatCurrency(final String source, final String locale, final String expected) {

[romanstrobl]

I played a little bit with test cases and it seems that $ is always USD and other dollars or pesos are safe to distinguish.

Perfect.

[banterCZ]

Reverted to currencyFormated using symbols, added test cases for dollars.

[romanstrobl]

Works for me.

[romanstrobl]

I disagree about the removal of JSR 354 implementation (JavaMoney) as a dependency and migrating to java.util.Currency.

Rationale:

• The world-wide currency list is being continuously updated and when we use the built-in java.util.Currency class we fix this list of currencies and their properties to a specific Java release. Hence, we may go back in time and new currencies which exist after the Java release are not known.
• We want to keep the currency list up-to-date, I believe the right way is to use the latest release of JSR 354, which is Java Money. This should be reflected in all classes which are used for working with currencies.

If you have other opinion, please let me know, I am open to discussion about what is a better solution, however the last change seems to me like a not properly thought-through step.

[petrdvorak]

@romanstrobl I have no strong opinion about this in particular (with Java native formatter, we will format currencies as good / as bad as the rest of the world). I agree about higher flexibility of an external library, we risk other vulnerabilities or deprecation. Also, I would consider ability to override formatting with our own mappings, to allow US$ formatting.

[banterCZ]

I disagree about the removal of JSR 354 implementation (JavaMoney) as a dependency and migrating to java.util.Currency.

Rationale:

• The world-wide currency list is being continuously updated and when we use the built-in java.util.Currency class we fix this list of currencies and their properties to a specific Java release which is used to compile the code. Hence, we may go back in time and new currencies which exist after the Java release are not known.
• We want to keep the currency list up-to-date, I believe the right way is to use the latest release of JSR 354, which is Java Money. This should be reflected in all classes which are used for working with currencies.

If you have other opinion, please let me know, I am open to discussion about what is a better solution, however the last change seems to me like a not properly thought-through step.

Yes, you are correct. I agree that is it important to limit dependencies, but in that case, it makes sense. The library is responsible for adding new currencies. JDKs may be far behind.
I am going to revert the last change and fix mixture of moneta and java util.

[romanstrobl]

I am going to revert the last change and fix mixture of moneta and java util.

Yes, we should check whether we can use the JavaMoney library for all tasks. I didn't review the imports, but it seems to me that we are using the mix of approaches. I think I tried to stick with JavaMoney in Web Flow, if not, we also have to fix it there.

[romanstrobl]

@romanstrobl I have no strong opinion about this in particular (with Java native formatter, we will format currencies as good / as bad as the rest of the world). I agree about higher flexibility of an external library, we risk other vulnerabilities or deprecation. Also, I would consider ability to override formatting with our own mappings, to allow US$ formatting.

Yes, let's review it, the library should be fine as official JSR 354 implementation.

[banterCZ]

Unfortunately, we have to stick with mixtures, because javax.money API does not yet support the display name of CurrencyUnit, see JavaMoney/jsr354-api#58
I have added a TODO

[romanstrobl]

Unfortunately, we have to stick with mixtures, because javax.money API does not yet support the display name of CurrencyUnit, see JavaMoney/jsr354-api#58

Hmm, sad. Hopefully this will addressed, the feature is targetted for next release.

[romanstrobl]

Let's merge this PR. We can make improvements later, if needed.

usage of Monetary is better from funtional point of view, then pure JDK Currency