wrap bitcoin_uri errors in PayjoinUriError type

[Johnosezele]

Add PayjoinUriError to provide better error handling for URI parsing. Update UriExt trait to use this new type instead of Box.

Closes #644

edit:
Wraps bitcoin_uri::de::Error properly

• Changes payjoin::{Uri, PjUri} from type aliases to wrapper structs
• Wraps bitcoin_uri::de::Error properly in PayjoinUriError::Parse variant
• Uses concrete error enums with thiserror instead of boxed errors
• Makes check_pj_supported() non-consuming to allow continued URI usage
• Enables proper FFI error handling by eliminating string-based conversions

Closes #644 likely closes #645 and #751
Contributes to #737

[coveralls]

Pull Request Test Coverage Report for Build 15929207574

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 769 of 842 (91.33%) changed or added relevant lines in 2 files are covered.
• 37 unchanged lines in 18 files lost coverage.
• Overall coverage increased (+0.04%) to 86.199%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin/src/uri/error.rs	23	33	69.7%
payjoin/src/uri/mod.rs	746	809	92.21%

Files with Coverage Reduction	New Missed Lines	%
payjoin-directory/src/lib.rs	1	75.0%
payjoin/src/ohttp.rs	1	70.97%
payjoin/src/receive/mod.rs	1	98.13%
payjoin/src/receive/multiparty/mod.rs	1	89.86%
payjoin/src/receive/v1/mod.rs	1	95.74%
payjoin/src/receive/v2/session.rs	1	94.02%
payjoin/src/send/v1.rs	1	94.7%
payjoin/src/send/v2/mod.rs	1	88.13%
payjoin/src/uri/mod.rs	1	92.45%
payjoin-test-utils/src/lib.rs	1	94.41%

Totals
Change from base Build 15905219803:	0.04%
Covered Lines:	8032
Relevant Lines:	9318

---

💛 - Coveralls

[DanGould]

This is 23 commits for what seems to be just one change (8af6c41). May you rebase that commit on a fetched master so that we can just add the atomic change please?

[DanGould]

It seems like that one commit also contains formatting changes that are unrelated to the change at hand. Please narrow your commit to just do one thing.

[DanGould]

This does not appear to wrap bitcoin_uri::de::Error which is the subject of #644. Such errors arise from the use of Uri::try_from(s: &'a str).

I believe in order to encapsulate that error we would need to change our payjoin::{Uri, PjUri} types into a wrappers rather than aliases. I would want to still be able to convert to bitcoin_uri::Uri for interoperability with other bitcoin_uri usage downstream.

The PayjoinUriError type introduced here also has unused constructors and only encapsulates an arbitrary Option<Box<dyn Error + Send + Sync>>, rather than concrete error or enumerated variants. It also prevents the continued use of a Uri after callingfn check_pj_supported(self) because the function takes ownership of the checked Uri.

I know real heart went into the drafting of the PR but it seems to me the best course of action is to close it, because it doesn't address the #644 problem i and I don't think the side effects of the change to check_pj_supported are desirable.

[Johnosezele]

Thanks Dan. This makes things clearer now. I can see the lapses from my previous approach in addressing #644 and how it also introduces some undesirable side effects with check_pj_supported.

Can I suggest a refactor to address these concerns instead of completely closing this pr?

Here's my approach for a refactor, pls let me know if I'm on the right track:

1. Redefine payjoin::Uri as a Wrapper: I'll change payjoin::Uri from a type alias to a new struct that wraps bitcoin_uri::Uri. To allow for custom parsing logic.
2. Implement Custom try_from_str: On this new PayjoinUri wrapper, I'll implement a try_from_str (or similar) method. This method will internally call bitcoin_uri::Uri::try_from and then map its Result, specifically wrapping any bitcoin_uri::de::Error into the PayjoinUriError.
3. Redesign PayjoinUriError as an Enum: I'll refactor PayjoinUriError into an enum with specific variants. This will include a variant for bitcoin_uri::de::Error (to address Wrap URI errors in a payjoin type #644 directly) and other variants for PJ-specific issues (like unsupported parameters, invalid endpoint schemes, etc.). The goal is to make the error handling more precise.
4. Revise check_pj_supported: I'll modify the check_pj_supported method (or integrate its logic into the new wrapper) to avoid consuming self unnecessarily, so the URI can still be used after the check. It will also use the new error enum.
5. Interoperability: I'll make sure the new PayjoinUri wrapper can be easily converted to/from the underlying bitcoin_uri::Uri for compatibility with other code.

I think these changes should align much better with the goal of #644 and address the concerns you raised. Pls let me know what you think.

[Johnosezele]

Hi @DanGould, I've made a complete refactor:
Brief highlight:

• Now Properly Wraps bitcoin_uri::de::Error

• Converted payjoin::{Uri, PjUri} from aliases to actual PayjoinUri<'a> and ValidatedPayjoinUri<'a> structs to allow custom error handling that wasn't possible with type aliases

• Replaced Option<Box<dyn Error + Send + Sync>> with concrete PayjoinUriError enum variants

• Used thiserror::Error for proper error handling

• All variants are enumerated and matchable for FFI error conversion

Also, FFI layers can now implement From<PayjoinUriError> properly since PayjoinUriError is a concrete payjoin type, not a foreign bitcoin_uri type.