Abstract send feature error variants into separate feature-specific abstractions#464
Conversation
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 |
spacebear21
left a comment
spacebear21
left a comment
There was a problem hiding this comment.
tACK 8b8f399
Some very minor observations below.
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:
| Proposal(e) => write!(f, "Proposal PSBT error: {}", e), | |
| Proposal(e) => write!(f, "proposal PSBT error: {}", e), |
payjoin/src/send/v2/error.rs
Outdated
| 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:
| 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. |
3 participants
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.