Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move types to units #2569

Merged
merged 2 commits into from
Mar 15, 2024
Merged

Conversation

tcharding
Copy link
Member

@tcharding tcharding commented Mar 11, 2024

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

@github-actions github-actions bot added C-bitcoin PRs modifying the bitcoin crate C-hashes PRs modifying the hashes crate C-units PRs modifying the units crate C-base58 labels Mar 11, 2024
@coveralls
Copy link

coveralls commented Mar 11, 2024

Pull Request Test Coverage Report for Build 8241935679

Details

  • 119 of 228 (52.19%) changed or added relevant lines in 7 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 84.159%

Changes Missing Coverage Covered Lines Changed/Added Lines %
bitcoin/src/blockdata/script/witness_version.rs 0 1 0.0%
bitcoin/src/blockdata/transaction.rs 2 4 50.0%
units/src/locktime/relative.rs 6 23 26.09%
units/src/parse.rs 28 50 56.0%
units/src/locktime/absolute.rs 66 133 49.62%
Totals Coverage Status
Change from base Build 8239659179: 0.1%
Covered Lines: 19540
Relevant Lines: 23218

💛 - Coveralls

@Kixunil
Copy link
Collaborator

Kixunil commented Mar 11, 2024

FTR the "unit type" term in Rust refers to the () (empty slice/"void") type, so the title sounds a bit weird. :)

@tcharding
Copy link
Member Author

Ah yes, of course - will change.

@tcharding tcharding changed the title Move unit types to units Move types to units Mar 11, 2024
The `absolute::Error` is not used, we originally intended it as possibly
useful for users of the library. We have not made effort in other
modules to provide such errors - lets remove it.
@tcharding tcharding force-pushed the 03-11-move-units-types branch 2 times, most recently from 1af8534 to 69e3513 Compare March 11, 2024 22:51
@tcharding tcharding marked this pull request as ready for review March 11, 2024 22:53
@tcharding tcharding added the P-high High priority label Mar 11, 2024
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.
@tcharding tcharding added this to the 0.32.0 milestone Mar 12, 2024
@sanket1729
Copy link
Member

In future for PRs that move stuff, it might be useful to separate things in commits which we just copy pasting files (even if they don't compile) and the commits where you made changes to make things compile.

We can squash/fixup commits to make sure that each and every commit compiles.

Copy link
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK cbee978

@tcharding
Copy link
Member Author

Thanks for the review @sanket1729, yeah I was a bit lazy on this one.

@tcharding
Copy link
Member Author

Are you willing to review this as it is @apoelstra or do you want me to spit it up. If it matters I made minimal changes to get it to build, including IIRC:

  • move the parse file and fix macro exports and fully qualified paths (feature gate on "alloc")
  • cutn'pasta the Height/Time stuff
  • cutn'pasta the LOCK_TIME_THRESHOLD
  • move the weight, and fee_rate files
  • remove the actual-serde attribute stuff
  • fix the path when calling the parse macros
  • fix prelude stuff
  • cutn'pasta unit tests as appropriate
  • Add and use error constructor for TimeOverflowError
  • delete rustdoc that mentions LockTime from newly create units modules

Phew, now I write that it does seem a lot in one commit. But the only logical way to break it is to do parse and locktimes as one commit and then do Weight and FeeRate as another commit - but that will not make it all that much easier to review.

Requesting special consideration to just review as is - pretty please :)

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK cbee978 lgtm. this is a good start. I think the LockTime types should follow Height and Time

@apoelstra apoelstra merged commit 750b4df into rust-bitcoin:master Mar 15, 2024
187 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-base58 C-bitcoin PRs modifying the bitcoin crate C-hashes PRs modifying the hashes crate C-units PRs modifying the units crate P-high High priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

units: ParseIntError is preventing crate smashing
5 participants