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

Handle receiver contribution errors as unavailable #534

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

DanGould
Copy link
Contributor

Our reference must not panic if input contribution fails, but instead show best practice for a non-automated receiver where the sender may choose to try to payjoin again or broadcast the transaction extracted from the original PSBT.

This is an implementation of my best idea for receiver error handling in my comment in #403

After this I think the only things to stabilize receiver errors is to decide whether the single-variant ContributionError is correct and to mark errors as non-exhaustive, and potentially expose variants as public.

contribute_inputs was called in two separate places, complicating error handling.
@DanGould DanGould force-pushed the handle-recv-contribution-err-as-unavailable branch from b0c2150 to 16f73d6 Compare February 12, 2025 19:20
If input selection or contribution fails, either broadcast the original PSBT or return
`unavailable`. Since payjoin-cli is a reference implementation of a non-automated receiver,
I prefer to return `unavailable` since the sender can choose to broadcast that original PSBT.
It makes less sense to me to return the original PSBT without modifications which will likely
cause the sender to broadcast the original despite no payjoin happening. Let the sender make
that choice.
@DanGould DanGould requested a review from spacebear21 February 12, 2025 19:47
@@ -394,21 +392,17 @@ impl App {
fn try_contributing_inputs(
payjoin: payjoin::receive::v1::WantsInputs,
bitcoind: &bitcoincore_rpc::Client,
) -> Result<payjoin::receive::v1::ProvisionalProposal> {
) -> Result<payjoin::receive::v1::WantsInputs, ImplementationError> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the reasoning for changing the return type to WantsInputs here? We could achieve the same result by changing the return value to:

    Ok(payjoin
        .contribute_inputs(vec![selected_input])
        .map_err(ImplementationError::from)?
        .commit_inputs())

and the caller to

        let provisional_payjoin = try_contributing_inputs(payjoin.clone(), &bitcoind)
            .map_err(ReplyableError::Implementation)?;

log::warn!("Failed to contribute inputs: {}", e);
payjoin.commit_inputs()
});
.map_err(|e| ReplyableError::Implementation(e.into()))?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we use:

Suggested change
.map_err(|e| ReplyableError::Implementation(e.into()))?
.map_err(ReplyableError::Implementation)?

here too?

@coveralls
Copy link
Collaborator

coveralls commented Feb 13, 2025

Pull Request Test Coverage Report for Build 13291189894

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 14 of 25 (56.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall first build on handle-recv-contribution-err-as-unavailable at 79.117%

Changes Missing Coverage Covered Lines Changed/Added Lines %
payjoin/src/receive/error.rs 0 11 0.0%
Totals Coverage Status
Change from base Build 13251464274: 79.1%
Covered Lines: 4016
Relevant Lines: 5076

💛 - Coveralls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants