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
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.
Tobin C. Harding
merge-script
Jamil Lambert, PhD
Fmt Bot
yhzlsm
Shing Him Ng