Add `bitcoin-units` crate by tcharding · Pull Request #1225 · rust-bitcoin/rust-bitcoin

tcharding · 2022-09-01T00:54:24Z

Create a new bitcoin-units crate as described here.

Only the amount module is currently included.

I've resolved the Encodale/Decodable issue by keeping the amount module in bitcoin.

sanket1729

strong concept ACK

tcharding · 2022-09-01T01:26:21Z

Excuse all the force pushes, I'm wrestling with a few CI things but the core of this PR should be good to go!

tcharding · 2022-09-01T03:39:07Z

Finally, all green :)

Kixunil

Mostly nits, what I require is to fix (remove) the doc link and I strongly want to remove core2 dep.

Also maybe make adding the crate and depending on it a single commit so that git would show the file as moved.

bitcoin-units/Cargo.toml

tcharding · 2022-09-02T02:21:38Z

Mostly nits,

nits implemented

what I require is to fix (remove) the doc link and I strongly want to remove core2 dep.

Still to resolve, commented above.

Also maybe make adding the crate and depending on it a single commit so that git would show the file as moved.

Done

apoelstra · 2022-09-02T14:10:54Z

Done reviewing 79102d447ca9607ab60fa9b50acf2063ab43e111. This looks great! ACK except I'd really like to drop core2 if we can do it now. (If it requires a more-involved followup PR that's ok too.)

37cd650 ci: Remove TOOLCHAIN env var (Tobin C. Harding) 8eb6c23 ci: Remove inner quotes from shell variable (Tobin C. Harding) 58ede47 ci: Use set -ex instead of /bin/sh -ex (Tobin C. Harding) 4c4846f Remove double blank line at EOF (Tobin C. Harding) Pull request description: Clean up the CI script. These patches are part of #1225, are beneficial to have before that PR resolves, and are trivial. ACKs for top commit: sanket1729: utACK 37cd650 apoelstra: ACK 37cd650 Tree-SHA512: f6b3379c8a1be7a3519461faeed35e35560e4228bddb59ef28ab3fb3ef2679afabfaccfeaffd78adbb66879aed02c092f3d9d1d1eb3193c811067ee75e876732

Kixunil · 2022-09-07T16:55:34Z

I wonder if Work&Target should be here. It feels right except we will have to separate out consensus encoding before we can do it.

tcharding · 2022-09-07T23:41:36Z

I wonder if Work&Target should be here.

I think so too.

It feels right except we will have to separate out consensus encoding before we can do it.

Why's that? Only CompactTarget needs consensus encoding.

Kixunil · 2022-09-08T06:58:10Z

Indeed, and not putting CompactTarget there would feel a bit weird, wouldn't it? Nothing wrong with intermediate step probably.

apoelstra · 2022-09-20T17:04:40Z

I don't think it'd be too weird if Target was in -units but CompactTarget was in -primitives (or -blockdata or whatever)

tcharding · 2022-09-21T02:24:16Z

Hey @Kixunil, I rebased on #1297 (fixed a minor issue in macro) but I couldn't get it to build because a static string cannot be used to create a What object so this call fails, am I using InputString correctly?

impl fmt::Display for ParseAmountError {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        use ParseAmountError::*;

        match *self {
            ...
            UnknownDenomination(ref d) => write!(f, "{}", d.display_cannot_parse("denomination")),
           ...

Kixunil · 2022-09-21T07:00:30Z

a static string cannot be used to create a What object

Oh, forgot to add ?Sized, you could also use &"denomination" while the Sized bound is there.

Edit: removed the bound.

Kixunil

ACK f2483a7c25f49f343063b1fd8853023c62b3a9dd

Kixunil · 2023-12-06T11:24:57Z

bitcoin/src/lib.rs

I don't see the inline here.

tcharding · 2023-12-06T19:41:56Z

Bizzare, the change is in my tree and git status says its up to date. I'll force push again and check it.

EDIT: In case you don't see the comment I made elsewhere, my mistake, I had only done it in units/lib.rs not in bitcoin/lib.rs

tcharding · 2023-12-06T19:46:46Z

@tcharding IIUC we don't need LICENSE in the crate, just the value in Cargo.toml.

I'm not sure it matters but the bitcoin_hashes crate does not include the license file. I used this address

https://crates.io/api/v1/crates/bitcoin_hashes/0.13.0/download to download the file, which is a tarball even though it ends in .crate, extracted with tar -xzvf and looked inside.

tcharding · 2023-12-06T19:49:40Z

Rebased, no other changes.

tcharding · 2023-12-06T19:54:09Z

Used doc(inline) in the bitcoin::amount module.

tcharding · 2023-12-06T21:40:27Z

I've dropped the copy license patch, its unrelated to this PR since we have other crates already without it. (Force push includes fix to rebase fail also.)

Kixunil

ACK 53fce7ba07be8927a7d66d5b6022f27e8b737b4e

Kixunil · 2023-12-07T11:14:07Z

units/Cargo.toml

IIUC the commits before this one didn't support no-alloc and would fail if tested but I think it's OK since the crate was just created here.

I ran cargo test --all--features in units on commit: f42033da Add ParseDenominationError just to make sure. It passes. The reason is that we only use write_err from internals until we introduce InputString usage, so I believe the features are updated in the correct patches.

Actually you need to run --no-default-features.

Still tests cleanly with no default features. Did you see the error or are you guessing from the diff?

What kind of sorcery is this?! I was judging from the diff because there was no #[cfg] to remove the string where InputString is now. Which means my review wasn't very good and I should look into it better.

oooo, took me a while to work this out, we have the very sneaky module declaration as:

// TODO: Make amount module less dependent on an allocator. #[cfg(feature = "alloc")] pub mod amount;

There is still a bit of work to do to remove that TODO because of all the functions that return String.

tcharding · 2023-12-07T19:45:42Z

@apoelstra you've been strangely quite on this PR, is it because it only moves amounts? I was planning on adding other types when it merges and before its released, I have not thought yet about which ones though.

apoelstra · 2023-12-08T00:43:47Z

@apoelstra you've been strangely quite on this PR, is it because it only moves amounts? I was planning on adding other types when it merges and before its released, I have not thought yet about which ones though.

It's because there were a zilion comments from Kix, then fixes, then repeats. Now that Kix has ACKed I have a TODO to review it. Maybe tomorrow.

tcharding · 2023-12-08T20:46:51Z

Sweet, no stress.

apoelstra · 2023-12-09T21:40:15Z

In f42033dadec7d94dbc9c8efbb301e04d70e9f25c, cargo build --no-default-features in bitcoin/ does not work. Something about needing to import ToOwned from alloc.

But utACK 53fce7ba07be8927a7d66d5b6022f27e8b737b4e once you get it compiling.

tcharding · 2023-12-09T22:23:38Z

I just used to_string since that is already imported, and in the following patch the call is removed anyway.

Kixunil

ACK 4f7779174ca99fb8bb5970b3f867464f0da64eb2

apoelstra · 2023-12-10T19:09:37Z

c36f07516e8f49ef7d9140848929795efefe611 has the same issue.

Add a new crate `bitcoin-units`, move the `amount` module over to it and re-export all types from `bitcoin::amount` so this as not a breaking change.

We have two variants in the `ParseAmountError` that both come from parsing a denomination, these should be a separate error.

Done so that we can correctly support builds without an allocator. Add two new error types that wrap `InputString`.

tcharding · 2023-12-10T21:54:09Z

Bother, one day I'll start remembering to pass --all to cargo.

Kixunil

ACK 396e049

apoelstra

ACK 396e049

tcharding force-pushed the 09-01-bitcoin-units branch 2 times, most recently from ad9dcfc to 5815d99 Compare September 1, 2022 01:09

sanket1729 reviewed Sep 1, 2022

View reviewed changes

tcharding force-pushed the 09-01-bitcoin-units branch from 5815d99 to f501fd5 Compare September 1, 2022 01:25

tcharding force-pushed the 09-01-bitcoin-units branch 4 times, most recently from 7c06523 to 29c733b Compare September 1, 2022 03:25

Kixunil previously requested changes Sep 1, 2022

View reviewed changes

tcharding force-pushed the 09-01-bitcoin-units branch from 29c733b to 6e6eac3 Compare September 2, 2022 02:18

tcharding force-pushed the 09-01-bitcoin-units branch from 6e6eac3 to 79102d4 Compare September 2, 2022 02:30

tcharding marked this pull request as draft September 5, 2022 01:18

tcharding mentioned this pull request Sep 5, 2022

CI clean ups #1238

Merged

tcharding added this to the 0.30.0 milestone Sep 13, 2022

tcharding added minor API Change This PR should get a minor version bump 1.0 Issues and PRs required or helping to stabilize the API aggregate release notes mention labels Sep 13, 2022

Kixunil added the crate smashing Issues and PRs splitting out parts of the main crate to smaller crates label Sep 20, 2022

tcharding force-pushed the 09-01-bitcoin-units branch from 79102d4 to 1f6eb92 Compare September 21, 2022 02:22

Kixunil previously approved these changes Dec 6, 2023

View reviewed changes

bitcoin/src/lib.rs Outdated

Copy link
Copy Markdown

Collaborator

Kixunil Dec 6, 2023

Choose a reason for hiding this comment

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

I don't see the inline here.

tcharding force-pushed the 09-01-bitcoin-units branch from f2483a7 to 40f2c7d Compare December 6, 2023 19:48

tcharding added 2 commits December 7, 2023 06:48

internals: Fix docs typo

fa8d300

internals: Add docs to InputString

4ecb1fe

tcharding dismissed Kixunil’s stale review via e3dab80 December 6, 2023 19:49

tcharding force-pushed the 09-01-bitcoin-units branch from 40f2c7d to e3dab80 Compare December 6, 2023 19:49

Kixunil approved these changes Dec 7, 2023

View reviewed changes

tcharding mentioned this pull request Dec 9, 2023

TryFrom<SignedAmount> for Amount and vice versa #2270

Closed

Kixunil approved these changes Dec 10, 2023

View reviewed changes

tcharding added 3 commits December 11, 2023 08:52

Add bitcoin-units crate

69e56a6

Add a new crate `bitcoin-units`, move the `amount` module over to it and re-export all types from `bitcoin::amount` so this as not a breaking change.

Add ParseDenominationError

acacf45

We have two variants in the `ParseAmountError` that both come from parsing a denomination, these should be a separate error.

Use InputString instead of String

396e049

Done so that we can correctly support builds without an allocator. Add two new error types that wrap `InputString`.

Kixunil approved these changes Dec 10, 2023

View reviewed changes

apoelstra approved these changes Dec 11, 2023

View reviewed changes

This was referenced Dec 11, 2023

Clean up io usage #2262

Merged

Document non-functional no-std for bitcoinconsensus #1352

Closed

Remove ToOwned from prelude #2279

Merged

tcharding mentioned this pull request Oct 18, 2025

Release tracking PR: bitcoin-primitives 1.0.0-rc.0 #4904

Merged

Conversation

tcharding commented Sep 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding commented Sep 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented Sep 1, 2022

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tcharding commented Sep 2, 2022

Uh oh!

apoelstra commented Sep 2, 2022

Uh oh!

Kixunil commented Sep 7, 2022

Uh oh!

tcharding commented Sep 7, 2022

Uh oh!

Kixunil commented Sep 8, 2022

Uh oh!

apoelstra commented Sep 20, 2022

Uh oh!

tcharding commented Sep 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kixunil commented Sep 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

Kixunil Dec 6, 2023

Choose a reason for hiding this comment

Uh oh!

tcharding commented Dec 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented Dec 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented Dec 6, 2023

Uh oh!

tcharding commented Dec 6, 2023

Uh oh!

tcharding commented Dec 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

Kixunil Dec 7, 2023

Choose a reason for hiding this comment

Uh oh!

tcharding Dec 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kixunil Dec 7, 2023

Choose a reason for hiding this comment

Uh oh!

tcharding Dec 7, 2023

Choose a reason for hiding this comment

Uh oh!

Kixunil Dec 7, 2023

Choose a reason for hiding this comment

Uh oh!

tcharding Dec 8, 2023

Choose a reason for hiding this comment

Uh oh!

tcharding commented Sep 1, 2022 •

edited

Loading

tcharding commented Sep 1, 2022 •

edited

Loading

tcharding commented Sep 21, 2022 •

edited

Loading

Kixunil commented Sep 21, 2022 •

edited

Loading

tcharding commented Dec 6, 2023 •

edited

Loading

tcharding commented Dec 6, 2023 •

edited

Loading

tcharding commented Dec 6, 2023 •

edited

Loading

tcharding Dec 7, 2023 •

edited

Loading