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.
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`.
f15e461baf Add an exclusion for a mutant in a deprecated fn (Jamil Lambert, PhD)
9a2b56f381 Modify test in Amount to kill mutants (Jamil Lambert, PhD)
1d1cf00b1e Add test to BlockTime to kill mutants (Jamil Lambert, PhD)
Pull request description:
Weekly mutation testing found mutants in `Amount` and `BlockTime`.
Add a test to `BlockTime`.
Add two asserts to `Amount` tests.
Exclude deprecated function from mutation testing.
Closes#4225, #4243, #4278
ACKs for top commit:
apoelstra:
ACK f15e461baf463da0bf9d89018180c1d5032106c1; successfully ran local tests
Tree-SHA512: 18a405362db1b2eabac7c7ac01a56d306a1bf5f705626b5c217ec329e6420daa2f2e62b37c72537f29b7ee76b9fd795adde2da71b226fec3a74d0f25dca6cd96
9a572dabde refactor: use path dependencies for workspace members in bitcoin/Cargo.toml (Eval EXEC)
Pull request description:
This PR want to:
1. make all workspace members use `workspace = true` syntax to import dependencies.
2. use `path = [balabala]` to define dependencies, instead of useing `[patch.crates-io.balabala]` , fix: https://github.com/rust-bitcoin/rust-bitcoin/issues/4283
ACKs for top commit:
Kixunil:
ACK 9a572dabde
apoelstra:
ACK 9a572dabdeb077f96b2ab66be1a80fcec3e805e3; successfully ran local tests
Tree-SHA512: 834ef881ed3fd324a9ecca440e8e591984a7e474eb6aeab86a0301cbd08b6dc96ecdc34b306ad146b11b50f7488024c289b8f8c7c6de1a2bdba7aec515b722ee
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
e744347022 Make usage of Self and type uniform across both modules (Erick Cestari)
dfb49f014c Rename impl_try_from_array to impl_from_array (Erick Cestari)
Pull request description:
This PR makes two main changes:
1. 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 improves code consistency, maintainability, and follows Rust's idiomatic practices.
2. Renames `impl_try_from_array` to `impl_from_array` to better reflect its functionality.
### Changes
**Consistent usage of Self instead of concrete types**
- 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 explicit type name for static method calls
**Function rename**
Renamed `impl_try_from_array` to `impl_from_array` for better clarity
### Related Issues
Closes#4210Closes#4241
ACKs for top commit:
Kixunil:
ACK e744347022
tcharding:
ACK e744347022
apoelstra:
ACK e744347022d1ad1e0ca0a83ec9350501af08297b; successfully ran local tests
Tree-SHA512: 3113f3ccf595b298afe6b23514f1de790284df7fcb55a13658aabe3ef4fcea0e401b65b0a2c67ac18da87a1bcd247bd1f1484856fe03470b98dfa2614958a3bb
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`.
We just added a `Timestamp` type without knowing that there was a push
by OpenTimestamps to also create a timestamp and that our new type may
lead to confusion. Our timestamp is explicitly for the `time` field in a
block so we can call it `BlockTime`. This name change makes the module
name stale but we will change that in a following patch to ease review.
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
90d909becc Kill mutants in primitives and units (Shing Him Ng)
Pull request description:
This kills 15 mutants found with the mutants workflow. Ran `cargo mutants` locally to confirm
Closes#4156Closes#4106
ACKs for top commit:
jamillambert:
ACK 90d909becc
tcharding:
ACK 90d909becc
apoelstra:
ACK 90d909becc4638c03003845154e9cc1eb5f3ad41; successfully ran local tests
Kixunil:
ACK 90d909becc
Tree-SHA512: e5c95a1c4054cf1c60c940ea605eec84dffcbff292f9c7c4d96813c6389e807c318f6c5f8f69ceeb9ffcab3c3e45aa0d5a8fda7335d540c6f070aab92bae7a0f
b3f122b399 Add Timestamp newtype (Tobin C. Harding)
Pull request description:
Bitcoin block headers have a timestamp. Currently we are using a `u32`. While this functions correctly it gives the compiler no chance to enforce type safety.
Add a `Timestamp` newtype that is a thin wrapper around a `u32`. Document it and test the API surface in `api.rs`.
ACKs for top commit:
apoelstra:
ACK b3f122b3996c1a73479be2f95b7f2ae642c9c56f; successfully ran local tests
Kixunil:
ACK b3f122b399
Tree-SHA512: 6f4a4a588bc836243ae28f3d36be6c0ae264cb2b7a0061277910b107d05e5ca0e679497d2890208f5d8ec148f37bf263bcd0b0410f9e5e6370d8e763ff30b78a
2434afc40d Make Weigth::from_kwu const (Tobin C. Harding)
1fe984552e Improve Weight rustdocs (Tobin C. Harding)
Pull request description:
Do two minor improvements to the `weight` module. Docs and add `const`.
ACKs for top commit:
apoelstra:
ACK 2434afc40dadd213f8695c5ce91d397c03f337be; successfully ran local tests
Kixunil:
ACK 2434afc40d
Tree-SHA512: e3e9653d5fcd060c27a2313e642d7b96f51b9342953505a30a9748cb7f0c19a87bcb1faadb1b07ecc770aaec45496e5a750ac48e3e9141e379c554f0875df6a1
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`.
8007840676 Add a test for remainder (Jamil Lambert, PhD)
4787aa1f89 Implement Rem for Weight (Jamil Lambert, PhD)
Pull request description:
Weight implements `Div` but not `Rem`.
Add the `Rem` implementation.
Add a test for the remainder operation on `Weight`
Close#4171
ACKs for top commit:
Kixunil:
ACK 8007840676
tcharding:
ACK 8007840676
apoelstra:
ACK 80078406768aa5f8e4d21d42cc1d1fe9abeed1ea; successfully ran local tests
Tree-SHA512: cfcbc49a944146f7ba24a2f7b7f79ed2e2758ba2a23e9e69d9662afb5379dd74c646ff787c0e8218053e70410353cb74115e4c03bfdaafc8a1f9a4b0e51a3e15
83d071e54b chacha20: Add whitespace (Tobin C. Harding)
4451724d31 chacha20: Add a docs heading (Tobin C. Harding)
d4417f9666 io: Improve crate docs heading (Tobin C. Harding)
c466554948 hashes: Improve crate docs heading (Tobin C. Harding)
6f4eb60936 Improve docs crate headings (Tobin C. Harding)
Pull request description:
Make them all uniform after taking 2 minutes online to try find a nice format.
ACKs for top commit:
apoelstra:
ACK 83d071e54be0bc4ebd760a490a3ca887c0bf90a8; successfully ran local tests; lgtm
Tree-SHA512: 6f08c6cda91a7a870f1080b497f89607ac3d6b3c0234cbd2ba2da8710d46816398acac0bca2a49a3bc9466b814ae446842d3d304a3735df9f983e3ff5df005db
ce19d40a80 chore: fix some typos in comments (looklose)
Pull request description:
fix some typos in comments
ACKs for top commit:
apoelstra:
ACK ce19d40a805e2b67f6199cce61a73c6592019b5c; successfully ran local tests
tcharding:
ACK ce19d40a80
Tree-SHA512: 4bf5d339e4e73a6f1378881ad62b17f78796e6000f24eea6b3a381d3e03f4fd8b9775710a7eb5a087bfe0d557d1ab1a8291dcc7b86a2c9ded3318774cf2e5b68
I took a look at the rendered HMTL of `bitcoin`, `primitives`, `units`,
`serde`, and `tokio` and picked a header style that I thought looked
good.
Use it for `primitives` and `units`.
We have a new macro for implementing ops with a bunch of reference
combos. Lets use it for block `Height` and `Interval`.
This patch is strictly additive.
Next patch will move all the impls of `Add` and `Sub` into a macro call.
In order to make that patch smaller move the assign impls to be together
below the add/sub impls.
Code move only, no logic change.
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.
bb8f833ca0 Update instruction.rs (kilavvy)
0ce622e668 Update message.rs (kilavvy)
f61941bbe6 Update serialized_signature.rs (kilavvy)
1d2de62e01 Update mod.rs (kilavvy)
Pull request description:
This PR fixes several typos in comments across multiple files:
- Fixed typo `interpretted` -> `interpreted` in `blockdata/script/instruction.rs`
- Fixed typo `neccessity` -> `necessity` in `p2p/message.rs`
- Fixed typo `underlflow` -> `underflow` in `taproot/serialized_signature.rs`
- Fixed typo `ambigous` -> `ambiguous"` in `units/src/amount/mod.rs`
These changes only affect comments and documentation, no functional code changes.
ACKs for top commit:
apoelstra:
ACK bb8f833ca01688eaae75e0fa322f698d34243185; successfully ran local tests; though all these commits could be squashed IMO
Tree-SHA512: d73dc2a86b20de87c0c5efb3e5042e3901c846236670e3a6501f4c93fd54328fef08bfeca276b93642e7b51d04cb8b9c8e1af558f3aabc3c924d60a61e58b031
8787304425 units: Improve code comment on macros (Tobin C. Harding)
Pull request description:
We have two macros that are hidden because they are code de-duplication tools. However the output they produce is, and has to be, stable so that we can use them in `units` and `primitives` without inadvertently breaking semver in `primitives`.
ACKs for top commit:
apoelstra:
ACK 8787304425c83cccfb1107bde0b0d82901470d38; successfully ran local tests
Tree-SHA512: 564bb27dcb608f391be8e50cbf2da02d2f339ddf575e5b5fd432a93df96233b50541616167c7c84a134ed31455a64149c822aea72e61c042fb1aeb8fab20d4ce
We have two macros that are hidden because they are code de-duplication
tools. However the output they produce is, and has to be, stable so that
we can use them in `units` and `primitives` without inadvertently
breaking semver in `primitives`.
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`.
Bitcoin block headers have a timestamp. Currently we are using a
`u32`. while this functions correctly it gives the compiler no chance
to enforce type safety.
Add a `Timestamp` newtype that is a thin wrapper around a `u32`.
Document it and test the API surface in `api.rs`.
6244cb75fa Introduce monadic AmountOpResult (Tobin C. Harding)
Pull request description:
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 `NumOpResult` 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.
Note please this removes `AddAssign` impls for amount types.
ACKs for top commit:
apoelstra:
ACK 6244cb75faf62aed4b47d63a59d14cb766e4e7a8; successfully ran local tests; let's do it -- but definitely want the followup issues addressed
Kixunil:
ACK 6244cb75fa
Tree-SHA512: 7a105acb1aa492ab3e97d94ae182ac4c30a364edd183f71cc320cf80d85060049e8caf1e5736ef6d1af32f39c3376f21382afe35ac65ea1b8c15130c622d9d64
43a7c66f50 units: Remove alloc from fee module (jrakibi)
Pull request description:
This PR removes the `alloc` feature gating from fee module
Closes#3815
ACKs for top commit:
tcharding:
ACK 43a7c66f50
apoelstra:
ACK 43a7c66f50b663aee503c958c5158127fa0b8d5c; successfully ran local tests; nice!
Tree-SHA512: 645d50cd6cde915972a576d7282a5dfc9aa27a8c3a3b44d3f3eb7a7f066cb3a697bed7e757bc86766498d92cc534607960caf20c90a1ac6fabf9246db4b30249
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`.
Use the more idiomatic to_fee instead of `fee_wu`. Since the method
takes a strongly typed argument, remove `wu` from the method name
to improve clarity.
Mirror FeeRate::checked_mul_by_weight functionality by adding a symmetrical method to Weight.
This allows users to calculate fees starting from either Weight or FeeRate with consistent behavior.
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
4dad4730a8 Add unreleased changelog entry (Tobin C. Harding)
08bb57e499 Use MAX_MONEY in serde regression test (Tobin C. Harding)
Pull request description:
We plan on enforcing MAX_MONEY in the amount types. In preparation use MAX_MONEY in the serde regression test instead of the arbitrary, and too large DEADBEEFCAFEBABE value.
ACKs for top commit:
apoelstra:
ACK 4dad4730a81f9f08a565f196e5699f054e28f977; successfully ran local tests
Tree-SHA512: 10fefdb75289f4c93537e031410eb729420729563b262d6104948a0176a78c4f6130956b550cc08ffc924b1bce1c42f0544542879a4d44165022196ef695718a
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.
We have an `_unchecked` amount constructor that makes no assumptions
about the argument. We would like to start enforcing MAX_MONEY but the
diff to introduce this is massive. In an effort to make it smaller we
can do all the hardcoded ints first. We did this already but a bunch
more snuck in or were missed.
In any amount constructor that passes in a hardcoded const as a decimal
integer (i.e., not hex) use the `_unchecked` version.
Done in preparation for enforcing MAX_MONEY.
4dcdf73cfa Add `µBTC` and `µbtc` to tests (Jamil Lambert, PhD)
afba28e188 Change `uBTC` to `µBTC` in rustdocs (Jamil Lambert, PhD)
2ca24f00f2 Add `µBTC` as a recognized `str` form of `uBTC` (Jamil Lambert, PhD)
Pull request description:
`µ` is the correct letter for the SI unit micro but is not on most standard keyboards. `u` was used instead because it looks similar.
Add `µBTC` to the list of recognized strings for MicroBitcoin. This is an addition only, `uBTC` still works as normal.
Change `uBTC` to `µBTC` in the rustdocs. The examples have been left as `uBTC` since this is easier for most people to use.
Add `µBTC` and `µbtc` to the tests.
Close#3941
ACKs for top commit:
apoelstra:
ACK 4dcdf73cfa896b2c095cda9064c6e0a0e9aeec2b; successfully ran local tests
storopoli:
ACK 4dcdf73cfa
tcharding:
ACK 4dcdf73cfa
Tree-SHA512: 0f6e8b8b9c04f1a4dc6536c0420b2ded568ab96d2301b7d488807cb26003b91a787a6cf9023705c731682580f73ae5247f3f3b1e8646e4eb720c5a65da582933
bfba2a85dd Kill remaining mutants (Shing Him Ng)
871fa08f61 Fix typo in serde docs (Shing Him Ng)
462c7a1130 Add weekly cargo-mutants workflow (Shing Him Ng)
Pull request description:
This PR introduces `cargo-mutants` via a Github weekly workflow, similar to how the formatter job runs. It can also be updated to run against [incremental changes in a PR](https://mutants.rs/pr-diff.html) or to create an issues that list the new mutants. To address #3796, I've configured it to only run in `units` for now since that's nearing 1.0.
Here's a [sample run](https://github.com/shinghim/rust-bitcoin/actions/runs/12457984710) i did in my fork, if anyone would like to see what's in the `mutants-out` artifact that gets generated.
ACKs for top commit:
tcharding:
ACK bfba2a85dd
apoelstra:
ACK bfba2a85ddaad6b366a7502cbda1ff2462dfd4c7; successfully ran local tests
Tree-SHA512: e4a44b6f5121e4238c1c3576616f551f4f43349cf5fd5ac1d6331f958a4458753a55519bdafc16965cb2e67201ef6c91b188c79ffcc222f780c421df9a701063
7482fcd934 Run just check-api (jrakibi)
bcc38c40e0 Add Amount division by FeeRate (jrakibi)
Pull request description:
Add checked_div_by_fee_rate method to Amount that computes the maximum weight for a transaction with a given fee rate. This complements the existing `fee = fee_rate * weight `and `fee_rate = fee / weight` operations
- Add `checked_div_by_fee_rate` method that returns Option<Weight>
- Implement` Div<FeeRate>` for Amount for operator syntax support
- Use `floor` division to ensure weight doesn't exceed intended fee
This allows calculating the maximum transaction weight possible for a given fee amount and fee rate.
Closes#3814
ACKs for top commit:
apoelstra:
ACK 7482fcd934c09e3cd6cd25fd4328960b02f8e976; successfully ran local tests
tcharding:
ACK 7482fcd934
Tree-SHA512: 622ca42bde1f67782a3c2996efcba0132fedb5e984f594603aece974de6acdeb4b22d63239d29d46fb8623c8082841c33b1d5b9ad2ea51e2f63e6f5d859daa7e
`µ` is the correct letter for the SI unit micro but is not on most
standard keyboards. `u` was used instead because it looks similar.
Add `µBTC` to the list of recognized strings for MicroBitcoin and to the
rustdocs.
Implement `checked_div_by_fee_rate_floor` and `checked_div_by_fee_rate_ceil` methods to compute the maximum and minimum transaction weights.
These methods provide precise weight calculations using both floor and ceiling divisions.
- Introduce `checked_div_by_fee_rate_floor` for floor-based weight division.
- Add `checked_div_by_fee_rate_ceil` for ceiling-based weight division.
- Update `ops::Div` implementation for `Amount` to use floor division by default.
- Include unit tests to validate both floor and ceiling division methods.
7c7fc36784 Improve docs on Denomination (Tobin C. Harding)
Pull request description:
Document what each denomination means.
ACKs for top commit:
jamillambert:
ACK 7c7fc36784
apoelstra:
ACK 7c7fc36784603480df1975f4a77ec583b74efa5c; successfully ran local tests; sure
Tree-SHA512: c0f3a5172e4dfdc6195efc34f36d65700591fe19015e455aa20cacaacc9c516518cec7716028779fd408e8a639eb4a5962a372b1d1d70db06a0ea8130c3bd703
55470f7357 Change `create` to `construct` in rustdocs (Jamil Lambert, PhD)
Pull request description:
In preparation for units 1.0 address #3868 and make sure "constructs" is used instead of "creates" in rustdocs.
ACKs for top commit:
apoelstra:
ACK 55470f73571ce0e0279f5697a252695e8c8df9b5; successfully ran local tests; sure
tcharding:
ACK 55470f7357
Tree-SHA512: 48f9b6cedfca782e698eec4520885f35834582ed098a65b00ccdfa54167c91fe62405f90c9c1e72a671e72ac349af74ac12808ce9e0708799f43d86749590649
7ca5c5ccae Fix rustdoc title on hex_u128_* (Tobin C. Harding)
f1e2564821 Improve docs on parse::int_from_string (Tobin C. Harding)
d97cbc6d27 units: Correct docs on private Sealed trait (Tobin C. Harding)
Pull request description:
Audit the `units::parse` module checking for sanity of the API. Could still possibly do with more improvements to the docs but for the `1.0-alpha` this is good to go IMO.
Close: #3710
ACKs for top commit:
apoelstra:
ACK 7ca5c5ccae1efe5862f21bfc670257837a202517; successfully ran local tests; looks good!
Tree-SHA512: 937e8f6e1ae0a17217b770daffe1968ec9046c86728a360f1297d7027318511fece31440f462fdaeb94c2a69901a970e20bd2599a6d963bfbdf06b25378fe543
289a521426 units: Test for dyn compatibility (Tobin C. Harding)
Pull request description:
Phew! dyn compatibility is a non-trivial concept. There are four public traits in `units`, only one is dyn compatible.
This patch is done in order to check off C-OBJECT from the Rust API guidelines checklist.
Add a test to check the public traits in `units` for dyn compatibility.
While we are at it add a code comment on `Integer` stating why its not dyn-compatible.
ref: https://rust-lang.github.io/api-guidelines/flexibility.html#c-object
ACKs for top commit:
apoelstra:
ACK 289a5214265a1f0d7b320b0121775f868a386c57; successfully ran local tests; nice!
Tree-SHA512: 6f247d4977a87acf68a03bfae12335a8f16ebbe44478da757128f54697c859fa278ddbc0e5c4479918797db91dbe8eef3d2adf60f83518182cb10e84e77120da
e0028239cf api: Run just check-api (Tobin C. Harding)
5eb5941215 Add FIFTY_BTC const to the amount types (Tobin C. Harding)
Pull request description:
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`.
ACKs for top commit:
storopoli:
ACK e0028239cf
jamillambert:
ACK e0028239cf
apoelstra:
ACK e0028239cf207660deb2873a92bacfa1315af634; successfully ran local tests
Tree-SHA512: 623ed8b1f5fe8dd95309179308fea83d68be4349becf6305769b0378cc9032961df2c062dc2bf702fec5e2394e8abb7360d2be6f19b6cf505db8769a5ae39e16
I only just worked on this function a week ago and already I couldn't
see from reading the code why it exists. Add a paragraph to the rustdocs
to save the next guy the trouble of working it out.
Phew! dyn compatibility is a non-trivial concept. There are four public
traits in `units`, only one is dyn compatible.
This patch is done in order to check off C-OBJECT from the Rust API
guidelines checklist.
Add a test to check the public traits in `units` for dyn compatibility.
While we are at it add a code comment on `Integer` stating why its not
dyn-compatible.
ref: https://rust-lang.github.io/api-guidelines/flexibility.html#c-object
f01bb30121 Use sat variable for constructor (Tobin C. Harding)
34f846c074 Use ssat variable for constructor (Tobin C. Harding)
Pull request description:
Add `sat` and `ssat` throughout the `amount::tests` module after new tests were added recently.
ACKs for top commit:
jamillambert:
ACK f01bb30121
apoelstra:
ACK f01bb30121ef5a59af069a55db727ee5a9ba71bb; successfully ran local tests
Tree-SHA512: 6a4cdc47000b22625132fe0d8faa7dbff59acfc0b6b4faffe091f885f8d0fd5f5ce15e2298c87e25bd4cc13a2611cecadb43b7d44ccd768a04cd150223577b6b
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`.
f9be30ddbe units: Fix `missing_errors_doc` clippy lint (Jamil Lambert, PhD)
Pull request description:
Change the `missing_errors_doc` clippy lint to `warn`.
Allow `missing_errors_doc` in `amount/serde.rs` and `fee_rate/serde.rs`. Add missing `# Errors` sections to rustdocs where the lint gives a warning.
One of the TODO lints in Issue https://github.com/rust-bitcoin/rust-bitcoin/issues/3825
ACKs for top commit:
tcharding:
ACK f9be30ddbe
apoelstra:
ACK f9be30ddbe5c0837ab3e408dfadabc6c6cd2068e; successfully ran local tests
Tree-SHA512: 8039804ab86c18dceadb425c8531cd4064431393367c6053249e00386f48998d8d84a3aee6ad139e7e2ca3ac3c94e05ee694d72270bf285f6b90d0ff821e622e
29f1a4613a Enable `ref_option` lint and allow individually (Jamil Lambert, PhD)
Pull request description:
The last `TODO` in #3825
Allow the `ref_option` lint in the cases where the lint wants us to change the API to ```use `Option<&T>` instead of `&Option<T>` ```
The other option would be to keep the lint as `allow` and add a comment to `Cargo.toml` as to why we are allowing it like the other cases that are allowed.
Close#3825
ACKs for top commit:
apoelstra:
ACK 29f1a4613a090cd23baded1ef577aa4e3da8d33f; successfully ran local tests; nice!
tcharding:
ACK 29f1a4613a
Tree-SHA512: 181d617060e8ae0e75f033b1f356106fc89bdee9841085c889b775e82711c147e5282ecb0e3bf80cdb058034365bbfc7289c029ba0bf8b0b0f26d22505c63aa2
As we do in other places in the `amount::tests` module; use a local
`sat` variable bound to the `Amount::from_sat` constructor.
Internal change, no logic changes.
As we do in other places in the `amount::tests` module; use a local
`ssat` variable bound to the `SignedAmount::from_sat` constructor.
Internal change, no logic changes.
Change the lint to `warn` in `units/Cargo.toml`.
Allow `missing_errors_doc` in `amount/serde.rs` and `fee_rate/serde.rs`.
Add missing `# Errors` sections to rustdocs where the lint gives a
warning.
Currently `InputString` is in the public API of `units` because of the
trait bound on `parse::int()`. We can just do the monomorphisisation
manually to remove it.
This patch renames `int` to have three different names, one for `&str`
one for `String`, and one for `Box<str>`.
7a3df57659 Reorder assertions in units::amount::tests to follow got, want order (yhzlsm)
Pull request description:
Reorder assertions in `units::amount::tests` to follow got, want order.
Makes debugging easier, as there's no need to check the test to verify the order.
Close#3860
ACKs for top commit:
tcharding:
ACK 7a3df57659
apoelstra:
ACK 7a3df57659d02ab610e328072435836386ed1c97; successfully ran local tests
Tree-SHA512: 7d07162ba930ca1471684a771cad08ba9153b0b38bf44c98fd41c2d70f05c36b95f022dd82e61e6b50614266f16a5615edd79d6d548b003642509866416021c9
9d1cba4994 units: Introduce fee module (Tobin C. Harding)
cd908fec51 Use explicit calc getters and setters (Tobin C. Harding)
Pull request description:
We have a bunch of functions and impl blocks scattered around the place for calculating fee from fee rate and weight.
In an effort to make the code easier to read/understand and also easier to audit introduce a private `fee` module and move all the code that is related to this calculation into it.
This is in internal change only.
ACKs for top commit:
apoelstra:
ACK 9d1cba4994e9ec94850a0e0e49f85fd0c595541e; successfully ran local tests
Tree-SHA512: a33c1ce4a1b62ff29ee65dd3adf2f19384a77f7e18f1c42019973631726cd710c2c8d9c200afb108d4f3f34fcce5cd5383a7ae512caf76c73604db9cdf9eaeda
96f427a8b8 units: Refactor Send/Sync api test (Tobin C. Harding)
Pull request description:
The `api` test for types implementing `Send` and `Sync` is part of both C-SEND-SYNC and also C-GOOD-ERR (for error types).
Refactor the two tests into a single one and document appropriately. This is mirrors how we do it in `io/tests/api.rs`.
ACKs for top commit:
apoelstra:
ACK 96f427a8b84129179e86f3914be8e4712a89f660; successfully ran local tests; lgtm
Tree-SHA512: 7b24780ac2b4f73d0cad952555f005553d9b8c248da6f92c28e7e9510b58eba6c165720ded9bd2f2db19f9a19d72fe7dd333e68312f1291a47e044a94902be0b
Reorder assertions in `units::amount::tests` to follow got, want order
Makes debugging easier, as there's no need to check the test to verify the order.
a7c44cebf9 Use _unchecked to construct amounts (Tobin C. Harding)
09df951760 Use sat variable in tests (Tobin C. Harding)
4a5b2c60c6 Use ssat variable in tests (Tobin C. Harding)
Pull request description:
We have a `_unchecked` constructor now for both `Amount` and `SignedAmount`. Soon we would like to start enforcing the `MAX_MONEY` invariant in both amount types. To make that change easier do a few refactorings:
- Patch 1 and 2 introduce local variables for amount constructors.
- Patch 3 replaces the local variables introduce in (1) and (2) with macros
- Patch 4 uses `_unchecked` constructor for hard coded integers
The strange patch separation is done intentionally so we don't inadvertently reduce test coverage by using the wrong constructor. I made this mistake already in a previous PR, lesson learned.
Note please, the macro introduced in patch 3 is in preparation for enforcing `MAX_MONEY`. The macros allow us to panic (`from_sat().unwrap()`) instead of using the `_unchecked` version.
ACKs for top commit:
apoelstra:
ACK a7c44cebf9975c4eeba56a65c0ea65be90e5c7f3; successfully ran local tests
Tree-SHA512: 55c2428ae231882542a4cfa724675341f7b493d158f4bec26277d3eefb04d9597cc29b05dce859661a96855fa6f4bac250d53c3dfa9f86a9611d43387ee18667
The `api` test for types implementing `Send` and `Sync` is part of both
C-SEND-SYNC and also C-GOOD-ERR (for error types).
Refactor the Send/Sync tests in both `units` and `io` and improve
comments.
9396041524 api: Run just check-api (Tobin C. Harding)
ffd8702cb3 units: Hide the remaining public macros (Tobin C. Harding)
Pull request description:
We do not want to commit to any public macros in `units`. Recently we (I at least) learned that adding `doc(hidden)` signals to users that the macro is unstable and should not be relied upon.
Hide the remaining two macros so we can release 1.0 and not worry about later breaking them.
With this applied there are no exported macros that are not hidden. Verify using `git grep -A 1 macro_export`.
ACKs for top commit:
apoelstra:
ACK 9396041524e291203d5c86665639872f9a6246b5; successfully ran local tests; Yeah, let's do it
Tree-SHA512: a3a59897a2fe16276ab2d364ff247f48772a63a25f91eabc17023a37b9fab3860639dc1e09193c938dd73711ba20c95b8d0ad9db9493d269ee9328b2132d61cb
55a999e0b5 Add serde roundtrip tests to relative locktime types (Tobin C. Harding)
Pull request description:
As we do for absolute locktime types add a couple of `serde` roundtrip tests to the relative locktime types.
ACKs for top commit:
apoelstra:
ACK 55a999e0b5c5867ed66ee64d2b516b80e059e402; successfully ran local tests
Tree-SHA512: 28923a0e2c2053a7346e91a21619dc5d2700ea131aa5ec5a5f6d89d09c70cb45ce9731055bbcc1447d46d5dbe231cb075436dd682229ee8530307e199af54ce2
We have a bunch of functions and impl blocks scattered around the place
for calculating fee from fee rate and weight.
In an effort to make the code easier to read/understand and also easier
to audit introduce a private `fee` module and move all the code that is
related to this calculation into it.
This is in internal change only.
We have a bunch of functions and impl blocks scattered around the place
for calculating fee from fee rate and weight.
In preparation for adding a `calc` module use getters and setters in
code that will move to the `calc` module.
(Remember, the `FeeRate` uses an inner sats per kwu value.)
Internal change only.
0e5d6db7df units: Don't ignore serde examples (Tobin C. Harding)
Pull request description:
There is no reason to ignore the rustdoc example in `units::amount::serde`. The `as_btc` function requires the `alloc` features so use `as_sat` in the example instead.
While we are at it clean up the whole block of rustdocs.
ACKs for top commit:
apoelstra:
ACK 0e5d6db7df9a1f9b4e9180d49cf1532551570e33; successfully ran local tests
Tree-SHA512: d108779faa8baf5c8a1cb533e44e8ac209e3e1001eb8ec8ae1f9ba9b0cf5f8c45c38df77e15193cdc990fcd0fbf48bca489aadbd196c6323560040b7fd043332
a18d28761b api: Run just check-api (Tobin C. Harding)
1175faca32 io: Introduce api test file (Tobin C. Harding)
01f3eefc12 io: Move bridge type constructors (Tobin C. Harding)
a8fbd5e302 units: Remove api docs (Tobin C. Harding)
aaa20fc098 units: Move code comment to correct imports (Tobin C. Harding)
Pull request description:
As we did for `units` add an integration test for the API surface of the `io` crate.
The first two patches are cleanup to the `units` test. Patch 3 is trivial refactor.
ACKs for top commit:
apoelstra:
ACK a18d28761b9f6ce3bd06ba72b9afe11773b7443d; successfully ran local tests
Tree-SHA512: e5a39f97f7d6e34cb63613f337101f13fcc5d66f85277b6f0026d339b5677ff60f7e3050ae23807c3d20d0ae4e343a10983f43dab757a66fe4d33afd09a96f62
We do not want to commit to any public macros in `units`. Recently we (I
at least) learned that adding `doc(hidden)` signals to users that the
macro is unstable and should not be relied upon.
Hide the remaining two macros so we can release 1.0 and not worry about
later breaking them.
With this applied there are no exported macros that are not hidden.
Verify using `git grep -A 1 macro_export`.
f94c7185fd Remove usage of impl_from_infallible in crates (Shing Him Ng)
Pull request description:
Fixes#3843
tcharding Copied your commit message from the other `impl_from_infallible` commit 😄
ACKs for top commit:
apoelstra:
ACK f94c7185fdd62e1ed98ed4016486406146c4d4f3; successfully ran local tests; nice!
tcharding:
ACK f94c7185fd
Tree-SHA512: 8c58c2c87f6892855d74a3306e1027a37394961f0a26b7bd88cc1654a190dda37234e7dde51a419dcd2f1bd1dd1ccceec16bbbc6fbdd5418ad21f10531b402b3
There is no reason to ignore the rustdoc example in
`units::amount::serde`. The `as_btc` function requires the `alloc`
features so use `as_sat` in the example instead.
While we are at it clean up the whole block of rustdocs.
We have a `_unchecked` constructor now for both `Amount` and
`SignedAmount`. In preparation for enforcing the `MAX_MONEY` invariant
use the `_unchecked` constructor throughout the codebase to construct
amounts from hard coded integer values.
286bf900b1 Address mutants in units (Shing Him Ng)
Pull request description:
Preemptively addressing these mutants before introducing the `cargo-mutants` workflow. There are several types of changes:
- Changes that address mutants that were actually missing
- Changes that address test values that cause `cargo-mutants` to think mutants were missed. For example, `cargo-mutants` will replace the return values for unsigned integer types with 0 and 1. While this case might be tested, the test might be testing this with a call that results in 0 or 1. When `cargo-mutants` substitutes the function call with `Ok(1)`, the test will still pass, and it will consider this a mutant. `cargo-mutants` also replaces operations (+, -, /, %), bitwise operations (&, |, ^, etc), so an operation such as `3 - 2` results in a mutant because changing it to `3 / 2` yields the same result
- TODOs to ignore functions/impls in the future
I uased the following `mutants.toml` config file when running `cargo mutants` and skipped the `Debug`, `Arbitrary`, `Display`, and `Error` impls and the `kani` test:
```
additional_cargo_args = ["--all-features"]
examine_globs = ["units/src/**/*.rs"]
exclude_globs = ["units/src/amount/verification.rs"]
exclude_re = ["impl Debug", "impl Arbitrary", "impl Display", ".*Error", "<impl Visitor for VisitOptAmt<X>>"]
```
I wasn't sure the code for Displaying things needed to be tested with `cargo mutants`, but I'm less sure about the errors so i can kill those mutants too if needed
ACKs for top commit:
tcharding:
ACK 286bf900b1
apoelstra:
ACK 286bf900b1c100d2f5d0a0d45f31a5bf5a0a26ce; successfully ran local tests
Tree-SHA512: 3db9598a5ad92f2013d43876221ce9363cc019cbf720a206768b517a812c8355b7f00594212eb0526c0feb2dc578f88e1df12548f72a2b2360c4d4227de749f7
Rust macros, while at times useful, are a maintenance nightmare. And
we have been bitten by calling macros from other crates multiple times
in the past.
In a push to just use less macros remove the usage of the
`impl_from_infallible` macro in the bitcoin, units, and internals crates
and just write the code.
Preemptively addressing these mutants before introducing the
cargo-mutants workflow
There are several types of changes:
- Changes that address mutants that were actually missing
- Changes that address test values that cause `cargo-mutants` to think
mutants were missed. For example, `cargo-mutants` will replace the
return values for unsigned integer types with 0 and 1. While a function
might be tested, the test might be testing the function with a call that
results in 0 or 1. When `cargo-mutants` substitutes the function call
with `Ok(1)`, the test will still pass, and it will consider this a
mutant. `cargo-mutants` also replaces operations (+, -, /, %), bitwise
operations (&, |, ^, etc), so an operation such as `3 - 2` results in a
mutant because changing it to `3 / 2` yields the same result
- TODOs to ignore functions/impls in the future
85e04315d5 Remove test_ prefix from unit tests (Tobin C. Harding)
Pull request description:
There is a loose convention in Rust to not use `test_` prefix. The reason being that `cargo test` outputs 'test <test name>' using the prefix makes the output stutter.
This patch smells a bit like code-churn but having the prefix in some places and not others is confusing to new contributors and is leading me to explain this many times now. Lets just fix it.
Remove the prefix unless doing so breaks the code.
ACKs for top commit:
shinghim:
ACK 85e04315d5
apoelstra:
ACK 85e04315d5eb90075ce55bf18fab8876a4583def; successfully ran local tests
Tree-SHA512: d90ae5ef75cc5e5a8f43f60819544f1a447f13cbe660ba71e84b8f27bfcc04a11d3afde0ed56e4eea5c73ebc3925024b800a1b995f73142cab892f97a414f14a
0d8e9ef096 Remove usage of impl_from_infallible in leaf crates (Tobin C. Harding)
Pull request description:
Rust macros, while at times useful, are a maintenance nightmare. And we have been bitten by calling macros from other crates multiple times in the past.
In a push to just use less macros remove the usage of the `impl_from_infallible` macro in all the leaf crates and just write the code.
ACKs for top commit:
apoelstra:
ACK 0d8e9ef096fd60fcdb7928697c8f554bf428b754; successfully ran local tests; this is a great change
Tree-SHA512: c4cff4517f7846d91142f26d451c2bcafc014a0921d11ac1487eab087449f12023c3b4fc57e1bc72ed3ea58406b4c3d24bbd846df21353f5d7ecb4a4b8bfb0b2
b1399d193f units: Improve rustdocs on macros (Tobin C. Harding)
706c7c9f5f Hide impl_tryfrom_str (Tobin C. Harding)
2675cd1040 units: Re-order impl_parse_str (Tobin C. Harding)
4cc7aae6b9 Hide impl_tryfrom_str_from_int_infallible (Tobin C. Harding)
fc9e40ab1a units: Re-order impl_parse_str_from_int_infallible (Tobin C. Harding)
Pull request description:
Document the public macros in `units::parse`. Done as part of #3632
First 2 patches are trivial preparatory refactor.
ACKs for top commit:
apoelstra:
ACK b1399d193fcb20c09457a1f22c5c1dd8009c84b1; successfully ran local tests
Tree-SHA512: e2b0196adb37b3616963e3e3ded1c2be95f98fe33a4e6edb269b7eca1ddb66b82be139f4edb3269a5cf69a73b3c803845fe83a5f6e300b08abf9fcb0da602084
f5eb8f4747 api: Run just check-api (Tobin C. Harding)
472b1d3ff3 units: Add serde regression test (Tobin C. Harding)
dedae8acf2 Implement custom serde modules for FeeRate (Tobin C. Harding)
d94e5f03e6 Move fee_rate.rs to module (Tobin C. Harding)
c3c1f6f82d Add missing license comment to test file (Tobin C. Harding)
Pull request description:
Implement and enforce explicit unit when serializing. This is as we do for `Amount` (see #3672 for similar).
To test it, and as part of the 1.0 effort; add regression tests for `serde` stuff in `units`.
With this applied one must use attributes to serialize `FeeRate`.
```rust
#[derive(Serialize, Deserialize)]
pub struct Foo {
#[serde(with = "bitcoin_units::fee_rate::serde::as_sat_per_kwu")]
pub fee_rate: FeeRate,
}
```
ACKs for top commit:
apoelstra:
ACK f5eb8f4747a7cd303cad2b7f8f442bb31862c52a; successfully ran local tests; great idea!
Tree-SHA512: 0968ead568b1e3142efd4c0e856192ddde0f441de84215cbb0950b60a56922f1abaf6d4ccfe243b722a6883c0a927d26bcfba979acf3ca84c4f21baba73af764
There is a loose convention in Rust to not use `test_` prefix. The
reason being that `cargo test` outputs 'test <test name>' using the
prefix makes the output stutter.
This patch smells a bit like code-churn but having the prefix in some
places and not others is confusing to new contributors and is leading me
to explain this many times now. Lets just fix it.
Remove the prefix unless doing so breaks the code.
Rust macros, while at times useful, are a maintenance nightmare. And
we have been bitten by calling macros from other crates multiple times
in the past.
In a push to just use less macros remove the usage of the
`impl_from_infallible` macro in all the leaf crates and just write the
code.
We have a public macro `impl_parse_str` and a helper macro used by it,
both pubicly exported.
As we did for `impl_parse_str_from_int_infallible`; to assist readers
understanding what is going on put the helper below the other.
Done as a separate patch to make the diff easier to read.
Internal change only, no logic change.
This macro is a helper for `impl_parse_str_from_int_infallible` used to
reduce code duplication. It does not, and should not, need to be part of
the public API - hide it.
We have a public macro `impl_parse_str_from_int_infallible` and a helper
macro used by it, both pubicly exported.
To assist readers understanding what is going on put the helper below
the other.
Code move only, no logic change.
We now have both `Amount::from_sat_unchecked` and
`SignedAmount::from_sat_unchecked`. These constructors are explicitly
for ignoring any invariant (implied or otherwise) especially in test
code.
Note we do not enforce an invariant currently. This patch is a baby step
towards getting the `amount` module in order.
Replace all calls to `from_sat` for const int values with the
`_unchecked` constructor. Done in `amount::tests` only.
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.
As we already do in this test function for other constructors; add a
local variable and bind it to the `SignedAmount::from_sat` constructor.
Refactor only, no logic change.
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
e13355318e Add From impl (yancy)
364e9ff775 Change method return type (yancy)
fdf3336ed5 Add unchecked variant (yancy)
Pull request description:
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.
ACKs for top commit:
tcharding:
ACK e13355318e
Tree-SHA512: c016b51bdd87a12eb09d9c1a82699dad1e866bf96bd3235eeb131f216f02422acb992ddb3a8135af00bbc10e240178fde5e37fb7860d9e6eaf433cf917d4d16a
Add an integration test the verifies we have serde traits implemented
for all the main crate types (excl. errors and helper structs).
This also acts as a regression test.
The `FeeRate` type wraps a `u64` but the inner value implicitly contains
information about the unit. As such when serializing and deserializing
the unit information is not explicit and if users try to deserialize
with a different unit their code will be silently buggy.
As we do for Amount; add custom serde modules so that users can
serialize in an explicit unit. Furthermore remove the derived impls
forcing users to make the decision. This is as we do for `Amount`.
With this applied one can write
```rust
#[derive(Serialize, Deserialize)]
pub struct Foo {
#[serde(with = "bitcoin_units::fee_rate::serde::as_sat_per_kwu")]
pub fee_rate: FeeRate,
}
```
In preparation for adding `serde` modules to the `fee_rate` module
create a sub directory.
Create a directory and move `fee_rate.rs` to `fee_rate/mod.rs`.
merge-script
Tobin C. Harding
Jamil Lambert, PhD
Fmt Bot
lfgtwo
VolodymyrBg
Eval EXEC
jike
Erick Cestari
yancy
Shing Him Ng
kilavvy
looklose
jrakibi
yhzlsm