Skip to content

Re-export psbt module from root level#790

Merged
RCasatta merged 1 commit intorust-bitcoin:masterfrom
tcharding:psbt-sigh-hash-type
Jan 19, 2022
Merged

Re-export psbt module from root level#790
RCasatta merged 1 commit intorust-bitcoin:masterfrom
tcharding:psbt-sigh-hash-type

Conversation

@tcharding
Copy link
Copy Markdown
Member

@tcharding tcharding commented Jan 16, 2022 •
edited
Loading

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
Copy Markdown
Member Author

tcharding commented Jan 16, 2022 •
edited
Loading

@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
Copy Markdown
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 737c48a2d989756728b82ca34f2dd5d039aa9147

sanket1729
sanket1729 previously approved these changes Jan 16, 2022
View reviewed changes
Copy link
Copy Markdown
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 737c48a2d989756728b82ca34f2dd5d039aa9147

dr-orlovsky
dr-orlovsky suggested changes Jan 17, 2022
View reviewed changes
Copy link
Copy Markdown
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
Copy link
Copy Markdown
Collaborator

@dr-orlovsky dr-orlovsky Jan 17, 2022 •
edited
Loading

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
Copy Markdown
Member Author

@tcharding tcharding Jan 17, 2022

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
Copy Markdown
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
Copy Markdown
Collaborator

dr-orlovsky commented Jan 17, 2022

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
Copy Markdown
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
Copy Markdown
Member

apoelstra commented Jan 17, 2022 •
edited
Loading

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
Copy Markdown
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
Copy Markdown
Member Author

tcharding commented Jan 17, 2022

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
Copy Markdown
Member

sanket1729 commented Jan 18, 2022

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 dismissed stale reviews from sanket1729 and thomaseizinger via b138428 January 18, 2022 01:57
@tcharding tcharding force-pushed the psbt-sigh-hash-type branch from 737c48a to b138428 Compare January 18, 2022 01:57
@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
Copy Markdown
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
Copy Markdown
Member Author

tcharding commented Jan 18, 2022

@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
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 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
Copy Markdown
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
Copy Markdown
Collaborator

RCasatta commented Jan 19, 2022

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

dr-orlovsky
dr-orlovsky approved these changes Jan 19, 2022
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Collaborator

dr-orlovsky commented Jan 19, 2022

@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
Copy Markdown
Collaborator

RCasatta commented Jan 19, 2022

@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

@Kixunil Kixunil Kixunil approved these changes

@RCasatta RCasatta RCasatta approved these changes

@sanket1729 sanket1729 sanket1729 approved these changes

+2 more reviewers

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

@thomaseizinger thomaseizinger thomaseizinger left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@tcharding @Kixunil @dr-orlovsky @apoelstra @sanket1729 @RCasatta @thomaseizinger