Remove min_value and max_value#1829
Conversation
056c219 to
38da9ac
Compare
bitcoin/src/amount.rs
Outdated
There was a problem hiding this comment.
We could leave this function name the same and change it to:
pub const fn max_value() -> Amount { Amount(u64::MAX) }
That way, a depreciation warning for Amount::max_value() can be added instead of making a breaking change. Same for Amount::min_value().
There was a problem hiding this comment.
Agreed, we should deprecate for a version or two.
|
Concept Ack, although I think it's better to deprecate |
|
Agree with deprecating instead. |
|
No worries, I'll deprecate them and re-spin. Thanks |
Add associated consts for minimum and maximum values to the `Amount` and `SignedAmount` types.
We currently use the functions `min_value` and `max_value` because the consts were not available in Rust 1.41.1, however we recently bumped the MSRV so we can use the consts now.
Our previous MSRV did not support MIN/MAX associated consts so we had methods min/max_value. Now that our MSRV is Rust 1.48.0 we can use the consts. Deprecate min/max_value methods in favor of MIN/MAX associated conts.
38da9ac to
6cab7be
Compare
|
ack 6cab7be |
Kixunil
left a comment
There was a problem hiding this comment.
Hmm, although I don't like having MAX until upper bound is decided we already have max_value. I'm not sure what to do about it.
| /// The minimum value of an amount. | ||
| pub const MIN: Amount = Amount::ZERO; | ||
| /// The maximum value of an amount. | ||
| pub const MAX: Amount = Amount(u64::MAX); |
There was a problem hiding this comment.
I would prefer not to add this until it's resolved whether Amount should have MAX equal to MAX_MONEY.
There was a problem hiding this comment.
I think we should punt on this for this PR. But my feeling is that no, MAX_MONEY feels too "arbitrary" in a language that already makes unchecked overflows quite difficult.
There was a problem hiding this comment.
I also note that Bitcoin Core will parse amounts greater than MAX_MONEY, and if we try to use that as an actual type-enforced maximum, then we won't be able to parse the same amounts, which seems like a discrepancy without a lot of value.
| /// The minimum value of an amount. | ||
| pub const MIN: SignedAmount = SignedAmount(i64::MIN); | ||
| /// The maximum value of an amount. | ||
| pub const MAX: SignedAmount = SignedAmount(i64::MAX); |
I understand the sentiment but this PR in no meaningful way changes the current behaviour, this is just a language version thing - const vs method. |
|
I'm gonna go ahead and merge, on the basis that the |
The new MSRV (1.48.0) uses associated consts MAX/MIN instead of functions, we had functions to be compliant with the old MSRV.
Remove all methodsmin_valueandmax_valueincluding calls to these methods on stdlib types.PR is now split into three patches:
min_valueandmax_value