FFI: Eliminate string fallbacks in error wrappers

[chavic]

This drops the last string-based conversions in the FFI surface. OhttpError now wraps the upstream ohttp::Error directly (with a simple message() helper for bindings), PjParseError and PjNotSupported carry the typed payjoin errors via internal constructors, and drops ForeignError call site now routes through ImplementationError::new(e).

This addresses issue #1117

---

AI Disclosure: Used GPT-5 Codex for multi-file edits in Cursor

[coveralls]

Pull Request Test Coverage Report for Build 18389775642

Details

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 83.737%

---

Totals
Change from base Build 18358420400:	0.0%
Covered Lines:	8954
Relevant Lines:	10693

---

💛 - Coveralls

[spacebear21]

This looks more complicated then before, what is the simplification?

[chavic]

Yes, I encountered this issue... When we get payjoin::persist::PersistedError, we occasionally need both:

1. A peek to see whether it contains a storage error (storage_error_ref), and
2. Ownership of the same value so we can pull that storage error out (storage_error), or fall back to api_error.

The borrow checker won’t let us call storage_error_ref() and then later move the same err without more juggling. Wrapping it in Some(err) gives us a tiny state machine.

It’s a bit more code than checking twice, but it keeps us from cloning errors or double-consuming the iterator.

[chavic]

Other approaches would be to....

• Require every storage error type to implement Clone so we can copy it off the reference (not ideal), or
• Restructure the code so we inspect and consume the PersistedError exactly once.

I'll try the second approach first...

[chavic]

Restructuring the code so that we inspect and consume the PersistedError exactly once seems to introduce some unusual semantics, such as Ok(api_err). I'm open to other ideas for now... @spacebear21

// payjoin-ffi/src/persist.rs
use payjoin::persist::PersistedError;

pub enum PersistedSplit<ApiErr, StorageErr> {
    Api(ApiErr),
    Storage(StorageErr),
    Unknown,
}

pub fn split_persisted_error<ApiErr, StorageErr>(
    err: PersistedError<ApiErr, StorageErr>,
) -> PersistedSplit<ApiErr, StorageErr>
where
    ApiErr: std::error::Error,
    StorageErr: std::error::Error,
{
    // `into` gives us the internal enum, so we can match without cloning.
    match err.into() {
        payjoin::persist::InternalPersistedError::Transient(api)
        | payjoin::persist::InternalPersistedError::Fatal(api) => PersistedSplit::Api(api),
        payjoin::persist::InternalPersistedError::Storage(storage) => PersistedSplit::Storage(storage),
    }
}

on the receiving side... sender is somewhat identical...

use crate::persist::{split_persisted_error, PersistedSplit};

impl<S> From<PersistedError<payjoin::receive::Error, S>> for ReceiverPersistedError
where
    S: std::error::Error + Send + Sync + 'static,
{
    fn from(err: PersistedError<payjoin::receive::Error, S>) -> Self {
        match split_persisted_error(err) {
            PersistedSplit::Api(api) => ReceiverPersistedError::Receiver(api.into()),
            PersistedSplit::Storage(storage) =>
                ReceiverPersistedError::from(ImplementationError::new(storage)),
            PersistedSplit::Unknown =>
                ReceiverPersistedError::Receiver(ReceiverError::Unexpected),
        }
    }
}

[spacebear21]

I don't understand why ImplementationError was changed to ImplementationExceptionInner with an outer enum wrapper? This seems to do exactly the same thing that ForeignError in a slightly re-arranged way. I thought the point of getting rid of ForeignError was so we could return ImplementationError directly. If that's not possible I'd rather not change it at all.

[chavic]

ohh yeah… so errors have to be exported as uniffi::Error enums, while anything you hand back for callbacks or stored handles has to be an uniffi::Object. UniFFI won’t let one type play both roles, so we tuck the core payjoin::ImplementationError inside that inner object and let the enum carry it across the error boundary.

[spacebear21]

I like what you did with OhttpError, changing it to:

pub struct OhttpError(#[from] ohttp::Error);

Is there something preventing us from doing the same thing for PjParseError and PjNotSupported? If yes it would be good to document why we take a different approach for these.

[chavic]

I went string-backed here because the core payjoin::PjParseError/PjNotSupported don’t currently derive PartialEq/Eq; our fixtures check these errors by comparing values, so swapping to a transparent wrapper would break those comparisons unless we taught the core types how to compare or wrote custom equality glue.
Without those traits, the generated bindings—and our existing tests that compare errors—would break once we stop stringifying.

[spacebear21]

I squashed superfluous commits together and reverted the changes to ForeignError - I don't think it would be appropriate to map all ForeignErrors to ImplementationError internally so I think a string representation is fine for now. I'd like to revisit this error category later once we've migrated to primitive types #738 at the FFI boundary, since that would also imply a new category of errors for runtime validation/conversions.

The second commit here does solve the immediate problem of exposing From<String> inadvertently in the public API, but it still seems like a hacky workaround for the root issue which is that the underlying error types in rust-payjoin are unwieldy. We should really be able to just use #[error(transparent)] for those as well.

[spacebear21]

ACK ded2e6d