Skip to content

Conversation

mehmetefeumit
Copy link

Description

This PR implements Payjoin sending functionality to the BDK CLI.

Draft Explanation

At the time of writing, this is a draft PR for implementing SendPayjoin to the BDK CLI. It still needs session persistence to be implemented, but I've been focused on the Payjoin receiver and sender commands so that I will work on later.

I also have a ReceivePayjoin branch on my local, but I am currently stuck at signing for both Sender and Receiver, so I am using this draft PR to get help regarding signing.

Regarding the current version of the PR, I need help with signing the original PSBT.

Notes to the reviewers

A couple of things I'd need more recommendation on:

  1. Is the current way of handling the errors (through mapping them to a generic BDKCLI error) the best way to handle it? It makes the code quite verbose, but I was not able to find a generic Payjoin error type to add to the BDKCLIError enum so that I can just ? throughout the code.
  2. I cannot get the signing for the original PSBT right. I am currently testing through a regtest on my local, and using the payjoin-cli for the receiver end. When I generate the URI with payjoin-cli receive and use that in the sending logic I've implemented here, the receiver returns the following. This does not have any witness data at all, and the finalized does return false. I currently cannot understand why it cannot sign the UTXOs owned by this wallet though...:
02000000014df3998e42e1fc60baa75a3bd16458d15d8842f7409c42c2d7498d1b8e945f590000000000fdffffff0210270000000000001600144fe208a8c68474e8d147804bface77057fc5ecdf63d10295000000001600149ef197c3deb0a655c92df6964306aeb96a1c991ef9010000
Error: Replied with error: Can't broadcast. PSBT rejected by mempool.

Changelog notice

I'm leaving the checklist to later since this is a draft PR for the purpose of asking for assistance.

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
  • I've updated CHANGELOG.md

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

@DanGould
Copy link

I love seeing this advance!

Error Handling

Much like how rust-bitcoin does not have a single generic error type because there are multiple errors to handle differently, rust-bitcoin does not have a generic error type. There are many errors that may need different handling. Some are recoverable and won't need to be printed. Some might be terminal. Our next update will make this easier, but for now, the payjoin-cli reference implementation is even incomplete with regard to error handling. If you want to create an single enum with variants for each of the payjoin error types, that could work. I'm not sure what BDK-CLI's error handling strategy is.

I'd want to tap another BDK-CLI contributor to figure out what the preference for error handling for this project is.

Receiver Signing

In order to sign, I think you'll need to re-introduce the UTXO data to the PSBT so BDK can figure out what keys go to each input since that data was stripped by the receiver. You can see that I faced a similar issue, and how I resolved it using BDK 1.0 alpha in Mutiny way back. I did the same with BitMask. This is a limitation of BDK.

https://github.com/MutinyWallet/mutiny-node/pull/647/files#diff-c9658b4aca938566e0b3f07bb42c10b33aa59c479c6e174d5cf840f6432bcd1aR489-R496

@coveralls
Copy link

Pull Request Test Coverage Report for Build 15279732847

Details

  • 0 of 91 (0.0%) changed or added relevant lines in 3 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.2%) to 2.461%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/commands.rs 0 2 0.0%
src/utils.rs 0 7 0.0%
src/handlers.rs 0 82 0.0%
Files with Coverage Reduction New Missed Lines %
src/handlers.rs 1 3.34%
Totals Coverage Status
Change from base Build 15127343813: -0.2%
Covered Lines: 25
Relevant Lines: 1016

💛 - Coveralls

@notmandatory notmandatory added the enhancement New feature or request label May 28, 2025
@notmandatory notmandatory mentioned this pull request May 28, 2025
@DanGould DanGould moved this from Backlog to In Progress in Payjoin Roadmap 📝 May 28, 2025
@mehmetefeumit mehmetefeumit deleted the payjoin-send branch June 3, 2025 16:04
@github-project-automation github-project-automation bot moved this from In Progress to Done in Payjoin Roadmap 📝 Jun 3, 2025
@github-project-automation github-project-automation bot moved this from In Progress to Done in BDK-CLI Jun 3, 2025
@mehmetefeumit mehmetefeumit restored the payjoin-send branch June 3, 2025 16:04
@mehmetefeumit mehmetefeumit deleted the payjoin-send branch June 3, 2025 16:04
@mehmetefeumit mehmetefeumit restored the payjoin-send branch June 3, 2025 16:04
@mehmetefeumit mehmetefeumit deleted the payjoin-send branch June 3, 2025 16:04
@mehmetefeumit
Copy link
Author

Accidentally closed. Please see the finalized PR: #200

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

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants