Apply 1.0 freeze-required API fixes (non_exhaustive, ShortIdError, OutputSubstitution, must_use)#1703
Draft
spacebear21 wants to merge 4 commits into
Draft
Apply 1.0 freeze-required API fixes (non_exhaustive, ShortIdError, OutputSubstitution, must_use)#1703spacebear21 wants to merge 4 commits into
spacebear21 wants to merge 4 commits into
Conversation
Adding a variant to a public exhaustive enum is a breaking change, so without #[non_exhaustive] the crate could not grow these enums after 1.0 without a 2.0. Several of them are explicitly expected to grow (the session event/state/status/outcome enums document planned additions). Mark the public exhaustive enums non-exhaustive: the receive/send session state, event, status, and outcome enums, ReceiveSession / SendSession, ProtocolError, IntoUrlError, UrlParseError, ShortIdError, Version, MaybePayjoinExtras, and the persist transition/outcome enums. payjoin-cli and payjoin-ffi match several of these, so add wildcard arms to keep the workspace compiling: graceful "unknown" fallbacks in the CLI (display and dispatch), and unreachable!() in the FFI's infallible mirror-conversion From impls, which are version-locked to payjoin.
ShortIdError is the error type of ShortId's TryFrom<&[u8]> and FromStr impls, but it derived only Debug. Without Display and std::error::Error it could not be `?`-propagated into Box<dyn Error> or anyhow, forcing callers to handle it specially. Implement Display and Error with source() chaining to the wrapped bech32 and slice-conversion errors.
The v2 API returns OutputSubstitution (Receiver<WantsOutputs>:: output_substitution and PayjoinExtras::output_substitution), but its crate-root re-export was gated on the v1 feature. Since v2 does not enable v1, default (v2-only) builds returned a value of a type callers could not name or import. Drop the v1 gate. The re-export lives inside the _core-gated core module, so it is now available whenever v1 or v2 is enabled.
Every state-machine transition returns a transition wrapper whose .save() both persists the session event and yields the next typestate. Dropping the wrapper without calling .save() silently loses the event and the state advance, with no diagnostic. Add #[must_use] to the transition types so that dropping one is a compiler warning.
Collaborator
Coverage Report for CI Build 28559155426Coverage decreased (-0.1%) to 85.616%Details
Uncovered Changes
Coverage Regressions2 previously-covered lines in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
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
2 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.
Summary
I ran a 1.0 API audit with Claude that surfaced these relatively low-hanging fruit issues that are best addressed before a 1.0 freeze.
Each commit is standalone:
state/event/status/outcome enums,
ReceiveSession/SendSession,ProtocolError,IntoUrlError,UrlParseError,ShortIdError,Version,MaybePayjoinExtras, and the persist transition/outcome enums. Adds therequired wildcard arms in
payjoin-cli(graceful "unknown" fallbacks indisplay and dispatch) and
payjoin-ffi(unreachable!()in the infalliblemirror-conversion
Fromimpls, which are version-locked topayjoin).Error+DisplayforShortIdErrorso it can be?-propagated — it is theErrtype ofShortId'sTryFrom/FromStrbutderived only
Debug.OutputSubstitutionunder v2 — it was gated on thev1feature,but v2 methods return it and v2 does not enable v1, so default (v2-only)
builds could not name the type.
#[must_use]so dropping a transitionwithout
.save()is a compiler warning instead of a silently lost step.Follows #1702 (bitcoin-ohttp / bitcoin-hpke insulation). Tracking: #976.
Disclosure: co-authored by Claude Code (Opus 4.8)
🤖 Generated with Claude Code