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
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`.
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
Add macros for implementing `ops::Add` and `ops::Sub` for various
combinations of references. Use the macro in `fee_rate`.
These are internal macros so no need to protect `core` using
`$crate::_export::_core`.
815330dad2 Clean up weight unit tests (Tobin C. Harding)
3f4365ee0c api: Run just check-api (Tobin C. Harding)
75594c7299 Add Weight::to_kwu_ceil (Tobin C. Harding)
Pull request description:
It is not immediately obvious why we have floor and ceil versions of `to_vbytes` but not for `to_kwu`.
Add a conversion function to get weight by kilo weight units rounding up.
And then clean up the `weight` unit tests.
ACKs for top commit:
apoelstra:
ACK 815330dad2d2026ecf11c998dd560217c8d05d52; successfully ran local tests
Tree-SHA512: 8ba6c255fb9a3bc4a3dd485d6875663976870805a639b0aa309727fd211460029d05154351522d2b753afd478df1187abd92b212512b140c1ddb24b34b7e2bc0
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).
It is not immediately obvious why we have floor and ceil versions of
`to_vbytes` but not for `to_kwu`.
Add a conversion function to get weight by kilo weight units rounding
up.
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
4cccbfdf76 units: Seal the Integer trait (Tobin C. Harding)
Pull request description:
This trait is an internal thing, users should never implement it.
ACKs for top commit:
sanket1729:
utACK 4cccbfdf76
apoelstra:
ACK 4cccbfdf7628442cc198f6399846ad4e98938494; successfully ran local tests
Tree-SHA512: f48e8ef96d15135990976c77e933ddc735dda577464548478526e37fff49bf453aaacf966967d98a0d5598588b531199627af8e353f845d4c7781f1e1d8de6db
3c8c956511 Remove Weight::from_wu_usize function (Tobin C. Harding)
Pull request description:
This constructor is an anomaly in this repo. Also it is ugly to have the type parameter hard coded in the function name.
The problem the constructor is trying to solve is that we don't like casts that don't obviously loose data. We have a solution for that already in `internals::ToU64`. This trait cannot be used in const contexts though so we do introduce a single cast in this patch.
ACKs for top commit:
apoelstra:
ACK 3c8c95651169fb9329ceb380162721c2d2f9b3aa; successfully ran local tests
Tree-SHA512: 168196edb7c151378d425b96ea3c9cdb0a4f6a879543e89facd02ed1fdf9bc69bde8ef862ffa0959b7c5ca21d6f4fe5ae38a933c379e7e88a946ca7cb68d61ec
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
60f2089dcd Clean up possibly confusing (Tobin C. Harding)
Pull request description:
Put the list of possibly confusing forms in the same order as the enum and fix the rustdocs to show the actual error variants returned.
ACKs for top commit:
apoelstra:
ACK 60f2089dcdd697b8ab53c9f96b2192ca030237ac; successfully ran local tests
Tree-SHA512: 109fd3e9b72860c3e2a1268aeafab30d6b2b6fac5d5835db6c216a434b9e175911e6445c187f6b80ab2d8c54184df9338230985a4e030aa146cd9fa410971216
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.
7725ca77c5 Rename private module to sealed (Tobin C. Harding)
Pull request description:
There are two `private` modules in `amount` but they do slightly different things. One provides a private `Token` and one is for trait sealing. We have various other trait sealing modules in the codebase and they are all called `sealed` not `private`. Also the seal trait is called `Sealed`.
Rename the `private` module and the trait to be uniform with the rest of the codebase.
ACKs for top commit:
apoelstra:
ACK 7725ca77c589a5215c50f1634f1b095d3e268540; successfully ran local tests; sure
Tree-SHA512: 5953686d7d22daaad8d2d59eff2338db3bb2a7765a37c08ed02ae1a4622509d628dbcb971781a6e85d750afa58609f5b058dfce6d5b4066f4f0d8ded45375b5b
a4a0f2921c units: Add regression tests for Default impls (Tobin C. Harding)
f5c2248a31 units: Derive Default for BlockInterval (Tobin C. Harding)
Pull request description:
Done while looking into [C-TOR](https://rust-lang.github.io/api-guidelines/predictability.html#c-ctor)
- Derive `Default` for `BlockInterval`
- Add regression test for all types that implement `Default`
ACKs for top commit:
apoelstra:
ACK a4a0f2921cd2425fa46a8ade42f888d5268e6709; successfully ran local tests
Tree-SHA512: ce39c2bcb37fc0fa70bd2553dd7843d9ac8f9528631706056ba89fda9623defadf32974091832b7a087121290b3f883bc8a8dcca070f1d90670eeee6b541de01
a2cab6f925 units: Add _export::_core (Tobin C. Harding)
Pull request description:
As we do in `bitcoin` add a module for usage in macros to prevent naming clashes if a module called `core` exists.
Overly paranoid yes but this is bitcoin after all.
ACKs for top commit:
apoelstra:
ACK a2cab6f925530b7e920362149ac4a472abf035c8; successfully ran local tests; lol sure
Tree-SHA512: f20d5b7aae55370891a2943cf3be6e04cb24a93f570093252eefee548f3e13931d03cfaef14613df2a75c477476c98f33173bd4a6da3a15beb7dc4e01648f574
4b926e1908 Change`MAX and `MIN` to equal `MAX_MONEY` (Jamil Lambert, PhD)
Pull request description:
To prevent rounding errors converting to and from f64 change `SignedAmount` `MAX` and `MIN` to +/- `MAX_MONEY` which are within the limit in f64 that has issues.
Add checks to `from_str_in`, `checked_add`, `checked_sub` and `checked_mul` that the result is within MIN and MAX.
Modify tests to work with new `MIN` and `MAX`.
Discussed in #3688 and #3691
ACKs for top commit:
tcharding:
ACK 4b926e1908
apoelstra:
ACK 4b926e1908f72af98d24cc64d7e1eef44d624e4e; successfully ran local tests
Tree-SHA512: 9053e761b3b74a7a9d826ae7271ced41cf7919752ac8ed8977a20b5b1a746ac8a6bfff68159f4a0dea733ea00f49cf41c0422de53c7aff39efd8482f8cba6069
This constructor is an anomaly in this repo. Also it is ugly to have the
type parameter hard coded in the function name.
The problem the constructor is trying to solve is that we don't like
casts that don't obviously loose data. We have a solution for that
already in `internals::ToU64`. This trait cannot be used in const
contexts though so we do introduce a single cast in this patch.
There are two `private` modules in `amount` but they do slightly
different things. One provides a private `Token` and one is for trait
sealing. We have various other trait sealing modules in the codebase and
they are all called `sealed` not `private`. Also the seal trait is
called `Sealed`.
Rename the `private` module and the trait to be uniform with the rest of
the codebase.
A block interval is a relative thing so it makes sense to default to
zero. This is the same as how we derive `Debug` for `relative::Height`
but not `absolute::Height`.
As we do in `bitcoin` add a module for usage in macros to prevent naming
clashes if a module called `core` exists.
Overly paranoid yes but this is bitcoin after all.
2fd8614f5d units: Add additional must_use (Tobin C. Harding)
79a229b391 units: Add pedantic lint return_self_not_must_use (Tobin C. Harding)
Pull request description:
Enable return_self_must_use and also run the linter locally with must_use_candidate.
Add must_use attribute as required excluding obvious functions (conversion, getters, etc).
Done as part of https://github.com/rust-bitcoin/rust-bitcoin/issues/3185
ACKs for top commit:
apoelstra:
ACK 2fd8614f5d091377f179440b04ec71f4853fd187; successfully ran local tests; nice! will one-ACK merge
Tree-SHA512: 90868846d6877830ca2b6931e8b94208434acc5a7b21808a32e2e1568c0ad33185644b931cae500e6619b8b4f19c39bce6922502d9233173722ef0e6455edba6
adaf4ac086 Set avoid-breaking-exported-api to false (Tobin C. Harding)
Pull request description:
These lints are valuable, lets get at em.
ACKs for top commit:
apoelstra:
ACK adaf4ac086553674803fadfde776d6dd013a7964; successfully ran local tests; will probably be a problem repeatedly for the next few days until we get through all currently-open PRs
Tree-SHA512: 56617a2f4aeafceef72008232cee817d45b62c040d7f1938631291c5d73627851cfbefc4b4000cd380ecb5c7e1379d1022f6cc90a4b68c819c78fb883bee0b3a
To prevent rounding errors converting to and from f64 change
`SignedAmount` `MAX` and `MIN` to +/- `MAX_MONEY` which are within the
limit in f64 that has issues.
Add checks to `from_str_in`, `checked_add`, `checked_sub` and
`checked_mul` that the result is within MIN and MAX.
Modify tests to work with new `MIN` and `MAX`
c27f443520 Add basic unit tests for Amount serde (Tobin C. Harding)
22530f6a2b Support serde serializing Amount as string (Tobin C. Harding)
Pull request description:
Sometimes JSON parsers may munge floats. Instead of using `f64` we can serialize BTC amounts as strings.
Close: #894
ACKs for top commit:
apoelstra:
ACK c27f4435208cc3ca7b98580fd7e2784e089b545e; successfully ran local tests
sanket1729:
utACK c27f443520.
Tree-SHA512: 084669a0622557b75fceae732fb485e7139ecada48c0b65642d122e1a02f6f7e41564c3579fd10adbf3aa14c82c9f10abc3f9201858e50b729852140b31a4216
These lints are valuable, lets get at em.
Changes are API breaking but because the changes make functions consume
self for types that are `Copy` downstream should not notice the breaks.
Use the `must_use_candidate` clippy lint to find all functions that are
candidates for having `must_use`.
Add `must_use` attribute but exclude obvious functions like `from_`,
`to_`, and `new`.
This patch is subjective.
ac74ed2144 Range check against SignedAmount::MAX instead of i64::MAX (yancy)
Pull request description:
Future proof this check by using SignedAmount::MAX in the case where the MAX SignedAmount changes to something other then i64::MAX.
ACKs for top commit:
tcharding:
ACK ac74ed2144
apoelstra:
ACK ac74ed2144e785fef7c395388a4fb7fb394e833e; successfully ran local tests; nice. Simple and obviously an improvement
Tree-SHA512: 4003a2f3b34e03330c57125622cab5e55a235b1a610dda622035c071bc5530811e275c2e25f40e1309cecf1c3bef35070ae690fa57fdf3e2c1b5c3f75ca5d29e
To prevent rounding errors converting to and from f64 change
`Amount::MAX` to `MAX_MONEY` which is below the limit in f64 that has
issues.
Add checks to `from_str_in`, `checked_add` and `checked_mul` that the
result is below MAX, where previously a u64 overflow was relied on.
Change tests to account for new lower MAX that is within the range of
SignedAmount and does not overflow so easily
Remove overflow tests
`Amount::MAX` is now below `u64::MAX` and within the range of values for
`SignedAmount`. These tests therefore do not overflow.
In effective_value there is no error with `Amount::MAX` and the correct
value is returned.
In psbt the removed test is effectively the same as the previous test.
Modify `Amount` tests to work with new `MAX`
Tests need to be changed that checked values above the new `MAX` or
`Amount::MAX` was out of range for `SignedAmount` which it isn't anymore
Sometimes JSON parsers may munge floats. Instead of using `f64` we can
serialize BTC amounts as strings.
Includes addition of `alloc` feature gate to `DisplayFullError` to
remove lint warnings when building with `--no-default-features`.
Close: #894
fd2a5c1ec7 Close amounts error types (Tobin C. Harding)
23c77275b1 Reduce code comment lines (Tobin C. Harding)
d595f421c6 Remove whitespace between enum variants (Tobin C. Harding)
Pull request description:
Close the two pubic enum error types in `units::amounts`. All the other structs are closed already because they either have private fields or marked `non_exhaustive`.
ACKs for top commit:
apoelstra:
ACK fd2a5c1ec79f337fb3695c030c9fb6b4671468f2; successfully ran local tests; thanks!
Tree-SHA512: f8d68ef821449e0829c926cf527df4b226b29c8d1d41b320a016fbf70b4b39cc54c8c218955caa0c3776158eeeae0ebacc1cc89dab67bafc399b94063324ab0e
Close the two pubic enum error types in `units::amounts`. All the other
structs are closed already because they either have private fields or
marked `non_exhaustive`.
433f70939c Implement iter::Sum for BlockInterval (Tobin C. Harding)
0369e64b56 Implement Sum for an iterator of references to amounts (Tobin C. Harding)
31f883ac00 Implement iter::Sum for FeeRate (Tobin C. Harding)
Pull request description:
Enables summing an iterator of values. Note that this does not include either `LockTime`s. `absolute::LockTime` should not be added and for `relative::LockTime` we have https://github.com/rust-bitcoin/rust-bitcoin/issues/3676Close: #1638
ACKs for top commit:
apoelstra:
ACK 433f70939c3ecc10702ab6502e3f9bcd94dab739; successfully ran local tests; nice!
sanket1729:
utACK 433f70939c
Tree-SHA512: 1eda00f3bbbc61f795198ce8525a5a9b690478a8abc268da6d2e40de7d91decc28dd8211df0c6abeaf30148c7ec3907b85e3c5351972c354590569840e84d562
22769268f3 units: Close the hex parse errors (Tobin C. Harding)
Pull request description:
As part of the 1.0 effort close the errors in the `units::parse` module.
ACKs for top commit:
apoelstra:
ACK 22769268f34b45c0bd86c548eb13cfe1e290f0d7; successfully ran local tests; thanks!
sanket1729:
ACK 22769268f3
Tree-SHA512: 598ba236f8c08c3f750112e0d8b8e0aa0568be2700a328e4a2d8999ca59eada8a16a6a1d9e9121e7f42ce9bbe3da3f87221ba67c36056996a687e489f4c6007c
The `Amount` and `SignedAmount` were not supposed to implement `serde`
traits by design because doing so implicitly uses sats. We provide two
modules `as_sat` and `as_btc` to allow users to explicitly serialize in
their preferred format.
In commit: `d57ec019d5 Use Amount type for TxOut value field` derives
were added for `serde` and we did not notice it during review.
17ca5018eb units: Comment alloc feature (Tobin C. Harding)
Pull request description:
Add a comment to the `serde` module about why we have all the `alloc` feature gating.
Done as part of #3556 - auditing the whole crate for use of `alloc` feature.
ACKs for top commit:
apoelstra:
ACK 17ca5018ebb28d5d392d61aee62170fb3e4f2a5b; successfully ran local tests
Tree-SHA512: 6d13129903a9c530db811e9f64ae4fa653dbcc952a42502c53c6c622a80c1e58b3037edd0638de78867823ca370b3a4595a08a9ec07b84c71afb6200454f5d6d
a8379bf005 Mark `checked_` functions const in bitcoin. (Jamil Lambert, PhD)
fe10ff2eb7 Mark functions `const` in `units` (Jamil Lambert, PhD)
Pull request description:
Following on #3608, #3627 and #1174 the rest of the `checked_` functions in all `rust-bitcoin` have been marked as const. Except for tests and traits.
ACKs for top commit:
apoelstra:
ACK a8379bf0053e66cf5984ce449d19e54e529b6b70; successfully ran local tests; thanks! I find this much easier to read
tcharding:
ACK a8379bf005
Tree-SHA512: b8d97b7a3d9fc33b57349f418ccc5aac2f3e8c4145a73bf0e24e85547217d41214696e9a80b1fb5a1bae7e766aad1c2b5df6044564772fe76cec9b3628bcd8e5
Add a comment to the `serde` module about why we have all the `alloc`
feature gating.
Done as part of #3556 - auditing the whole crate for use of `alloc`
feature.
Mark `checked_` functions const. Replace `map()` and `?` operators,
which are not allowed in const context, with match statements.
Use descriptive variable names in ceiling division to make
it easier to follow.
4223655829 Add test case (yancy)
Pull request description:
Since there are test cases for other functions here, add test case as follow up for https://github.com/rust-bitcoin/rust-bitcoin/pull/3604. I forgot to request this during review.
ACKs for top commit:
apoelstra:
ACK 422365582964cf405529de3a1cf63d766b0d0e2e; successfully ran local tests
tcharding:
ACK 4223655829
Tree-SHA512: 8629cfff3e3eb800aa4d4625627e0a777c389566f4125bc63e67fe4c5a597ca63b9fb92da42f6c9dab12e6c63839eef35a13957f98606a37ed687eb96d896c2e
0348801ef3 Remove Amount::fmt_value_in (Shing Him Ng)
Pull request description:
Saw this mentioned in #3556Fixes#2952
ACKs for top commit:
apoelstra:
ACK 0348801ef30d1cdfdc40cd2316671532d4c06141; successfully ran local tests
tcharding:
ACK 0348801ef3
Tree-SHA512: 29a1d230672a22d059d2ef0870e68dec0fe6d6ae9d3fed51275d9b6779d097578a38a514544df11bdf23908c2d8e3c5fd5c6de2e460aab9792770d6b9e396fc6
4c94695942 Mark funtions const (yancy)
Pull request description:
May as well mark these const as well. Follow up to https://github.com/rust-bitcoin/rust-bitcoin/pull/3605
ACKs for top commit:
apoelstra:
ACK 4c94695942de41e5eca4ac120db2e069c0bb037d; successfully ran local tests; yeah, I think this are all worth consting
tcharding:
ACK 4c94695942
sanket1729:
ACK 4c94695942
Tree-SHA512: bf41e416cb8a2eb90e26d7bd640df5c2a5a868e3b721bde7cc8a3b97c687bab4d1cb4ab1c4b04ed625e8657e25c59e386720cb0f9d735ffc492e98f5b8b2ca3c
1db4b723dc Mark function as const (yancy)
Pull request description:
We discussed marking from_vb as const here: https://github.com/rust-bitcoin/rust-bitcoin/issues/3602
However, from what I can tell, map() isn't const and I don't see a tracking issue for changing it. Also, the question mark operator isn't available in const context either, although there is a tracking issue for that: https://github.com/rust-lang/rust/issues/74935. It will be a long while before that makes it into this projects MSRV if/when it lands.
There are some other functions in this module that could use the same re-write to make them const as well it looks like.
ACKs for top commit:
tcharding:
ACK 1db4b723dc
apoelstra:
ACK 1db4b723dc59568c14c76598e7c63c58651ed1cd; successfully ran local tests
Tree-SHA512: 62b612791dd3ce2f6ebf27f586a1262633a46566b9fd3583984171f62209714ad979439345fe86d8ef87d2f78a2cee21d619e2eb3621689faf59d81640e9f77c
There is a range of different wordings used in the docs of constructor
type functions.
Change all to start with `Constructs a new` or `Constructs an empty`.
In functions that act like constructors there is a mixture of the usage
of `creates` and `constructs`.
Replace all occurrences of `creates` with `constructs` in the first line
of docs of constructor like functions.
81d8699b55 units: Put no_std up top (Tobin C. Harding)
3e332c3839 amount: Fix docs on FromStr (Tobin C. Harding)
5bec76aa51 amount: Fix rustdocs (Tobin C. Harding)
41c80cc476 amount: Improve docs on div by weight (Tobin C. Harding)
2b4a61739a Add type to debug output for Amount (Tobin C. Harding)
42e5043b33 Add from_int_btc group of functions (Tobin C. Harding)
Pull request description:
Improve the `amount` module by doing:
- Patch 1: Add/update `from_int_btc` and `from_int_btc_const` functions
- Patch 2: Add type to debug output for `Amount`
- Patch 3: Fix incorrect docs
- Patch 4: Make docs use correct style and be uniform across the two amount types
- Patch 5: Fix docs on `FromStr`
ACKs for top commit:
apoelstra:
ACK 81d8699b55dad570247cb14d2b3eea64270a83cf; successfully ran local tests
jamillambert:
ACK 81d8699b55
Tree-SHA512: ddd6e02b4cd9a9aa8cbdd2d2f7ae289a6bcca9eb002ef0c6d83a6eca950b2cd08f5918286bb33e814f321e3bfe83fdf75ae051cf8f91ee45d3e4abe76c1c2a4c
The `no_std` attribute has significant ramifications, as opposed to the
clippy attributes etc.
Put the `no_std` attribute up top so it catches the eye. This is also
uniform with other crates in this repo.
The docs on `FromStr` say "string slice is zero" but actually mean to
document the behaviour when the value represented is zero.
Also the fact that `FromStr` returns an error if parsing fails is self
evident.
Fix the rustdocs on `Amount` and `SignedAmount` by doing:
- Make them uniform
- Use correct style (looks like `SignedAmount` got left behind when we
improved `Amount`)
Add/update the from_int group of functions to provide one that errors
and one that is const and panics (errors in const context are not useful
because one cannot call `unwrap` in const context).
In an effort to reduce requirement for `alloc`; remove the `String` and
use `InputString` in the hex prefix related error types.
Tested with:
(Note in test code we have to use `"cafe".to_owned()` before this patch
is applied.)
rust```
#[test]
fn hex_prefix_errors() {
let err = hex_remove_prefix("cafe").unwrap_err();
std::println!("{}", err);
std::println!("{:?}", err);
let err = hex_check_unprefixed("0xcafe").unwrap_err();
std::println!("{}", err);
std::println!("{:?}", err);
}
```
Before:
hex string is missing a prefix (e.g. 0x): cafe
MissingPrefixError { hex: "cafe" }
hex string contains a prefix: 0xcafe
ContainsPrefixError { hex: "0xcafe" }
After:
failed to parse 'cafe' as hex because it is missing the '0x' prefix
MissingPrefixError { hex: InputString("cafe") }
failed to parse '0xcafe' as hex because it contains the '0x' prefix
ContainsPrefixError { hex: InputString("0xcafe") }
3f2e760d1f Replace String with InputString in ParseIntError (Tobin C. Harding)
aa5c78430c Replace invalidInteger with ParseIntError (Tobin C. Harding)
9b7a706bfd Remove From<ParseIntError> (Tobin C. Harding)
c90f4b6033 Fix bug in error output (Tobin C. Harding)
c986b2f620 internals: Move error.rs to error/mod.rs (Tobin C. Harding)
Pull request description:
This PR hopefully clears the way for removing many of the `alloc` feature gates in `units` and `primitives`
The three final patches were tested by adding the following test to `units::locktime::absolute`:
```rust
#[test]
pub fn debug_absolute_error_conversion_height() {
let invalid_height = LOCK_TIME_THRESHOLD + 1;
let err = Height::from_consensus(invalid_height).unwrap_err();
std::println!("{:?}", err);
std::println!("{}", err);
let invalid_time = LOCK_TIME_THRESHOLD - 1;
let err = Time::from_consensus(invalid_time).unwrap_err();
std::println!("{:?}", err);
std::println!("{}", err);
let invalid_height = std::format!("{:x}", LOCK_TIME_THRESHOLD + 1);
let err = Height::from_hex(&invalid_height).unwrap_err();
std::println!("{:?}", err);
std::println!("{}", err);
let invalid_time = std::format!("{:x}", LOCK_TIME_THRESHOLD - 1);
let err = Time::from_hex(&invalid_time).unwrap_err();
std::println!("{:?}", err);
std::println!("{}", err);
let err = Height::from_hex("somerandomshit").unwrap_err();
std::println!("{:?}", err);
std::println!("{}", err);
let err = Time::from_hex("somerandomshit").unwrap_err();
std::println!("{:?}", err);
std::println!("{}", err);
}
```
Gives the following output (the last four lines is the bit that changes, the rest just proves we don't break other variants)
On commit: `d47ff1c25 Remove From<ParseIntError>`
ConversionError { unit: Blocks, input: 500000001 }
invalid lock time value 500000001, expected lock-by-blockheight (must be < 500000000)
ConversionError { unit: Seconds, input: 499999999 }
invalid lock time value 499999999, expected lock-by-blocktime (must be >= 500000000)
ParseHeightError(Conversion(500000001))
block height 500000001 is above limit 499999999
ParseTimeError(Conversion(499999999))
block height 499999999 is below limit 500000000
ParseHeightError(InvalidInteger { source: ParseIntError { kind: InvalidDigit }, input: "somerandomshit" })
failed to parse somerandomshit as block height
ParseTimeError(InvalidInteger { source: ParseIntError { kind: InvalidDigit }, input: "somerandomshit" })
failed to parse somerandomshit as block time
On commit: `0155a0d9a Replace invalidInteger with ParseIntError`
ConversionError { unit: Blocks, input: 500000001 }
invalid lock time value 500000001, expected lock-by-blockheight (must be < 500000000)
ConversionError { unit: Seconds, input: 499999999 }
invalid lock time value 499999999, expected lock-by-blocktime (must be >= 500000000)
ParseHeightError(Conversion(500000001))
block height 500000001 is above limit 499999999
ParseTimeError(Conversion(499999999))
block height 499999999 is below limit 500000000
ParseHeightError(ParseInt(ParseIntError { input: "somerandomshit", bits: 32, is_signed: true, source: ParseIntError { kind: InvalidDigit } }))
failed to parse somerandomshit as block height
ParseTimeError(ParseInt(ParseIntError { input: "somerandomshit", bits: 32, is_signed: true, source: ParseIntError { kind: InvalidDigit } }))
failed to parse somerandomshit as block time
On Commit: `3f2e760d1 Replace String with InputString in ParseIntError`
ConversionError { unit: Blocks, input: 500000001 }
invalid lock time value 500000001, expected lock-by-blockheight (must be < 500000000)
ConversionError { unit: Seconds, input: 499999999 }
invalid lock time value 499999999, expected lock-by-blocktime (must be >= 500000000)
ParseHeightError(Conversion(500000001))
block height 500000001 is above limit 499999999
ParseTimeError(Conversion(499999999))
block time 499999999 is below limit 500000000
ParseHeightError(ParseInt(ParseIntError { input: InputString("somerandomshit"), bits: 32, is_signed: true, source: ParseIntError { kind: InvalidDigit } }))
failed to parse 'somerandomshit' as absolute Height/Time (block height)
ParseTimeError(ParseInt(ParseIntError { input: InputString("somerandomshit"), bits: 32, is_signed: true, source: ParseIntError { kind: InvalidDigit } }))
failed to parse 'somerandomshit' as absolute Height/Time (block time)
ACKs for top commit:
apoelstra:
ACK 3f2e760d1fef2951f93a2554cd53340b0d7a6e0b; successfully ran local tests; nice!
Tree-SHA512: f7fd55acfb83082419db22c24a6b375c20e2631263401e500410c5b5659463f06dc4bdb145621e475dc15d75e764668cdcbf8f88006a487248a05fdb237ad136
Currently the `ParseIntError` contains an owned copy of the input
string, this is causing us to have to use `alloc` everywhere.
We already have a alloc-friendly string replacement type, the
`InputString` - use it.
We have a special type for wrapping integer parsing errors, use it.
To test this I added the following tests:
#[test]
pub fn debug_absolute_error_conversion_height() {
let invalid_height = LOCK_TIME_THRESHOLD + 1;
let _ = Height::from_consensus(invalid_height).unwrap();
}
#[test]
pub fn debug_absolute_error_conversion_time() {
let invalid_time = LOCK_TIME_THRESHOLD - 1;
let _ = Time::from_consensus(invalid_time).unwrap();
}
#[test]
#[cfg(feature = "std")]
pub fn debug_absolute_error_conversion_height_string() {
let invalid_height = std::format!("{:x}", LOCK_TIME_THRESHOLD + 1);
let _ = Height::from_hex(&invalid_height).unwrap();
}
#[test]
#[cfg(feature = "std")]
pub fn debug_absolute_error_conversion_time_string() {
let invalid_time = std::format!("{:x}", LOCK_TIME_THRESHOLD - 1);
let _ = Time::from_hex(&invalid_time).unwrap();
}
#[test]
#[cfg(feature = "std")]
pub fn debug_absolute_error_height_invalid_hex_string() {
let _ = Height::from_hex("somerandomshit").unwrap();
}
#[test]
#[cfg(feature = "std")]
pub fn debug_absolute_error_time_invalid_hex_string() {
let _ = Time::from_hex("somerandomshit").unwrap();
}
Which resulted in the following output
Before:
---- locktime::absolute::tests::debug_absolute_error_conversion_height stdout ----
thread 'locktime::absolute::tests::debug_absolute_error_conversion_height' panicked at units/src/locktime/absolute.rs:431:56:
called `Result::unwrap()` on an `Err` value: ConversionError { unit: Blocks, input: 500000001 }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
---- locktime::absolute::tests::debug_absolute_error_conversion_height_string stdout ----
thread 'locktime::absolute::tests::debug_absolute_error_conversion_height_string' panicked at units/src/locktime/absolute.rs:444:51:
called `Result::unwrap()` on an `Err` value: ParseHeightError(Conversion(500000001))
---- locktime::absolute::tests::debug_absolute_error_conversion_time stdout ----
thread 'locktime::absolute::tests::debug_absolute_error_conversion_time' panicked at units/src/locktime/absolute.rs:437:52:
called `Result::unwrap()` on an `Err` value: ConversionError { unit: Seconds, input: 499999999 }
---- locktime::absolute::tests::debug_absolute_error_height_invalid_hex_string stdout ----
thread 'locktime::absolute::tests::debug_absolute_error_height_invalid_hex_string' panicked at units/src/locktime/absolute.rs:457:52:
called `Result::unwrap()` on an `Err` value: ParseHeightError(InvalidInteger { source: ParseIntError { kind: InvalidDigit }, input: "somerandomshit" })
---- locktime::absolute::tests::debug_absolute_error_conversion_time_string stdout ----
thread 'locktime::absolute::tests::debug_absolute_error_conversion_time_string' panicked at units/src/locktime/absolute.rs:451:47:
called `Result::unwrap()` on an `Err` value: ParseTimeError(Conversion(499999999))
---- locktime::absolute::tests::debug_absolute_error_time_invalid_hex_string stdout ----
thread 'locktime::absolute::tests::debug_absolute_error_time_invalid_hex_string' panicked at units/src/locktime/absolute.rs:464:50:
called `Result::unwrap()` on an `Err` value: ParseTimeError(InvalidInteger { source: ParseIntError { kind: InvalidDigit }, input: "somerandomshit" })
After:
---- locktime::absolute::tests::debug_absolute_error_conversion_height stdout ----
thread 'locktime::absolute::tests::debug_absolute_error_conversion_height' panicked at units/src/locktime/absolute.rs:432:56:
called `Result::unwrap()` on an `Err` value: ConversionError { unit: Blocks, input: 500000001 }
---- locktime::absolute::tests::debug_absolute_error_conversion_height_string stdout ----
thread 'locktime::absolute::tests::debug_absolute_error_conversion_height_string' panicked at units/src/locktime/absolute.rs:445:51:
called `Result::unwrap()` on an `Err` value: ParseHeightError(Conversion(500000001))
---- locktime::absolute::tests::debug_absolute_error_conversion_time stdout ----
thread 'locktime::absolute::tests::debug_absolute_error_conversion_time' panicked at units/src/locktime/absolute.rs:438:52:
called `Result::unwrap()` on an `Err` value: ConversionError { unit: Seconds, input: 499999999 }
---- locktime::absolute::tests::debug_absolute_error_conversion_time_string stdout ----
thread 'locktime::absolute::tests::debug_absolute_error_conversion_time_string' panicked at units/src/locktime/absolute.rs:452:47:
called `Result::unwrap()` on an `Err` value: ParseTimeError(Conversion(499999999))
---- locktime::absolute::tests::debug_absolute_error_height_invalid_hex_string stdout ----
thread 'locktime::absolute::tests::debug_absolute_error_height_invalid_hex_string' panicked at units/src/locktime/absolute.rs:458:52:
called `Result::unwrap()` on an `Err` value: ParseHeightError(ParseInt(ParseIntError { input: "somerandomshit", bits: 32, is_signed: false, source: ParseIntError { kind: InvalidDigit } }))
---- locktime::absolute::tests::debug_absolute_error_time_invalid_hex_string stdout ----
thread 'locktime::absolute::tests::debug_absolute_error_time_invalid_hex_string' panicked at units/src/locktime/absolute.rs:465:50:
called `Result::unwrap()` on an `Err` value: ParseTimeError(ParseInt(ParseIntError { input: "somerandomshit", bits: 32, is_signed: false, source: ParseIntError { kind: InvalidDigit } }))
The errors in `units::locktime::absolute` are complex, I'd like to make
them more simple so they are more understandable.
I have no clue why this is implemented - remove it.
This has been fixed and we use nightly to lint so we have access to the
merged fix.
Removing the attribute uncovers a bunch of real lint warnings, fix
them while we are at it.
In an effort to make the `amount` module more readable move the
`SignedAmount` type to a private submodule.
Re-export everything so this is not a breaking change.
Code move and re-exports only.
When we move `SignedAmount` to a submodule linking to `self` introduces
a clippy warning, I'm not exactly sure why but lets remove the link in
preparation for the move.
In an effort to make the `amount` module more readable move the `Amount`
type to a private submodule.
Re-export everything so this is not a breaking change.
Code move and re-exports only.
There is _a lot_ of error types in the `amount` module. Move them to a
separate `error` module.
Add a bunch of `pub(super)` to keep things private to the `amount`
module.
Eventually we will want to close all these errors.
In preparation for splitting the error types out of `amount.rs` into
their own file move the `amount.rs` file to `amount/mod.rs`.
File move only, no other changes.
88b53a471e Unify deprecated note field format (Jamil Lambert, PhD)
Pull request description:
Following the suggestion in Issue #3306 all the deprecated note fields have been changed to be lower case and in the format "use `abc` instead".
ACKs for top commit:
tcharding:
ACK 88b53a471e
apoelstra:
ACK 88b53a471e successfully ran local tests
Tree-SHA512: 5c20eda7140f37ce78eb58dfdf03ecc11a67fcb10f294d860e81eaaee696e3a4209516017a9885cef9bfff1aa69b845534d139578b674933770fa24d59e4275f
We use `TBD` in our `deprecated` string and it was discovered that there
is an exception on this string so as not to warn because it is used
internally by the Rust language. However there is a special lint to
enable warnings, lets use it.
Add `#![warn(deprecated_in_future)]` to the coding conventions section
of all crates except `fuzz`.
a0c58a4a8b Add checked weight division to Amount (yancy)
8def40a991 Add assertions to checked_weight_mul test (yancy)
16ce70d3a6 Add div_by_weight test to fee_rate (yancy)
Pull request description:
Adds the checked variant of `amount / weight`. I also added a test to the non-checked version for comparison so the reviewer knows they compute the same way (integer division rounded down).
Also added assertion to `checked_weight_mul test` showing the results are rounded up.
ACKs for top commit:
tcharding:
ACK a0c58a4a8b
apoelstra:
ACK a0c58a4a8b successfully ran local tests
Tree-SHA512: cf14123ed261d100e3261a720c26f8c10368f05225e32eaa246f25ab766d20515db5feb98335d4e3e08a8146a70db65ff64670da3f75e7764e8f86ef534d2663
de319670ae feat: Create relative lock times at compile time (Christian Lewe)
53e1fb6b0c feat: Create absolute lock time at compile time (Christian Lewe)
Pull request description:
I noticed that I cannot create lock times as compile-time constants. This PR tries to remedy this issue by marking lock time constructors as `const`.
Because the `internals` crate depends on the `units` crate in a way that I don't fully understand yet, this PR updates the `units` crate only.
If `from_consensus` is being kept non-`const` by design, to keep the API flexible to future changes, then please close this PR. In this case, I overlooked existing discussions.
ACKs for top commit:
apoelstra:
ACK de319670ae successfully ran local tests; good start; should backport
tcharding:
ACK de319670ae
Tree-SHA512: 69f9147707bcf8f91f755dd6d1be5ed08945e775ee46918e33d77a9d07ce474047a80ed1226134a3914ead51d1ddbbc657552ca934dc3c079b92ad3d50b13152
Also mark these methods as const. Because <u32 as From<u16>>::from
is not available in const contexts, I had to cast u16 as u32. I try to
avoid casts as much as I can, but in this case a cast seems unavoidable.
Casting u16 as u32 should be safe on all architectures.
Mark the from_consensus and to_consensus methods of the absolute
lock time structs as const. In theory. these methods do some sanity
checking and wrap a u32 value in a newtype. This should be possible
to do in const. Marking the methods as const should not break existing
call sites.
f6abdcc001 Allow unused in `macros.rs` docs (Jamil Lambert, PhD)
fd89ddf401 Remove or fix unused variables and methods in docs (Jamil Lambert, PhD)
ff6b1d4f19 Remove unused variables and methods from docs (Jamil Lambert, PhD)
e58cda6f92 Remove `unused_imports` in docs (Jamil Lambert, PhD)
Pull request description:
As mentioned in #3362 examples in documentation are not linted in the same way as other code, but should still contain correctly written code.
#![doc(test(attr(warn(unused))))] has been added to all lib.rs files
In the docs throughout all crates:
- Unused imports have been removed.
- Unused variables, structs and enums have been used e.g. with an `assert_eq!` or prefixed with `_`
- Unused methods have been called in the example code.
ACKs for top commit:
tcharding:
ACK f6abdcc001
apoelstra:
ACK f6abdcc001 successfully ran local tests
Tree-SHA512: c3de1775ecde6971056e9fed2c9fa1621785787a6a6ccbf3a6dbd11e18d42d4956949f3f8adfc75d94fd25db998b04adb1c346cc2c2ba47f4dc37402e1388277
f5cae1cddd Comment from_str methods (yancy)
Pull request description:
Follow up from https://github.com/rust-bitcoin/rust-bitcoin/pull/3346
ACKs for top commit:
tcharding:
ACK f5cae1cddd
apoelstra:
ACK f5cae1cddd successfully ran local tests
Tree-SHA512: 2b95381e5281754e2b3a49aa8dfaac5742c244970fb54f68024dc23b61a74955ae95b9a0e7ae848095ac0686df5faf93faf7de3371b2f341b108cc10e5d4a9cd
8c29fe08f8 Revise doc comment (yancy)
Pull request description:
Update doc comment to make clear that the ceiling is computed instead of the default behavior for integer division.
ACKs for top commit:
tcharding:
ACK 8c29fe08f8
apoelstra:
ACK 8c29fe08f8 successfully ran local tests
Tree-SHA512: 3793dccab5b5a3e59b3949ecb54475c76263e1debcc18df42f3b0251189435ba87e537c4b5d80c91f4b916449618a75e6efac32b4ba2fc29c42563e1b0fb4a89
Examples in documentation are not linted in the same way as other code,
but should still contain correctly written code.
Throughout all of the crates except internals (another commit) unused
variables have been prefixed with `_`, unused imports have been removed,
and a warn attribute added to all of the `lib.rs` files.
894f82e7cc Add a condition for parsing zero from string when not denominated. (yancy)
Pull request description:
closes https://github.com/rust-bitcoin/rust-bitcoin/issues/3307
ACKs for top commit:
Kixunil:
ACK 894f82e7cc
tcharding:
ACK 894f82e7cc
apoelstra:
ACK 894f82e7cc9eb459a297d43e82734621e0824610; successfully ran local tests.
Tree-SHA512: 6d32847c74ccedc3355d615838e2dd1e08add29cc47ff69d9ef22683eda93049d41131dd0c992c8d7be3a018f5438856c415a3d2f3cb9de9c986534b268ee386
b6371b5801 Fix clippy rustdocs warnings (Tobin C. Harding)
Pull request description:
A new nightly version (`nightly-2024-08-28`) introduces a few warnings because of our rustdocs. These are valid warnings and should be fixed, thanks `clippy` team.
(The `bip152` change is a bit sloppy, open to suggestions.)
ACKs for top commit:
apoelstra:
ACK b6371b5801 successfully ran local tests
Kixunil:
ACK b6371b5801
Tree-SHA512: 503fb9d48772b74a5acdb26c0f77a85c52323c03360f983204fccee0f28bedeff142237b067caa1ce6ea04ea9842cc493e0d06dc141ca00a98151fa002b62392
3f244db19d Add Arbitrary to SignedAmount type (yancy)
Pull request description:
While proptesting, I've found Arbitrary for SingedAmount to be useful. Would appreciate adding this as well.
ACKs for top commit:
tcharding:
ACK 3f244db19d
Kixunil:
ACK 3f244db19d
Tree-SHA512: f293215505db85998135c75e147f4cb0031a08c7db619b706e6c7d632c6f23783af56992531396aecc6e3da55b107a7f2c7ec4d68a3e5737ebcf4225f306ed29
A new nightly version (`nightly-2024-08-28`) introduces a few warnings
because of our rustdocs. These are valid warnings and should be fixed,
thanks `clippy` team.
(The `bip152` change is a bit sloppy, open to suggestions.)
f8edbd1f45 Add Arbitrary to Weight (yancy)
Pull request description:
Adds Arbitrary to the Weight type. I had meant to add this with the initial Arbitrary PR but I overlooked it.
ACKs for top commit:
tcharding:
ACK f8edbd1f45
Kixunil:
ACK f8edbd1f45
Tree-SHA512: b886f78285b5cd06b2531a1e53272cad623a8e43e76e8108abe90668b45d00674b6a867c5ff980ee7f0a34919e1d6bb534624bd0bc67a53c8c9a41ea2ed504e8
`s.parse` is more idiomatic and produces more helpful error messages.
This has been changed repo wide in the main codebase, not including
examples, rustdocs, and in the test module.
`use std::str::FromStr;` has been removed where this change makes
it unnecessary.
3e034d5ede Add Arbitrary dependency (yancy)
Pull request description:
Adds an example draft showing what is needed to use Arbitrary for coin selection.
Shot out to how nice Arbitrary is for fuzzing a target by taking unstructured randomness and creating structured rust-bitcoin types for fuzzing. Is there a way we could add this to rust-bitcoin for structuring the fuzz data needed?
This is then the example to fuzz test a SRD algo (after applying this PR to rust-bitcoin) using rust-bitcoin types :)
```
#![no_main]
use arbitrary::Arbitrary;
use bitcoin::{Amount, FeeRate};
use bitcoin_coin_selection::{select_coins_srd, WeightedUtxo};
use libfuzzer_sys::fuzz_target;
use rand::thread_rng;
#[derive(Arbitrary, Debug)]
pub struct Params {
target: Amount,
fee_rate: FeeRate,
weighted_utxos: Vec<WeightedUtxo>,
}
fuzz_target!(|params: Params| {
let Params { target: t, fee_rate: f, weighted_utxos: wu } = params;
select_coins_srd(t, f, &wu, &mut thread_rng());
});
```
ACKs for top commit:
tcharding:
ACK 3e034d5ede
Kixunil:
ACK 3e034d5ede
apoelstra:
ACK 3e034d5ede successfully ran local tests
Tree-SHA512: accd565815de3b37730d2ff12a24fcfc84e52ad357e5c940b1500a1e0bb17f4ff5fd6e52d31e8e96bb5290ee4fa050cfd2a9bbd6bbae13fc378f43093b64177f
c427d8b213 bitcoin: Compile time assert on index size (Tobin C. Harding)
49a6acc1a0 internals: Remove double parenthesis in const_assert (Tobin C. Harding)
2300b285ef units: Remove compile time pointer width check (Tobin C. Harding)
Pull request description:
3 patches in preparation for other size related work, this PR does not touch the `ToU64` issue which will be handled separately.
- Patch 1: Don't check pointer width in `units` because its not consensus code
- Patch 2: Modify internal macro `const_assert`
- Patch 3: Use index size to enforce not building on a 16 bit machine
ACKs for top commit:
Kixunil:
ACK c427d8b213 though I think the last commit was kinda a waste of time and it should have been adding the trait instead or leave it for later.
apoelstra:
ACK c427d8b213 successfully ran local tests; unsure if we want to merg this or wait for #3215
Tree-SHA512: 823df5b6a5af3265bce2422c00d287f45816faeb5f965685650ac974a1bd441cf548e25ac2962591732ff221bee91a55703da936382eb166c014ca5d4129edf8
The `units` crate does not contain consensus logic and since our
requirement to only support 32-bit and 64-bit machines is due to
consensus logic we do not need to enforce the `target_pointer_width` in
the `units` crate.
Remove the compile time check on pointer width from the `units` crate.
We re-export to help users keep their dependencies in sync but `serde`
is at `v1.0` so this is not really a problem.
Remove the public re-export of `serde` (and with current MSRV we don't
need the `extern crate` at all).
ab581a90f8 Remove re-export of ParseIntError (Tobin C. Harding)
Pull request description:
In d242125 I claimed that `ParseIntError` was somehow special, I no longer thing this is the case. As we pin down the re-export policy (for errors and other types) it is hard if we have one non-typical re-export.
We have https://github.com/rust-bitcoin/rust-bitcoin/issues/3068 to discuss the policy, for now just remove the unusual re-export.
ACKs for top commit:
Kixunil:
ACK ab581a90f8
shinghim:
ACK ab581a90f8
apoelstra:
ACK ab581a90f8
Tree-SHA512: 5ac4123aeb27c8cee78e5760f21e70be8035d526ba7e14e72759cba27f98b51cc2cba9b2bf0eeb99e0f6b7210ec4a750986bb6c5dc0725ed892730fdec8a7e06
a76c13f675 Add more unit test coverage for relative LockTime (Shing Him Ng)
Pull request description:
Adding some test cases for `from_seconds_floor` and one more for `from_seconds_ceil`
ACKs for top commit:
tcharding:
ACK a76c13f675
Kixunil:
ACK a76c13f675
Tree-SHA512: 5d773a30fa56842af21109d232ec3eae7fde7ce38aca93dadccc7bfbf14d5207ea329ef737f847e45ba755fd22e1862c3e78e7e64a8a0babebcb0e27a2bb7a90
In d242125 I claimed that `ParseIntError` was somehow special, I no
longer thing this is the case. As we pin down the re-export policy (for
errors and other types) it is hard if we have one non-typical re-export.
We have https://github.com/rust-bitcoin/rust-bitcoin/issues/3068 to
discuss the policy, for now just remove the unusual re-export.
2169b75bba Use lower case error messages (Jamil Lambert, PhD)
Pull request description:
Error messages should be lower case, except for proper nouns and variable names. These have all been changed.
~~They should also state what went wrong. Some expect error messages were positive, giving the correct behaviour or correct input. These have been changed so that they are now negative, i.e. saying what went wrong.~~
EDIT: After further discussion it was decided not to change the expect messages.
ACKs for top commit:
Kixunil:
ACK 2169b75bba
tcharding:
ACK 2169b75bba
Tree-SHA512: 92442c869e0141532425f6fca5195fd319b65026f68c4230a65ad70253565d98931b2b44ee202975c307280525c505147e272297dc81207312e40c43d007021c
7874db8fc3 Throw error instead of panic when from_second_ceil input is too large (Shing Him Ng)
Pull request description:
Update `from_second_ceil` per [comment](https://github.com/rust-bitcoin/rust-bitcoin/issues/3029#issuecomment-2227459248) and add unit tests
Resolves#3029
ACKs for top commit:
Kixunil:
ACK 7874db8fc3
tcharding:
ACK 7874db8fc3
Tree-SHA512: 0d60397f867d4536ff00b81441c27ea3dc4bf9e5ecc2fb5afbe50cb3153101df54c2bb5056da1916d83c3e84367f987f5bac07a381b7433a5701a0ea8a587f95
84dd04cf60 Rename variable assignment (yancy)
Pull request description:
The type created after assignment is a Weight type. Using a var name vb which is short for virtual byte is incorrect.
Pulled this out of stale PR 2215
ACKs for top commit:
shinghim:
ACK 84dd04cf60
apoelstra:
ACK 84dd04cf60
Kixunil:
ACK 84dd04cf60
Tree-SHA512: 1bfc875f53037b2c1e8da25fe44e9ca3303981bdce4e48661a8fb2061833e3cd90318d854f7119c805e861cd8a591697378f829f32eb74ac99a71dc4c947abde
1cce1b5aa6 Remove private prelude module from units crate (Jamil Lambert, PhD)
Pull request description:
The private prelude module has been removed from the units crate and instead imports are stated in full when needed. As discussed in #2926.
ACKs for top commit:
Kixunil:
ACK 1cce1b5aa6
apoelstra:
ACK 1cce1b5aa6
Tree-SHA512: 58b93ff66f74399938bc1a7f59fe8d2a21d0437c7e90e0c190d3d6a8de30f9c9268c8e4288d1db287b4d190624968937b1ad6c6e54d29025370e47e71be925c1
6dd5af9678 Add missing links (Jamil Lambert, PhD)
47e367f011 Standardize error headings (Jamil Lambert, PhD)
75f317a689 Fix rustdoc grammar (Jamil Lambert, PhD)
573f8ce724 Add backticks to rustdoc links (Jamil Lambert, PhD)
Pull request description:
The rustdocs in the `units` crate has been reviewed and improved.
Some of the links were missing backticks, these have been added.
Some grammatical changes have been made to improve the documentation.
The use of links was inconsistent and has been changed to have links everywhere that seemed appropriate.
A couple of error headings were added and a description as to why a capital M is not accepted in the denomination for MegaSatoshi or MegaBitcoin.
ACKs for top commit:
apoelstra:
ACK 6dd5af9678
tcharding:
ACK 6dd5af9678
Tree-SHA512: f5481a7c3aa99d7882cda9ccda9df9e27f092ff91ef584f07dc887f38bfe12e5ea801cfa7f42bb4022301860489ee6be55557752a8cbe70932f258ea753495dc
7fa53440dc Move serde_round_trip macro to internals (Tobin C. Harding)
Pull request description:
We currently duplicate the serde_round_trip macro in `units` and `bitcoin`, this is unnecessary since it is a private test macro we can just throw it in `internals`.
While we are at it lets improve the macro by testing a binary encoding also, elect to use the `bincode` crate because we already have it in our dependency graph.
Add `test-serde` feature to `internals` to feature gate the macro and its usage (preventing the transient dependency on `bincode` and `serde_json`).
ACKs for top commit:
Kixunil:
ACK 7fa53440dc
apoelstra:
ACK 7fa53440dc
Tree-SHA512: f40c78bf2539940b7836ed413d5fe96ce4e9ce59bad7b3f86d831971320d1c2effcd23d0da5c785d6c372a2c6962bf720080ec4351248fbbdc0f2cfb4ffd602c
The use of links in the rustdocs was inconsistent.
Links have been added when missing.
[`locktime::absolute::Height`] and [`locktime::relative::Height`] did
not work and `(crate::locktime)` was appended to fix it.
Created headings for a couple of function error descriptions to be
consistent with the rest of the crate.
Added a description explaining why Mega is not allowed in a
denomination.
We currently duplicate the serde_round_trip macro in `units` and
`bitcoin`, this is unnecessary since it is a private test macro we can
just throw it in `internals`.
While we are at it lets improve the macro by testing a binary encoding
also, elect to use the `bincode` crate because we already have it in
our dependency graph.
Add `test-serde` feature to `internals` to feature gate the macro and
its usage (preventing the transient dependency on `bincode` and
`serde_json`).
3196c271ac Deprecate `Amount::fmt_value_in` (Martin Habovstiak)
467546fc0c Round `Amount` when requested precision is too low (Martin Habovstiak)
7c95a777c1 Fix `Amount` decimals handling (Martin Habovstiak)
Pull request description:
Displaying with minimal number of zeros is the default in Rust and we want to follow it. This was originally implemented in #716 but #2604 reversed it claiming this is common, however it broke people who rely on minimal display (e.g. BIP21) without fixing the root cause of #2136.
This reverts commit d887423efc adds a test to prevent this change and also fixes the problem with `{:0.8}` not working.
Closes#2136Closes#2948
Can we backport this one too?
ACKs for top commit:
tcharding:
ACK 3196c271ac
apoelstra:
ACK 3196c271ac I also really like how this reduces the complexity of the module, basically passing everything through to `display_in`
Tree-SHA512: 3221f83086ac55af3d4caad03fe2b619be303533bba12096040419d119600c8597938809e171662f11b515d469156b083b2072b901d445e4fdfc7b1062cf7b6a
`fmt_value_in` was added when `display_in` wasn't available. However
common usage patterns seem to favor `display_in`. It can be used within
format strings and supports formatting options.
Removing it will simplify the codebase, so this deprecates it.
Low precision was previously not considered because it was believed it
could lead to misleading data being displayed. However it's quite
possible that people using low precision know what they are doing and
it's sometimes useful to show rounded numbers.
To enable low precision we just compute what the rounded number would be
and display that one instead.
This actually fully closes#2136 since this issue was mentioned there
along with previously-fixed displaying of higher precisions.
Displaying with minimal number of zeros is the default in Rust and we
want to follow it. This was originally implemented in #716 but #2604
reversed it claiming this is common, however it broke people who rely on
minimal display (e.g. BIP21) without fixing the root cause of #2136.
This reverts commit d887423efc adds a test
to prevent this change and also fixes the problem with `{:0.8}` not
working.
Closes#2136Closes#2948
Wildcards have been replaced with what is actually used.
In a couple of cases an additional use statement was added to the test
module to import `DisplayHex` which is only used in test, but
previously imported with the wildcard at the top.
The comment in `FromStr` says that any combination of upper and lower case is considered valid, but the code only allowed a set list of variations. It also had a non exhaustive list of `CONFUSING_FORMS`.
The comment and code has been changed to only consider all upper case or all lower case denominations as valid or where BTC/btc is prefixed with a lower case c, m or u. `CONFUSING_FORMS` has been changed to contain all combinations of an upper case prefix on an otherwise valid denomination.
Add two simple integer wrapper types for abstracting over block
height (from genesis block) and block interval.
This does not include hex because block height is typically written in
decimal.
These types are very thin wrappers, their usecase is to assist in code
readability instead of enforcing any logic.
d2a597c90d unit: Group re-exports (Tobin C. Harding)
d242125ae4 units: Fix error re-exports (Tobin C. Harding)
Pull request description:
First patch is the meat and potatoes, second one is just a trivial refactor to the same code, done separately so as to make the changes in the first patch more clear.
From patch 1
```
units: Fix error re-exports
Currently we re-export two error types at the crate root, this is
surprising because:
- Why not none or all the rest?
- Why these two?
Observe that the `ParseIntError` is special in that it is used by
other modules so its good to have at the crate root (other errors are
expected to be used with a module prefix eg, `amount::ParseError`).
There is no obvious reason why `ParseAmountError` is re-exported.
Comment and doc inline the `ParesIntError`, remove the re-export of
the `ParseAmountError`.
```
ACKs for top commit:
apoelstra:
ACK d2a597c90d
Tree-SHA512: 38d3f590357e66d07cbd7fedff134c39e0920e076ea99cb34ba276749a14695d428345d7b0f9ec8222f7899cb57e7c97068d3b6e7b2a9be25a0278e0a1abf762
11bb1ff6ff Standardize function doc Safety, Returns and Parameters (jamil.lambert)
df83016c98 Standardize function doc Errors (jamil.lambert)
d219ceb68e Standardize function doc Examples (jamil.lambert)
233a9133d8 Standardize function doc Panics (jamil.lambert)
Pull request description:
The subheadings in the rustdocs have been standardized according to [./CONTRIBUTING.md](https://github.com/rust-bitcoin/rust-bitcoin/blob/master/CONTRIBUTING.md):
```rust
impl FooBar {
/// Constructs a `FooBar` from a [`Baz`].
///
/// # Errors
///
/// Returns an error if `Baz` is not ...
///
/// # Panics
///
/// If the `Baz`, converted to a `usize`, is out of bounds.
pub fn from_baz(baz: Baz) -> Result<Self, Error> {
...
}
}
```
ACKs for top commit:
apoelstra:
ACK 11bb1ff6ff
tcharding:
ACK 11bb1ff6ff
Tree-SHA512: 163af3cd1cfb47cea3e55eddeaeb6843ff7ec89c57354e3247d6bae85e756b183e8045c2555cfcf87e8c23c1388ff9d7592cfb6a951a37a9ec41d27263e5a2e4
Add to `units::parse` the complete suit of hex unit parsing functions:
- remove prefix
- assert without prefix
- parse with or without prefix
- parse with prefix
- parse without prefix
- parse prefix unchecked
Refactor `bitcoin` to use the exact function we need, removing code
duplication.
This is a breaking change to `units`, it does however keep the current
re-exports from the public, now empty, `bitcoin::error` module.
Currently we re-export two error types at the crate root, this is
surprising because:
- Why not none or all the rest?
- Why these two?
Observe that the `ParseIntError` is special in that it is used by
other modules so its good to have at the crate root (other errors are
expected to be used with a module prefix eg, `amount::ParseError`).
There is no obvious reason why `ParseAmountError` is re-exported.
Comment and doc inline the `ParesIntError`, remove the re-export of
the `ParseAmountError`.
In a kani test we are checking if the result of `to_unsigned()` is an
error but setting `is_signed` to `true`, this field represents the
signed-ness of the return type of `to_unsigned` (`Amount`) so should be
`false`.
Fix kani daily job.
The tests currently include the word "add" but they test addition as
well as subtraction. Elect to keep the multiple assertions per test and
just make the names more general.
`cargo` is reporting a redundant import warning:
warning: the item `TryInto` is imported redundantly
Remove the import statement from the `verification` module.
051c358bcb Remove deprecated legacy numeric methods (Divyansh Gupta)
Pull request description:
As `rustc 1.79.0-nightly (9d79cd5f7 2024-04-05)` is released which solves the issue mentioned , but the release has deperacted legacy numeric methods.
Thus replaced `u16::max_value()` etc with `u32::MAX` & `core::u16` to directly `u16`.
fix#2639
ACKs for top commit:
tcharding:
ACK 051c358bcb
apoelstra:
ACK 051c358bcb thanks! I will remove an equivalent commit from my #2669
Tree-SHA512: c08c856f7f3b281417c29283351eac5e0f75cc1c8d23d9aae58d969219a327b2337fe57932053e53773ebb9dbec04254f90149266b6639a66c5c09f2ad1675ef
As `rustc 1.79.0-nightly (9d79cd5f7 2024-04-05)` is released which solves the issue mentioned , but the release has deperacted legacy numeric methods.
Thus replace `u16::max_value()` etc with `u32::MAX` & `core::u16` to directly `u16`.
fix#2639
Make an attempt to improve the ergonomics and docs clarity of the
`units` crate.
- Don't inline error type re-exports, this keeps them up in the
"Re-exports" section and saves cluttering the other inlined docs.
- Re-export and inline the docs for `FeeRate` and `Weight` same as we do
for `Amount`. This makes the "Structs" section of the docs nice except
for the exclusion of the locktime types (which cannot be helped).
Move the `strip_hex_prefix` helper function to below where it is called.
Note that I was the original author of this helper so there is no excuse
for it being above - bad Tobin no biscuit.
This lint triggers when parsing a reference to a large struct as a
generic argument, which is wrong.
Allow it crate wide because [subjectively] this lint never warns for
anything useful.
af49841433 Hide base58::Error internals (Tobin C. Harding)
4f68e79da0 bitcoin: Stop using base58 errors (Tobin C. Harding)
669d5e8fc6 base58: Add InvalidCharacterError for decoding (Tobin C. Harding)
ec8609393b base58: Add error module (Tobin C. Harding)
42fabbab03 base58: Run the formatter (Tobin C. Harding)
Pull request description:
Improve the error code in the new `base58` crate.
ACKs for top commit:
apoelstra:
ACK af49841433
sanket1729:
ACK af49841433
Tree-SHA512: c05479f02a9a58c7c98fd5987e760288562372e16cceeeb655f0a5385b4a8605945a3b6f7fcf473a7132a40f8dc90d204bc5e9e64fd2cc0bdc37dbcabb4ddc5c
Adds constructors to allow directly creating locktimes from time or
block counts; adds a flooring constructor to Time to match the ceiling
one; adds an explicit constructor to Height since the From<u16> was not
very discoverable.
Currently we are deriving the serde traits for the `absolute::{Height,
Time}` types, this is incorrect because we maintain an invariant on
the inner `u32` of both types that it is above or below the threshold.
Manually implement the serde traits and pass the deserialized `u32` to
`from_consensus` to maintain the invariant.
Close: #2559
When creating the ParseIntError in `hex_u32` I (Tobin) just cargo cult
programmed the generic stuff without thinking.
- The `is_signed` field is used to denote whether we were attempting to
parse a signed or unsigned integer, it should be `false`.
- The `bits` field should be directly set to 32.
d887423efc Enforce displaying Amount with trailing zeros (448 OG)
Pull request description:
It is common to display bitcoins using trailing zeros upto 8 decimals. This commit enforces:
- Displaying Amount in BTC with trailing zeroes by eight decimal places if a precision on the Amount is not specified.
- Displaying Amount in BTC upto the precision specified truncating the insignificant zeros.
- Displaying amount in BTC without any decimals if the remainder of the amount divided by the satoshis in 1 BTC is equal to zero using formula `satoshis.rem_euclid(Amount::ONE_BTC.to_sat()) != 0`
These are not breaking changes and all previous tests pass.
A testcase is added to for changes introduced.
Resolves: #2136
ACKs for top commit:
sanket1729:
ACK d887423efc
apoelstra:
ACK d887423efc
Tree-SHA512: c32e41216477f60a8d95f164bf4a1f6644ea14adc7e169743ce419b0f26ecb6603f3a516f9b18d6508c04ce658f6a0a78ff3b0b062aeb7101b28bbb1e9d522bc
It is common to display bitcoins using trailing zeros upto 8 decimals.
This commit enforces:
- Displaying Amount in BTC with trailing zeroes by eight decimal places
if a precision on the Amount is not specified.
- Displaying Amount in BTC upto the precision specified truncating the insignificant zeros.
- Displaying amount in BTC without any decimals if the remainder of the amount
divided by the satoshis in 1 BTC is equal to zero using formula `satoshis.rem_euclid(Amount::ONE_BTC.to_sat()) != 0`
These are not breaking changes and all previous tests pass.
A testcase is added to for changes introduced.
Resolves: #2136
cbee9781e8 Move unit types to units (Tobin C. Harding)
5bd0d7194b Remove unused absolute::Error (Tobin C. Harding)
Pull request description:
Move the following unit types to the new `units` crate:
- `locktime::absolute::{Height, Time}`
- `locktime::relative::{Height, Time}`
- `FeeRate`
- `Weight`
Also move the `parse` module as well as constants as required.
Do minimal changes to get things building:
- Feature gate on "alloc" as needed.
- Remove rustdocs that use `bitcoin` types.
- Re-export units types so this is a non-breaking change.
- Fix import paths.
Patch 1 was originally #2526, putting it in via this PR to try and speed up the process.
Close: #2282
ACKs for top commit:
sanket1729:
ACK cbee9781e8
apoelstra:
ACK cbee9781e8 lgtm. this is a good start. I think the LockTime types should follow Height and Time
Tree-SHA512: 6b0d63c7b054008598d7fa81be7d8c112f2778883b5529d79d446617b94b3c196c9ac735f840d1dfb488700894d3161c6976d44ab0e12ac3af4008068eac5f87
No clue how this got into master; found with `just sane`.
Clippy emits:
useless conversion to the same type: `amount::ParseAmountError`
As suggested, remove the useless conversion.
Move the following unit types to the new `units` crate:
- `locktime::absolute::{Height, Time}`
- `locktime::relative::{Height, Time}`
- `FeeRate`
- `Weight`
Also move the `parse` module as well as constants as required.
Do minimal changes to get things building:
- Feature gate on "alloc" as needed.
- Remove rustdocs that use `bitcoin` types.
- Re-export units types so this is a non-breaking change.
- Fix import paths.
e3db95226a Tidy description (yancy)
Pull request description:
I noticed `uncheced_add` looked really bad with two spaces (my mistake). Fixed some others as well.
ACKs for top commit:
apoelstra:
ACK e3db95226a oops, we should have caught this. Thanks for the fix!
tcharding:
ACK e3db95226a
Tree-SHA512: 8a2e7f85262f17063bea6ac22855ae45d99a1559a2d30f2627ffba1108f0fd8ebd0b541b50fe746b5af2ebb013cb3e9ea432987b90f37c046120388a808c5443
Sometimes people don't remember the exact number of decimal places
supported by denomination or don't want to count (e.g. when converting
fiat to BTC the calculator may yield too precise value). It's helpful to
say in error message at which digit the precision is too high.
This adds `TooPreciseError` struct containing the information and
improves the error message to display it.
This variant lacked pretty important information: what is the limit. We
could just write it in the error message but it's even better to move it
to a separate struct which also says how many characters exceed the
limit - this helps users know how many need to be deleted.
The `InvalidFormat` variant was pretty bad: it didn't make it clear what
exactly is wrong with the input string, especially since it was used
when the denomination was missing. Not only was this unhelpful to the
users who don't know they have to write the denomination when the amount
was otherwise correct but it was also attributed to a problem with the
amount rather than a problem with denomination.
To fix this the variant is replaced by `MissingDigitsError`,
`MissingError` and `InvalidCharError` - all the cases `InvalidFormat`
was originally used in.
`InvalidCharError` is effectively the same as the existing variant but
it was made into a separate error to enable special casing `.` and make
extensions possible. Further this opportunity was used to add a special
case for `-` as well.
`MissingDigitsError` currently contains special casing for empty string
and a string only containing minus sign. It's currently unclear if it's
useful so this change makes this distinction private and only makes it
affect error messages.
As opposed to the previous two variants, `MissingDenominationError` was
added to `ParseError`. The struct is currenly empty and may be extended
in the future with e.g. span information.
df1d2f6eb5 Add unchecked variants to Amount and SignedAmount (yancy)
Pull request description:
The checked variants have worse performance than the unchecked variants due to the additional branching operations. To improve performance where overflow is either not possible or not a concern, unchecked variants of `Amount` and `SignedAmount` are introduced for addition, subtraction and multiplication.
Note, it seems the default behavior for the test framework is to panic on overflow, so I haven't figured out a good way to add tests for this. Marking as a draft for now.
closes: https://github.com/rust-bitcoin/rust-bitcoin/issues/2434
ACKs for top commit:
Kixunil:
ACK df1d2f6eb5
apoelstra:
ACK df1d2f6eb5 gonna go ahead and merge this, we can revisit if necessary when we look at `units` overflow behavior in general
Tree-SHA512: 3fbb0ec81a758b350569226c44e25f6ca49e551566bee83c05c1c2b343874ef657d63a36b5f51c41582d8a8e36466275c574ebff6d363ed7c112ac8b4d5376fa
The checked variants have worse performance than the unchecked variants
due to the additional branching operations. To improve performance where
overflow is either not possible or not a concern, unchecked variants
of Amount and SignedAmount are introduced.
251579f4ef feat: implement TryFrom trait to SignedAmount and Amount (Sumit Kumar)
Pull request description:
Closes: #2245
Adds `TryFrom<SignedAmount> for Amount` and `TryFrom<Amount> for Amount` in units module
ACKs for top commit:
tcharding:
ACK 251579f4ef
Kixunil:
ACK 251579f4ef
apoelstra:
ACK 251579f4ef
Tree-SHA512: 3e58d7a891019ccd272417eadc977037167439e3385a7b47c060fe93eba9c47fc37cdc0ca5551174ef2d93b256b2804ad7c01f5f2470ef9e9b7b912877aed11c
c69caafefc Remove attribute comments (Tobin C. Harding)
3e83ef9276 Remove consensus error wrapper TODO (Tobin C. Harding)
bfabea94e9 Remove unwrap comment (Tobin C. Harding)
8bdaf4a34d Remove carrying_mul TODO (Tobin C. Harding)
Pull request description:
Add issues and remove the TODOs from the code.
Resolves: #2368
ACKs for top commit:
apoelstra:
ACK c69caafefc
Kixunil:
ACK c69caafefc
Tree-SHA512: b10a3de8da7ace890735023f8441605dd11b0227c27a2357556b8aaa8276a7f34ed220e3bcbc93aad4b35357319318ff7de27210e8f60dd90f6c55af23e21470
0997382772 io: Enable alloc from std (Tobin C. Harding)
ba1166a63b Make crate level attributes uniform (Tobin C. Harding)
Pull request description:
Make the trait level attributes uniform across all released crates in the repo. Excludes things that are obviously not needed, eg, bench stuff if there is not bench code.
- Remove `uninhabited_references` - this is allow by default now.
- Remove `unconditional_recursion` and mark the single false positive we have with an `allow`.
Note, this does not add `missing_docs` to the `io` crate. There is an open PR at the moment to add that along with the required docs.
ACKs for top commit:
apoelstra:
ACK 0997382772
Kixunil:
ACK 0997382772
Tree-SHA512: ef1f638aca171536287cce369be98998e871d26468ad2d8c39d9004db610b406471809c283540a4a19bcede78b12b8976a1bb37e5d431fbff8c8a3e53a64d4e3
Recently (in #2379) we patched the `ParseAmountError` but we don't check
kani code on every pull request so we broke it.
Fix kani test to use the new `OutOfRangeError`.
Close: #2424
Make the trait level attributes uniform across all released crates in
the repo. Excludes things that are obviously not needed, eg, bench stuff
if there is not bench code.
- Remove `uninhabited_references` - this is allow by default now.
- Remove `unconditional_recursion` and mark the single false positive we
have with an `allow`.
Note, this does not add `missing_docs` to the `io` crate. There is an
open PR at the moment to add that along with the required docs.
Previously the crate used negative reasoning to enable `std` which was
hard to understand, required the `prelude` module and wasn't really
needed because it's only needed when a crate wants to add `alloc`
feature-backwards compatibly and this crate always had the feature.
This cleans up usage to unconditionally use `#[no_std]` and then just
add `extern crate` on top as needed by activated features.
Using the crate without allocation was previously disabled making the
crate empty without the feature. This chage makes it more fine-grained:
it only disables string and float conversions which use allocator. We
could later provide float conversions by using a sufficiently-long
`ArrayString`.
7bf478373a Model `TooBig` and `Negative` as `OutOfRange` (Martin Habovstiak)
54cbbf804f Express `i64::MAX + 1` as `i64::MIN.unsigned_abs()` (Martin Habovstiak)
b562a18914 Move denomination error out of `ParseAmountError` (Martin Habovstiak)
5e6c65bc1a Clean up `unsigned_abs` (Martin Habovstiak)
Pull request description:
Closes#2265Closes#2266
Disclaimer: I did this in December and don't remember why I haven't pushed it. Maybe because it's somehow broken but I don't see how so please review a bit more carefully just in case.
ACKs for top commit:
tcharding:
ACK 7bf478373a
apoelstra:
ACK 7bf478373a
Tree-SHA512: 1f6e9adae9168bd045c9b09f06d9a69efd47ccc7709ac9ecaf48cb86e265b448b9b52a199ac5e6838d5029f5bc7514c5d7deb15a4d7c8a4606a353f390745570
The error returned when parsing amount had a `Negative` variant which
was weird/unreachable when parsing `SignedAmount`. Also weirdly, parsing
would return `TooBig` when the amount was negative - too low.
To resolve this we merge them into one `OutOfRange` variant that nuges
the consumers to make principled decisions and print error messages as
amounts being more than or less than a specific value which is easier to
understand for the users. Notably, the API still allows getting
information about which type was parsed and which bound was crossed but
in a less obvious way. This is OK since users who have a very good
reason can use this information but most won't.
Closes#2266
The `from_str_in` methods on amounts returned `ParseAmountError` which
contained `InvalidDenomination` among its variants. This one was never
returned because the method doesn't parse denomination.
This change separates the error out.
Closes#2265
Previousle we copied `unsigned_abs` method from `core` because it was
unstable in older MSRV. Our current MSRV allows using the method
directly so this removes our old one and uses the one from standard
library instead.
b2344e019d units: Assert roundtrip SignedAmount/str overflows (Tobin C. Harding)
baadcf4c0a units: Test that SignedAmount float conversion overflows (Tobin C. Harding)
d768f25da8 units: Remove duplicate assertion (Tobin C. Harding)
1d536ac8b2 units: Enable parsing Amount from u64::MAX (Tobin C. Harding)
Pull request description:
Our `Amount` type uses an internal `u64` and we maintain no invariants on the inner value. Therefore we should be able to parse `u64::MAX`.
Fix the parsing code by removing the explicit, incorrect check and fix unit tests to mirror this behaviour.
Fix: #2297
ACKs for top commit:
Kixunil:
ACK b2344e019d
apoelstra:
ACK b2344e019d
Tree-SHA512: 944f8d0bfedc559f0444f75eca7d3fba042fbc204c4c032d09ff0edc29be280a3707f5b363dbc04f0d7bdf64701c0c4619e2e0de683d804a2663c2a20ac963f6
We should not be able to roundtrip a `SignedAmount` value greater than
`MAX`, add a test to prove so.
While we are at it document the assertion above that proves we can parse
a float representing an `Amount` greater than `SignedAmount::MAX`.
`Denomination::Bitcoin` and `Denomination::Satoshi` are often used,
especially in test code so this change adds `BTC` and `SAT` - short,
readable constants. Notably this doesn't add the other constants as that
would lead to either unidiomatic names or confusing casing (MSAT meaning
millisat not megasat) and they are not used that much anyway.
Our `Amount` type uses an internal `u64` and we maintain no invariants
on the inner value. Therefore we should be able to parse `u64::MAX`.
Fix the parsing code by removing the explicit, incorrect check and fix
unit tests to mirror this behaviour.
Fix: #2297
f06d12455f bitcoin: Remove the custom sink (Tobin C. Harding)
b503aa1544 Run the formatter (Tobin C. Harding)
3ca55fb163 Remove qualifying path from Read and Write (Tobin C. Harding)
ebeb21fa7a Import fmt::Write using underscore (Tobin C. Harding)
e2dbcb1d28 Use W for writer generic type (Tobin C. Harding)
8704d9f0ae docs: Fix grammar (Tobin C. Harding)
Pull request description:
A few cleanups to how we use the `io` crate, this is reasonably trivial but commit `a6c7e696 Remove qualifying path from Read and Write` is big, I have however gone to some effort to make sure it is easy to flick through the diff.
Done in preparation for another go at the `BufRead` stuff.
ACKs for top commit:
apoelstra:
ACK f06d12455f
Kixunil:
ACK f06d12455f
Tree-SHA512: 751c489c67901c7563f1cc91f7761a4e3c276ae1981010338134e8c13200720ba69fcc74948c1dc1e6e65390197da0da27b2b69b86034029748321b404142cba
When we use the `fmt::Write` trait it is just to call its methods, we
can therefore, without any change to the logic, use `as _` when
importing the trait. This prevents naming conflicts.
Done in preparation for importing the `io::Write` trait.
Tobin C. Harding
merge-script
yancy
calciumbe
Fmt Bot
Shing Him Ng
Jamil Lambert, PhD
Rob N
Casey Rodarmor
Christian Lewe
Martin Habovstiak
Divyansh Gupta
448 OG
Liam Aharon
yancy
Sumit Kumar
Jiri Jakes