Making globals part of PSBT struct. Closes #652#654
Making globals part of PSBT struct. Closes #652#654sanket1729 merged 1 commit intorust-bitcoin:masterfrom BP-WG:psbt/global
Conversation
src/util/psbt/mod.rs
Outdated
There was a problem hiding this comment.
I don't like the verify name for the API. Maybe sanity_check is better. or perhaps unsigned_tx_checks
But I think this check should be private and should also be checked when we decode into a PartiallySignedTransaction.
|
@sanket1729 addressed your comment. As for the user ability to modify unsigned tx, this is a separate problem which was present before this PR. I think it should be addresses in some other PR (since there might be multiple ways of doing that). I think that we need to make the field private and provide a read-only getter for it. |
|
Agreed that the unsigned tx should be private and accessed through a getter. In PSBT2 there will not even necessarily be an unsigned tx, and we may need to compute it on the fly and/or cache it. Agreed also that this change belongs in another PR. |
|
@apoelstra I do agree that the stuff belongs to |
|
@sanket1729 pls re-review this PR, such that we can go forward with it and merge |
sanket1729
left a comment
There was a problem hiding this comment.
ACK 55c6277 . Reviewed range diff with ac0c908 that I previously ACKed
|
is there a reason this PR deletes the PSBT merge function? |
|
It does not, it just now uses merge function which is a part of |
|
@dr-orlovsky there used to be a publicly exposed We should restore it -- though I'd prefer it be called |
|
FTR fixed in #842 |
I took the most non-invasive approach to reduce diff size. Many parts of the code can be improved in style or further refactored (like some functions are not necessary and can be just moved to be part of other functions), but I'd prefer to do that as a separate PR once this will be merged.
My approach with this PR:
Globalstruct by moving its fields right intoPartiallySignedTransaction- but keep theutil/psbt/map/global.rsfile with all its logicMap for Globalimplementation in the same file, but just change it toMap for PartiallySignedTransactionGlobaldeserialization into crate-private function and use it fromPartiallySignedTransactiondeserializationThe refactoring will be followed by PR(s) adding support for Taproot