Replyable errors by spacebear21 · Pull Request #71 · LtbLightning/payjoin-ffi

spacebear21 · 2025-03-28T22:53:38Z

This PR addresses #59, #68, #69, and to a lesser extent #70.

I strove to replace all the opaque error types that were previously struct { msg: String } into the much cleaner and less boilerplate struct WrapperError(#[from] payjoin::InnerError) with #[error(transparent)]. There are a few stragglers that I'd like to address but don't have a clear way forward yet, so I highlighted those in PR comments.

spacebear21 · 2025-03-29T21:22:36Z

src/receive/error.rs

+impl From<String> for ImplementationError {
+    fn from(value: String) -> Self {
+        Self(value.into())
    }
 }


ImplementationError can be derived from any String. I believe this is fine and similar to how anyhow works.

anyhow::Error seems to impl From<E: StdError + Send + Sync + 'static> seems like that's not the same as From but String just qualifies

The problem (iiuc) is that a blanket implementation for From<E> also covers the case where E = ImplementationError, which results in:

error[E0119]: conflicting implementations of trait `From<ImplementationError>` for type `ImplementationError` --> src/receive/error.rs:77:1 | 77 | / impl<E> From<E> for ImplementationError 78 | | where 79 | | E: std::error::Error + Send + Sync + 'static, | |_________________________________________________^ | = note: conflicting implementation in crate `core`: - impl<T> From<T> for T;

I think anyhow gets around this with the anyhow_no_core_error macro but can't find implementation details for the macro.

A potential workaround would be to implement and use a pub fn new<E>(error: E) constructor instead of from.

seems like anyhow_no_core_error is a feature, not a macro. And when that feature is disabled this happens

https://github.com/dtolnay/anyhow/blob/bfb89ef244fa60af17fb844dc3bddf4b62e4ac9f/src/context.rs#L18-L30

I have an odd feeling this will bite us but I'm not sure what to do about it

spacebear21 · 2025-03-29T21:26:38Z

src/uri/error.rs

PjParseError derives from payjoin::Uri::from_str(uri.as_str()) which returns a bitcoin_uri::de::Error<payjoin::PjParseError>>, but bitcoin_uri is an inner dependency of payjoin so we can't implement From for that error type. The current hack turns the error into a string and that string into a PjParseError.

We should almost definitely be wrapping bitcoin_uri::de::Error<payjoin::PjParseError>> in a payjoin type.

You mean wrap it in rust-payjoin?

spacebear21 · 2025-03-29T21:28:17Z

src/uri/error.rs

PjNotSupported is also interesting because check_pj_supported returns a Box<bitcoin_uri::Uri<'a>> for the error type, which doesn't implement std::Error. The same string hack is used to wrap this error type.

I think this is passable. We'll see when doing integrations.

It was refactored in payjoin/rust-payjoin@2041f54 and is now now a simple struct containing a well-known error message.

The receive::Error type was previously overloaded to map address parse errors. Instead, take a parsed address as parameter and let the implementer handle parsing/network validation.

receive::Error was previously overloaded to map IntoUrlError. Instead, match the return signature of Receiver::new by exposing a IntoUrlError wrapper and returning it as the error type.

DanGould

A few comments but overall looking much better than what we had before

DanGould · 2025-04-08T20:35:55Z

src/uri/error.rs

I think this is passable. We'll see when doing integrations.

DanGould · 2025-04-08T20:36:31Z

src/uri/error.rs

We should almost definitely be wrapping bitcoin_uri::de::Error<payjoin::PjParseError>> in a payjoin type.

DanGould · 2025-04-08T20:38:50Z

src/receive/uni.rs

    Error, ImplementationError, InputContributionError, OutputSubstitutionError, ReplyableError,
    SelectionError, SerdeJsonError,
 };
+use crate::uri::error::IntoUrlError;


I can't help but wonder if we ought to have BadScheme, ParseError variants in bindings. Although I don't believe further processing beyond display and debug are necessary in the bindings, which suggests we don't need the variants.

Agreed on leaving it opaque unless we find a need for it.

DanGould · 2025-04-08T20:40:05Z

src/receive/error.rs


 /// The top-level error type for the payjoin receiver
-#[derive(Debug, PartialEq, Eq, thiserror::Error)]
-#[non_exhaustive]


is this necessarily and purposefully non_exhaustive and not PartialEq and Eq? Or was this an oversight?

I do think partialEq would require a manual implementation because of the Arc but non_exhaustive seems to work fine.

non_exhaustive seems unnecessary because we map any unexpected inner type to Error::Unexpected, so there's a catch-all in place of the non_exhaustive pattern.

PartialEq and Eq were removed because of the Arcs. Most of the internal rust-payjoin error types don't implement PartialEq/Eq so I'm not sure we can even implement them manually except with string matching?

But might we change this signature to have another variant if some Unexpected error becomes expected? e.g. with v3? In that case, without non_exhaustive we'd make an API break

I reckon if the inner types were Eq we could compare the (*arctype).clone()s

DanGould · 2025-04-08T20:40:42Z

src/receive/error.rs

+#[derive(Debug, thiserror::Error)]
+#[error(transparent)]
+#[cfg_attr(feature = "uniffi", derive(uniffi::Object))]
+pub struct ReplyableError(#[from] receive::ReplyableError);


We lost PartialEq and Eq here too. It seems that non_exhaustive is preserved through the wrapper. I suppose OK if our inner types aren't Eq (they should be) and we can update when they are.

DanGould · 2025-04-08T20:47:13Z

src/receive/error.rs

+impl From<String> for ImplementationError {
+    fn from(value: String) -> Self {
+        Self(value.into())
    }
 }


anyhow::Error seems to impl From<E: StdError + Send + Sync + 'static> seems like that's not the same as From but String just qualifies

DanGould · 2025-04-08T20:48:35Z

src/receive/mod.rs

+
+    /// Extract an OHTTP Encapsulated HTTP POST request to return
+    /// a Receiver Error Response
+    pub fn extract_err_req(
+        &self,
+        err: &JsonReply,
+        ohttp_relay: String,
+    ) -> Result<(Request, ClientResponse), SessionError> {
+        self.0
+            .clone()
+            .extract_err_req(&err.clone().into(), ohttp_relay)
+            .map(|(req, ctx)| (req.into(), ctx.into()))
+            .map_err(Into::into)
+    }
+
+    /// Process an OHTTP Encapsulated HTTP POST Error response
+    /// to ensure it has been posted properly
+    pub fn process_err_res(
+        &self,
+        body: &[u8],
+        context: &ClientResponse,
+    ) -> Result<(), SessionError> {
+        self.0.clone().process_err_res(body, context.into()).map_err(Into::into)
+    }


We should have a test to make sure this works but that's ok in integration imo

Note there's a extract_err_req unit test in rust-payjoin but no tests for process_err_res, we should cover them in integration tests too.

Opaque error types can be majorly simplified by declaring struct wrappers, then using the #[from] and #[transparent] macros from thiserror to eliminate boilerplate.

Expose the extract_err_req and process_err_res methods on Receiver to return replyable errors to the sender. Expose JsonReply wrapper and relevant From implementations to allow conversions from bound error types into JSON.

spacebear21 · 2025-04-10T18:39:12Z

Added non_exhaustive back to receive::Error

spacebear21 force-pushed the replyable-errors branch from 548e171 to 1f95bbc Compare March 29, 2025 21:14

spacebear21 commented Mar 29, 2025

View reviewed changes

spacebear21 requested a review from DanGould March 29, 2025 21:29

spacebear21 added 3 commits March 29, 2025 21:30

Update WellKnownError

3bf7394

It was refactored in payjoin/rust-payjoin@2041f54 and is now now a simple struct containing a well-known error message.

Use bitcoin_ffi::Address in Receiver::new

5223c19

The receive::Error type was previously overloaded to map address parse errors. Instead, take a parsed address as parameter and let the implementer handle parsing/network validation.

Expose IntoUrlError binding

af19849

receive::Error was previously overloaded to map IntoUrlError. Instead, match the return signature of Receiver::new by exposing a IntoUrlError wrapper and returning it as the error type.

spacebear21 force-pushed the replyable-errors branch from 1f95bbc to 4d1ef52 Compare March 30, 2025 01:31

spacebear21 marked this pull request as ready for review March 30, 2025 18:38

DanGould reviewed Apr 8, 2025

View reviewed changes

spacebear21 added 2 commits April 10, 2025 14:18

Refactor error wrappers

b0984a1

Opaque error types can be majorly simplified by declaring struct wrappers, then using the #[from] and #[transparent] macros from thiserror to eliminate boilerplate.

Expose replyable error functionality

5e90125

Expose the extract_err_req and process_err_res methods on Receiver to return replyable errors to the sender. Expose JsonReply wrapper and relevant From implementations to allow conversions from bound error types into JSON.

spacebear21 force-pushed the replyable-errors branch from 4d1ef52 to 5e90125 Compare April 10, 2025 18:19

spacebear21 merged commit 8700ae1 into LtbLightning:main Apr 10, 2025
14 checks passed

This was referenced Jun 3, 2025

Bind rust-payjoin error types #48

Closed

Audit payjoin-ffi's ImplementationError From implementation payjoin/rust-payjoin#737

Closed

Conversation

spacebear21 commented Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spacebear21 Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spacebear21 Mar 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanGould left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spacebear21 commented Apr 10, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

spacebear21 commented Mar 28, 2025 •

edited

Loading

spacebear21 Apr 9, 2025 •

edited

Loading

spacebear21 Mar 29, 2025 •

edited

Loading