913360b112 Make struct titles consistent (Jamil Lambert, PhD)
afe9ddd5e6 Remove - in fee rate (Jamil Lambert, PhD)
ebc6b4a876 Make warning text bold (Jamil Lambert, PhD)
Pull request description:
I have read through all of the `units` docs and made a few changes.
- Highlight `Warning!` in bold in `Amount` and `SignedAmount`
- Change the one occurrence of fee-rate to fee rate to be consistent with the rest.
- Make all of the error structs have the same title format of `Error returned...`
- Make all other structs have the same format concisely stating what it is opposed to what it does.
ACKs for top commit:
tcharding:
ACK 913360b112
Tree-SHA512: 4cb08d1dae091f5b827cf9f1e931b057c6670002146a22da54886148f3052f6ea7050fcd7f62c0d83438ef170e2f109c1a36f47a280808f31466da6f3177dd01
From my reading of the new sanity rules (#4090) we should only have a
single constructor that accesses the inner field of the amount types.
Furthermore we have one const constructor inside the privacy boundry and
a couple outside.
Move the const constructors outside of the privacy boundry.
Internal change only.
Please note
The function being inside privacy boundary allows it to not have the
"runtime" check (most likely optimized-away after inlining). But if we
wanted to get rid of that check we should have _unchecked method
instead. But we don't want that (yet), since the check here will have
zero performance impact in optimized builds and it's not worth the
cost of dealing with unchecked constructors to optimize debug builds.
Recently I wrote a panic message that included the maximum value of an
integer however I used the max of a 16 bit value for both signed and
unsigned - this is incorrect.
Use the correct values for `u16::MAX` and `i16::MAX`.
This commit standardizes the function signatures in the Amount and SignedAmount
implementations by consistently using Self as the return type instead of the concrete
type names. This makes the code more consistent, easier to maintain, and follows Rust's
idiomatic practices.
Changes:
Replace all occurrences of -> Amount with -> Self in unsigned.rs
Replace all occurrences of -> SignedAmount with -> Self in signed.rs
Make similar replacements for Option/Result return types
Use Self:: instead of the explicit type name for static method calls
It's conceptually a bit tortured to have an `Amount` type defined in a
private module, with an _unchecked method allowing you to set values out
of range, which needs to be used outside of the module to *define* the
range and the constructors that check it.
Move the constants and constructors inside the privacy module, where they
can be written directly. This is easier to understand and eliminates a couple
_unchecked calls.
This private function is used for string-parsing an amount. It returns a
sign boolean and a u64, but its surrounding logic can be simplified if
it just returns a i64 (which is OK now since the range of `Amount` fits
into the range of i64).
Along the way we eliminate some instances of from_sat_unchecked.
Annoyingly we still need the bool to distinguish -0 from +0; when
parsing Amount we reject -0 (and we have tests for this).
This causes a couple error constructors to no longer be used outside of
unit tests. May be worth looking at whether we need them, or whether we
should be using them in parse_signed_to_satoshi.
We have a ton of calls to `from_sat_unchecked` for small constants which
were clearly in range, e.g. in fee.rs. Add a new constfn for these
cases. Don't bother making a generic Into<u32>/Into<u16> variant because
there isn't an obvious name for it.
There are 7 instances where we're using this method with values that are
out of range, which we leave as from_sat_unchecked for now.
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`.
merge-script
Jamil Lambert, PhD
Tobin C. Harding
Fmt Bot
Erick Cestari
yancy
yancy