Handle receiver contribution errors as unavailable#534
Merged
spacebear21 merged 2 commits intopayjoin:masterfrom Feb 16, 2025
Merged
Conversation
DanGould
force-pushed
the
handle-recv-contribution-err-as-unavailable
branch
from
b0c2150 to
16f73d6
Compare
spacebear21
reviewed
Feb 12, 2025
payjoin-cli/src/app/v1.rs
Outdated
| payjoin: payjoin::receive::v1::WantsInputs, | ||
| bitcoind: &bitcoincore_rpc::Client, | ||
| ) -> Result<payjoin::receive::v1::ProvisionalProposal> { | ||
| ) -> Result<payjoin::receive::v1::WantsInputs, ImplementationError> { |
Collaborator
There was a problem hiding this comment.
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)?;
Contributor
Author
There was a problem hiding this comment.
This was vestigial from my original de-duplication but I was doing too much. Took your suggestion in the last commit by removing the deduplication commit.
spacebear21
reviewed
Feb 12, 2025
payjoin-cli/src/app/v1.rs
Outdated
| log::warn!("Failed to contribute inputs: {}", e); | ||
| payjoin.commit_inputs() | ||
| }); | ||
| .map_err(|e| ReplyableError::Implementation(e.into()))? |
Collaborator
There was a problem hiding this comment.
Can we use:
Suggested change
| .map_err(|e| ReplyableError::Implementation(e.into()))? | |
| .map_err(ReplyableError::Implementation)? |
here too?
Collaborator
Pull Request Test Coverage Report for Build 13350980975Details
💛 - Coveralls |
DanGould
force-pushed
the
handle-recv-contribution-err-as-unavailable
branch
from
16f73d6 to
befba9a
Compare
If input selection or contribution fails, either broadcasting the original PSBT or returning `unavailable` error code is possible. 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
force-pushed
the
handle-recv-contribution-err-as-unavailable
branch
from
befba9a to
16f4bbb
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
ContributionErroris correct and to mark errors as non-exhaustive, and potentially expose variants as public.