bitcoindevkit / bitcoin-ffi

Other
12 stars 7 forks source link

lib: add Txid and Outpoint #4

Closed rustaceanrob closed 1 month ago

rustaceanrob commented 1 month ago

I deviated a little from the bdk-ffi approach as I think other projects can make use of Txid without an Outpoint.

Edit: took this a much simpler direction in response to comments on a trait from UniFFI I was unaware of.

rustaceanrob commented 1 month ago

Added a separate commit to also convert from a vector of bytes to Txid and access bytes with as_bytes similar to Script.

reez commented 1 month ago

Sorry I didn't even see you already started this before I pushed up https://github.com/bitcoindevkit/bitcoin-ffi/pull/5 ! I started my PR just wanting to mess around with macros, etc. Will close my PR.

At first glance I like that you added a separate Txid type, I'll think on it more but seems to be a nice decision! (and I assume you have a need for this with Kyoto too?)

And then the only small nit would be if there's no real changes in macros.rs then we remove that line change that's in there at the moment.

rustaceanrob commented 1 month ago

At first glance I like that you added a separate Txid type, I'll think on it more but seems to be a nice decision! (and I assume you have a need for this with Kyoto too?)

Yes I would use this. The example I came across is a user (likely not a BDK wallet) generates a transaction that is rejected by a node, perhaps for being non-standard. I warn users that their transaction was rejected and reference the Txid.

And then the only small nit would be if there's no real changes in macros.rs then we remove that line change that's in there at the moment.

Will clean that up

rustaceanrob commented 1 month ago

Rebased and did a trick to avoid the FromStr import

thunderbiscuit commented 1 month ago

This is looking good. The only thing missing I think is the From traits, which prevent us from using the OutPoint type in bdk-ffi. See this run for example.

Our current implementation of this is the following:

impl From<&OutPoint> for BdkOutPoint {
    fn from(outpoint: &OutPoint) -> Self {
        BdkOutPoint {
            txid: Txid::from_str(&outpoint.txid).unwrap(),
            vout: outpoint.vout,
        }
    }
}

impl From<&BdkOutPoint> for OutPoint {
    fn from(outpoint: &BdkOutPoint) -> Self {
        OutPoint {
            txid: outpoint.txid.to_string(),
            vout: outpoint.vout,
        }
    }
}
rustaceanrob commented 1 month ago

There are a couple issues with this:

  1. To use the alias BitcoinOutPoint, the UDL is expecting a BitcoinOutPoint since it is a re-export.
  2. This is already the bitcoin OutPoint so it isn't possible to implement From (it is a foreign type)

Basically we would just be defining Outpoint to Outpoint. Unless I am misunderstanding I think this From should live in bdk-ffi

thunderbiscuit commented 1 month ago

No I mean those converters need to exist here instead of at the bdk-ffi layer if that makes sense, because when I remove the type form bdk-ffi (and the From traits) the code can't compile (it needs the traits defined). This is basically adding the same as the macros you defined earlier, but these require a slightly different structure to work.

Oh sorry I'm re-reading your message. You'd like those to exist at the bdk-ffi layer? I'm having the same issue with implementing the From in bdk-ffi, where it doesn't want to implement it on a foreign type...

rustaceanrob commented 1 month ago

There is actually nothing to implement From. The OutPoint type from bitcoin-ffi is just the one from rust-bitcoin and we can use it directly. Can show you on discord.

thunderbiscuit commented 1 month ago

Yes my bad I see now that there is no conversion needed at all. I got it to pass my local tests on bdk-ffi!