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.

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

Add finalize to Psbt #630

Merged
merged 1 commit into from
Nov 21, 2024

Conversation

rustaceanrob
Copy link
Contributor

@rustaceanrob rustaceanrob commented Nov 16, 2024

Description

#580 does the initial work to adding finalize to Psbt. I would like to get this in for @andreasgriffin and revisit this later in bitcoin-ffi. I think #580 has the right idea, but I would like a more detailed system of why finalizing might have failed. Unfortunately, the finalize function using a Vec<E> of errors, which is not expressible by UniFFI types, so I created a new FinalizedPsbtResult type that contains either the original Psbt or the finalized Psbt, and any errors that may be associated with the finalize step.

I have thought about this for a while and I think this is the best approach, but open to any ideas.

Notes to the reviewers

Please confirm this works with your example Psbt @andreasgriffin

Changelog notice

  • Add finalize method to Psbt

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@andreasgriffin
Copy link
Contributor

andreasgriffin commented Nov 16, 2024

Thanks! Yes I can confirm that it finalizes the PSBT in #469 correctly.

And the could_finalize is super helpful!!!

@reez
Copy link
Collaborator

reez commented Nov 17, 2024

but I would like a more detailed system of why finalizing might have failed

Same, more details are great for things like this

Unfortunately, the finalize function using a Vec of errors, which is not expressible by UniFFI types, so I created a new FinalizedPsbtResult type that contains either the original Psbt or the finalized Psbt, and any errors that may be associated with the finalize step.

This makes sense to me.

Concept ACK c432e2d

Built and tested the swift bindings locally but didn't get to finish putting it thru a few different scenarios to test it out more (not a blocker on the PR, just ran out of time while taking a quick look at it tonight), did get this as expected when testing out a scenario where I expected couldFinalize to be false 👍:

finalized: FinalizedPsbtResult(psbt: BitcoinDevKit.Psbt, couldFinalize: false, errors: Optional([BitcoinDevKit.PsbtFinalizeError.InputError(reason: "Could not satisfy Tr descriptor", index: 0)]))

@thunderbiscuit
Copy link
Member

I think this is a decent way to deal with our inability to offer the Rust API directly.

It reminds me that I meant to keep track of those small reshapes of the API somewhere. Not sure if it should just be a personal file on my local system or part of a doc/readme somewhere, but I'm starting one today!

Thanks for taking the time to think through this one @rustaceanrob.

ACK c432e2d.

@thunderbiscuit
Copy link
Member

One thought I just had is that if this is not really agreed upon by other users of bitcoin-ffi, we'll end up with a breaking change if we migrate our Psbt type to the bitcoin-ffi type, or we'll have to just keep supporting our custom Psbt type.

@thunderbiscuit
Copy link
Member

@rustaceanrob do you mind rebasing this? Ready to merge.

@rustaceanrob
Copy link
Contributor Author

My branch is showing I am up to date

@thunderbiscuit thunderbiscuit merged commit c432e2d into bitcoindevkit:master Nov 21, 2024
25 checks passed
@rustaceanrob rustaceanrob deleted the finalize-11-16 branch November 21, 2024 19:38
@rustaceanrob rustaceanrob mentioned this pull request Nov 21, 2024
5 tasks
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.

4 participants