-
Notifications
You must be signed in to change notification settings - Fork 11
Add psbt ffi bindings #36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
9626734
to
9615770
Compare
@DanGould I chose these bindings for now based on some integration tests from rust-payjoin. namely, |
9615770
to
ecb2fd4
Compare
src/lib.rs
Outdated
pub fn combine(&self, other: Arc<Self>) -> Result<Psbt, PsbtError> { | ||
let mut psbt = self.0.clone(); | ||
let other_psbt = other.0.clone(); | ||
psbt.combine(other_psbt)?; | ||
Ok(psbt.into()) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opted to not wrap the Psbt in a Mutex<_>
so this cannot return self, would it be better to wrap the inner psbt in a mutex instead?
This add some basic ffi bindings for psbts including serialization and deserizlization as well as a few methods used in payjoin integration tests that are needed for downstream bindings there.
ecb2fd4
to
37f47c8
Compare
#[derive(Debug, thiserror::Error, uniffi::Error)] | ||
pub enum PsbtError { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe remote error types can be used with uniffi::remote(Enum)
. Then I'd guess they don't need to derive thiserror::Error
, or require a conversion, or require the explicit display #[error] attributes. Not sure If it has been tried in this repo before. I think an enum still needs to be written with each variant but some of the other boilerplate that might get out of sync can go. I am not certain of this but the documentation suggests it.
https://mozilla.github.io/uniffi-rs/latest/types/remote_ext_types.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the Psbt Error Enum there are a lot of complex error payload types, wouldn't these all still need to be converted for this to be easily used as a remote(Enum)
? I'm just not sure its worth it to avoid error messages getting out of sync unless there is a way to more easily avoid those conversions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think #[uniffi(flat_error)]
might be able to keep the upstream payload types but inherit debug/display implementations (perhaps with #[uniffi::export(Debug), uniffi::export(Display)]
, not sure to what extent these macros can be combined). You may need to rely on the bitcoin/std
feature so errors are std::error::Error
in order to make sure UniFFI knows the remote PsbtError
can be thrown as an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Errors have been a pain in our side for a long time, but I'm curious to see if some of the new features in uniffi might fix some of those pain points for us. In particular, simply using the remote error types would be awesome instead of reimplementing errors.
For a comprehensive dive into this from a year ago (some things have changed since then so I still have hope!) see this issue: bitcoindevkit/bdk-ffi#509.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I remember our problem was mostly about having to choose between offering the complex types in the payload vs correct and interesting error messages for users (which they expect in their target languages).
I think if you have complex types in the payload, your Display becomes a stringified version of the type, and that's what you get in Kotlin for your exception.message field (where you expect a good message). Same thing for Swift (I think it's the description
field). If you want to instead fix this message, you cannot have fields in your enum (something like that). The issue as far as I remember is that for some errors you'd much rather have the message, and for others you'd rather have the data... 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that you cannot use #[uniffi::export(Display)]
on enums (you get the error unsupported item: only functions and impl blocks may be annotated with this attribute
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure that message is related? I definitely use #[uniffi::export(Display)]
on this struct which is neither a function nor an impl block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok weird. Maybe I got this wrong. I tried a bunch of stuff 2 weeks ago to play with the errors a bit more and got to the same place we ended up so far.
Let me know how you make out I'm definitely interested in better errors!
This add some basic ffi bindings for psbts including serialization
and deserizlization as well as a few methods used in payjoin
integration tests that are needed for downstream bindings there.
supersedes #22 as it build off the work @rustaceanrob did