-
Couldn't load subscription status.
- Fork 83
Add experimental silent payment transaction creation support #220
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
base: master
Are you sure you want to change the base?
Conversation
…actions Add experimental silent payment sending capabilities with new CreateSpTx command. This command creates signed transactions directly rather than PSBTs due to current limitations in secure shared derivation. - Add bdk_sp dependency with "sp" feature flag - Implement CreateSpTx subcommand for offline wallet operations - Add silent payment recipient parsing utility - Support mixed recipients (regular addresses + silent payments) - Generate signed transactions ready for broadcast - For the moment is not possible to enable RBF for the created transactions. Note: This is experimental functionality for testing only, not recommended for mainnet use.
e0a13af to
bc40b76
Compare
There was a problem hiding this 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 adding this feature @nymius,
I couldn't test because I don't know how to construct sp recipients. I will check the documentation on the library and will come back to test it. In the meantime, I have left some comments.
Thank you
| } | ||
|
|
||
| if let Some(utxos) = utxos { | ||
| tx_builder.add_utxos(&utxos[..]).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use the Error enum from the error module to gracefully handle errors rather than unwrapping?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm reviewing this unwraps again. I was basing these changes in the CreateTx command, that has the same number of unwraps on the same places.
I can come up with a graceful error handling for this PR, but then we would need to open a new PR to update the other command.
Also, as I'm using the unwraps on the same places, then the Error variants should be as general as possible to reuse them in the CreateTx command, and then this PR wouldn't be doing a single thing, but two.
Any recommendations about how to proceed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I have noticed that the CreateTx command has unwrappings, which we have not updated after adding the Error enum. If you can come up with variants to handle both cases, that would be great. After you have handled the CreateSpTx case, you can add an extra commit to fix the case for CreateTx.
| } | ||
|
|
||
| #[cfg(feature = "sp")] | ||
| pub(crate) fn parse_sp_code_value_pairs(s: &str) -> Result<(SilentPaymentCode, u64), String> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can return the Error and use the Generic variant instead of String here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment than above, this function is following the style of parse_recipient. The error should be generic in terms of parsing or should be particular to silent payments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if your library is returning a specific error for this, you can add it to the enum or you can just use the generic variant
| // Clap Doesn't support complex vector parsing https://github.com/clap-rs/clap/issues/1704. | ||
| // Address and amount parsing is done at run time in handler function. | ||
| #[arg(env = "ADDRESS:SAT", long = "to", required = false, value_parser = parse_recipient)] | ||
| recipients: Option<Vec<(ScriptBuf, u64)>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a need to add these recipients again since the focus is on sp_recipients?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to show that's still possible to send a non silent payment output together with a silent payment output in the same transaction, that's why I left this here.
| recipients: Option<Vec<(ScriptBuf, u64)>>, | ||
| /// Parse silent payment recipients | ||
| #[arg(long = "to-sp", required = true, value_parser = parse_sp_code_value_pairs)] | ||
| silent_payment_recipients: Vec<(SilentPaymentCode, u64)>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how can one construct sp_recipients? can you add that to the readme?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each silent payment recipient is a string of the form <silent-payment-address>:<amount>, for example: sprt1qq0u4yswlkqx36shz7j8mwt335p4el5txc8tt6yny3dqewlw4rwdqkqewtzh728u7mzkne3uf0a35mzqlm0jf4q2kgc5aakq4d04a9l734u5ddn6e:1000 (this is a regtest address, because human readable prefix is sprt).
You cannot get addresses from your bdk-cli because I don't think the receiving side is mature enough yet, but;
SilentPaymentCode is prepared to decode any silent payment encoded address, so I recommend you to take a look in bdk-sp encoding tests to get some fake addresses from there.
If you want to create silent payment addresses dynamically you would have to compile sp-cli2, the cli implemented in bdk-sp, maybe that's too much to add to the README (to ask to compile another tool to get one), what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I think is that, if it is possible, you should add deriving SP addresses to this PR. I wanted to test how you implemented it in the CLI v2 but it broke. Else you can add a link to the test addresses so it is easy for users to find them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if we add the chances to derive addresses, then we are allowing the creation of transactions locking funds into those addresses. I could restrict this functionality to only derive testnet addresses, so no one lose any real funds.
I mean, is problematic to open the door to receive without implementing the whole functionality to scan for new outputs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, only testnet addresses are fine. We generally warn users against using this tool for mainnet.
Description
This PR adds experimental support for creating silent payment transactions through a new
CreateSpTxcommand. The implementation integrates thebdk_spcrate to enable sending Bitcoin to silent payment addresses.Key changes:
bdk_spdependency as an optional featureCreateSpTxcommand with support for silent payment recipientsaddress:amountpairsNotes to the reviewers
.expect()calls that should be addressed in future iterationsChangelog notice
Added: Experimental silent payment transaction creation via
CreateSpTxcommand (feature-gated behindspflag)Checklists
All Submissions
New Features
* [ ] I've added tests for the new feature