Skip to content

Improve the public API for Feerate and Weight#1685

Merged
apoelstra merged 1 commit intorust-bitcoin:masterfrom
yancyribbens:improve-fee-rate-weight-api
Mar 4, 2023
Merged

Improve the public API for Feerate and Weight#1685
apoelstra merged 1 commit intorust-bitcoin:masterfrom
yancyribbens:improve-fee-rate-weight-api

Conversation

@yancyribbens
Copy link
Copy Markdown
Contributor

Small nit for #1627 to re-export Weight and FeeRate to shorten the use path.

use bitcoin::Weight;
use bitcoin::FeeRate;

Copy link
Copy Markdown
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 b0b0cdb

Thanks man.

Copy link
Copy Markdown
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 b0b0cdb

I don't mind this, just please consider potential improvement in renaming Weight to BlockWeight.

pub use crate::blockdata::locktime::{self, absolute, relative};
pub use crate::blockdata::script::{self, Script, ScriptBuf};
pub use crate::blockdata::transaction::{self, OutPoint, Sequence, Transaction, TxIn, TxOut};
pub use crate::blockdata::weight::Weight;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Maybe as BlockWeight to improve clarity? bitcoin::Weight looks a bit strange.

Copy link
Copy Markdown
Member

@sanket1729 sanket1729 Mar 3, 2023

Choose a reason for hiding this comment

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

FWIW, I would be confused if it's TxWeight or BlockWeight where the latter refers to segwit calculated total bitcoin block weight.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think the docstring does a great job of this clarification already

/// Represents block weight - the weight of a transaction or block.
///
/// This is an integer newtype representing weigth in wu. It provides protection against mixing
/// up the types as well as basic formatting features.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think "blockweight" makes sense. Weight can apply to transactions just as well (and this is the more common case).

I think bitcoin::Weight looks fine. "weight" is a standard unit in Bitcoin.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

In my mind calling transaction weight "block weight" was OK because it refers to how much weight does it take in block. But OK, looks like simple Weight is fine.

Copy link
Copy Markdown
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 b0b0cdb

@apoelstra apoelstra merged commit 1679ad8 into rust-bitcoin:master Mar 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants