Change Amount::MAX from u64::MAX to Amount::MAX_MONEY#3693
Change Amount::MAX from u64::MAX to Amount::MAX_MONEY#3693apoelstra merged 1 commit intorust-bitcoin:masterfrom
Amount::MAX from u64::MAX to Amount::MAX_MONEY#3693Conversation
a957b7f to
0922c82
Compare
Pull Request Test Coverage Report for Build 12161613662Details
💛 - Coveralls |
|
utACK 0922c82c167e62e92cc57a2f9e284c5c9885dcb3 -- but I guess the tests don't pass on your first commit? If not, can you squash everything together so that they will pass on every commit? (Alternately, in your first commit, you can just add |
0922c82 to
a2bf6b1
Compare
|
I went with the easier option of squashing |
To prevent rounding errors converting to and from f64 change `Amount::MAX` to `MAX_MONEY` which is below the limit in f64 that has issues. Add checks to `from_str_in`, `checked_add` and `checked_mul` that the result is below MAX, where previously a u64 overflow was relied on. Change tests to account for new lower MAX that is within the range of SignedAmount and does not overflow so easily Remove overflow tests `Amount::MAX` is now below `u64::MAX` and within the range of values for `SignedAmount`. These tests therefore do not overflow. In effective_value there is no error with `Amount::MAX` and the correct value is returned. In psbt the removed test is effectively the same as the previous test. Modify `Amount` tests to work with new `MAX` Tests need to be changed that checked values above the new `MAX` or `Amount::MAX` was out of range for `SignedAmount` which it isn't anymore
a2bf6b1 to
6950c0a
Compare
apoelstra
left a comment
There was a problem hiding this comment.
ACK a2bf6b134e080169e2e2903b24821b3fc3251d86; successfully ran local tests
I can just rebase these changes into 3652 since this a smaller subset of changes I propose making in 3652. |
| pub const MIN: Amount = Amount::ZERO; | ||
| /// The maximum value of an amount. | ||
| pub const MAX: Amount = Amount(u64::MAX); | ||
| pub const MAX: Amount = Amount::MAX_MONEY; |
There was a problem hiding this comment.
Can we just remove this if we are not enforcing this in constructor and not doing #3692 ? It would be confusing to have something named MAX, but we can construct values more than that.
There was a problem hiding this comment.
I think we do intend to enforce this in the constructor.
There was a problem hiding this comment.
I can change from_sat in this patch to check if the value is above MAX. The other functions that create an Amount use either checked_mul or from_str_in which do check already.
If from_sat is called with a value above MAX should it panic?
There was a problem hiding this comment.
Let's discuss the from_sat change in another PR. My feeling is yes, and we should introduce a new from_sat_unchecked.
There was a problem hiding this comment.
If from_sat is called with a value above MAX should it panic?
In #3692 I created from_sat_unchecked which panics if above MAX and also from_sat which returns None if greater than MAX. I've based that branch off of this one and I think this PR looks good to merge as is. As for MAX_MONEY, an argument to keep it in for now is that core defines the same const named MAX_MONEY as the supply cap.
There was a problem hiding this comment.
Yeah, I think we should keep both the Rustic MAX constant and the Bitcoin MAX_MONEY constant, and just document that they're synonyms.
There was a problem hiding this comment.
I have opened issue #3696 as suggested below to address enforcing MAX.
|
Can you write an issue please @jamillambert "enforce MAX invariant in Amount" or some such thing. Then link to it here please. |
|
Just because I hit it today; here is a pleasant example of a usecase for an |
|
@tcharding I guess you mean "more than MAX_MONEY". And that display, depending on how it is calculated, will easily overflow the mantissa of a f64 and less-easily overflow a u64. So anything displaying it needs to have special logic anyway (or just accept that it'll be wrong). |
|
Oh I was just commenting that here was an example of a float that should be able to be parsed into a type and the value be less that MAX_MONEY. It was purely out of interest. |
As discussed in #3688 and #3691 using
u64::MAXcauses errors when converting tof64soMAX_MONEYis should be used as the maximumAmount.Amount::MAXchanged to equalMAX_MONEYDifferent approach to the existing PR #3692. I have only done Amount and not SignedAmount until there is feedback on which approach to use.