Stabilize 1.0 errors by carving out errors used in state machine control flow & keeping the rest opaque#1602
Merged
Merged
Conversation
Collaborator
Coverage Report for CI Build 27028790564Coverage decreased (-0.04%) to 85.376%Details
Uncovered Changes
Coverage Regressions2 previously-covered lines in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
Persisted-session callers that catch expiry on SessionError, CreateRequestError, and ReplayError could only detect it by matching on the Display string, since the Expired variant of each internal error is hidden behind the opaque public type. Add a payload-free is_expired() predicate to each, mapped from the internal Expired discriminant. This gives callers a stable signal for the one lifecycle condition they branch on, without widening the public error surface. Also add is_expired() to the top-level receive::Error, true only when it wraps an expired v2 SessionError, so receiver callers holding the aggregate error can branch on expiry without unwrapping it.
Bullbitcoin detects session expiry across the binding errors it catches (SessionError, CreateRequestError, SenderReplayError, ReceiverReplayError, and the top-level ReceiverError) by string-matching the Display output, which is brittle. Add an is_expired() accessor to each uniffi Object that delegates to the core predicate. uniffi attaches methods to Object types, not to error enums, so the top-level ReceiverError enum instead gets a receiver_error_is_expired() free function over the caught error. Bullbitcoin can drop its _isExpiredString check at every site.
Senders could read a BIP-78 well-known error code only by parsing the Display string, because ErrorCode was pub(crate) and WellKnownError kept its code field private. Re-export ErrorCode publicly from the send module and add a WellKnownError::code() accessor that returns it, leaving supported_versions pub(crate). Mark ErrorCode and ResponseError non_exhaustive to reserve room for new variants; this is an intended, pre-1.0 downstream-breaking change that locks the surface now. Marking ResponseError non_exhaustive forces payjoin-ffi's exhaustive match to gain a fallback arm, which maps any future variant to an unrecognized error.
Senders using the bindings could only read a BIP-78 well-known error code by parsing the Display string of WellKnownError. With the core WellKnownError::code() accessor now public, mirror it across the FFI boundary. Add a uniffi ErrorCode enum and a WellKnownError::code() method that maps the core code into it. The enum carries an Unrecognized catch-all so a future core code maps cleanly while the binding catches up. uniffi exports methods on Object types, so the accessor reaches the bindings.
When resuming sessions, a replay that fails because the session expired is a normal lifecycle end, not a fault. Branch on the typed ReplayError::is_expired() predicate so an expired session is reported as expired and closed quietly, instead of being logged as an error and printed as a replay failure.
create_poll_request and create_post_request returned the broad receive::Error, while the sender's equivalents return a narrow CreateRequestError. Callers branching on expiry had to use the top-level receive::Error::is_expired predicate (or, over FFI, a receiver_error_is_expired free function), since an error enum cannot carry a method cleanly across the binding boundary. Add receive::v2::CreateRequestError, symmetric to the sender's: an opaque type over a pub(crate) enum holding only the constructors' real failure modes (URL parse, HPKE, OHTTP encapsulation, expiry), with an is_expired() method. Return it from both create_poll_request and create_post_request. full_relay_url now yields the neutral into_url::Error so each caller converts into its own error type. Over FFI, expose it as the uniffi Object ReceiverCreateRequestError with an is_expired() method. Drop the now-redundant receive::Error::is_expired and the receiver_error_is_expired free function it backed; SessionError and ReplayError keep is_expired for the process_response and replay paths. With create-request no longer producing the broad receive::Error, the From<HpkeError>, From<OhttpEncapsulationError>, and From<InternalSessionError> conversions into it are now unused; remove them and the ProtocolError import they required.
The public error returned by coin-selection paths was named SelectionError, which is ambiguous in a wallet context. Rename it (and its pub(crate) InternalSelectionError) to CoinSelectionError to name what it is, before the 1.0 surface locks the name. Requested on payjoin#403.
DanGould
marked this pull request as ready for review
June 5, 2026 18:23
Member
Author
|
Last commit was a specific spacebear request related to 403 |
This was referenced Jul 15, 2026
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Close #403
Keep external error types opaque, with one exception: a minimal caller-switchable carve-out for persisted-session lifecycle errors, propagated through payjoin-ffi
ErrorCode#[non_exhaustive]+WellKnownError::code()+ResponseError#[non_exhaustive]. supported_versions stays private (surfaced, not negotiated by protocol. Payload format already used to distinguish v1/v2).Rationale
See decision in #403 (comment) pasted for convenience:
Everything else should follow the advice I got from Tobin
rust-bitcoin's "commit to nothing" error design preference fits our leaf errors (library internalizes the BIP-78 reply + HasReplyableError typestate) but not the persisted-session state machine where retry/abort/restart is caller policy.
Tested by ensuring BB impl can drop
_isExpiredString; Cake drops recoverable/nrecoverable; sender readscode().Deviations for review
ResponseError#[non_exhaustive]needs anUnrecognizedfallback arm in the ffi From to keep each commit compiling.resume_payjoins branches on e.is_expired() so an expired session prints "Session expired" and closes quietly instead of being logged as a "failed to replay" error just to show it's workin'
Disclosure: co-authored by Claude Code
Pull Request Checklist
Please confirm the following before requesting review:
AI
in the body of this PR.