Abstract send feature error variants into separate feature-specific abstractions #464
Conversation
This file contains 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
Pull Request Test Coverage Report for Build 12644657849Details
💛 - Coveralls |
|
Until we address #403, it seems difficult to test error variants in integration tests under protocol runs, assuming that's how it should be done, since there is no way for the public interface to access error variants. One way to enable that testing would be #425, though it makes more sense to me to aim straight at error stabilization |
5 tasks
spacebear21
approved these changes
Jan 7, 2025
payjoin/src/send/error.rs
Outdated
| match &self.internal { | ||
| Parse => write!(f, "couldn't decode as PSBT or JSON",), | ||
| Io(e) => write!(f, "couldn't read PSBT: {}", e), | ||
| Proposal(e) => write!(f, "Proposal PSBT error: {}", e), |
There was a problem hiding this comment.
meganit:
Suggested change
| Proposal(e) => write!(f, "Proposal PSBT error: {}", e), | |
| Proposal(e) => write!(f, "proposal PSBT error: {}", e), |
payjoin/src/send/v2/error.rs
Outdated
Comment on lines
66
to
68
| pub struct EncapsulationError { | ||
| internal: InternalEncapsulationError, | ||
| } |
There was a problem hiding this comment.
In other places where we wrap an internal type we just use a tuple struct:
Suggested change
| pub struct EncapsulationError { | |
| internal: InternalEncapsulationError, | |
| } | |
| pub struct EncapsulationError(InternalEncapsulationError) |
DanGould
force-pushed
the
separate-send-versions-p2-v2
branch
from
8b8f399 to
e34302d
Compare
|
changed the nit and removed internal for bot the source of the design |
DanGould
force-pushed
the
separate-send-versions-p2-v2
branch
from
e34302d to
be7df15
Compare
These variants don't have to do with proposal validation. They are unique to payjoin v2 encapsulation and ought to be abstracted as such.
These variants all have to do with Proposal PSBT checks distinct from other encapsulation validation
This includes redundant documentation we don't want to get out of sync and the critical Unrecognized variant that requires special handling.
These errors are enumerated and not subject to the kinds of phishing attacks Unrecognized errors are. Downstream implementations may choose to display these in e.g. a CLI environment. Only the Unrecognized errors MUST be swallowed because their contents are defined by the counterparty.
Make explicit that this type is only used in the v1 state path. Yes, it can be used in the v2 path, but only if extract_v1 is called to shift onto the v1 path explicitly.
No need for them to be named
|
I also applied a similar nit to an EncapsulationError variant |
|
@nothingmuch I'd like to get your feedback on these error abstractions since we discussed it earlier. I want to make sure the points of separation make sense to you. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This reduces the feature flag coupling toward #392 and works toward #403.
It makes tiers of errors so that ResponseError has clear differences between its WellKnown (should displayable), Validation (may display), and Unknown (MUST NOT display) variants.
It separates v2-specific error definitions into the v2 module. And it isolates ProposalErrors that should only crop up when validating against Proposal PSBT state machine rules. the goal of this is so that we may stabalize and expose some of these error types in a payjoin-1.0 implementation.
I also edited some tests to match against error variants rather than strings. We ought to be able to do this with numerous error variants as they stabilize.