Skip to content

Commit 0acbd95

Browse files
agentboxagentbox
agentbox
authored and
agentbox
committed
Add audit documentation for error type issues
Document findings from issue payjoin#749 audit of SessionError, CreateRequestError, and DecapsulationError (formerly EncapsulationError): - Variant duplication across send and receive error types - Naming inconsistencies (Url vs ParseUrl) - Hpke variant placement concerns - process_response return type asymmetry Items requiring maintainer design decisions are cataloged separately from the implemented rename fix.
1 parent 0dee49b commit 0acbd95

File tree

2 files changed

+193
-0
lines changed

2 files changed

+193
-0
lines changed

‎PULL-REQUEST.md‎

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
# Audit and rename error types for issue #749
2+
3+
## Summary
4+
5+
- Rename `EncapsulationError` to `DecapsulationError` (and internal types)
6+
- Document duplication and naming issues across `SessionError`,
7+
`CreateRequestError`, and the former `EncapsulationError`
8+
9+
## What changed
10+
11+
`EncapsulationError` was a misnomer: it is exclusively raised when
12+
_decapsulating_ responses (`process_response`), never during request
13+
_encapsulation_ (`create_v2_post_request` / `create_poll_request`, which
14+
produce `CreateRequestError`). Renamed to `DecapsulationError` throughout
15+
core and FFI crates, including the `InternalValidationError::V2Encapsulation`
16+
variant (now `V2Decapsulation`).
17+
18+
## Audit findings
19+
20+
### Error type inventory
21+
22+
| Type | Location | Variants |
23+
| --------------------------------------------------------------- | ---------- | ------------------------------------------------------------------------ |
24+
| `InternalCreateRequestError` | send v2 | `Url`, `Hpke`, `OhttpEncapsulation`, `Expired` |
25+
| `InternalDecapsulationError` (was `InternalEncapsulationError`) | send v2 | `Hpke`, `DirectoryResponse` |
26+
| `InternalSessionError` | receive v2 | `ParseUrl`, `Expired`, `OhttpEncapsulation`, `Hpke`, `DirectoryResponse` |
27+
28+
### Duplication between send and receive error types
29+
30+
`InternalSessionError` is a superset containing all variants from both
31+
`InternalCreateRequestError` and `InternalDecapsulationError`:
32+
33+
| Variant | CreateRequest | Decapsulation | Session |
34+
| ------------------ | :-----------: | :-----------: | :------: |
35+
| Url / ParseUrl | Url | - | ParseUrl |
36+
| Hpke | x | x | x |
37+
| OhttpEncapsulation | x | - | x |
38+
| Expired | x | - | x |
39+
| DirectoryResponse | - | x | x |
40+
41+
### Issues requiring maintainer decisions
42+
43+
1. **Cross-side deduplication**: `InternalCreateRequestError` and
44+
`InternalSessionError` have nearly identical variant sets but serve
45+
different sides of the protocol (send vs receive). Collapsing them
46+
requires architectural decisions about shared error types.
47+
48+
2. **Decapsulation/Session subset**: `InternalDecapsulationError` is a
49+
strict subset of `InternalSessionError` (`Hpke` + `DirectoryResponse`).
50+
Could potentially reuse a shared type.
51+
52+
3. **Inconsistent variant naming**: `Url` (send) vs `ParseUrl` (receive)
53+
for the same semantic concept (`into_url::Error`).
54+
55+
4. **Hpke variant placement**: The `Hpke` variant in `DecapsulationError`
56+
represents HPKE decryption (a distinct crypto operation from OHTTP
57+
decapsulation). May belong at a different level.
58+
59+
5. **process_response return type asymmetry**: Send POST returns
60+
`DecapsulationError`, send GET returns `ResponseError`. The asymmetry
61+
may be intentional (POST only confirms acceptance; GET retrieves and
62+
validates a full proposal).
63+
64+
## Test plan
65+
66+
- [x] `cargo clippy --all-targets --keep-going --all-features -- -D warnings`
67+
- [x] `cargo fmt --all -- --check`
68+
- [x] `codespell`
69+
- [x] `nix fmt -- --ci`
70+
71+
Disclosure: co-authored by Claude

‎STATUS.md‎

Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,122 @@
1+
# Status: Issue #749
2+
3+
## Task
4+
5+
Audit error types for duplication and naming issues
6+
7+
## Progress
8+
9+
- [x] Understanding complete
10+
- [x] Audit documented
11+
- [x] Targeted fixes implemented
12+
- [x] Pre-commit checks passing
13+
- [x] All changes committed
14+
15+
## Audit Findings
16+
17+
### Error Type Inventory
18+
19+
**`InternalCreateRequestError`** (send-side, `payjoin/src/core/send/v2/error.rs:15`):
20+
21+
- `Url(crate::into_url::Error)`
22+
- `Hpke(crate::hpke::HpkeError)`
23+
- `OhttpEncapsulation(crate::ohttp::OhttpEncapsulationError)`
24+
- `Expired(Time)`
25+
26+
**`InternalEncapsulationError`** (send-side, `payjoin/src/core/send/v2/error.rs:63`):
27+
28+
- `Hpke(crate::hpke::HpkeError)`
29+
- `DirectoryResponse(DirectoryResponseError)`
30+
31+
**`InternalSessionError`** (receive-side, `payjoin/src/core/receive/v2/error.rs:26`):
32+
33+
- `ParseUrl(crate::into_url::Error)`
34+
- `Expired(Time)`
35+
- `OhttpEncapsulation(OhttpEncapsulationError)`
36+
- `Hpke(HpkeError)`
37+
- `DirectoryResponse(DirectoryResponseError)`
38+
39+
### Duplication: InternalEncapsulationError vs InternalSessionError
40+
41+
| Variant | InternalEncapsulationError | InternalSessionError |
42+
| ----------------------------------------- | -------------------------- | -------------------- |
43+
| Hpke(HpkeError) | YES | YES |
44+
| DirectoryResponse(DirectoryResponseError) | YES | YES |
45+
46+
`InternalEncapsulationError` is a strict subset of `InternalSessionError`.
47+
48+
### Duplication: InternalCreateRequestError vs InternalSessionError
49+
50+
| Variant | InternalCreateRequestError | InternalSessionError |
51+
| ------------------------------------------- | -------------------------- | -------------------- |
52+
| Url / ParseUrl (into_url::Error) | `Url` | `ParseUrl` |
53+
| Hpke(HpkeError) | YES | YES |
54+
| OhttpEncapsulation(OhttpEncapsulationError) | YES | YES |
55+
| Expired(Time) | YES | YES |
56+
57+
`InternalCreateRequestError` maps 1:1 to `InternalSessionError` (with the
58+
naming difference of `Url` vs `ParseUrl`). `InternalSessionError` has one
59+
extra variant: `DirectoryResponse`.
60+
61+
### Naming Issues
62+
63+
1. **EncapsulationError is a misnomer**: Used exclusively in `process_response`
64+
methods (decapsulating responses), never during encapsulation (creating
65+
requests, where CreateRequestError is used).
66+
2. **Hpke variant placement**: In `InternalEncapsulationError`, the `Hpke`
67+
variant represents HPKE _decryption_ failure (`decrypt_message_b`), which
68+
is a distinct crypto operation from OHTTP en/decapsulation.
69+
3. **Inconsistent variant naming**: `Url` (send) vs `ParseUrl` (receive) for
70+
the same semantic concept.
71+
72+
### Return Type Inconsistency
73+
74+
- Send POST `process_response` → `EncapsulationError`
75+
- Send GET `process_response` → `ResponseError`
76+
- Receive `process_response`-like methods → `SessionError` or `Error`
77+
78+
The POST/GET asymmetry may be intentional (POST only confirms acceptance;
79+
GET retrieves and validates a proposal). The receive side is different because
80+
errors may need to be returned to the sender as JSON.
81+
82+
### Classification
83+
84+
**CLEAR FIX:**
85+
86+
1. Rename `EncapsulationError` → `DecapsulationError` (and internal variant)
87+
88+
**NEEDS DECISION (documented, not fixed):**
89+
90+
1. Deduplicating InternalCreateRequestError / InternalSessionError — they serve
91+
different sides (send vs receive) and collapsing requires architectural
92+
decisions about shared error types.
93+
2. Deduplicating InternalEncapsulationError / InternalSessionError — strict
94+
subset but different roles (send response processing vs receive session).
95+
3. Standardizing Url vs ParseUrl naming across send/receive.
96+
4. Whether to move the Hpke variant out of EncapsulationError/DecapsulationError
97+
since HPKE decryption is distinct from OHTTP decapsulation.
98+
5. Unifying process_response return types across POST/GET/receive.
99+
100+
## Decisions
101+
102+
- Rename EncapsulationError → DecapsulationError: clear-cut fix, only
103+
used during response decapsulation. ~15 sites to update across core +
104+
FFI, manageable scope.
105+
- Not deduplicating cross-side error types: too architecturally entangled
106+
for a targeted fix; would require maintainer design input.
107+
108+
## Questions
109+
110+
- Should InternalCreateRequestError and InternalSessionError share a common
111+
type or trait? Both represent "session operation failed" with nearly
112+
identical variant sets.
113+
- Should Hpke be a separate error variant at a higher level rather than
114+
being embedded in en/decapsulation error types?
115+
116+
## Commits
117+
118+
- `0dee49bd` — Rename EncapsulationError to DecapsulationError
119+
Renamed EncapsulationError → DecapsulationError across core crate
120+
(send/v2/error.rs, send/v2/mod.rs, send/error.rs) and FFI crate
121+
(send/error.rs, send/mod.rs). Updated V2Encapsulation variant to
122+
V2Decapsulation and fixed display string.

0 commit comments

Comments
 (0)