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
Now that we have the `NumOpResult<Amount>` type that is used to show a
math calculation returned a valid amount we can use it when multiplying
weight and fee rates thus removing panics.
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.
There is an as yet unresolved discussion about the unchecked amount
constructor. In an effort to focus the amount of changes required later
and also to make the `tests` module uniform use the `sat` and `ssat`
constructor functions everywhere.
Internal change only, no logic changes.
Throughout the `amount::tests` module we use `sat` and `ssat` as aliases
to amount constructors but in on test we use them as `Denomination`
variables. To assist clarity and so we can introduce uniform usage of
the constructor aliases change the variable names to use the `den_`
prefix.
Internal change only, no logic changes.
0a9f14f7b0 Implement Div by amount for amount types (Tobin C. Harding)
b57bfb9bc5 Add missing Mul impls for amount types (Tobin C. Harding)
501c9ab89e Test amount ops that involve an integer (Tobin C. Harding)
851080d3b1 Add more add/sub tests (Tobin C. Harding)
47923957b1 Improve add/sub tests for amount types (Tobin C. Harding)
8bb9ce3e47 Add tests for amount op int (Tobin C. Harding)
Pull request description:
Improve the test coverage and add missing implementations of math operations for the amount types.
Along the way close#4030.
ACKs for top commit:
apoelstra:
ACK 0a9f14f7b036c5232449d058fb6d425c8376d87a; successfully ran local tests; nice!
Tree-SHA512: f303b2a90b5bb9e77091e047f8325821a5c89f52dfe242d849968dba0d097d3868d444009c2c05b9d7c0e91fa2ce6898cdc4733977699ca4b1ae226562878cdf
97453ef9bc units: Prevent casting pub enums as ints (Tobin C. Harding)
Pull request description:
A public enum with simple variants gets an automatic integer variant that can be cast by library consumers. This puts a unnecessary maintenance burden upon us because we cannot then add variants in the middle of others.
Add a hidden variant to the single public non-error enum in `units`.
ACKs for top commit:
Kixunil:
ACK 97453ef9bc
apoelstra:
ACK 97453ef9bc2b99a67252419ff015f13679df7312; successfully ran local tests
Tree-SHA512: 2515152107fb21a2dbdef9b46308fef6bd45f4a9719da7a39149b3bdbce6a93dc0f98e112ac246eb32dbe4df1210d5e6328c26ea8678e3da15276e893b39cc9c
A public enum with simple variants gets an automatic integer variant
that can be cast by library consumers. This puts a unnecessary
maintenance burden upon us because we cannot then add variants in the
middle of others.
Add a hidden variant to the single public non-error enum in `units`.
It is semantically valid to divide an amount by another amount. The
result of the operation is an integer.
Note that we cannot implement `Div` by `NumOpResult` because there is no
way to show the div by invalid case.
Implement `Div` by amount for both amount types.
Add a few macros to test `Add` and `Sub` impls for both amount types,
all combos of type and res (eg `Amount` and `NumOpResult<Amount>`), and
all combos of references.
This is a bit ugly and requires that we put our where-clauses in
parentheses because the macro_rules parser sucks, but it allows us to
move the blanket-impls on NumOpResult into the macro.
This commit moves one instance and updates the macro; the next commits
will change the rest.
Looks like a large diff but if you run
git show --color-moved-ws=allow-indentation-change
you will see that it's 100% moves (though moves of code into the
reference macro). May be easier to just look at src/amount/result.rs
after this; it's pretty short now.
The next commit changes a lot of code, but almost entirely by moving and
indenting it. We try to do the moves here ahead of time, so it the diff
for the next commit will be just deletions and indentations.
This macro can generally handle a lot of different cases where we
implement "the same trait but on references". We introduce it here and
use it in two places. We will use it in many more, but I wanted to make
the diff small on this commit, which introduces the actual macro code
and might take a bit of reading to understand.
You may want to use --color-moved-ws=allow-indentation-change to review
this, and the next commit.
The next set of changes will mechanically delete other macros that are
made redundant by this.
The `NumOpResult` type is way more ergonomic to use if it derives
`Copy`. This restricts the `NumOpResult` to being `Copy` as well.
This does restrict what we can include in the error type in the future.
Derive Copy for `NumOpResult` and `NumOpResult`.
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`.
5f75bfaa63 Improve examples on Denomination (Tobin C. Harding)
Pull request description:
Reduce the noise in the examples.
ACKs for top commit:
apoelstra:
ACK 5f75bfaa63309c7526136d430ca8092197ab7c8e; successfully ran local tests; yeah, agreed, this is nicer to read
Tree-SHA512: 01c5863f8712a8ca3b38d3f96be9d08078ca28d8cfc3dd8e8528c388e5f82406a0d43def552b7b53f034c9bf440f7d2d0fec6a760cf69a245b109d0ce4e288c3
13a3f490b8 Use Self instead of amount type (Tobin C. Harding)
34e3049ae0 Use sats instead of satoshi (Tobin C. Harding)
00b71a670f Use from_sat_unchecked for hardcoded ints (Tobin C. Harding)
8fdec67f7d Change local var ua to sat (Tobin C. Harding)
c6f056672b Change local var sa to ssat (Tobin C. Harding)
f3e853e07a units: Do trivial refactor of amount::tests (Tobin C. Harding)
dbec9807f9 Shorten identifiers by removing _in_sats (Tobin C. Harding)
154a4420fc Stop using FQP on Amount type (Tobin C. Harding)
8e16a48252 Run the formatter (Tobin C. Harding)
Pull request description:
Do a bunch of refactorings to tease out changes from #3794.
The first 8 are uncontroversial. The 9th one is subjective. The last one is unusual but IMO worth doing because of the relationship between the two amount modules.
Do note that this PR is 100% internal changes - please please don't bike shed this to death.
ACKs for top commit:
apoelstra:
ACK 13a3f490b80e4c8f8e1753111a914315eefd73e6; successfully ran local tests; lgtm
Tree-SHA512: e2ef0e7fbdaaf632a9840920a227a901fbeb55a29398013cd6cb764b1ff7c0a7c5a1648fd8f606e8b5f7523943886f5eff54cf4054d24349feb72f0b4de05b91
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.
Jamil Lambert, PhD
Erick Cestari
Andrew Poelstra
Tobin C. Harding
Fmt Bot
yancy
Shing Him Ng
kilavvy
looklose