Insulate bitcoin uri - #1756
Merged
Merged
Conversation
spacebear21
force-pushed
the
insulate-bitcoin-uri
branch
from
July 22, 2026 02:14
972f06d to
56b3d26
Compare
Collaborator
Coverage Report for CI Build 29973580452Coverage increased (+0.1%) to 86.49%Details
Uncovered Changes
Coverage Regressions1 previously-covered line in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
DanGould
reviewed
Jul 22, 2026
| /// | ||
| /// The foreign error is erased behind a boxed trait object so that it does | ||
| /// not appear in the public API, while preserving the `source()` chain. | ||
| Bip21(Box<dyn std::error::Error + Send + Sync>), |
Member
There was a problem hiding this comment.
Suggested change
| Bip21(Box<dyn std::error::Error + Send + Sync>), | |
| Bip21(bitcoin_uri::de::UriError), |
Because this is an internal variant in InternalUriParseError it doesn't need to be and shouldn't be boxed because boxing buys us nothing. Comment is not right.
Collaborator
Author
There was a problem hiding this comment.
You're absolutely right! just kidding but yes, fixed.
| pub(crate) fn from_err(err: impl std::fmt::Display) -> Self { Self { msg: err.to_string() } } | ||
| } | ||
| #[derive(Debug, thiserror::Error, uniffi::Object)] | ||
| #[uniffi::export(Debug, Display)] |
Member
There was a problem hiding this comment.
Any rationale for dropping Eq from this list?
Collaborator
Author
There was a problem hiding this comment.
Yes, added the rationale to the commit message: Eq cannot be derived anymore because the Bip21 variant of payjoin::UriParseError holds a bitcoin_uri::de::UriError, which doesn't implement Eq.
| /// parameters were invalid. | ||
| #[cfg(test)] | ||
| pub(crate) fn payjoin_params(&self) -> Option<&PjParseError> { | ||
| match &self.0 { |
Member
There was a problem hiding this comment.
I spent some time considering making this pub bc it prevents some hacks happening down stream. But I don't recomend it
Suggested change
| match &self.0 { | |
| pub fn pj_parse_error(&self) -> Option<&PjParseError> { |
Cove blind-retries today on any Uri::try_from failure it hand-strips pj/pjos and parses a second time (address.rs#L245) because one opaque error can't distinguish a bad address from bad pj params.
Rather than papering over here though, since they have something that ~works I think we're better just leaving what you have, a more minimal api, and doing a bitcoin_uri crate 0.2 overhaul sooner than later
spacebear21
force-pushed
the
insulate-bitcoin-uri
branch
from
July 23, 2026 01:27
56b3d26 to
5b6eb8a
Compare
spacebear21
marked this pull request as ready for review
July 23, 2026 01:31
Part of the 1.0 API hardening: remove the foreign bitcoin_uri types from payjoin's public API so that a breaking release of bitcoin_uri no longer forces a breaking release of payjoin. This follows the same approach as the bitcoin-ohttp and bitcoin-hpke insulation in payjoin#1702. Previously `Uri` and `PjUri` were type aliases to `bitcoin_uri::Uri`, so the wrapped type, its public fields (including `bitcoin_uri::Param` labels and messages), the `bitcoin_uri::de::Error` parse error, and the `bitcoin_uri::Uri` returned by `check_pj_supported` were all reachable through the public API. The parser traits were also implemented directly on the public `MaybePayjoinExtras` and `PayjoinExtras` types. The crate now only names `bitcoin_uri` in private newtype fields and on the private adapters. No `bitcoin_uri` type or trait impl is reachable through the public API.
The FFI `PjParseError` flattened the parse error into a `String` because `payjoin::Uri`'s parse error used to be the foreign `bitcoin_uri::de::Error`, whose type could not be carried across the FFI boundary. Now that parsing returns payjoin's own `UriParseError`, which implements `std::error::Error`, it can be wrapped directly with `#[error(transparent)]` and `#[from]`. As a result it is also renamed to `UriParseError` to match the core library naming convention. `Eq` cannot be derived anymore because the `Bip21` variant of `payjoin::UriParseError` holds a `bitcoin_uri::de::UriError`, which doesn't implement `Eq`.
spacebear21
force-pushed
the
insulate-bitcoin-uri
branch
from
July 23, 2026 02:07
5b6eb8a to
35a107c
Compare
DanGould
reviewed
Jul 23, 2026
DanGould
approved these changes
Jul 23, 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.
Follow up to #1702 for the bitcoin-uri crate.
Co-authored-by: Claude Opus 4.8
Pull Request Checklist
Please confirm the following before requesting review:
AI
in the body of this PR.