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 Wallet::get_psbt_input function and Descriptor::max_satisfaction_weight #362

Closed

Conversation

notmandatory
Copy link
Member

@notmandatory notmandatory commented Jun 1, 2023

Description

Add Wallet::get_psbt_input and Descriptor::max_satisfaction_weight functions and required structs and enums needed by #358.

Also added Input from_json constructor and json_serialize functions.

Notes to the reviewers

After the TxBuilder::add_foreign_utxo function is added the Input._inner field should be renamed to inner.

Changelog notice

  • Add Input, PsbtSighashType, EcdsaSighashType and SchnorrSighashType
  • Add Input from_json constructor and json_serialize functions
  • Add Wallet::get_psbt_input function
  • Add Descriptor::max_satisfaction_weight function

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

@notmandatory notmandatory self-assigned this Jun 1, 2023
@notmandatory notmandatory added the enhancement New feature or request label Jun 1, 2023
@notmandatory notmandatory changed the title Add Wallet::get_psbt_input function and required structs and enums Add Wallet::get_psbt_input function and Descriptor::max_satisfaction_weight Jun 1, 2023
@notmandatory notmandatory marked this pull request as ready for review June 1, 2023 02:41
@notmandatory notmandatory added this to the Release 0.29.0 milestone Jun 1, 2023
Copy link
Member

@thunderbiscuit thunderbiscuit left a comment

Choose a reason for hiding this comment

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

ACK 1e1b794. Only thing is it appears your commits are not signed anymore..?

/// for converting to/from [`PsbtSighashType`] from/to the desired signature hash type they need.
#[derive(Debug)]
pub(crate) struct PsbtSighashType {
inner: BdkPsbtSighashType,
Copy link
Member

Choose a reason for hiding this comment

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

I like the idea of using inner as the variable name, and wonder if we should eventually refactor other structs that use the same design pattern (simple wrappers over a BDK types).

@notmandatory notmandatory force-pushed the add_input_info branch 3 times, most recently from 58bd3bf to 5e0dfbf Compare June 7, 2023 18:25
@notmandatory
Copy link
Member Author

I fixed commit signatures, and added Input from_json constructor and json_serialize functions.

@thunderbiscuit
Copy link
Member

ACK 7b79f39 with some questions.

Is this JSON serialization/deserialisation deterministic? Like can someone arrive from an psbt input built from a different tool and exported in json and simply import it here? Or are there corner cases? I'm not clear enough on the Rust serialization system. It looks like its not done by the rust-bitcoin crate, but just the sort of "standard" serde crate (it's a derived method). Sorry these feel like beginner questions but I was thinking it'd be cool if we can make sure we add the methods that allow us to be compatible with other implementations. Just not clear on whether a input in JSON format coming from Sparrow (Java) or Blue (bitcoin-js) would just work here, or if it's just the "jsonization" of the Rust struct itself.

@notmandatory
Copy link
Member Author

As far as I could find there's no standard serialization format for just the PSBT Output, only the whole PSBT has a standard cross-implementation serialization format. So you have a good point that this JSON approach would not be compatible across other wallets. That makes me think this could be a dead-end approach and that we should encourage users to collaboratively add inputs and outputs by constructing PSBTs and using PartiallySignedTransaction.combine instead of Wallet.add_foreign_utxo, but I'm not sure if that solves the required use-cases or not.

https://docs.rs/bitcoin/0.29.2/bitcoin/util/psbt/struct.PartiallySignedTransaction.html#method.combine

@notmandatory
Copy link
Member Author

Closing for now, may bring it back later after we finish updating to BDK 1.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants