We have `from_int_btc_const` on both `Amount` and `SignedAmount` because
the "real" `from_int_btc` is generic over the integer type it accepts,
which means that it cannot be a constfn. But we do want a constfn.
However, just because `from_int_btc_const` exists for the sake of
constfn doesn't mean that that's what it *is*. So rename these methods
to reflect what they *are*.
The `from_int_btc_const` constructors are specifically designed for
easily creating amount types in const context but currently they return
an error which is annoying to handle in const context. If we make the
`whole_bitcoin` parameter a 16 bit integer this gives us a nicer const
constructor with the downside that it can only create values upto a
maximum of
- unsigned: 65_536
- signed: 32_767
That is plenty high enough for most use cases.
Then use the new `from_int_btc_const` in associated consts.
Note that because `from_sat` checks max (and min) values we must
define max and min from sats directly.
Enforcing the MAX_MONEY invariant is quite involved because it means
multiple things:
- Constructing amounts is now fallible
- Converting from unsigned to signed is now infallible
- Taking the absolute value is now infallible
- Integer overflow is illuminated in various places
Details:
- Update from_sat to check the invariant
- Fix all docs including examples
- Use the unchecked constructor in test code
- Comment any other use of the unchecked constructor
- Deprecate unchecked_abs
- Fail serde (using the horrible string error variant)
- Try not to use the unchecked constructor in rustdocs, no need to encourage unsuspecting users to use it.
- Use ? in rustdoc examples (required by Rust API guidlines)
- Remove TryFrom<Amount> for SignedAmount because the conversion is now infallible. Add a From impl.
- Fix the arbitrary impls
- Maintain correct formatting
- Remove private check_max function as its no longer needed
I royally botched the recent effort to make const amount constructors
use a smaller type. I left in an unnecessary panic and forgot to do
both of them.
Note these function return values will change again very shortly when we
start enforcing the MAX_MONEY invariant. However the 64 to 32 bit change
is unrelated to that and is easier to review if done separately.
Whole bitcoin can not in any sane environment be greater than 21,000,000
which fits in 32 bits so we can take a 32 bit integer in the whole
bitcoin constructors without loss of utility. Doing so removes the
potential panic.
This is a breaking API change. We elect not to deprecate because we want
to keep the same function names.
The unchecked-should-be-unsafe conversation is out of scope for this
patch. We want to bite off small chunks so the constructors are left as
they currently are - we are just doing the encapsulation here. This is
in preparation for enforcing the MAX_MONEY invariant which is not
currently enforced.
As per the sanity rules policy outline in:
https://github.com/rust-bitcoin/rust-bitcoin/discussions/4090
For both amount types create a private `encapsulate` module that
consists of exactly the type and a single constructor and a single
getter.
We are about to start enforcing the MAX_MONEY invariant. Doing so will
change constructors to return an error type.
In preparation use the `_unchecked` constructor for all the consts.
Internal change only, no logic changes.
We would like to return an error when doing math ops on amount types.
We cannot however use the stdlib `Result` or `Option` because we want to
implement ops on the result type.
Add an `AmountOpResult` type. Return this type from all math operations
on `Amount` and `SignedAmount`.
Implement `core::iter::Sum` for the new type to allow summing iterators
of amounts - somewhat ugly to use, see tests for example usage.
The ops traits return `Self::Output` not `Self`. The current code builds
because `Self` and `Self::Output` are both the same type.
Use `Self::Output` as the return value of `ops::Rem`.
I claim that if the two amount modules are coded as similarly as
possible it will be easier to ensure that we have the API's uniform and
bug free. To make auditing the modules easier and less error prone use
`Self` instead of the explicit type. This makes it easier to see
differences in the modules and to ensure the differences are correct and
required.
Internal change, no logic changes whatsoever.
The mining reward for the first epoc is 50 bitcoin. For mainnet this is
old news but for regtest it is still relevant.
Add and use a new const `FIFTY_BTC` to the `Amount` type. To keep the
amount types uniform also add it to the `SignedAmount`.
As we did for `SignedAmount` add a constructor to the `Amount` type that
does no checks on its argument.
(This is in preparation for enforcing the MAX_MONEY invariant.)
Use the `_unchecked` version in a single unit test. The rest of the unit
tests will be refactored later to minimise the size of this patch.
2e482f0fdd Remove unnecessary floating code comment (Tobin C. Harding)
Pull request description:
Code comments that comment and arbitrary block "section" of code are almost always pointless and almost always go stale over time.
These particular code comments add almost no value.
Remove code comments.
ACKs for top commit:
jamillambert:
ACK 2e482f0fdd
apoelstra:
ACK 2e482f0fddb55da897f0ba8ea4d3fa5bb0fba1b5; successfully ran local tests; yeah, in this case I agree
Tree-SHA512: 9cd5891e4d91af5206d99b5a2021bc82cc33e3c11d66364442a1a16866d2329ed3a005865cec1a76db80eb3191495a1710a683bc5a69284a29f164a1285b42ea
Code comments that comment and arbitrary block "section" of code are
almost always pointless and almost always go stale over time.
These particular code comments add almost no value.
Remove code comments.
Any SignedAmount can now be cast to Amount since the range is the same.
Specifically, the range for SignedAmount is (- 21 million, 21 million)
while the range for Amount is (0, 21 million). Therefore any value from
Amount can be cast to a SignedAmount and it will work. Note it's not
the same and still requires checking when going from SignedAmount to
Amount since Amount can't handle the negative range.
As a side effect of changing the return type, TryFrom is no longer valid
and does not compile. Therefore in addition to changing the return
type, TryFrom is also removed.
Add all the pedantic lints to the repository by way of the repository
manifest. Then enable these lints in the `units` manifest.
Some things worth mentioning:
- Fix `needless_pass_by_value` by adding derives to `FormatOptions`.
- Fix lint `cast_lossless` using `cargo clippy --fix``
- While fixing `lint enum_glob_use` introduce a new style to the
codebase; import enums using a single character. Doing so prevents
namespace clashes, improves clarity, and maintains terseness.
Audit:
Use the following lints locally and audit all the warnings, they produce
many false positives so we can't enable them permentently.
- `cast_possible_truncation`
- `cast_possible_lint`
- `cast_sign_loss`
f97c04cf07 api: Run just check-api (Tobin C. Harding)
99b32c95b8 Change paramater type used for whole bitcoin (Tobin C. Harding)
Pull request description:
We have multiple constructors for `Amount`, one of which accepts an integer representing whole bitcoin.
In an attempt to make the API crystal clear change the parameter type used for whole bitcoin to `u32` instead of a `u64` which may well be mapped to sats in the mind of developers (it is in mine anyway).
ACKs for top commit:
apoelstra:
ACK f97c04cf07c2ac5acb5d312b573d100afa009b57; successfully ran local tests
Tree-SHA512: fb22f08f182a96ff8d067337e32174137e3d73acd2faeddc2bac974ea41f8de9ec06baa2e6fb76a2fa1a7afa7bf6b4c8f606666d966413a14d39b129e36d0674
We have multiple constructors for `Amount`, one of which accepts an
integer representing whole bitcoin.
In an attempt to make the API crystal clear change the parameter type
used for whole bitcoin to either `Into<u64>` or `u32` instead of a
`u64` which may well be mapped to sats in the mind of developers (it
is in mine anyway).
These functions do not add value because one can just call `to_sat` and
`from_sat` if doing ops in a tight loop.
Note we need to remove these deprecated functions before we release
`units v1.0`.
Currently the consts in the `amount` modules repeat the type. For
`Amount` this isn't a big deal but for `SignedAmount` its quite noisy.
Use `Self` as the type in const declarations inside `Amount` and
`SignedAmount`.
Internal change, no logic changes.
4b926e1908 Change`MAX and `MIN` to equal `MAX_MONEY` (Jamil Lambert, PhD)
Pull request description:
To prevent rounding errors converting to and from f64 change `SignedAmount` `MAX` and `MIN` to +/- `MAX_MONEY` which are within the limit in f64 that has issues.
Add checks to `from_str_in`, `checked_add`, `checked_sub` and `checked_mul` that the result is within MIN and MAX.
Modify tests to work with new `MIN` and `MAX`.
Discussed in #3688 and #3691
ACKs for top commit:
tcharding:
ACK 4b926e1908
apoelstra:
ACK 4b926e1908f72af98d24cc64d7e1eef44d624e4e; successfully ran local tests
Tree-SHA512: 9053e761b3b74a7a9d826ae7271ced41cf7919752ac8ed8977a20b5b1a746ac8a6bfff68159f4a0dea733ea00f49cf41c0422de53c7aff39efd8482f8cba6069
To prevent rounding errors converting to and from f64 change
`SignedAmount` `MAX` and `MIN` to +/- `MAX_MONEY` which are within the
limit in f64 that has issues.
Add checks to `from_str_in`, `checked_add`, `checked_sub` and
`checked_mul` that the result is within MIN and MAX.
Modify tests to work with new `MIN` and `MAX`
Use the `must_use_candidate` clippy lint to find all functions that are
candidates for having `must_use`.
Add `must_use` attribute but exclude obvious functions like `from_`,
`to_`, and `new`.
This patch is subjective.
fd2a5c1ec7 Close amounts error types (Tobin C. Harding)
23c77275b1 Reduce code comment lines (Tobin C. Harding)
d595f421c6 Remove whitespace between enum variants (Tobin C. Harding)
Pull request description:
Close the two pubic enum error types in `units::amounts`. All the other structs are closed already because they either have private fields or marked `non_exhaustive`.
ACKs for top commit:
apoelstra:
ACK fd2a5c1ec79f337fb3695c030c9fb6b4671468f2; successfully ran local tests; thanks!
Tree-SHA512: f8d68ef821449e0829c926cf527df4b226b29c8d1d41b320a016fbf70b4b39cc54c8c218955caa0c3776158eeeae0ebacc1cc89dab67bafc399b94063324ab0e
Close the two pubic enum error types in `units::amounts`. All the other
structs are closed already because they either have private fields or
marked `non_exhaustive`.
433f70939c Implement iter::Sum for BlockInterval (Tobin C. Harding)
0369e64b56 Implement Sum for an iterator of references to amounts (Tobin C. Harding)
31f883ac00 Implement iter::Sum for FeeRate (Tobin C. Harding)
Pull request description:
Enables summing an iterator of values. Note that this does not include either `LockTime`s. `absolute::LockTime` should not be added and for `relative::LockTime` we have https://github.com/rust-bitcoin/rust-bitcoin/issues/3676Close: #1638
ACKs for top commit:
apoelstra:
ACK 433f70939c3ecc10702ab6502e3f9bcd94dab739; successfully ran local tests; nice!
sanket1729:
utACK 433f70939c
Tree-SHA512: 1eda00f3bbbc61f795198ce8525a5a9b690478a8abc268da6d2e40de7d91decc28dd8211df0c6abeaf30148c7ec3907b85e3c5351972c354590569840e84d562
The `Amount` and `SignedAmount` were not supposed to implement `serde`
traits by design because doing so implicitly uses sats. We provide two
modules `as_sat` and `as_btc` to allow users to explicitly serialize in
their preferred format.
In commit: `d57ec019d5 Use Amount type for TxOut value field` derives
were added for `serde` and we did not notice it during review.
a8379bf005 Mark `checked_` functions const in bitcoin. (Jamil Lambert, PhD)
fe10ff2eb7 Mark functions `const` in `units` (Jamil Lambert, PhD)
Pull request description:
Following on #3608, #3627 and #1174 the rest of the `checked_` functions in all `rust-bitcoin` have been marked as const. Except for tests and traits.
ACKs for top commit:
apoelstra:
ACK a8379bf0053e66cf5984ce449d19e54e529b6b70; successfully ran local tests; thanks! I find this much easier to read
tcharding:
ACK a8379bf005
Tree-SHA512: b8d97b7a3d9fc33b57349f418ccc5aac2f3e8c4145a73bf0e24e85547217d41214696e9a80b1fb5a1bae7e766aad1c2b5df6044564772fe76cec9b3628bcd8e5
Mark `checked_` functions const. Replace `map()` and `?` operators,
which are not allowed in const context, with match statements.
Use descriptive variable names in ceiling division to make
it easier to follow.
There is a range of different wordings used in the docs of constructor
type functions.
Change all to start with `Constructs a new` or `Constructs an empty`.
In functions that act like constructors there is a mixture of the usage
of `creates` and `constructs`.
Replace all occurrences of `creates` with `constructs` in the first line
of docs of constructor like functions.
Andrew Poelstra
Tobin C. Harding
yancy
Jamil Lambert, PhD
yancy
Fmt Bot
Shing Him Ng