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.

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

Re-export psbt module from root level #790

Merged
merged 1 commit into from Jan 19, 2022
Merged

Re-export psbt module from root level #790

merged 1 commit into from Jan 19, 2022

Conversation

tcharding
Copy link
Member

@tcharding tcharding commented Jan 16, 2022 •
edited

We currently have the map module private but containing a bunch of types that are needed in the public API (specifically in a PartiallySignedTransaction).

To give access to them re-export the util::psbt module at the root level.

Found while testing master with rust-miniscript.

@tcharding
Copy link
Member Author

tcharding commented Jan 16, 2022 •
edited

@sanket1729 there is a branch rust-bitcoin-0.28.0-rc.1 on my rust-miniscript tree if you want to save yourself doing it. I'm not sure if its correct but it builds.

thomaseizinger
thomaseizinger previously approved these changes Jan 16, 2022
View reviewed changes
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

I really, really wish clippy/rustc would have a lint against this. Needing to check this manually is just tedious.

ACK 737c48a

sanket1729
sanket1729 previously approved these changes Jan 16, 2022
View reviewed changes
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.

utACK 737c48a

dr-orlovsky
dr-orlovsky suggested changes Jan 17, 2022
View reviewed changes
Copy link
Collaborator

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

Pls see my comment

src/lib.rs Outdated
@@ -135,7 +135,7 @@ pub use util::amount::Denomination;
pub use util::amount::SignedAmount;
pub use util::merkleblock::MerkleBlock;
pub use util::sighash::SchnorrSigHashType;

pub use util::psbt::{Input, Output, TapTree, PsbtSigHashType};
Copy link
Collaborator

Choose a reason for hiding this comment

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

I really think exporting PSBT-specific types as bitcoin::Input at root level gives highly misleading names. They are for sure should be kept as bitcoin::psbt::Input or bitcoin::util::psbt::Input, as they always were

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh yes this is much better.

@Kixunil
Copy link
Collaborator

Kixunil commented Jan 17, 2022

Strongly agree with @dr-orlovsky however with a bit of resistance I might accept use util::psbt::Input as PsbtInput. :)

@dr-orlovsky
Copy link
Collaborator

I propose not to increase confusion with type names. It will be really strange to export the same type under different names, psbt::Input and PsbtInput. We should either rename the type - or continue to use it like it was.

@Kixunil
Copy link
Collaborator

Kixunil commented Jan 17, 2022

Agree. Removing util would help though. Unrelated: PartiallySignedTransaction is crazy long name. I would be fine with Psbt

@apoelstra
Copy link
Member

apoelstra commented Jan 17, 2022 •
edited

concept ACK

  • renaming PartiallySignedTransaction to Psbt
  • reexporting psbt at top-level (so users don't need util)
  • reexporting all the other modules in util (and deprecating the old paths?)?

Not sure how much of these would uncontroversial and acceptable for the imminent release.

@Kixunil
Copy link
Collaborator

Kixunil commented Jan 17, 2022

I believe that 0.29 will be more suitable for renaming. We even have it in description. :)

@tcharding
Copy link
Member Author

With psbt module public users now have to do bitcoin::EcdsaSigHashType and bitcoin::psbt::PsbtSigHashType. Perhaps we want to re-export just the sighash type in the root level to be uniform? Probably should be part of this release/PR if folks agree?

@sanket1729
Copy link
Member

Probably should be part of this release/PR if folks agree

Yup, we need to export PsbtSigHashType type for this release. We can leave the other imports as is for now

@tcharding
Re-export public map types from root level
b138428 
We currently have the `map` module private but containing a bunch of
types that are needed in the public API (specifically in a
`PartiallySignedTransaction`).

Re-export the publicly required types to the `psbt` module and then
again at the root level of `rust-bitcoin` as we do for other types.
@tcharding tcharding changed the title Re-export public map types from root level Re-export psbt module from root level Jan 18, 2022
sanket1729
sanket1729 approved these changes Jan 18, 2022
View reviewed changes
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.

utACK b138428

@tcharding
Copy link
Member Author

@sanket1729, I didn't quite understand your comment. I've just exported psbt module for now.

@tcharding tcharding mentioned this pull request Jan 18, 2022
Merged
Kixunil
Kixunil approved these changes Jan 18, 2022
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 b138428

Even though this is jumping to flattening before planned I don't see anything harmful with it. Maybe it's even useful since people can change the code upfront.

RCasatta
RCasatta approved these changes Jan 19, 2022
View reviewed changes
Copy link
Collaborator

@RCasatta RCasatta left a comment

Choose a reason for hiding this comment

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

ACK b138428

@RCasatta
Copy link
Collaborator

can't merge with "review requested" from @dr-orlovsky

dr-orlovsky
dr-orlovsky approved these changes Jan 19, 2022
View reviewed changes
Copy link
Collaborator

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

ACK b138428

@dr-orlovsky
Copy link
Collaborator

@RCasatta sorry for being slow. Will be able to merge in 1-2hrs, if you will not merge that before that time

@RCasatta RCasatta merged commit 1f0810a into rust-bitcoin:master Jan 19, 2022
@RCasatta
Copy link
Collaborator

@dr-orlovsky very fast when requested though, thanks

@tcharding tcharding deleted the psbt-sigh-hash-type branch January 20, 2022 01:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dr-orlovsky dr-orlovsky dr-orlovsky approved these changes

@Kixunil Kixunil Kixunil approved these changes

@RCasatta RCasatta RCasatta approved these changes

@sanket1729 sanket1729 sanket1729 approved these changes

@thomaseizinger thomaseizinger thomaseizinger left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@tcharding @Kixunil @dr-orlovsky @apoelstra @sanket1729 @RCasatta @thomaseizinger