NeptuneCoins display issues

[dan-da]

I made these simple tests for impl Display that fail:

    #[test]
    fn display_one_nau()  {
        assert_ne!(NeptuneCoins::one().to_string(), "0");
    }

    #[test]
    fn display_decimal_without_trailing_zero() {
        assert_eq!(NeptuneCoins::from_str("0.1").unwrap().to_string(), "0.1");
    }

failures:

---- models::blockchain::type_scripts::neptune_coins::amount_tests::display_one_nau stdout ----
thread 'models::blockchain::type_scripts::neptune_coins::amount_tests::display_one_nau' panicked at src/models/blockchain/type_scripts/neptune_coins.rs:611:9:
assertion `left != right` failed
  left: "0"
 right: "0"

---- models::blockchain::type_scripts::neptune_coins::amount_tests::display_decimal_without_trailing_zero stdout ----
thread 'models::blockchain::type_scripts::neptune_coins::amount_tests::display_decimal_without_trailing_zero' panicked at src/models/blockchain/type_scripts/neptune_coins.rs:617:9:
assertion `left == right` failed
  left: "0.10"
 right: "0.1"

I first noticed that there is a display issue with one nau because a test was using it and the log was printing "0" coins inside a block reachable only by comparing greater than another NeptuneCoin of known zero value. So either the Ord or Display impl is wrong. Ord tests ok, but Display does not.

I'm not sure if it is only one nau that displays as "0", or possibly a larger range. I do see that "0.1" displays as "0.10" for some reason which is not mathematically incorrect but doesn't seem "right" either.

[dan-da]

these lines in the impl Display look suspect to me, and indeed we go inside this if for the 1 nau case where we actually have a non-zero value.

        let rounded = (100.0 * rational).round();
        if rounded.is_zero() {
            write!(f, "0")
        }

why 100.0? Seems short and arbitrary. And I'd be much happier if we could avoid floating point math and rounding altogether. floating point is usually avoided like the plague when representing monetary amounts.

that said, I don't understand this code well enough yet to be certain that is the problem or suggest a proper fix.

[aszepieniec]

According to how I intended NeptuneCoins to work, these tests should fail.

In my understanding, to_string() does not need to be lossless. So it is okay to round amounts to the nearest hundredth Neptune. Also, whenever both digits after the period are zero, they are dropped along with the period; but otherwise the period and both digits are shown. In that case:

• 1 nau, after rounding to the nearest hundredth Neptune, is 0 and should display as "0".
• 0.1 Neptune rounds to 0.1 Neptune and should display as "0.10".

That said, the first test seems suspect because NeptuneCoins::one() gives one nau rather than one Neptune. Due to the possibility for confusion, I suggest removing the implementation of One.

I'm not sure if it is only one nau that displays as "0", or possibly a larger range.

It is a larger range. Any number of nau that is less than 0.005 Neptune should display as "0".

why 100.0? Seems short and arbitrary.

Well, usually on receipts and invoices people write a number of dollars and a number of cents, with a comma or a point separating the whole dollar number from the two remaining digits. I'm not sure how much capacity average users have for reasoning about Neptune amounts in finer granularity than hundredths; and I don't think it's worth degrading the experience of average users for the benefit of a select few who can think in terms of higher granularities.

floating point is usually avoided like the plague when representing monetary amounts.

Agreed. You'll find that floating point is not used internally and used here only for converting to a user-facing display. And you could even make the argument that the temporary floating point variables used for computing this display should be fixed point, and I'll be happy to merge a "fix" to that effect.

And I'd be much happier if we could avoid floating point math and rounding altogether.

The display form (to_string()) is only used for user-facing information. Internally, NeptuneCoins is a wrapper around a u128 which counts the number of nau, which are indivisible units. Internal mechanics only ever invoke u128 arithmetic, never floating point arithmetic. If you want a display function that doesn't lose information to rounding, how about adding a new one? And then the question revolves around which one to use for user-facing info.