rust-payjoin icon indicating copy to clipboard operation
rust-payjoin copied to clipboard

Uniformly use primitive types in FFI boundary, with runtime validation

Open spacebear21 opened this issue 6 months ago • 5 comments

On one hand strong types make it harder to misue the API.

on the other hand, they require conversion on the implementers side. It could get even trickier if there are different versions of bdk-ffi and payjoin-ffi that want to work together but use different bitcoin-ffi versions.

Perhaps to compose these, we need a bdk-ffi that depends on payjoin-ffi with a feature gate so that the same bitcoin-ffi dependency can be used without duplication. @thunderbiscuit Have you given this any thought before?

_Originally posted by @DanGould in https://github.com/LtbLightning/payjoin-ffi/issues/70#issue-2953954751

spacebear21 avatar Jun 03 '25 22:06 spacebear21

Adding to 1.0 milestone not necessarily because I think we should use strong types, but we should make a decision for 1.0 either way.

spacebear21 avatar Sep 18 '25 17:09 spacebear21

Re-assigning this to @chavic

arminsabouri avatar Sep 29 '25 17:09 arminsabouri

Earlier, I raised a concern about some of the BTC types generating here. I performed a smoke test, and the Bitcoin types work just fine here...

Here are my thoughts on the issue: It may be possible to maintain the public FFI surface for simple primitives (PSBT strings, Satoshi u64, plain URLs) in the 1.0 cycle and compensate with stricter runtime validation. That avoids binding ourselves to a specific Bitcoin-FFI release that we don’t control and keeps BDK-FFI integrators from juggling multiple wrapper hierarchies.

If we want to help adopters use strong types, expose them via optional helpers: e.g., add a small compat module that accepts/returns bitcoin_ffi::Address/Amount behind a feature flag. This keeps the core API stable while giving teams who manage their dependency graph tightly the choice to opt in. Which looks something like this:

#[cfg(feature = "typed-helpers")]
pub mod typed {
    use crate::{uri::PjUri, bitcoin_ffi::{Address, Amount}};
    use std::sync::Arc;

    pub fn parse_sender_inputs(psbt: &str, uri: &PjUri) -> Result<(payjoin::bitcoin::Psbt, Address), String> {
        let psbt = payjoin::bitcoin::psbt::Psbt::from_str(psbt).map_err(|e| e.to_string())?;
        let address = Address::new(uri.address(), bitcoin_ffi::Network::Bitcoin)
            .map_err(|e| e.to_string())?;
        Ok((psbt, address))
    }

    pub fn amount_from_sats(sats: u64) -> Arc<Amount> {
        Arc::new(Amount::from_sat(sats))
    }
}

As opposed to:

#[uniffi::constructor]
pub fn new(psbt: Arc<Psbt>, uri: Arc<PjUri>) -> Result<Self, BuildSenderError> { ... }

pub fn build_with_additional_fee(
    &self,
    max_fee_contribution: Arc<Amount>,
    change_index: Option<u8>,
    min_fee_rate: Arc<FeeRate>,
    clamp_fee_contribution: bool,
) -> Result<InitialSendTransition, BuildSenderError> { ... }

The default API stays on plain strings/u64, so anyone who ignores the helper feature can treat bitcoin-ffi as an internal implementation detail and avoid synchronizing versions. Only users who opt in to the helper module (and therefore choose to work with bitcoin_ffi::Address, Amount, etc.) would take on the dependency-alignment responsibility; it becomes a conscious trade-off rather than a requirement for everyone.

I'd say I'm biased towards a stricter, but I'm making the assumption that library versions won't be in sync much of the time

chavic avatar Oct 01 '25 09:10 chavic

It may be possible to maintain the public FFI surface for simple primitives (PSBT strings, Satoshi u64, plain URLs) in the 1.0 cycle and compensate with stricter runtime validation. That avoids binding ourselves to a specific Bitcoin-FFI release that we don’t control and keeps BDK-FFI integrators from juggling multiple wrapper hierarchies.

I generally agree with this. I think our current approach is basically a mix of both, which is the worst of both worlds. Given the interop requirements with bdk-ffi it seems cleaner to use primitives for all boundary crossings, with runtime validation.

We can keep exporting bitcoin_ffi alongside payjoin_ffi as a convenience for implementers (for example converting callback inputs for processing), but remove the bitcoin_ffi types from the API.

@DanGould / @nothingmuch any objections?

spacebear21 avatar Oct 02 '25 02:10 spacebear21

sounds good to me. I hadn't considered the bitcoin-ffi release cadence. Crossing boundaries with primitives and validating at runtime sounds appropriate for the usecases in our medium-term roadmap. Perhaps bitcoin-ffi will. get up a more predictable release cadence sometime in the future at which point we can reconsider.

DanGould avatar Oct 02 '25 15:10 DanGould