Separate receive::{v1,v2} error modules #482

DanGould · 2025-01-14T18:54:09Z

This big move isolates v1/v2 module-specific error variants into clean module v1/v2 error modules.

First commit renames the crate::receive::Error variants to be module agnostic. Before they were v1, http response-specific. Reviewers: What do you think about the Validation and External variant names? (part of Review usage of crate::receive::Error #312)
Second commit roughly separates the modules
Third commit unifies some variants that lived in module-specific error types but were actually shared
Fourth commit recognizes unifies v2-module-specific error variant from the remaining RequestError stub
Fifth commit properly scopes the crate::receive::Error variant to its own module, and I think that may resolve Review usage of crate::receive::Error #312

There are probably too many impl<From> for error variants, but I think the straight path forward is addressing the structure first with this PR and then 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

coveralls · 2025-01-14T18:57:17Z

Pull Request Test Coverage Report for Build 12814297177

Details

48 of 188 (25.53%) changed or added relevant lines in 7 files are covered.
2 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-0.3%) to 60.489%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin/src/receive/v2/error.rs	2	9	22.22%
payjoin/src/receive/v1/mod.rs	16	25	64.0%
payjoin/src/receive/v2/mod.rs	13	22	59.09%
payjoin-cli/src/app/v2.rs	0	16	0.0%
payjoin-cli/src/app/v1.rs	0	27	0.0%
payjoin/src/receive/error.rs	17	53	32.08%
payjoin/src/receive/v1/error.rs	0	36	0.0%

Files with Coverage Reduction	New Missed Lines	%
payjoin/src/receive/error.rs	1	21.28%
payjoin-cli/src/app/v1.rs	1	0.0%

Totals
Change from base Build 12795849419:	-0.3%
Covered Lines:	2947
Relevant Lines:	4872

💛 - Coveralls

spacebear21

This all seems very well thought-out. The only thing I'd add to this PR is docstrings for all touched Error types and their variants, so that your thought process is at least partially captured in source (e.g. what distinguishes a PayloadError from a RequestError).

spacebear21 · 2025-01-15T15:00:24Z

payjoin/src/receive/error.rs

+pub enum ValidationError {
+    Payload(PayloadError),
+    V1(v1::RequestError),
+    #[cfg(feature = "v2")]
+    V2(v2::SessionError),
+}


Can you add docstring descriptions for each variant here and in other error enums where they're missing (e.g. v1::InternalRequestError)? It would also be good to document the enums themselves.

Even if things might change in follow-up PRs, I think adding descriptions now forces us to think thoroughly about naming, where each type belongs, etc.

Describe the variants based on their source, not the response they're supposed to produce in the v1 protocol, which is what they were set to do before this change.

This is the big move, but it doesn't completely clean up the variants into clean module fault lines. It chunks the work into submodules but still has some leaky abstractions. The next commits will try to draw cleaner lines.

Unify error variants produced by both v1 and v2 state machines but handled in different paths. Deduplicate the code to handle them together.

The v2-specific error types in the receive module deserve their own type refleting those types in v1. I do not believe they are yet fully handled in payjoin-cli so its error handling will need to be audited.

It is not a generic payjoin error. It's receive-specific.

A `_ => None` catchall could cause new error variants to be overlooked.

Functions should return the least abstract appropriate error which is InternalRequestError in this case.

DanGould · 2025-01-16T01:34:11Z

I added docstrings for all touched Error types and their variants, which was a great idea and made me reconsider names in some places.

receive::Error::External became receive::Error::Implementation because it didn't make sense to refer to it as External when working on an implementation. When your point of reference is external to the payjoin crate, the payjoin crate is external to you.

it also uncovered that some of the PayloadError types is not like the others: InternalPayloadError::FeeTooHigh only arises from receiver preferences, not from the Original PSBT Payload being invalid afaict. InternalPayloadError::PsbtBelowFeeRate is similar, in that it's a before-unexpressed preference problem rather (feerate isn't in the fragment params, for example) than a pure protocol validation problem.

So is InternalPayloadError::InputWeight which I believe will only get triggered by improper receiver input, so perhaps that data should be validated in contribute_inputs and then ignored by the point we get to the apply_fee state machine.

Either way both errors should make their way into the top level receive::Error to be returned to the Sender but they might belong in their own variants. I'd like to handle these in a follow up PR which propagats JSON errors more beautifully than is done now. (Or even split it into 2 prs, because this one is getting a bit large)

I made a couple other relevant changes in this push because I couldn't resist and they seemed non-controversial

Specify explicit PayloadError::source() variants, explained in the commit, might miss new error variants if it uses _ to catch them all
psbt_fee_rate is internal and should thus return its most specific error.

spacebear21

Amazing, glad that suggestion helped! These error modules are starting to look S-tier.

I noticed one more spot where we use a catch-all in Error::source but otherwise this looks ready for merge.

spacebear21 · 2025-01-16T17:10:30Z

payjoin/src/receive/v1/error.rs

+            InternalRequestError::Psbt(e) => Some(e),
+            InternalRequestError::Io(e) => Some(e),
+            InternalRequestError::InvalidContentLength(e) => Some(e),
+            _ => None,


Similar to your comment in bfd6f98 we should specify explicit variants here to not overlook new variants accidentally.

specified 🫡

A `_` catchall could cause new error variants to be overlooked.

spacebear21

ACK 44cbf91

DanGould force-pushed the sep-receive-errors branch 4 times, most recently from d28133e to e002ea2 Compare January 15, 2025 00:02

DanGould requested a review from spacebear21 January 15, 2025 00:08

DanGould mentioned this pull request Jan 15, 2025

Rework receive::v2, send::v2 submodule into additive crate::v2 and crate::v1 submodules features #392

Closed

5 tasks

DanGould changed the title ~~Initate receive v1,v2 error separation~~ Separate receive::{v1,v2} error modules Jan 15, 2025

DanGould marked this pull request as ready for review January 15, 2025 00:10

spacebear21 requested changes Jan 15, 2025

View reviewed changes

DanGould added 7 commits January 15, 2025 15:14

Rename receive::Error variants based on source

1c101cd

Describe the variants based on their source, not the response they're supposed to produce in the v1 protocol, which is what they were set to do before this change.

Separate receive::{v1,v2} error modules

f6f1281

This is the big move, but it doesn't completely clean up the variants into clean module fault lines. It chunks the work into submodules but still has some leaky abstractions. The next commits will try to draw cleaner lines.

Unify PayloadError::{ParsePsbt,Base64,Utf8} var'ts

bc3d5df

Unify error variants produced by both v1 and v2 state machines but handled in different paths. Deduplicate the code to handle them together.

Make SessionError the v2-specific module error

70e71bd

The v2-specific error types in the receive module deserve their own type refleting those types in v1. I do not believe they are yet fully handled in payjoin-cli so its error handling will need to be audited.

Expose receive::Error only in receive module

8bc8927

It is not a generic payjoin error. It's receive-specific.

Specify explicit PayloadError::source() variants

bfd6f98

A `_ => None` catchall could cause new error variants to be overlooked.

Return internal error from private psbt_fee_rate

ec4cd31

Functions should return the least abstract appropriate error which is InternalRequestError in this case.

DanGould force-pushed the sep-receive-errors branch from e002ea2 to ec4cd31 Compare January 16, 2025 01:20

DanGould requested a review from spacebear21 January 16, 2025 01:34

spacebear21 approved these changes Jan 16, 2025

View reviewed changes

Specify explicit RequestError::source() variants

44cbf91

A `_` catchall could cause new error variants to be overlooked.

spacebear21 approved these changes Jan 16, 2025

View reviewed changes

DanGould merged commit e439b69 into payjoin:master Jan 16, 2025
6 checks passed

DanGould deleted the sep-receive-errors branch January 16, 2025 17:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate receive::{v1,v2} error modules #482

Separate receive::{v1,v2} error modules #482

DanGould commented Jan 14, 2025 •

edited

Loading

coveralls commented Jan 14, 2025 •

edited

Loading

spacebear21 left a comment

spacebear21 Jan 15, 2025

DanGould commented Jan 16, 2025

spacebear21 left a comment

spacebear21 Jan 16, 2025

DanGould Jan 16, 2025

spacebear21 left a comment

Separate receive::{v1,v2} error modules #482

Separate receive::{v1,v2} error modules #482

Conversation

DanGould commented Jan 14, 2025 • edited Loading

coveralls commented Jan 14, 2025 • edited Loading

Pull Request Test Coverage Report for Build 12814297177

Details

💛 - Coveralls

spacebear21 left a comment

Choose a reason for hiding this comment

spacebear21 Jan 15, 2025

Choose a reason for hiding this comment

DanGould commented Jan 16, 2025

spacebear21 left a comment

Choose a reason for hiding this comment

spacebear21 Jan 16, 2025

Choose a reason for hiding this comment

DanGould Jan 16, 2025

Choose a reason for hiding this comment

spacebear21 left a comment

Choose a reason for hiding this comment

DanGould commented Jan 14, 2025 •

edited

Loading

coveralls commented Jan 14, 2025 •

edited

Loading