a1ce2d1ac8 Use _u32 in FeeRate constructor instead of _unchecked (Tobin C. Harding)
Pull request description:
When constructing an `Amount` it was observed recently that a `u32` is often big enough. For the same reason we can use a `u32` to construct a fee rate instead of overflowing.
Use `_u32`in the constructor and remove the panic.
ACKs for top commit:
apoelstra:
ACK a1ce2d1ac8d646b63532ed9ac459ffea39ec8c20; successfully ran local tests
Tree-SHA512: 339974c954ad613b0be684f7b2fa1402f59fe401969f3785a702ffbb14ac913ecf4c8228240d068ba4b482e38263590154167a071d823ccc9b4d0691036ca484
0a0e23fedf style: use Self:: instead of type aliases in error impls (frankomosh)
Pull request description:
Replace `use FooError as E` with `Self::` , during pattern matching
on the same type in Display/Error trait implementations.
close#4536
ACKs for top commit:
tcharding:
ACK 0a0e23fedf
apoelstra:
ACK 0a0e23fedf3552df677b379d86a1e0ac6b063152; successfully ran local tests
Tree-SHA512: 25c45a890635f990afc3bc09096929f8793007852ac435f061348bff2bd79e3faabf034d1e1e277596f4f7365477f477798f1b1e0c4b918c5b0fa08f8c49e732
395252c6d9 Fix FeeRate::checked_add/sub (Tobin C. Harding)
Pull request description:
Currently the `checked_add` and `checked_sub` functions use a `u64` for the right hand side. This leaks the inner unit because the RHS value is implicitly in the same unit.
Make the functions have `FeeRate` on the RHS.
ACKs for top commit:
apoelstra:
ACK 395252c6d95fdd581ce5e8ad5d7978515aafea23; successfully ran local tests; will one-ACK merge as this is units-only
Tree-SHA512: 1f0893f75c47f720ac741ace0274171ed24efcb6d2724d0fed941899d43b213e165b97aa050d6e40eea78c527af45d090a81b9d6cbd95835ef7105585786fca6
c63f80baec Remove impl From<u64> for FeeRate (Tobin C. Harding)
Pull request description:
This function leaks the inner unit of `FeeRate`. We want to change the unit, best to break downstream so they notice.
ACKs for top commit:
apoelstra:
ACK c63f80baec0780622d70e4c8699369b0a972cb62; successfully ran local tests; will one-ACK merge as this is units-only
Tree-SHA512: 5748f2a4cb29d6554226fe20c5479cb53081da8c2788ac31abfe1a2edb4d17f13a3b3037a840fbdc29e842af3e1accc27835fd5429c7355c1351eb8883943fdc
Currently the `checked_add` and `checked_sub` functions use a `u64` for
the right hand side. This leaks the inner unit because the RHS value is
implicitly in the same unit.
Make the functions have `FeeRate` on the RHS.
We have 4 functions, 2 ceil, 2 floor but they are ordered ceil, floor,
floor, ceil - this causes my head to twitch when I read it.
The logic in the floor versions is easier so put them first, this is
uniform with `fee_rate/mod.rs`.
Code move only.
When constructing an `Amount` it was observed recently that a `u32` is
often big enough. For the same reason we can use a `u32` to construct a
fee rate instead of overflowing.
Use `_u32`in the constructor and remove the panic.
b538a10956 Add deprecated functions to mutants exclude list (Jamil Lambert, PhD)
fd0a756344 Add tests to relative locktime (Jamil Lambert, PhD)
24cc059a78 Add tests to result (Jamil Lambert, PhD)
c1d2f0386d Add tests to block (Jamil Lambert, PhD)
Pull request description:
Weekly mutation testing found new mutants.
Add tests to kill the valid mutants.
Add deprecated functions to the exclude list so they are not mutated.
Closes#4488, Closes#4528
ACKs for top commit:
apoelstra:
ACK b538a1095652f535aeb15f6e3bbc44969db1ea88; successfully ran local tests
tcharding:
ACK b538a10956
Tree-SHA512: 8393ded6c073b2580fbb0fde9a8ce702a3d1e8c581c035870c2ba6a12d718cee577e345c9d92d0761552765248a6fb5ae9bbacbc88cac75e7153516de46de4ca
29b12bec6b Remove Display/FromStr for FeeRate (Tobin C. Harding)
Pull request description:
Parsing and displaying strings is a PITA. `FeeRate` is likely not that heavily used at the moment and users can always just call `to_sat_per_kwu()` to format it.
So we can get the `units-1.0-alpha.0` release out the door just remove the `Display` and `FromStr` impls for now. They can be added back in later when we have time to get #4339 in.
ACKs for top commit:
apoelstra:
ACK 29b12bec6b34148d6df9a4e6207132a667c53b4c; successfully ran local tests
Tree-SHA512: ffb49ab5d4f98be603eb1ca2f2e9d28ff7eaae66607eb9d2d5fef1f98ba2ac284a0007a86c3cae88f06e5b44f1e3e3ecb2014323b82ad4007e8ec59d6d04759b
Parsing and displaying strings is a PITA. `FeeRate` is likely not that
heavily used at the moment and users can always just call
`to_sat_per_kwu()` to format it.
So we can get the `units-1.0-alpha.0` release out the door just remove
the `Display` and `FromStr` impls for now. They can be added back in
later when we have time to get #4339 in.
9a9b41008f units: Make block-related types have private inner fields (Tobin C. Harding)
Pull request description:
We recently added a bunch of types in the `block` module that are wrappers around `u32`. When we did we slapped `pub` on the inner fields.
Lets be more mindful and make the inner fields private. Note all these types have `to_u32`, `from_u32` and `From` in both directions.
ACKs for top commit:
apoelstra:
ACK 9a9b41008f72cb8a00cfed2890d839aea2ea27c7; successfully ran local tests
Tree-SHA512: d4719bef57944000142ec110d4701486b4a7e5c5b24426379ed596ad83555ca1f75191fe618cc62f12b78b9d7a1ac5d18eff96bc407c9688b1210529c25329b3
We recently added a bunch of types in the `block` module that are
wrappers around `u32`. When we did we slapped `pub` on the inner fields.
Lets be more mindful and make the inner fields private. Note all these
types have `to_u32`, `from_u32` and `From` in both directions.
e2d03fef72 units: Manually implement serde traits (Tobin C. Harding)
Pull request description:
For types that have private inner fields we would like to manually implement `serde` traits instead of deriving them.
This hardens the crate against making future mistakes if we want to change the inner fields.
Do so for:
- `Weight`
- `BlockTime`
- `NumberOfBlocks`
- `NumberOf512seconds`
ACKs for top commit:
apoelstra:
ACK e2d03fef72f4fcbdacd1cbecc0c2dddfbdeb3031; successfully ran local tests
Tree-SHA512: 6d3cc4106e44e08d59f8da6f8f3285923408ed05d1c0b2f6f2d83a22718744a38bf67381177856b5713810a5f5318d929daa2d12447c722628e0576fcd075e3d
For types that have private inner fields we would like to manually
implement `serde` traits instead of deriving them.
This hardens the crate against making future mistakes if we want to
change the inner fields.
Do so for:
- `Weight`
- `BlockTime`
- `NumberOfBlocks`
- `NumberOf512seconds`
According to the `serde` docs:
> This is used in error messages. The message should complete the
> sentence “This Visitor expects to receive …”, for example the message
> could be “an integer between 0 and 64”. The message should not be
> capitalized and should not end with a period.
Use a lower case character to start the string.
According to the `serde` docs:
> This is used in error messages. The message should complete the
> sentence “This Visitor expects to receive …”, for example the message
> could be “an integer between 0 and 64”. The message should not be
> capitalized and should not end with a period.
However we have the `expecting` str using the converted type not the
thing the visitor expects.
Use `u64` instead of `FeeRate` since that is what is being parsed. Note
that in `amount` we got it _almost_ correct, subsequent patch will fix
the case.
fe577cd04e Implement Div<NonZeroU64|I64> for Amount, SignedAmount, FeeRate, and Weight (frankomosh)
Pull request description:
The pr implements `Div<NonZeroU64>` and `Div<NonZeroI64>` for the following types in `units` crate: `Amount`, `SignedAmount`, `FeeRate`, `Weight`
For handling owned/borrowed variants, each impl is wrapped in the existing `impl_op_for_references!` macro, which generates: `T / NonZero*` , `&T / NonZero*`, `T / &NonZero*`, `&T / &NonZero*`
close: #4442
ACKs for top commit:
Kixunil:
ACK fe577cd04e
apoelstra:
ACK fe577cd04e64488371aa62b872e2b88050d4948f; successfully ran local tests
tcharding:
ACK fe577cd04e
Tree-SHA512: b9b4a9f46d2fcf559d0a7f62ec397b6c2b174dd8ca9d80b37c0c393894ab4bc32019d64e2afcad1f780442b16aa3d15240ce607fc14b5e17b112243f7556b5b4
Adds an implementation of div by NonZeroU64 for Amount, FeeRate, and Weight
types. Also adds a div by NonZeroI64 for SignedAmount. The operations
helps to prevent div-by-zero errors at compile time, rather than runtime.
It follows same pattern as existing div operations
but leverages safety guarantees offered by non-zero types
a66ff8f8b1 Introduce `map` function for `NumOpResult` (Shing Him Ng)
Pull request description:
Closes#4476
ACKs for top commit:
Kixunil:
ACK a66ff8f8b1
apoelstra:
ACK a66ff8f8b143ebebb80ddc6b052cbc55e8dbb070; successfully ran local tests
Tree-SHA512: 63c79666685895033b9df0c46004fa4b042d038cc61e5ef443f56690be268ac6dd1ba461ab4f7d97c684e68623dfa53cdd37091f40ff7e6a5d3e53920c3fd40c
fd90c8782a Mark method as constant (yancy)
Pull request description:
Allow external const calls to access this method
Followup from https://github.com/rust-bitcoin/rust-bitcoin/pull/4428
ACKs for top commit:
tcharding:
ACK fd90c8782a
apoelstra:
ACK fd90c8782ab602861a715c72ca1481ed963e5c39; successfully ran local tests
Tree-SHA512: 2d04da9bddd58040972942e70096b2714405740236889f0909c00cb6993e38f3ae51ff05bbda13792a31243d3598f5976649590eecb0572c4f00c166f717399d
When checking a locktime against block height we add 1 because when the
next block is being validated that is what the height will be and
`is_satisfied_by` is defined to return true if a transaction with this
locktime can be included in the next block.
As we have in `relative`, and for the same reasons, add an API to the
absolute `Height` and `MedianTimePast`: `is_satisfied_by`. Also add
`is_satisfied_by_{height, time}` variants to `absolute::LockTime`.
Call through to the new functions in `units`.
Define 'is satisfied by' - this is a classic off-by-one problem, if a
relative lock is satisfied does that mean it can go in this block or the
next? Its most useful if it means 'it can go in the next' and this is
how relative height and MTP are used in Core.
Ramifications:
- When checking a time based lock we check against the chain tip MTP,
then when Core verifies a block with the output in it it uses the
previous block (and this is still the chain tip).
- When checking a height base lock we check against chain tip height + 1
because Core checks against height of the block being verified.
Additionally we currently have a false negative in the satisfaction
functions when the `crate` type (height or MTP) is to big to fit in a
u16 - in this case we should return true not false because a value too
big definitely is > the lock value.
One final API paper cut - currently if the caller puts the args in the
wrong order they get a false negative instead of an error.
Fix all this by making the satisfaction functions return errors, update
the docs to explicitly define 'satisfaction'.
For now remove the examples in rustdocs, we can circle back to these
once the dust settles.
API test of Errors:
Some of the errors are being 'API tested' tested in `primitives` but
they should be being done in `units/tests/api.rs` - put all the new
errors in the correct places.
a7d059151e Assert error type (yancy)
2f7e74da45 Add MathOp helper methods (yancy)
Pull request description:
Follow up from https://github.com/rust-bitcoin/rust-bitcoin/pull/4428 to assert the error type which I agree improves the test.
Added some helper functions since it can be nice to see what type of overflow happened.
ACKs for top commit:
tcharding:
ACK a7d059151e
apoelstra:
ACK a7d059151eb47bf4202302604309c95c0d66371d; successfully ran local tests
Tree-SHA512: e1b3eba640de2e4f98e075270fd797582601c541e1eebef2959a4e609bf51129e8ad38baab1253b40474c39f82ee4802658ec545cc5c3590637f2ddb13f873f7
Name the type exactly what it is. This used to be `Time`, then we tried
`MtpInterval`.
Note that this makes some of the original function names overly verbose
e.g., `NumberOf512seconds::from_512_second_intervals()` but given the
curlyness of locktimes too verbose is better than too terse. Also this
type, along with `NumberOfBlocks` is not going to be in very wide use so
the ergonomic hit is worth the additional clarity.
Name this type exactly what it is. Note for the error we just use
'height' even though this is a bit stale but the general concept is ok
in the error type because the name is long already.
47c77afaac units: delete MtpAndHeight type (Andrew Poelstra)
d82b8c0bcb primitives: stop using MtpAndHeight (Andrew Poelstra)
72d5fbad73 units: stop using MtpAndHeight in locktime::relative is_satisfied_by methods (Andrew Poelstra)
d933c754f5 units: change type of MtpHeight::to_mtp to BlockMtp (Andrew Poelstra)
dcbdb7ca8a units: add checked arithmetic to Block{Height,Mtp}{Interval,} (Andrew Poelstra)
4300271f0c units: add constructor for absolute::Mtp from timestamps (Andrew Poelstra)
4e4601b3d5 units: rename BlockInterval to BlockHeightInterval (Andrew Poelstra)
cb882c5ce1 units: add global `BlockMtpInterval` type (Andrew Poelstra)
4e3af5162f units: add global `BlockMtp` type (Andrew Poelstra)
a3228d4636 units: pull u32 conversions for BlockHeight/BlockInterval into macro (Andrew Poelstra)
Pull request description:
This is a more involved PR than I'd expected but hopefully the individual commits make sense and are well-motivated. Essentially, my goal was to replace `MtpAndHeight` as used by relative locktimes with a pair of `Mtp` and `Height`.
However, relative locktimes, when given a MTP/Height for the UTXO creation and the chain tip, are roughly modeled as "take a diff of MTPs to get a `relative::MtpInterval`, a diff of heights to get a `relative::HeightInterval`, and compare to the locktimes". *However*, we have no standalone MTP type to "take a diff of", and also there are failure modes when creating the diffs (e.g. if the diff would exceed the range of `MtpInterval` or `HeightInterval`).
So I backed up and decided to use the existing `BlockHeight`/`BlockInterval` as the type to "take a diff of". I needed to introduce a `BlockMtp`/`BlockMtpInterval` to work with MTPs. These types have full-u32 range, unlike the similarly-named types in `units::locktimes::absolute`. I then needed to add some conversion methods. Along the way, I cleaned up the APIs and documentation, added checked arithmetic, etc., as needed.
See the individual commit messages for more detail.
I believe the resulting API is much more consistent and discoverable, even though it has more surface than the old API.
I considered splitting this into 2 PRs but I think the first half of the changes aren't well-motivated with out the second half. Let me know.
ACKs for top commit:
tcharding:
ACK 47c77afaac
Tree-SHA512: ebe19a5b1684db8c2d913274347c994026aaa0dcdd79349c237920a82fe55560777278efdbbc7f1b1424c9391d9bbd891ae844db885deea75288000437a8a287
13cbead947 Use NumOpResult instead of Option (yancy)
002a0382aa Mark function constant (yancy)
Pull request description:
Prefer the more descriptive NumOpResult return type over Option where return types are fallible.
Closes https://github.com/rust-bitcoin/rust-bitcoin/issues/4419
ACKs for top commit:
apoelstra:
ACK 13cbead94766987f59482b1fbc1d0ebd0799737c; successfully ran local tests
tcharding:
ACK 13cbead947
Kixunil:
ACK 13cbead947
Tree-SHA512: 1a870962dcafe901a07abd93bd8075e41696341c1a4b3efef615493c73d5e5728bbc2326f8c2c95b9034ab001d0b3c668c9d64793ab03486d3a19f31df907c96
52940d4e12 Prefix unused variables with _ in rustdocs (Jamil Lambert, PhD)
a852aef4b8 Remove unused imports in rustdocs (Jamil Lambert, PhD)
Pull request description:
There is a lint warning about unused variables and imports in the rustdoc examples.
Remove the unused imports and prefix the unused variables with an underscore.
ACKs for top commit:
apoelstra:
ACK 52940d4e1216ad5118f7980cb2e6b8b425c61589; successfully ran local tests
tcharding:
ACK 52940d4e12
Tree-SHA512: 953862d546dc6e0bcd64172e8b383f0fc2a1a851971a1bcad0c1e30cbaeeaea993a0de7dd8b424c4ac1410053e179c52d0b5c90cd1b6560c27123b6b7fa49732
We are going to delete MtpHeight in a couple commits, but to let us do
the transition with smaller diffs, we will first change it to be easily
convertible to a BlockHeight/BlockMtp pair.
See the previous commit message for justification; for sensible
arithmetic on block timestamps we need the ability to do MTP
calculations on arbitrary MTPs and arbitrary intervals between them.
However, the absolute::Mtp and relative::MtpInterval types are severely
limited in both range and precision.
Also adds a bunch of arithmetic ops to match the existing ops for
BlockHeight and BlockInterval. These panic on overflow, just like the
underlying std arithmetic, which I think is reasonable behavior for
types which are documented as being thin wrappers around u32.
We may want to add checked_add, checked_sub and maybe checked_sum
methods, but that's out of scope for this PR.
For our relative locktime API, we are going to want to take differences
of arbitrary MTPs in order to check whether they meet some relative
timelock threshold.
However, the `locktime::absolute::Mtp` type can only represent MTPs that
exceed 500 million. In practice this is a non-issue; by consensus MTPs
must be monotonic and every real chain (even test chains) have initial
real MTPs well above 500 million, which as a UNIX timestamp corresponds
to November 5, 1985.
But in theory this is a big problem: if we were to treat relative MTPs
as "differences of absolute-timelock MTPs" then we will be unable to
construct relative timelocks on chains with weird timestamps (and on
legitimate chains, we'd have .unwrap()s everywhere that would be hard to
justify). But we need to treat them as a "difference of MTPs" in *some*
sense, because otherwise they'd be very hard to construct.
There is a lot of duplicated code between BlockHeight and BlockInterval.
It obfuscates the differences between them: which timelock types they
can be converted to/from and what their arithmetic properties are.
There is a new lint error on nightly-2025-04-25 "variables can be used
directly in the `format!` string".
Exclude the lint to allow the existing syntax in `format!` strings.
Prefer the more descriptive NumOpResult return type over Option where
return types are fallible.
The returned type NumOpResult is already annotated as must_use per the
lint, so must_use can be removed after changing the method return types
to NumOpResult.
Rename `value` to `to_height` to be symmetric with `from_height`;
deprecate `to_consensus_u32` which had no symmetric `from_consensus_u32`
and was only used to implement the corresponding methods in primitives
and bitcoin.
This is disruptive, but makes the type name consistent with
`MtpInterval` and also greatly improves clarity, helping to distinguish
between absolute and relative locktimes and reminding the author (and
reviewer) of locktime code that this needs to be a diff.
This method is weird. It's basically just used internally to implement
the locktime methods in `primitives` and `bitcoin`. It has no symmetric
from_consensus_u32.
Conversely the constructors from 512-second intervals have no symmetric
to_* method -- the inverse of these functions is called `value`, which
is a meaningless and undiscoverable name.
As with absolute::Mtp, there is no "consensus encoding" of a block
height, except that obtained by converting it to a locktime. For
symmetry with `Mtp`, rename the methods.
There is no "consensus encoding" for a MTP. The intention for these
methods was that a user could interpret the MTP as a locktime and then
consensus-encode that locktime. However, it was instead interpreted as
the MTP representing a *blocktime* as it is consensus-encoded in a block
header.
Evidence of this misinterpretation is in several doccomments, which
casually refer to the Mtp (which used to be just called Time) as a
"block time", which is simply incorrect.
This is not a generic UNIX timestamp, but rather a MTP restricted to
have values between 500 million and u32::MAX. Most importantly, it is
*not* a blocktime, which is what is implied by its name and
constructors.
The output of `Display` should not change in stable crates for types
that have well defined formatting and ones that implement `FromStr`.
Error types do not need to be tested.
Add missing tests for all implementations in `units`.
The existing display regression tests only tested height locktimes.
Denomination display regression and round trip were not tested.
Add tests for time locktimes and denomination.
Existing display tests only test lock by block height. New tests are
needed for lock by block time.
Change existing test names to make it clear which type of locktime is
being tested.
8b47068a2e feat(locktime): implement MtpAndHeight structure and validation logic (aagbotemi)
Pull request description:
This PR fixes#4299
- Computed MtpAndHeight structure
- Checked if relative time and height is satisfied by MtpAndHeight
- Compared the Ordering of MtpAndHeight with time and height
- Checked MtpAndHeight satisfaction and comparison in Locktime
- Added unit tests for all the implementation
I've reviewed and adhered to the contribution guidelines
ACKs for top commit:
apoelstra:
ACK 8b47068a2efada30aec21c61ae4be0da4d8e8fc8; successfully ran local tests
Kixunil:
ACK 8b47068a2e
tcharding:
ACK 8b47068a2e
Tree-SHA512: b00d1384d5deaa038b486ca9d77ad33cfa6cd8c987e08407863f2be8d540014bdcc971cd9d46acb51a2d105341accc04ba151e5cccb276e8352a5d45b33097eb
a92cc71f65 Create impl_mul_assign and impl_div_assign macros (Shing Him Ng)
Pull request description:
The macros were called on type-rhs pairs that have an existing implementation of ops::Mul and ops::Div, respectively, that have an `Output` of `Self`
Not as many types as I would have thought, but most of the operations result in a `NumOpResult`, which can't be then assigned back to the variable.
Closes#4172
ACKs for top commit:
apoelstra:
ACK a92cc71f658771776557ea0a40d1d095d3b6d482; successfully ran local tests
Tree-SHA512: 30cfb077b9ba65af991eb17fa05ffc4a870c3f4ded746355d3a8577a71fe9a569588a882c2a936edcc9c88feede4d8bb1379a998e3f330894084a4e2fc434e6e
- Add MtpAndHeight for relative locktime checks
- Include unit tests for time/height comparisons
- Fix API design for mtp_as_time() error handling
- Update documentation and dependencies
- Fix BlockTime, CI, remove Ordering, and PR discussion fixed
- Fix UTXO height and timestamps
- Fix: chain_state and utxo_state handled seperately for is_satisfied_by
- Fix: panic on overflow fixed with check_add
- Fix: documentation updated and trailing whitespaces removed
- docs(mtpheight): documentation updated
- used accessors to_height and to_mtp over From impl
There was and inconsistent usage of `#`, `##` and `###` in rustdoc
headings. The difference in the rendered rustdocs is a minimal font
size change.
Change all headings to be H1 `#`.
Change all subheadings to be `###` to have a noticeable difference in
font size in the rendered docs.
0f62c9a582 units: Make minor improvements to MathOp (Tobin C. Harding)
Pull request description:
Follow up from #4312, improve the `MathOp` type by:
- Do not provide public constructor
- Add cast protection
ACKs for top commit:
apoelstra:
ACK 0f62c9a5822a813be5c6f6b19d160458243f174a; successfully ran local tests
Kixunil:
ACK 0f62c9a582
Tree-SHA512: dbb06c5afd8df5364a2aec12b7c8632620a1e8f3955b83e91a9b4f5c2e0daaa1ecdb050d8e395e95bf018d718847cddbf3338bd89f70cbb0382bf5e080d5cf21
As per policy in #4090 add a privacy boundary to the `BlockTime` type.
Use the module name `encapsulate` as is done in `amount` - its private
so the name can easily be changed later if needed.
Explicitly do not run the formatter or update rustdoc column width so
that review is easier.
As per policy in #4090 add a privacy boundary to the `Weight` type. Use
the module name `encapsulate` as is done in `amount` - its private so
the name can easily be changed later if needed.
Explicitly do not run the formatter or update rustdoc column width so
that review is easier.
As per policy in #4090 add a privacy boundary to the `FeeRate` type. Use
the module name `encapsulate` as is done in `amount` - its private so
the name can easily be changed later if needed.
Explicitly do not run the formatter or update rustdoc column width so
that review is easier.
c30a504ea6 units: Document the NumOpResult type (Tobin C. Harding)
Pull request description:
Document the `NumOpResult` type.
Note that this includes two new getters on the `NumOpResult`, API hole found during review of the new docs.
Fix: #4222
ACKs for top commit:
apoelstra:
ACK c30a504ea6a5140bdf5667ea42b76bdfa2457456; successfully ran local tests; nice!
Tree-SHA512: ab8d971b74ff4bb06f5737943740c5c748f6313ce1b82798c7d709f8747779efdffe0aa8ed8620afa449fd0dd502b5a2050729a538c51428215972a4f7b6ebf7
913360b112 Make struct titles consistent (Jamil Lambert, PhD)
afe9ddd5e6 Remove - in fee rate (Jamil Lambert, PhD)
ebc6b4a876 Make warning text bold (Jamil Lambert, PhD)
Pull request description:
I have read through all of the `units` docs and made a few changes.
- Highlight `Warning!` in bold in `Amount` and `SignedAmount`
- Change the one occurrence of fee-rate to fee rate to be consistent with the rest.
- Make all of the error structs have the same title format of `Error returned...`
- Make all other structs have the same format concisely stating what it is opposed to what it does.
ACKs for top commit:
tcharding:
ACK 913360b112
Tree-SHA512: 4cb08d1dae091f5b827cf9f1e931b057c6670002146a22da54886148f3052f6ea7050fcd7f62c0d83438ef170e2f109c1a36f47a280808f31466da6f3177dd01
ca6c607953 Adhere to sanity rules for amount types (Tobin C. Harding)
6c614d9320 units: Fix panic message (Tobin C. Harding)
Pull request description:
This is a follow up to #4256 - onwards and upwards!
- Patch 1: Fix the incorrect BTC value in panic message
- Patch 2: Strictly adhere to the sanity rules (#4090)
Close: #4140
ACKs for top commit:
apoelstra:
ACK ca6c607953c03aa2dc168f58329681d9e69eee04; successfully ran local tests
Tree-SHA512: 6d7fd60830e1a0f6d6262ab02ec6e297b095d0fe8fb7737563979652e4a3b4a9477a79982201c42b08e2555fd23dc5c430549966b534bdf45f40621ae81da83a
812c21e2e4 refactor: Replace fold with try_fold (yancy)
Pull request description:
The and_then combinator performs a kind of bitwise and operation on two Option types here. This is useful since the `checked` arithmetic returns an option thereby accumulating Option types. Therefore, either the checked arithmetic operation performs the addition of the unwrapped accumulator, or it returns None.
Instead of using `and_then` use the provided `try_fold` method which will short circuit on `None` when the checked arithmetic is used. Also, simplify the staring condition using `Amount:ZERO` since this is logically equivalent to using the first value if one exists.
Lastly, by using the built in `try_fold`, it's possible the performance will be improved by making use of the short circuit ability instead of evaluating each item even when the accumulator holds a None type.
ACKs for top commit:
apoelstra:
ACK 812c21e2e4a868046b44728c1a6209a866452820; successfully ran local tests
tcharding:
ACK 812c21e2e4
Tree-SHA512: 1cfcd4fa28e2b59daf3744bb5f654f65eb9853c5a36f747cb0859783e7e46c1d02ccb296612b75f7cca10782979ce052cd670c0f23c1030e0a347000d1f6df83
Structs had various phrasings of titles.
Make the wording consistent by concisely stating what it is, instead of
what it does.
Make the wording of all error structs consistent.
The rest of the rustdocs use fee rate with no hyphen when using it in
normal language, i.e. not a function argument or the type.
Change it to match the others.
merge-script
frankomosh
Tobin C. Harding
Jamil Lambert, PhD
Fmt Bot
Shing Him Ng
yancy
leopardracer
aagbotemi