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.

Sign up for GitHub

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

Jump to bottom

Stabilize external error types #403

Open
spacebear21 opened this issue Nov 20, 2024 · 5 comments
Open

Stabilize external error types #403

spacebear21 opened this issue Nov 20, 2024 · 5 comments
Assignees
@DanGould
Labels
tech debt
Milestone
payjoin-1.0

Comments

@spacebear21
Copy link
Collaborator

Currently we use opaque type externally because we aren't sure which variants will stay. Eventually these should be transparent and we can remove the Internal variants.
@DanGould DanGould mentioned this issue Jan 2, 2025
Merged
DanGould added a commit that referenced this issue Jan 2, 2025
@DanGould
Separate send version modules (part 1) (#453)
592d1d2 
Expose descriptive, accurate send errors so that we can switch on
them in implementations and mark payjoin sessions in persistent
storage accurately. We only want to spawn sessions that are capable
of making forward progress and drop those that cannot.

Patch overview:
1. housekeeping
2. Introduce specific SenderBuilder errors
3. make CreateRequestError v2 only since v1 can only make have sender
builder errors
4. separate the modules to reduce feature flags

This separation is incomplete because I'm still uncertain what to do
with send::ValidationError's feature gated variants. Do I make a
send::ValidationError and a send::v2::ValidationError separate? How do
those get handled in ResponseError? Does ResponseError get split into
two versions, or do I just leave a feature gated variant? That's left
for the next PR predicated on this design being an appropriate one.

Note this pays back some tech debt but leaves some slop in payjoin-cli.
Rather than making this PR 10 commits to review I left combining FeeRate
parsing to a later PR (#452).

This error puts us on the path to #392 and #403
@DanGould DanGould added this to the payjoin-1.0 milestone Jan 3, 2025
@DanGould DanGould mentioned this issue Jan 4, 2025
Merged
DanGould added a commit that referenced this issue Jan 7, 2025
@DanGould
Abstract send feature error variants into separate feature-specific a…
1baa8a0 
…bstractions (#464)

This reduces the feature flag coupling toward #392 and works toward
#403.
@DanGould DanGould mentioned this issue Jan 14, 2025
Merged
DanGould added a commit that referenced this issue Jan 16, 2025
@DanGould
Separate receive::{v1,v2} error modules (#482)
e439b69 
This big move isolates v1/v2 module-specific error variants into clean
module v1/v2 error modules.

There are probably too many `impl<From>` for error variants, but this
addresses the module structure first with this PR and will be followed by
taking a magnifying glass to the way specific variants are handled.

This still also leaves `OutputSubstitutionError`, `SelectionError` and
`InputContributionError` from producing JSON error responses to cancel a
session with a v2 receiver. We're going to need to address that as part
of #403. v2 Error variants also improperly produce JSON errors, but I
think the way this was done by abusing `fmt::Display` is also an
antipattern to fix at the same time that's addressed. I'd like to leave
the JSON receiver error fixes to a follow up since what gets revealed is
potentially sensitive and we should be reviewing that change with extra
scrutiny.

See: #312, #392, #403
@DanGould DanGould mentioned this issue Jan 20, 2025
Closed
@DanGould
Copy link
Contributor

@DanGould DanGould mentioned this issue Jan 23, 2025
Merged
@DanGould DanGould self-assigned this Jan 23, 2025
spacebear21 added a commit that referenced this issue Jan 23, 2025
@spacebear21
Accomodate Updated BIP 78 Spec (#505)
e88f972 
See #480

`InternalInputContributionError` has only one error variant as a result
of this, which means it's a smell to clean up in #403

One thing I note is that the BIP says "Our recommendation for
<code>maxadditionalfeecontribution=</code> is <code>originalPSBTFeeRate
* 110</code>." and our actual use is `let recommended_additional_fee =
min_fee_rate * input_weight;` where input_weight is 110 only where mixed
inputs appear. I did not remove the `expected_input_weight` function for
a blanket 110 recommendation, which I believe is in line with actual
incentives to use a matching input. but I could go either way.
This was referenced Jan 23, 2025
Merged
Open
@DanGould DanGould mentioned this issue Feb 4, 2025
Merged
DanGould added a commit that referenced this issue Feb 10, 2025
@DanGould
Only derive JsonError for errors that can return Json (#526)
03c160a 
- 1st commit de-duplicates
- 2nd commit: Only some errors are actually recoverable and must return
JSON. Fix #522
- 3rd introduces an ImplementationError alias for downstream ease

Re: #403
@DanGould
Copy link
Contributor

DanGould commented Feb 11, 2025 •
edited
Loading

There are a few remaining receiver Error variants that can probably be handled as ReplyableError depending on the receiver's preferences.

SelectionError

SelectionError arises from receiver coin selection failing, when attempting to apply a certain algorithm like avoid_uih. This error should be preserved, in case a downstream implementation wants to do some more manual coin selection than we currently offer, but ultimately if it fails it must be convertable into an unavailable ReplyableError and potentially broadcast the original PSBT automatically.

The current implementation of payjoin-cli just proceeds without providing inputs if one of these errors arises, which seems like the only correct option if not converted to an Implementation Error variant and returning Unavailable.

OutputSubstitutionError

A downstream implementation should be able to try to move on with a simple consolidation to payjoin, and otherwise convert this error (or the one from failing to consolidate) into an unavailable from this variant

Right now this arises when we force new address substitution for demo purposes in the V1 payjoin-cli and return it as an Implementation error, which actually seems correct, since that produces an Unavailable error.

InputContributionError

This panics in payjoin-cli, though it must definitely be wrapped in an Implementation error and returned as Unavailable if it's really unavailable. Must not panic. I can see an iterative implementation where a receiver keeps trying to add inputs until this variant is not triggered, and then replying, and otherwise coercing the error into Implementation and replying as a ReplyableError.

@DanGould DanGould mentioned this issue Feb 12, 2025
Merged
@spacebear21
Copy link
Collaborator Author

While we're doing this, I'd like to see SelectionError renamed to CoinSelectionError for max clarity.

spacebear21 added a commit that referenced this issue Feb 16, 2025
@spacebear21
Handle receiver contribution errors as unavailable (#534)
39cfaff 
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](#403 (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.
@spacebear21
Copy link
Collaborator Author

Until we're sure we can remove the Internal types we may want to consider making those pub(crate) so that testers can check for specific error types.

@DanGould
Copy link
Contributor

DanGould commented Mar 4, 2025

  • When we make error variants public, remember to reflect the change in tests by matching variants instead of messages.

This was referenced Mar 4, 2025
Merged
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@DanGould DanGould

Labels
tech debt
Projects
Payjoin Roadmap 📝
Status: No status
Milestone
payjoin-1.0
Development

No branches or pull requests
2 participants
@DanGould @spacebear21