Skip to content

Make Map Trait Visible#841

Closed
JeremyRubin wants to merge 1 commit intorust-bitcoin:masterfrom
JeremyRubin:map-visible
Closed

Make Map Trait Visible#841
JeremyRubin wants to merge 1 commit intorust-bitcoin:masterfrom
JeremyRubin:map-visible

Conversation

@JeremyRubin
Copy link
Copy Markdown
Contributor

@JeremyRubin JeremyRubin commented Feb 22, 2022

It seems like it is a clear mistake for the Map trait to not be externally visible since PSBTs cannot be merged otherwise.

This should be blocking for release!

@dr-orlovsky dr-orlovsky added this to the 0.28.0 milestone Feb 22, 2022
dr-orlovsky
dr-orlovsky approved these changes Feb 22, 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 d6f7b13

@Kixunil Kixunil added the RC fix label Feb 22, 2022
@Kixunil
Copy link
Copy Markdown
Collaborator

Kixunil commented Feb 22, 2022

I'm not sure why Map being public would be a requirement for merging. Wouldn't it make more sense to have inherent merge method?

@sanket1729
Copy link
Copy Markdown
Member

sanket1729 commented Feb 22, 2022

I'm not sure why Map being public would be a requirement for merging. Wouldn't it make more sense to have inherent merge method?

+1. Making the map public causes weird inconsistencies and bugs as listed #576.

@tcharding
Copy link
Copy Markdown
Member

tcharding commented Feb 22, 2022

Oh this is my mistake, the issue linked above that I worked off of when making Map trait private clearly says we need a public merge API but I neglected to add this.

If we end doing this, we still need to design so that the merge method should be available as a psbt API.

I'll do that.

@tcharding tcharding mentioned this pull request Feb 22, 2022
Merged
@tcharding
Copy link
Copy Markdown
Member

tcharding commented Feb 22, 2022

Thanks @JeremyRubin for catching this.

sanket1729 added a commit that referenced this pull request Feb 28, 2022
@sanket1729
Merge #842: Separate out merge method into public trait
1ec9e87 
5e24499 Separate merge logic out of Map trait (Tobin Harding)

Pull request description:

  Recently we (*cough* Tobin) made the `Map` trait private and neglected
  to add a public API for merging together two PSBTs. Doing so broke the
  `psbt` module.

  Add a public trait `Merge` and implement it for
  `PartiallySignedTransaction` using the code currently in the `merge`
  method of the now private `Map` trait.

  Motivated by #841

ACKs for top commit:
  JeremyRubin:
    > ACK 5e24499
  apoelstra:
    ACK 5e24499
  sanket1729:
    ACK 5e24499. Also verified that the vectors are same of that of BIP174

Tree-SHA512: 79eefe93e870b61231b388aa28a95ee5c8ac06b68910f4ff324569512a79eafe5b86239fd45f54ca7a868cf59dc6301e45d1f046c039a64b2493a8ffcea659fd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

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

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.28.0

Development

Successfully merging this pull request may close these issues.

5 participants

@JeremyRubin @Kixunil @sanket1729 @tcharding @dr-orlovsky