Looks like a large diff but if you run
git show --color-moved-ws=allow-indentation-change
you will see that it's 100% moves (though moves of code into the
reference macro). May be easier to just look at src/amount/result.rs
after this; it's pretty short now.
The next commit changes a lot of code, but almost entirely by moving and
indenting it. We try to do the moves here ahead of time, so it the diff
for the next commit will be just deletions and indentations.
This macro can generally handle a lot of different cases where we
implement "the same trait but on references". We introduce it here and
use it in two places. We will use it in many more, but I wanted to make
the diff small on this commit, which introduces the actual macro code
and might take a bit of reading to understand.
You may want to use --color-moved-ws=allow-indentation-change to review
this, and the next commit.
The next set of changes will mechanically delete other macros that are
made redundant by this.
The `NumOpResult` type is way more ergonomic to use if it derives
`Copy`. This restricts the `NumOpResult` to being `Copy` as well.
This does restrict what we can include in the error type in the future.
Derive Copy for `NumOpResult` and `NumOpResult`.
We would like to return an error when doing math ops on amount types.
We cannot however use the stdlib `Result` or `Option` because we want to
implement ops on the result type.
Add an `AmountOpResult` type. Return this type from all math operations
on `Amount` and `SignedAmount`.
Implement `core::iter::Sum` for the new type to allow summing iterators
of amounts - somewhat ugly to use, see tests for example usage.
The ops traits return `Self::Output` not `Self`. The current code builds
because `Self` and `Self::Output` are both the same type.
Use `Self::Output` as the return value of `ops::Rem`.
5f75bfaa63 Improve examples on Denomination (Tobin C. Harding)
Pull request description:
Reduce the noise in the examples.
ACKs for top commit:
apoelstra:
ACK 5f75bfaa63309c7526136d430ca8092197ab7c8e; successfully ran local tests; yeah, agreed, this is nicer to read
Tree-SHA512: 01c5863f8712a8ca3b38d3f96be9d08078ca28d8cfc3dd8e8528c388e5f82406a0d43def552b7b53f034c9bf440f7d2d0fec6a760cf69a245b109d0ce4e288c3
13a3f490b8 Use Self instead of amount type (Tobin C. Harding)
34e3049ae0 Use sats instead of satoshi (Tobin C. Harding)
00b71a670f Use from_sat_unchecked for hardcoded ints (Tobin C. Harding)
8fdec67f7d Change local var ua to sat (Tobin C. Harding)
c6f056672b Change local var sa to ssat (Tobin C. Harding)
f3e853e07a units: Do trivial refactor of amount::tests (Tobin C. Harding)
dbec9807f9 Shorten identifiers by removing _in_sats (Tobin C. Harding)
154a4420fc Stop using FQP on Amount type (Tobin C. Harding)
8e16a48252 Run the formatter (Tobin C. Harding)
Pull request description:
Do a bunch of refactorings to tease out changes from #3794.
The first 8 are uncontroversial. The 9th one is subjective. The last one is unusual but IMO worth doing because of the relationship between the two amount modules.
Do note that this PR is 100% internal changes - please please don't bike shed this to death.
ACKs for top commit:
apoelstra:
ACK 13a3f490b80e4c8f8e1753111a914315eefd73e6; successfully ran local tests; lgtm
Tree-SHA512: e2ef0e7fbdaaf632a9840920a227a901fbeb55a29398013cd6cb764b1ff7c0a7c5a1648fd8f606e8b5f7523943886f5eff54cf4054d24349feb72f0b4de05b91
I claim that if the two amount modules are coded as similarly as
possible it will be easier to ensure that we have the API's uniform and
bug free. To make auditing the modules easier and less error prone use
`Self` instead of the explicit type. This makes it easier to see
differences in the modules and to ensure the differences are correct and
required.
Internal change, no logic changes whatsoever.
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.
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
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.
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
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
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.
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.
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
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 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
Code comments that comment and arbitrary block "section" of code are
almost always pointless and almost always go stale over time.
These particular code comments add almost no value.
Remove code comments.
Any SignedAmount can now be cast to Amount since the range is the same.
Specifically, the range for SignedAmount is (- 21 million, 21 million)
while the range for Amount is (0, 21 million). Therefore any value from
Amount can be cast to a SignedAmount and it will work. Note it's not
the same and still requires checking when going from SignedAmount to
Amount since Amount can't handle the negative range.
As a side effect of changing the return type, TryFrom is no longer valid
and does not compile. Therefore in addition to changing the return
type, TryFrom is also removed.
29811ba82c api: Run just check-api (Tobin C. Harding)
0a16382fa3 Rename rhs to weight (Tobin C. Harding)
Pull request description:
In ops functions we typically use `rhs` but for the more unconventional `checked_*_by_*` functions lets use a more descriptive parameter name.
Interestingly `cargo public-api` thinks this is an API breaking change. This is obviously an internal change but the api files are updated because the parameter names appear in the api text files.
ACKs for top commit:
apoelstra:
ACK 29811ba82cc598d08dc877825ecf8890c48d23b7; successfully ran local tests; sure
sanket1729:
ACK 29811ba82c
Tree-SHA512: b44c958ab3ef024c867d81f12819775afa62f1762b96afb93831bb4857ddb9bc95ae5b5f42f32b1a1d23832c69c3cae55f12a80d109fadda7d6763bc764d06aa
The `untis::error` module is just a code organisation thing it should
never have been public. We already re-export all the error types and
this is verified by the `units/tests/api.rs` test file.
Make the module private and remove it from the paths in the `api` test.
In ops functions we typically use `rhs` but for the more unconventional
`checked_*_by_*` functions lets use a more descriptive parameter name.
This is an internal change but the api files are updated because the
paramater names appear in the api text files.
04dfe8dd45 Add api test to check Arbitrary impls (Shing Him Ng)
678fc71b88 Implement Arbitrary for units types (Shing Him Ng)
Pull request description:
Implement Arbitrary for the rest of the types in `units`. Also moved the implementation in `FeeRate` right before the `tests` module
Closes#3705
ACKs for top commit:
apoelstra:
ACK 04dfe8dd45fae9b55dacfe9eb0d73ea306db14ba; successfully ran local tests
tcharding:
ACK 04dfe8dd45
Tree-SHA512: 156bd26d4de85d484711d476df1d2758805387125209f0307aa786dd1585ff9953dbe41b0864b00ae101419176647e3bde7994ed9257c18307d161463b1c8d2e
f4617e71f5 kani: Verify no out of bounds for ArrayVec (Tobin C. Harding)
e378cdd8fa kani: Don't bother checking signed to unsigned conversion (Tobin C. Harding)
50224eecc2 kani: Don't overflow the tests (Tobin C. Harding)
Pull request description:
PR does two things because a recent upgrade of `kani` broke our setup. I'm not sure why it just showed up.
We fix the verification for `amount` types which recently broke because of `MAX_MONEY` changes and then we add a verification function to `internals` because build fails and one fix is to just add something.
- Patch 1: units: Assumes correct values
- Patch 2: units: Removes a stale check now that MAX_MONEY is being used for MAX
- Patch 3: Add verification to `internals::ArrayVec`.
ACKs for top commit:
apoelstra:
ACK f4617e71f5fd074000d1e3a9376644c744210562; successfully ran local tests
Tree-SHA512: dfef05a7bbb5372415efa8acab7f79801aa7326ac298c007b173786f00bcccd0b1b81d327113723c359fb2797895414a586cc3fb86e495476a03fcac02a96899
f08d8741d3 Test types MIN/MAX instead of i64::MIN/i64::MAX (yancy)
Pull request description:
The MIN/MAX for SignedAmount recently changed from i64::MIN and i64::MAX to MAX_MONEY/MIN_MONEY. Update the tests to reflect this new MIN/MAX since it is no longer valid to create a value above or bellow MAX_MONEY/MIN_MONEY.
ACKs for top commit:
apoelstra:
ACK f08d8741d39685b636830680bb891bd414826e88; successfully ran local tests
tcharding:
ACK f08d8741d3
Tree-SHA512: 563408240dffaf95f88a9d570e56f9b9b161b422cb59a89828c18b9c784e7acb717f57fe55c80411f104443ac2a3f908f2a98ab1a4b34edab69b6946a723b30c
2513e05501 api: Run just check-api (Tobin C. Harding)
9619f68956 units: Move excluded lints to manifest (Tobin C. Harding)
5290a93a38 units: Add all pedantic lints (Tobin C. Harding)
Pull request description:
Add all pedantic clippy lints but leave a few marked as TODO.
The coment on the list claims to it is an exhaustive list of pedantic lints, that claim is going to go stale. I would be nice if we had tooling to catch new lints as they were added.
ACKs for top commit:
apoelstra:
ACK 2513e05501e3a014c097f24eb9178c291785db81; successfully ran local tests
Tree-SHA512: 33b2a7448d49d6a5571c9e4e9922b6042ab03aaaa9f7acad243a926f8a03a0ffed75d4f5f37be4705f23862c32f96879582214cd17c7e5ab81e47517a84745e0
Amount add and sub now enforce the MAX_MONEY invariant when doing
addition and subtraction. We need to tell kani to assume we don't
overflow before doing actual tests.
Note also that `ops::Add` calls through to `checked_add` and
`ops::Sub` calls through to `checked_sub` so separate kani tests for
these are unnecessary.
Add all the pedantic lints to the repository by way of the repository
manifest. Then enable these lints in the `units` manifest.
Some things worth mentioning:
- Fix `needless_pass_by_value` by adding derives to `FormatOptions`.
- Fix lint `cast_lossless` using `cargo clippy --fix``
- While fixing `lint enum_glob_use` introduce a new style to the
codebase; import enums using a single character. Doing so prevents
namespace clashes, improves clarity, and maintains terseness.
Audit:
Use the following lints locally and audit all the warnings, they produce
many false positives so we can't enable them permentently.
- `cast_possible_truncation`
- `cast_possible_lint`
- `cast_sign_loss`
The MIN/MAX for SignedAmount recently changed from i64::MIN and
i64::MAX to MAX_MONEY/MIN_MONEY. Update the tests to reflect this new
MIN/MAX since it is no longer valid to create a value above or bellow
MAX_MONEY/MIN_MONEY.
68f3c3e5d7 api: Run just check-api (Tobin C. Harding)
b956940a6d Add ops unit tests for amount types (Tobin C. Harding)
d88306fb68 units: Implement ops for amount types (Tobin C. Harding)
4a3aa4a08b Add ops unit tests for Weight (Tobin C. Harding)
43ef9a618c units: Implement ops for Weight (Tobin C. Harding)
c5fbc7e117 units: Add assign macros (Tobin C. Harding)
20a79da344 units: Use one level of path for ops traits (Tobin C. Harding)
6ce5385914 units: Add macros for Add and Sub (Tobin C. Harding)
b31ca72b33 units: Use rhs instead of other (Tobin C. Harding)
cd0fa2d1dc Make FeeRate ops impls more terse (Tobin C. Harding)
Pull request description:
Sort out `Add`, `AddAssign`, `Sub`, and `SubAssign` for the following types:
- `Amount`
- `SignedAmount`
- `FeeRate`
- `Weight`
And clean up as we go.
Note that `BlockInterval` isn't touched in this PR - it looks correct already.
The unit tests in the two patches "Add ops tests ..." can be moved if you want to verify that they don't build without the patch they follow.
ACKs for top commit:
apoelstra:
ACK 68f3c3e5d728a60a4b52ca64523770bcdd32f957; successfully ran local tests
Tree-SHA512: a82d3bf288f61b169b8cff498e81bd2cd123c8dcbf534413233ff03f06102a42508e09b2f7e5b268b21f82d4bf2b3612cd88dea1231b4d3e6455c7e99f82e729
183ecf1fe6 Add units tests for cBTC (Tobin C. Harding)
Pull request description:
We added the "cBTC" denomination but forgot to test it.
Add units test for "cBTC" as we do for the other denominations. Put all the tests lines of code in the same order as the enum variants so a reader can see easily that all cases are tested.
ACKs for top commit:
apoelstra:
ACK 183ecf1fe6944187151036e36d6726fae9b64392; successfully ran local tests
Tree-SHA512: 93a3f3e5430f7b6f525d7c55ad3d318f8b30fef8cdef24ea92a1687e898f73d3829875861f026af70dde013743d310d87331b5654b524f4ee90c20f7e6ed7f7c
f97c04cf07 api: Run just check-api (Tobin C. Harding)
99b32c95b8 Change paramater type used for whole bitcoin (Tobin C. Harding)
Pull request description:
We have multiple constructors for `Amount`, one of which accepts an integer representing whole bitcoin.
In an attempt to make the API crystal clear change the parameter type used for whole bitcoin to `u32` instead of a `u64` which may well be mapped to sats in the mind of developers (it is in mine anyway).
ACKs for top commit:
apoelstra:
ACK f97c04cf07c2ac5acb5d312b573d100afa009b57; successfully ran local tests
Tree-SHA512: fb22f08f182a96ff8d067337e32174137e3d73acd2faeddc2bac974ea41f8de9ec06baa2e6fb76a2fa1a7afa7bf6b4c8f606666d966413a14d39b129e36d0674
We added the "cBTC" denomination but forgot to test it.
Add units test for "cBTC" as we do for the other denominations. Put all
the tests lines of code in the same order as the enum variants so a
reader can see easily that all cases are tested.
We have multiple constructors for `Amount`, one of which accepts an
integer representing whole bitcoin.
In an attempt to make the API crystal clear change the parameter type
used for whole bitcoin to either `Into<u64>` or `u32` instead of a
`u64` which may well be mapped to sats in the mind of developers (it
is in mine anyway).
These functions do not add value because one can just call `to_sat` and
`from_sat` if doing ops in a tight loop.
Note we need to remove these deprecated functions before we release
`units v1.0`.
55092aab47 Use Self in amount consts (Tobin C. Harding)
Pull request description:
Currently the consts in the `amount` modules repeat the type. For `Amount` this isn't a big deal but for `SignedAmount` its quite noisy.
Use `Self` as the type in const declarations inside `Amount` and `SignedAmount`.
Internal change, no logic changes.
ACKs for top commit:
apoelstra:
ACK 55092aab47051e740c1ee43de73e367a2f9299a3; successfully ran local tests
Tree-SHA512: 8220541f3bac85f0a40382e9545500709dac6d96496cfae76c0ff41afd937b6452e3fa12390b38f80e586a2b52baa384c580c5fe3e8ac9e80ea394b3b3ee26db
01b58d668c Add rustdoc examples (Jamil Lambert, PhD)
b881f1b54e Add `# Errors` to rustdocs (Jamil Lambert, PhD)
66f36b3048 Correct doc errors and add links in `unsigned` (Jamil Lambert, PhD)
Pull request description:
Update rustdocs in `units::Amount` to conform to the API guidelines.
`# Examples` API guideline quote:
> "is not always to show how to use the item. ... Rather, an example is often intended to show why someone would want to use the item."
This guideline is subjective, and in the cases in `Amount` the why is often trivial. Where the example is trivial it has been included once, and for similar functions left off e.g. included for `from_sat` but not `from_btc`, etc.
ACKs for top commit:
tcharding:
ACK 01b58d668c
apoelstra:
ACK 01b58d668cf03dda6aeff7c556e359436ccc4132; successfully ran local tests
Tree-SHA512: 5dde89949e0091f166b9903d28ca44275ca3d283de4ef1b6142dba88f0c45e9ba0a8f7cefa0c36a0f1e3e9e2b61d4992f0d3324d30c09b9877b481e9f343002a
edfdd575d5 Test all the valid denomination forms (Tobin C. Harding)
Pull request description:
Exhaustively ...
ACKs for top commit:
apoelstra:
ACK edfdd575d5af1c676c0640f1e38056d7da9168e0; successfully ran local tests
Tree-SHA512: 76d078988b68f68cf8d4f18439b94cfef6b8b222596aa0b64e5270797c1d7d0f15b6376fb81d1655638e2617c33747d8a5623d3f52db0c95061723a3e36ce7de
Andrew Poelstra
Tobin C. Harding
Jamil Lambert, PhD
Shing Him Ng
jrakibi
Fmt Bot
yhzlsm
yancy