Error bindings #58

spacebear21 · 2025-03-20T19:28:01Z

Addresses #48

This exposes new top-level error types, and in cases where inner types are transparent also exposes those. Opaque inner types are kept as simple String representations.

PayjoinError is obsoleted by the new error types and removed in the last commit.

DanGould

I left quite a few comments on the "Expose receiver error types" commit. You may see that of the conversions may be able to be simplified.

I wonder if all of the impl From blocks are appropriate on the public API. Most of them certainly are, but some from specific variants make me wonder.

I wonder if we're complicating our life by using these #[error(message)] macros or if they're relatively self documented. Did you think they were just better documenting than comments, or something else? We don't have them on the error types upstream, and the bound errors will print different messages because of the #[error(...)] macros

Over all I think this constitutes a much more workable experience for users, though we're going to need to test it, and I can see many places where our API would benefit users to upgrade to using bitcoin_ffi types instead of the raw e.g. String or u64 types.

DanGould · 2025-03-24T22:20:59Z

src/receive/error.rs

+impl From<payjoin::receive::ReplyableError> for ReplyableError {
+    fn from(value: payjoin::receive::ReplyableError) -> Self {
+        use payjoin::receive::ReplyableError::*;
+
+        match value {
+            Payload(_) => ReplyableError::Payload { message: value.to_string() },
+            V1(_) => ReplyableError::V1 { message: value.to_string() },
+            Implementation(_) => {
+                ReplyableError::Implementation(ImplementationError { message: value.to_string() })
+            }
+        }
+    }
+}


By using value.to_string() instead of the variant e.to_string, isn't the thiserror annotation prefix redundant?

e.g. won't

#[error("Error while validating V1 request: {message}")] V1 { message: String },`

double print "Error while validating V1 request" ?

I prefer using value instead of e.g. V1(e)'s e because we can remove this redundancy, but we do have to make sure we're not being redundant.

Hmm good point about the double printing. I can't get tests to run because of the ohttp changes so can't verify easily but it seems likely that it does double print. The solution may be to use #[error(transparent)] everywhere...

The OHTTP changes should work with production servers now. Which problem are you experiencing?

The problem is that I need to upgrade the payjoin-directory dev dependency, but if I upgrade it to say:

payjoin-directory = { git = "https://github.com/payjoin/rust-payjoin", branch = "bindings-0.23", features = ["_danger-local-https"] }

I get:

❯ cargo test --features=_danger-local-https Updating git repository `https://github.com/payjoin/rust-payjoin.git` Updating crates.io index error: failed to select a version for `payjoin`. ... required by package `payjoin-directory v0.0.1 (https://github.com/payjoin/rust-payjoin.git?branch=bindings-0.23#0fabc7aa)` ... which satisfies git dependency `payjoin-directory` of package `payjoin_ffi v0.22.1 (/Users/spacebear/Projects/payjoin-ffi)` versions that meet the requirements `^0.22.0` are: 0.22.0 the package `payjoin-directory` depends on `payjoin`, with features: `directory` but `payjoin` does not have these features.

DanGould · 2025-03-24T22:23:55Z

src/receive/error.rs

+impl From<payjoin::bitcoin::address::ParseError> for Error {
+    fn from(value: payjoin::bitcoin::address::ParseError) -> Self {
+        Error::V2 { message: value.to_string() }
+    }
+}


This can be removed by accepting a bitcoin_ffi::Address rather than String and Network in Receiver::new [edit: perhaps in a follow up]

This would be a nice follow-up, though I couldn't get it to work in receive/uni.rs due to:

the trait `Lift<UniFfiTag>` is not implemented for `bitcoin_ffi::Address`

I believe It just needs to be passed as Arc<crate::bitcoin_ffi::Address> in the argument. follow on would work.

DanGould · 2025-03-24T22:26:35Z

src/receive/error.rs

+impl From<IntoUrlError> for Error {
+    fn from(value: IntoUrlError) -> Self {
+        Error::V2 { message: value.to_string() }
+    }
+}


All of these From implementations end up in the public API and I wonder if they're being used because they are indeed universal or because they make implementation less noisy.

src/receive/mod.rs

DanGould · 2025-03-24T22:35:14Z

src/receive/error.rs

@@ -0,0 +1,135 @@
+use payjoin::IntoUrlError;


use payjoin::receive would prevent some payjoin::receive::<errortype> redundancy quite a few times in this file.

I opted for mimicking the bdk-ffi approach of aliasing e.g. use payjoin::receive::Error as PdkError

Why not just payjoin::receive as pdk? The bdk case is a bit different because the errors cross so many modules. I don't even really like aliasing the module, but I like it better than aliasing every single type.

I realize now that we're using fully qualified names elsewhere, e.g. payjoin::receive::v2::PayjoinProposal in receiver/mod.rs, but I should just let this in to progress since it's internal.

src/receive/error.rs

src/send/mod.rs

spacebear21 · 2025-03-25T21:51:18Z

All of these From implementations end up in the public API and I wonder if they're being used because they are indeed universal or because they make implementation less noisy.

Great point, I mostly used this pattern because it was used previously and looked less noisy. At first glance some of them seem appropriate (e.g. impl From<String> for ImplementationError) and others seem harmless (e.g. converting internal Pdk types into wrapper types) but we'll want to review them case-by-case.

I wonder if we're complicating our life by using these #[error(message)] macros or if they're relatively self documented. Did you think they were just better documenting than comments, or something else? We don't have them on the error types upstream, and the bound errors will print different messages because of the #[error(...)] macros

I added them because the uniffi proc-macros were causing the compiler to complain otherwise:

error[E0277]: `receive::error::Error` doesn't implement `std::fmt::Display`
   --> src/receive/uni.rs:375:1
    |
375 | #[uniffi::export]
    | ^^^^^^^^^^^^^^^^^ `receive::error::Error` cannot be formatted with the default formatter

I wasn't aware of #[error(transparent)] though, so that may be the better solution.

DanGould

The one major concern was addressed

Everything else mostly relates to form rather than function, and given our time constraints, I'm inclined to move forward.

I do think an audit of thiserror macro usage to impl Display is in order, but that's secondary to getting the semantic error handling correct. And this gets a whole lot closer.

ACK f1ffa2f

DanGould · 2025-03-26T00:57:27Z

src/receive/error.rs

@@ -0,0 +1,135 @@
+use payjoin::IntoUrlError;


Why not just payjoin::receive as pdk? The bdk case is a bit different because the errors cross so many modules. I don't even really like aliasing the module, but I like it better than aliasing every single type.

I realize now that we're using fully qualified names elsewhere, e.g. payjoin::receive::v2::PayjoinProposal in receiver/mod.rs, but I should just let this in to progress since it's internal.

DanGould · 2025-03-26T01:01:49Z

src/receive/error.rs

+impl From<payjoin::bitcoin::address::ParseError> for Error {
+    fn from(value: payjoin::bitcoin::address::ParseError) -> Self {
+        Error::V2 { message: value.to_string() }
+    }
+}


I believe It just needs to be passed as Arc<crate::bitcoin_ffi::Address> in the argument. follow on would work.

DanGould · 2025-03-26T01:08:44Z

src/receive/mod.rs

 //     use super::*;

-//     fn get_proposal_from_test_vector() -> Result<UncheckedProposal, PayjoinError> {
+//     fn get_proposal_from_test_vector() -> Result<UncheckedProposal, Error> {


he thorough

DanGould · 2025-03-26T01:12:04Z

src/receive/error.rs

+impl From<payjoin::receive::ReplyableError> for ReplyableError {
+    fn from(value: payjoin::receive::ReplyableError) -> Self {
+        use payjoin::receive::ReplyableError::*;
+
+        match value {
+            Payload(_) => ReplyableError::Payload { message: value.to_string() },
+            V1(_) => ReplyableError::V1 { message: value.to_string() },
+            Implementation(_) => {
+                ReplyableError::Implementation(ImplementationError { message: value.to_string() })
+            }
+        }
+    }
+}


The OHTTP changes should work with production servers now. Which problem are you experiencing?

DanGould · 2025-03-26T01:24:43Z

something was merged that requires this to be rebased before merge

DanGould · 2025-03-26T15:13:58Z

Sounds also like message field on error types needs to be renamed msg as well

see: #64

Point to the bindings-0.23 rust-payjoin branch until payjoin 0.23 is released, then we can pin it back to an official version.

spacebear21 · 2025-03-26T18:13:43Z

Resolved conflicts, renamed message fields on all new error types to msg, and removed the error aliases in favor of use payjoin::receive/use payjoin::send.

Follow-up todos:

Remove fully qualified names in the mod.rs files as well.
Evaluate using #[error(transparent)] on all error types instead of potentially double-printing messages.
Accept a bitcoin_ffi::Address rather than String and Network in Receiver::new
Audit From implementations that will end up in the public API

This exposes the top-level receiver error categories while keeping the internal variants opaque (stringified). The receiver functions are updated to return the corresponding error types.

It was renamed to `new` in rust-payjoin so ensure the bindings stay in sync.

The inner error types are complex so they are first converted to strings and the wrapper types implement From<String>.

Handle error conversions for `to_json` and `from_json` helpers.

Now that there are bindings available for all error types, we can get rid of the catch-all PayjoinError.

DanGould

LGMT. Huge progress to have contextual bindings. Can't wait to see the implementations improve because of these.

I hope to see the conversion audit sooner than later. Letting it through is known tech debt.

84436d3

DanGould · 2025-03-27T17:16:15Z

src/receive/error.rs

+impl From<receive::Error> for Error {
+    fn from(value: receive::Error) -> Self {
+        match value {
+            receive::Error::ReplyToSender(e) => Error::ReplyToSender(e.into()),
+            receive::Error::V2(_) => Error::V2 { msg: value.to_string() },
+            _ => Error::Unexpected,
+        }
+    }
+}
+
+impl From<receive::ReplyableError> for ReplyableError {
+    fn from(value: receive::ReplyableError) -> Self {
+        match value {
+            receive::ReplyableError::Payload(_) => {
+                ReplyableError::Payload { msg: value.to_string() }
+            }
+            receive::ReplyableError::V1(_) => ReplyableError::V1 { msg: value.to_string() },
+            receive::ReplyableError::Implementation(_) => {
+                ReplyableError::Implementation(Arc::new(ImplementationError {
+                    msg: value.to_string(),
+                }))
+            }
+        }
+    }
+}
+
+impl From<payjoin::bitcoin::address::ParseError> for Error {
+    fn from(value: payjoin::bitcoin::address::ParseError) -> Self {
+        Error::V2 { msg: value.to_string() }
+    }
+}
+
+impl From<IntoUrlError> for Error {
+    fn from(value: IntoUrlError) -> Self {
+        Error::V2 { msg: value.to_string() }
+    }
+}


These impls definitely spook me and must be audited.

DanGould · 2025-03-27T17:21:27Z

src/uri/error.rs

+impl From<String> for PjParseError {
+    fn from(msg: String) -> Self {
+        PjParseError { msg }
+    }
+}


This is very smelly, address it in the audit. If we need this because we depend on bitcoin_uri errors in the public API, maybe we should reconsider those rather than making arbitrary string conversions part of the API.

Manual map_err seems a lot more appropriate in this circumstance.

spacebear21 force-pushed the error-bindings branch from 93d07af to 484afec Compare March 21, 2025 19:06

spacebear21 marked this pull request as ready for review March 21, 2025 19:15

spacebear21 requested a review from DanGould March 21, 2025 19:15

DanGould reviewed Mar 24, 2025

View reviewed changes

src/send/mod.rs Outdated Show resolved Hide resolved

spacebear21 force-pushed the error-bindings branch 2 times, most recently from 6fa3073 to f1ffa2f Compare March 25, 2025 21:32

DanGould approved these changes Mar 26, 2025

View reviewed changes

DanGould mentioned this pull request Mar 26, 2025

Rename message field on custom error types to msg to be compatible with Kotlin bindings #64

Merged

Update Cargo.toml

6ba6322

Point to the bindings-0.23 rust-payjoin branch until payjoin 0.23 is released, then we can pin it back to an official version.

spacebear21 force-pushed the error-bindings branch from f1ffa2f to e6aaf28 Compare March 26, 2025 18:06

spacebear21 added 9 commits March 26, 2025 16:26

Expose receiver error types

2cb8795

This exposes the top-level receiver error categories while keeping the internal variants opaque (stringified). The receiver functions are updated to return the corresponding error types.

Fix renamed SenderBuilder constructor

7842265

It was renamed to `new` in rust-payjoin so ensure the bindings stay in sync.

Expose sender error types

c1d071b

Move uri.rs to uri/mod.rs

4662b4a

Expose uri error types

6764488

The inner error types are complex so they are first converted to strings and the wrapper types implement From<String>.

Add OhttpError mapping

b38b18c

Add IoError mapping

e063224

Add SerdeJsonError mapping

86e03f2

Handle error conversions for `to_json` and `from_json` helpers.

Delete PayjoinError

84436d3

Now that there are bindings available for all error types, we can get rid of the catch-all PayjoinError.

spacebear21 force-pushed the error-bindings branch from e6aaf28 to 84436d3 Compare March 26, 2025 20:26

spacebear21 mentioned this pull request Mar 27, 2025

Fix integration test #67

Merged

DanGould approved these changes Mar 27, 2025

View reviewed changes

DanGould merged commit f80095d into LtbLightning:main Mar 27, 2025
13 checks passed

DanGould mentioned this pull request Mar 27, 2025

Audit Error From implementations since they are public API #68

Closed

spacebear21 mentioned this pull request Jun 3, 2025

Bind rust-payjoin error types #48

Closed

Error bindings #58

Error bindings #58

Uh oh!

Conversation

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

Uh oh!

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!

DanGould Mar 24, 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!

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!

Uh oh!

Uh oh!

Uh oh!

spacebear21 commented Mar 25, 2025

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!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

spacebear21 commented Mar 26, 2025

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

spacebear21 commented Mar 20, 2025 •

edited

Loading

DanGould Mar 24, 2025 •

edited

Loading

DanGould commented Mar 26, 2025 •

edited

Loading

DanGould commented Mar 26, 2025 •

edited by spacebear21

Loading