Add a condition for parsing zero from string when not denominated.

[yancyribbens]

closes #3307

[coveralls]

Pull Request Test Coverage Report for Build 10833023984

Details

• 30 of 31 (96.77%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.02%) to 82.801%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
units/src/amount.rs	30	31	96.77%

Totals
Change from base Build 10815518805:	0.02%
Covered Lines:	18944
Relevant Lines:	22879

---

💛 - Coveralls

[Kixunil]

Please do it as I recommended in the issue (try to parse it with denomination and if it fails also try the usual parsing function and check for zero). The way you wrote it makes the code possibly inconsistent and I suspect it'd break for inputs like 0.00000000000000001.

[yancyribbens]

Please do it as I recommended in the issue (try to parse it with denomination and if it fails also try the usual parsing function and check for zero). The way you wrote it makes the code possibly inconsistent and I suspect it'd break for inputs like 0.00000000000000001.

I'll take a look again. It seemed overly complicated to do it that way, but maybe I overlooked something.

[yancyribbens]

@Kixunil I've updated it as you suggested. It's a bit more verbose but I understand wanting consistency. I'm kinda surprised not just using standard float parsing under the hood, although I imagine there is a reason..

[yancyribbens]

It looks like alloc is now required unless I'm missing something.

[Kixunil]

I'm kinda surprised not just using standard float parsing under the hood, although I imagine there is a reason..

The reason is floats are absolutely wrong for financial applications because they are not precise enough. There's a ton about this on the Internet, I'm sure you'll find more information easily if you need it.

alloc shouldn't be needed but it might if we have input string that is not InputString. I'll look into it later, I can only do surface-level stuff right now.

[yancyribbens]

The reason is floats are absolutely wrong for financial applications because they are not precise enough. There's a ton about this on the Internet, I'm sure you'll find more information easily if you need it.

Hmm that's interesting. I read that financial applications usually use decimal notation with fixed precision vs a float where the precision "floats". I take it that means the rust library rust_decimal would be comparable to Amounts numeric representation.

alloc shouldn't be needed but it might if we have input string that is not InputString. I'll look into it later, I can only do surface-level stuff right now.

The source string "0" has " sat" concatenated to the end so that it doesn't fail to parse. somehow there needs to be an allocation so that the source string can resize to hold the denomination which is why I think it needs alloc, unless Im mistaken.

[Kixunil]

Also this needs documenting how FromStr differs from from_str_with_denomination. I think the difference makes sense. I've briefly considered deprecating from_str_with_denomination but it looks like it might be useful to keep it.

[yancyribbens]

Also this needs documenting how FromStr differs from from_str_with_denomination. I think the difference makes sense. I've briefly considered deprecating from_str_with_denomination but it looks like it might be useful to keep it.

I would have thought there be a compile error about from_str not being documented. Anyway, now the difference I see is just that from_str doesn't imply denomination is required, however the only case where that makes sense is with zero. Also its counter intuitive the way the naming is done using preposition like with to mean that the denomination is provided in the string and not as formal argument. For example, from_str_in takes denom as an argument, so I would have thought this would be named .._with_denom. I think from_str_with_denomination is more precise as from_str_include_denomination.

[yancyribbens]

/// If you want to parse only the amount without the denomination, use [Self::from_str_in].

On topic, if you want to parse only the amount given a denomination parameter, or something like that is better imo

[Kixunil]

ACK 894f82e

[Kixunil]

A test confirming that 0.00 parses would be nice too.

[yancyribbens]

can just do in another comment since this one is already acked. This is also something that could just be part of a proptest given arbitrary dot placement which is probably more effective.

[Kixunil]

Good idea turning it to proptest, but I'd like to see also various lengths not just dot placement being changed.

[tcharding]

ACK 894f82e

[apoelstra]

ACK 894f82e; successfully ran local tests.