8000 Add kilo weight unit conversion by yancyribbens · Pull Request #1735 · rust-bitcoin/rust-bitcoin · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add kilo weight unit conversion #1735

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 23, 2023
Merged

Add kilo weight unit conversion #1735

merged 1 commit into from
Mar 23, 2023

Conversation

yancyribbens
Copy link
Contributor

The FeeRate module defaults to sats per kwu so when doing fee calculations, it would be convenient to easily convert weight to the same units.

@yancyribbens yancyribbens force-pushed the add-kwu-conversion branch 4 times, most recently from ae2069a to 30731da Compare March 22, 2023 17:21
Kixunil
Kixunil requested changes Mar 22, 2023
View reviewed changes
Copy link
Collaborator
@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

Concept ACK, other than the naming, this looks good.

Kixunil
Kixunil reviewed Mar 22, 2023
View reviewed changes
Copy link
Collaborator
@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

Would you mind improving the doc a bit too?

Kixunil
Kixunil reviewed Mar 22, 2023
View reviewed changes
@yancyribbens yancyribbens force-pushed the add-kwu-conversion branch 2 times, most recently from cd3fa52 to d2ea967 Compare March 22, 2023 18:22
@yancyribbens
Copy link
Contributor Author

@Kixunil I think I made the changes you requested. For some reason rust nightly kept failing so I applied RustFmt to just my changes so hopefully that fixes the failing builds.

@Kixunil
Copy link
Collaborator
Kixunil commented Mar 22, 2023

Yeah, looks good now, will wait for CI and then ACK.

@yancyribbens
Copy link
Contributor Author

hmm nightly is still failing without much information about why :/

@Kixunil
Copy link
Collaborator
Kixunil commented Mar 22, 2023

Hmm, still failing. So running cargo fmt messes up unrelated code?

Cc @tcharding we might have a problem here.

@Kixunil
Copy link
Collaborator
Kixunil commented Mar 22, 2023

without much information about why :/

I pretty clearly see it's complaining about formatting:

-    pub fn from_kwu(wu: u64) -> Option<Self> {
-        wu.checked_mul(1000).map(Weight)
-    }
+    pub fn from_kwu(wu: u64) -> Option<Self> { wu.checked_mul(1000).map(Weight) }

     /// Constructs `Weight` from virtual bytes.
     ///
Diff in /home/runner/work/rust-bitcoin/rust-bitcoin/bitcoin/src/blockdata/weight.rs at line 61:
     pub const fn to_wu(self) -> u64 { self.0 }
 
     /// Converts to kilo weight units rounding down.
-    pub const fn to_kwu_floor(self) -> u64 {
-        self.0 / 1000
-    }
+    pub const fn to_kwu_floor(self) -> u64 { self.0 / 1000 }

@Kixunil
Copy link
Collaborator
Kixunil commented Mar 22, 2023

This doesn't look right, there should be much fewer changes.

@Kixunil
Copy link
Collaborator
Kixunil commented Mar 22, 2023

Maybe the configuration file doesn't get applied for some reason?

@yancyribbens
Copy link
Contributor Author

I'm a bit confused because it seems to be complaining about formatting in sections that I didn't change. I added another commit which applys rustfmt to the entire file to see if it passes.

@Kixunil
Copy link
Collaborator
Kixunil commented Mar 22, 2023

Indeed. For some reason it has different behavior on your machine. Maybe try cargo +stable fmt instead?

@apoelstra
Copy link
Member

On my machine cargo +stable fmt changes a bunch of stuff. You need to use +nightly for it to respect our rustfmt.toml.

@Kixunil
Copy link
Collaborator
Kixunil commented Mar 22, 2023

Oh, so we have undocumented contribution guideline and also require nightly for development. I'm not happy about this...

@yancyribbens yancyribbens force-pushed the add-kwu-conversion branch 2 times, most recently from 97f8774 to dbd2ea0 Compare March 22, 2023 18:56
@yancyribbens
Copy link
Contributor Author

fingers crossed cargo +nightly fmt worked

Kixunil
Kixunil approved these changes Mar 22, 2023
View reviewed changes
Copy link
Collaborator
@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

ACK dbd2ea0

@Kixunil
Copy link
Collaborator
Kixunil commented Mar 22, 2023

Thanks!

@Kixunil Kixunil added enhancement one ack PRs that have one ACK, so one more can progress them labels Mar 22, 2023
@apoelstra
Copy link
Member

Oh, so we have undocumented contribution guideline and also require nightly for development. I'm not happy about this...

Well, FWIW, we do actually have a paragraph in CONTRIBUTING.md about this :). But yes, we should update that and put it in the README where somebody might see it.

Regarding "requiring nightly for development", I agree that this is an onerous requirement. But stable fmt doesn't work because it doesn't respect certain rustfmt.toml settings.

In any case, I'd be fine removing the CI fmt check for now, and just periodically reformatting the whole crate, or doing it prior to releases, or something like that. Under normal circumstances I'd expect this to be a fairly noninvasive process ... at least compared to the gargantuan #1434 ... though maybe these days during crate smash it actually will be a big deal.

@Kixunil
Copy link
Collaborator
Kixunil commented Mar 22, 2023

I wonder if super-huge CONTRIBUTING.md is really that useful. People may ignore it or be scared off by it.

Maybe we should add basic steps into PR template.

Let's keep the check in and see what happens.

@tcharding
Copy link
Member
tcharding commented Mar 22, 2023 •
edited
Loading

Yeah I've been writing cargo +nightly fmt in every commit message and PR description I can for the last year to try and remind folks they need to use nightly - I couldn't think of a better way to spread that knowledge :)

During the "rustfmt config wars" I couldn't get a suitable configuration without using nightly. This is actually quite a problem if devs work on other projects and run cargo fmt in their editors on save because that will constantly introduce rubbish changes in rust-bitcoin files. FTR I have not got fmt-on-save set in my editor.

EDIT: Its actually not so bad because of the githooks, so if new contibutors mention they are having trouble with this we just need to point them towards enabling the git pre-commit hook and it will catch the formatting before they proceed. Well done Tobin from the past :) (cc @Kixunil, not sure if post edit shows up in notifications and you already emoji'd)

tcharding
tcharding approved these changes Mar 22, 2023
View reviewed changes
Copy link
Member
@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

ACK dbd2ea0

apoelstra
apoelstra approved these changes Mar 22, 2023
View reviewed changes
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 dbd2ea0

@apoelstra apoelstra merged commit 3ca9de8 into rust-bitcoin:master Mar 23, 2023
@realeinherjar realeinherjar mentioned this pull request Nov 1, 2023
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kixunil Kixunil Kixunil approved these changes

@apoelstra apoelstra apoelstra approved these changes

@tcharding tcharding tcharding approved these changes

Assignees
No one assigned
Labels
enhancement one ack PRs that have one ACK, so one more can progress them
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yancyribbens @Kixunil @apoelstra @tcharding
0