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 psbt_signer.rs example #744

Merged
merged 1 commit into from
Sep 13, 2022

Conversation

notmandatory
Copy link
Member

Description

Adding a simple example of how to create a PSBT with a watch only wallet and then sign it with a signing wallet.

Notes to the reviewers

This example was inspired by a question from a user.

Changelog notice

none.

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

@notmandatory notmandatory self-assigned this Sep 2, 2022
@notmandatory
Copy link
Member Author

@vladimirfomene please take a look and see if it works for you.

@notmandatory notmandatory force-pushed the example/psbt_signer branch 4 times, most recently from 0741362 to 470af23 Compare September 2, 2022 18:42
Copy link
Contributor

@vladimirfomene vladimirfomene left a comment

Choose a reason for hiding this comment

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

Tested ACK

Copy link
Contributor

@vladimirfomene vladimirfomene left a comment

Choose a reason for hiding this comment

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

Tested ACK

Copy link
Member

@danielabrozzoni danielabrozzoni left a comment

Choose a reason for hiding this comment

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

Concept ACK

It looks good to me, I think it should also be added to the Cargo.toml in the examples section :)

examples/psbt_signer.rs Outdated Show resolved Hide resolved
examples/psbt_signer.rs Outdated Show resolved Hide resolved
examples/psbt_signer.rs Outdated Show resolved Hide resolved
examples/psbt_signer.rs Outdated Show resolved Hide resolved
@notmandatory notmandatory force-pushed the example/psbt_signer branch 4 times, most recently from 469f9df to f320e3c Compare September 8, 2022 01:28
@notmandatory
Copy link
Member Author

I incorporated the suggested changes and also added a list of any unconfirmed tx if the wallet balance is less than the desired 10k sats. Ready to re-review! 🙏

Copy link
Member

@danielabrozzoni danielabrozzoni left a comment

Choose a reason for hiding this comment

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

Looking great!

ACK fa998de

Comment on lines +80 to +82
.add_recipient(return_address.script_pubkey(), 9_800)
.enable_rbf()
.fee_rate(FeeRate::from_sat_per_vb(1.0));
Copy link
Member

Choose a reason for hiding this comment

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

I would use .drain_wallet().drain_to(return_address.script_pubkey()) to make sure a change is not created. It's also a chance to show that BDK can do more than "just send x to this address".

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good idea but I do like having some coin selection and change going on, which I think will be how most new projects will be doing rather than sweeping all funds.

Copy link
Member

Choose a reason for hiding this comment

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

Now that I think about it, the problem of receiving 10_000 and sending 9_800 is that you create a small change (~150 sats?), which will be uneconomical to spend (the fees for spending it are greater than the utxo value) - which BDK's coin selection won't pick, unless it's explicitly instructed to do so.

What this means is: after running this example many times, we might have the wallet full of these rather small utxos which we can't spend, and users will start to see some weird errors: for example, they might see that the balance is 10_000 (~70 utxos, ~150 each), but the coin selection will fail saying that there are not enough funds.

We can prevent this either by sweeping all funds every time, or by asking users to fill the wallet with 10_000 sats, but then spending only a smaller number, like 5_000

Copy link
Member

Choose a reason for hiding this comment

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

Oh, nevermind, the CS is smart enough that it picks 200 sats as fee, leaving no residue in the wallet :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes I guess also in my testing it used the change for fee and left an empty wallet balance. Yea for BDK coin selection! So we think it's ok to do it this way and not use drain_to to ensure there's no change?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine :)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah the end result should be the same because the leftover is lower than the dust threshold. I can go ahead and merge it like this, my suggestion was based on what I would have done personally, but it also looks good like this.

@afilini
Copy link
Member

afilini commented Sep 12, 2022

I was also about to suggest using the descriptor!() macro but decided not to because I think it could be a bit overwhelming at first. Most users will probably have a string for their descriptor and this would look more familiar to them.

Copy link
Member

@afilini afilini left a comment

Choose a reason for hiding this comment

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

ACK fa998de

@afilini afilini merged commit b12dec3 into bitcoindevkit:master Sep 13, 2022
@afilini afilini mentioned this pull request Oct 7, 2022
24 tasks
@notmandatory notmandatory deleted the example/psbt_signer branch October 24, 2022 22:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants