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

docs(wallet): add example usage of descriptor and plan #1559

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

futurechimp
Copy link

@futurechimp futurechimp commented Aug 15, 2024

Description

Example usage of a vault policy, descriptor, and spend plan with a BDK wallet.

Changelog notice

docs(wallet): add example usage of descriptor and plan

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 (they are docs)

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

Copy link
Contributor

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

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

Thank you for working on this! It looks good and it's a great addition.

I guess you could rename the commits to the standard Conventional Commits, to something like: docs(wallet): add example usage of descriptor and plan and maybe squash the clippy fixes into a single commit.

@futurechimp futurechimp changed the title Example usage of descriptor and plan with BDK wallet docs(wallet): add example usage of descriptor and plan Aug 16, 2024
@futurechimp
Copy link
Author

All done, thanks for the suggestions, and if there's anything else needed just let me know.

Copy link
Contributor

@ValuedMammal ValuedMammal left a comment

Choose a reason for hiding this comment

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

Nice work on this. My initial thoughts are that we should try to narrow the scope to a few concepts including the descriptor, plan, and psbt.

It would be nice if the vault was a bdk Wallet which we can simulate funding by just inserting a transaction into it (no need for get_funded_wallet or TxBuilder, and you can just hardcode a receive address for spending out of the vault).

We can use the Psbt::sign API and just pass in the xpriv instead of using sign_ecdsa.

Maybe add more printlns and assertions along the way if you think that would help. Another future enhancement would be to use the wallet to create a psbt, but I think more needs to be done in the library to fully utilize the Plan construct. Having this example helps demonstrate how that could be implemented.

crates/wallet/examples/descriptor_with_plan.rs Outdated Show resolved Hide resolved
crates/wallet/examples/descriptor_with_plan.rs Outdated Show resolved Hide resolved
crates/wallet/examples/descriptor_with_plan.rs Outdated Show resolved Hide resolved
@futurechimp
Copy link
Author

futurechimp commented Aug 17, 2024

Ok great, happy to get rid of the ecdsa signing (I did it this way only because the only code example I could see that seemed as if it'd work came from the rust-miniscript repository). I'll try using Psbt::sign() although I haven't run across any examples of how to set that up.

And I will switch to using a Wallet for the descriptor and try to simplify the usage of it to reduce code. And add some println!s.

Do the transaction inputs, outputs, and witness appear to be set up correctly?

I'll keep working on it in a series of commits in my fork, and we can squash it down at the end once the example looks good. This is my first attempt with bdk so I'm very happy to receive feedback about how to structure things.

@futurechimp
Copy link
Author

futurechimp commented Aug 19, 2024

Ok, updated with the following:

  1. turned the vault descriptor in a Wallet
  2. used Psbt::sign instead of Ecdsa signing
  3. added printlns to show the general flow. I didn't print out the internals of many things as the output quickly gets pretty big - happy to show more things if helpful
  4. removed the get_funded_wallet function and just inserted the deposit transaction into the vault wallet directly
  5. prefixed with type path absolute::LockTime so that types are more clear
  6. used Sequence::ENABLE_RBF_NO_LOCKTIME instead of Sequence::ZERO.
  7. used Psbt::from_unsigned_tx instead of rolling my own

@ValuedMammal
Copy link
Contributor

I think it's looking good. I put some ideas for extra improvements in this commit ValuedMammal/bdk@60601f1 that include

  • refactor deposit_transaction and print the vault balance
  • refer to compiler example (instead of duplicating it)
  • get the vault descriptor from the Wallet structure

@notmandatory notmandatory added the documentation Improvements or additions to documentation label Aug 21, 2024
@notmandatory notmandatory added this to the 1.0.0-beta milestone Aug 21, 2024
@futurechimp
Copy link
Author

Looking quite a bit nicer now. Is it possible for you to squash merge it into the main bdk repo when ready? I've tied myself in a few git knots in my branch/repo, missed squashing your PR to my PR branch.

@futurechimp
Copy link
Author

Ok, all squashed in place now, sorry for the noise.

Comment on lines 123 to 130
// Format an output which spends some of the funds in the vault
let txout = TxOut {
script_pubkey: emergency_wallet
.next_unused_address(KeychainKind::External)
.script_pubkey(),
value: Amount::from_sat(750),
};
Copy link
Contributor

Choose a reason for hiding this comment

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

I realized we're not sending any change back to the vault, so in this case the majority of funds would be lost to fees. If we want to keep it to 1-input 1-output for simplicity we could just send all the original funds (76_000) minus some reasonable amount for fees.

Copy link
Author

Choose a reason for hiding this comment

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

Good point. I've added a 500 sat fee subtraction and sent the rest of the change back to the emergency_wallet's change address.

@futurechimp
Copy link
Author

It looks like the API for wallet.apply_update() has been changed in the past week, so this PR fails CI: https://github.com/bitcoindevkit/bdk/actions/runs/10577332007/job/29489915320?pr=1559

The old API took a graph in an Update struct:

pub struct Update {
    pub last_active_indices: BTreeMap<KeychainKind, u32>,
    pub graph: TxGraph<ConfirmationBlockTime>,
    pub chain: Option<CheckPoint>,
}

The new API is basically the same but has a tx_update in place of the graph:

pub struct Update {
    pub last_active_indices: BTreeMap<KeychainKind, u32>,
    pub tx_update: TxUpdate<ConfirmationBlockTime>,
    pub chain: Option<CheckPoint>,
}

I can't see how to get access to TxUpdate though, because that's now in the core crate, not the wallet crate, and I not seeing an import path which allows me to import the type.

Am I missing an easy way to get access to bdk_core from the wallet?

Or is there a way to easily change this for a fake wallet funding transaction so that it doesn't depend on graph/tx_update?

fn deposit_transaction(wallet: &mut Wallet, tx: Transaction) -> OutPoint {
    use bdk_chain::{ConfirmationBlockTime, TxGraph};
    use bdk_wallet::Update;

    let txid = tx.compute_txid();
    let vout = 0;
    let mut graph = TxGraph::<ConfirmationBlockTime>::new([tx]);
    let _ = graph.insert_seen_at(txid, 42);
    wallet
        .apply_update(Update {
            graph,
            ..Default::default()
        })
        .unwrap();

    OutPoint { txid, vout }
}

@ValuedMammal
Copy link
Contributor

You could try rebasing on current master to pick up the bdk_core changes and then import TxUpdate via bdk_chain crate, so use bdk_chain::TxUpdate then call into on a TxGraph.

Note after #1425 you might also need to add #![allow(clippy::print_stdout)] to the file descriptor_with_plan.rs

@futurechimp
Copy link
Author

futurechimp commented Oct 10, 2024

I've synced my branch with the bdk master and fixed compilation on top of that. Should be ready for review now.

@futurechimp futurechimp reopened this Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
Status: Needs Review
Development

Successfully merging this pull request may close these issues.

4 participants