812c21e2e4 refactor: Replace fold with try_fold (yancy)
Pull request description:
The and_then combinator performs a kind of bitwise and operation on two Option types here. This is useful since the `checked` arithmetic returns an option thereby accumulating Option types. Therefore, either the checked arithmetic operation performs the addition of the unwrapped accumulator, or it returns None.
Instead of using `and_then` use the provided `try_fold` method which will short circuit on `None` when the checked arithmetic is used. Also, simplify the staring condition using `Amount:ZERO` since this is logically equivalent to using the first value if one exists.
Lastly, by using the built in `try_fold`, it's possible the performance will be improved by making use of the short circuit ability instead of evaluating each item even when the accumulator holds a None type.
ACKs for top commit:
apoelstra:
ACK 812c21e2e4a868046b44728c1a6209a866452820; successfully ran local tests
tcharding:
ACK 812c21e2e4
Tree-SHA512: 1cfcd4fa28e2b59daf3744bb5f654f65eb9853c5a36f747cb0859783e7e46c1d02ccb296612b75f7cca10782979ce052cd670c0f23c1030e0a347000d1f6df83
53837d9a2e units: Improve crate level docs (Tobin C. Harding)
Pull request description:
Add a bit more to the crate level docs. This is a simple crate so we don't need all that much.
Done for: C-CRATE-DOC
ACKs for top commit:
apoelstra:
ACK 53837d9a2e1cc70e180de39c16e2d212e958a9f3; successfully ran local tests
Tree-SHA512: 374c27a25cdc9bd4edd0755be02cad66ccccedcd69836506c1f4eb86a1254bfafe11eeb6fcc27b7efd2ab3ca0acd1daa304d482c7e5a7f84ffbcffbb1bcd21d6
The and_then combinator performs a kind of bitwise and operation on two
Option types here. This is useful since the `checked` arithmetic
returns an option thereby accumulating Option types. Therefore, either
the checked arithmetic operation performs the addition of the unwrapped
accumulator, or it returns None.
Instead of using `and_then` use the provided `try_fold` method which
will short circuit on `None` when the checked arithmetic is used. Also,
simplify the staring condition using `Amount:ZERO` since this is
logically equivalent to using the first value if one exists.
Lastly, by using the built in `try_fold`, it's possible the performance
will be improved by making use of the short circuit ability instead of
evaluating each item even when the accumulator holds a None type.
0361604bab Add impls for NumOpResult div and mul (Tobin C. Harding)
Pull request description:
We recently added div and mul for combinations of `Amount`, `FeeRate`, and `Weight`. When doing so we forgot to add variations for `NumOpResult`.
ACKs for top commit:
apoelstra:
ACK 0361604bab6c6ef260410d0bd6e33ce24a41e775; successfully ran local tests
Tree-SHA512: 6d262b9079b8a670f32d58d49e3c7e9a79d5d795a4c9f37f6bc2213879649d41900e95f515d8685c3870c935358bcb25567b2f6f332301e1ad88188056047b7b
da69e636a9 units: Use 100 column width in rustdoc comments (Tobin C. Harding)
53c6ae4d40 units: Remove expect from rustdoc example (Tobin C. Harding)
Pull request description:
A couple of quick docs fixes while trying to polish `units`.
ACKs for top commit:
apoelstra:
ACK da69e636a9d21e602289062279ed5ebc6b1429b6; successfully ran local tests
Tree-SHA512: acfbec90b0327850b882c5e1b1e7eaadbf0a09a30dcc46529386ea419ed74846a678a5980f5706f8d280f30ec6f6d06af2db8f0e1748523b15ad47a654caee4b
Currently we use a std numeric type for the output of various `Div`
implementations while other ops use `NumOpResult`. This makes it
difficult to chain operations.
Throughout the crate use `Output = NumOpResult<Foo>` when implementing
`Div`.
Later we want to enable users differentiating between an overflow and a
div-by-zero. Explicitly do not implement that yet, done separately to
assist review.
We are going to add implementations of `OptionExt` for various other
types and all impls are almost identical. To make doing so easier
macroize the implementation for `Amount` and `SignedAmount`.
Internal change only, no logic changes.
We currently use the `NumOpResult` for operations involving more than
just amount types (e.g. `FeeRate`) however when the `result` module was
written we only used amount types.
To make the intention of the custom result types more clear introduce a
top level `result` module and move the general code there. Leave the
amount implementations in the `amount` module. Note that both `result`
modules are private.
Move the `OptionExt` impls because later we will add a bunch more of them.
Internal change only, no logic changes.
We currently use the `NumOpResult` for operations involving more than
just amount types (e.g. `FeeRate`) however when the `result` module was
written we only used amount types.
To make the docs and code clearer use 'numeric type' instead of
'amount' in docs. And for local variables use `x` instead of `amount`.
This is docs and internal changes only.
3ae21d5111 Use impl_add/sub_assign for block interval (Tobin C. Harding)
9d55922952 Use impl_op_for_references for block height/interval (Tobin C. Harding)
f5e17914b6 Move Assign impls together (Tobin C. Harding)
cc66838e80 units: Remove unnecessary code comments (Tobin C. Harding)
Pull request description:
Improve the ops impls in the `block` module using the already present macros.
ACKs for top commit:
apoelstra:
ACK 3ae21d5111444f5e01f6cfb1a2b9b314f66418a3; successfully ran local tests; nice!
Tree-SHA512: 6565426a06bb47d337d21cf5c59acca43e69228dbec8319fc95373025d220d8ec6273c54f214f312c4229603c455d08e4c6a8c108663c6db5086df36266979de
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.
ab4ea7c13d Enforce the MAX_MONEY invariant in amount types (Tobin C. Harding)
Pull request description:
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 eliminated 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
Close#620
ACKs for top commit:
apoelstra:
ACK ab4ea7c13d08411044bd5f9c17457e926c80ed4d; successfully ran local tests
Tree-SHA512: bec963d8ea69e202f399cd19bca864b06f3e86323d376c2d2126d74093598f8bbbf19792b2327dba0862ef6f0201202778014a2be7a14991f02917d8ca312afb
93c6c8cef5 Use impl_op_for_references macro in fee module (Erick Cestari)
Pull request description:
This pr replaces the individual operator implementations in the fee module with the impl_op_for_references macro to handle reference operations. This removes the need to manually implement reference combinations for operands, simplifying the code and improving consistency.
### Changes:
- Replaces direct implementations of operators with macro usage
- Adds tests to verify that reference operations work correctly
- Maintains the same semantics as the original implementation
Closes#4173
ACKs for top commit:
tcharding:
ACK 93c6c8cef5
apoelstra:
ACK 93c6c8cef59ceed56932d62daeb212c2b40fc4a1; successfully ran local tests; yeah, I think the docs loss is fine -- the docs are hard to find and say exactly what users expect of the / operator
Tree-SHA512: 51d7643c2cecd16a0cb16afcd195fd87bc9eca9116e16518d888ba61a8edb5684162af987ea52611c9463f5299810f92a057dedc3fa8e89cdef21ef40528bca1
c707b959b7 Rename timestamp module to time (Tobin C. Harding)
e2dee4900f Re-name Timestamp to BlockTime (Tobin C. Harding)
Pull request description:
Done in two patches so we can bikeshed the name of the type and separately the name of the module.
- Rename type: `Timestamp` to `BlockTime`
- Rename module: `timestamp` to `time`
ACKs for top commit:
apoelstra:
ACK c707b959b72dd89ca6df581a6102f32daedb8368; successfully ran local tests
Tree-SHA512: de3855b38445a58b6767a6081919eecb81c6c12aee3f6699f3bfa10efaf5770b54fb412da23991a9ee734e14dfb642af670f0218d1886cdc8c8d3f393ef65d7e
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
This commit replaces the individual operator implementations in the fee
module with the impl_op_for_references macro to handle reference operations.
This removes the need to manually implement reference combinations for
operands, simplifying the code and improving consistency.
The change:
- Replaces direct implementations of operators with macro usage
- Adds tests to verify that reference operations work correctly
- Maintains the same semantics as the original implementation
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
We just re-named `Timestamp` to `BlockTime`. We have a `units::block`
module but it currently holds abstractions (`BlockHeight` and
`BlockInterval`) that are not onchain abstractions and therefore
somewhat different from the `BlockTime`. Instead of making `block` a
block 'utils' module instead re-name the `timestamp` module to `time`.
merge-script
yancy
Tobin C. Harding
Jamil Lambert, PhD
Fmt Bot
lfgtwo
VolodymyrBg
Erick Cestari