bitcoindevkit / bitcoin-ffi

Other
12 stars 7 forks source link

Add Rust and Kotlin CI checks #15

Closed tnull closed 1 week ago

tnull commented 3 weeks ago

Closes #3.

This PR adds CIs for Rust and Kotlin, as well as licensing files for this project.

To this end we generate the Kotlin bindings and run some trivial tests to ensure the binding generation worked.

This is mainly so we immediately see if we were to break binding generation going forward (and to enforce formatting, for example).

FWIW, the package names might be debatable (e.g., org.rustbitcoin.bitcoin vs org.bitcoindevkit.bitcoinffi), but for now I simply made them uniform so everything builds.

reez commented 3 weeks ago

Awesome good idea (CI and Licensing) 👍

Looking good to me, even though I'm not a Kotlin expert I was just reviewing that part by comparing to bdk-ffi mostly

tnull commented 3 weeks ago
  1. Did you have in mind to publish this library on its own to Maven Central? If not, I think some of the Gradle build scripts can be simplified. Let me know and I'll comment on those directly.

Yes, I think we definitely should publish the Swift/Kotlin/python/Go?/C#? bindings eventually once we have a bit better coverage of the rust-bitcoin library to warrant a 'proper' release. I think users don't have a good bitcoin library of choice in many of these languages available, and making the high code quality of rust-bitcoin available in these other languages would be a huge win for this project, even if we wouldn't use it as a direct Uniffi dependency in our own projects, IMO.

thunderbiscuit commented 2 weeks ago

I'm not against publishing the library on its own of course, but I'd be tempted to wait until there is demand for it before adding all that plumbing to the codebase (especially that it implies engineering time/resources I'm not sure either of our projects has to spare at the moment). As is, I'm not yet sure why/how/who would use just a bunch of bitcoin types on their own without much logic (I'm sure there are use cases for it! but there are also great libraries in all of those languages that provide these primitives very well). The one exception to that might be the miniscript-related types, which are of course much more advanced in Rust than in any other languages. Still I'm tempted to wait until someone requests it.

As is, just as a good collaboration location for types many uniffi-based libraries will need (if we can be useful enough for others to start contributing their types as well), I suspect we will be able to grow the library to have a nice coverage of the rust-bitcoin ecosystem over the next year or so. From there if people need it on its own we could look at what languages exactly are required, who has the expertise to maintain, etc.? Just my 2 sats, I'm not actually against anything in this PR I just want to keep the conversation going.

tnull commented 2 weeks ago

before adding all that plumbing to the codebase

Not sure what you're referring to here exactly? This PR didn't take me long and should get us ~80-90% towards Kotlin packages? Happy to eventually follow-up with the actual publishing CI scripts as well as Swift and Python counterparts if engineering time/resources is an issue.

I'm not yet sure why/how/who would use just a bunch of bitcoin types on their own without much logic

Well, a lot of people get use out of rust-bitcoin which is exactly that, no?

but there are also great libraries in all of those languages

Hmm, care to name a few? bitcoin-js might be available for Java/Kotlin, but what libraries are available for Swift and Python that have undergone nearly as strict code review as rust-bitcoin? And for Go/C#?

tnull commented 2 weeks ago

Rebased to resolve minor conflict.

thunderbiscuit commented 2 weeks ago

Yeah I mean I really didn't want to imply I'm against publishing it, but I was just thinking it'd be great to see if there is even any demand for these (currently) 10 types. The "plumbing" I was referring to is many-fold and ever changing with the evolving uniffi, but roughly:

  1. Directories for each library
  2. Each of those has build, test, release scripts
  3. Tests
  4. Packaging and publishing workflows
  5. Accounts with the respective packaging directories and familiarity with their release (Maven Central, PyPI, the C# and Go ones, etc.).

To answer your specific questions:

  1. libraries that have undergone nearly as strict code review as rust-bitcoin?

That one is easy... 😆 probably none. (at least for miniscript stuff!)

  1. Great libraries in our target languages

That one is easy too. Note that many of those have seen much more "in-production-hours" than rust-bitcoin by maybe an order of magnitude. I'm not knocking on rust-bitcoin, I'm just saying we're not the only ones producing good bitcoin software. The need for good, high-level, well-reviewed and well thought-out wallet and lightning software libraries is evident (to me at least) in this niche and for these languages, and is why bdk-ffi and ldk-node are such a good fit for the bindings. But the need for just a few low-level types in these languages... I don't see it as clearly yet. Java 👉 BitcoinJ, Kotlin 👉 bitcoin-kmp (ACINQ) Python 👉 pycoin/bitcoinlib/bit Go 👉 btcd (the library maintained by Lightning Labs) C# 👉 The library maintained by NIcolas Dorier for btcpayserver Swift 👉 I don't know this one enough to say

thunderbiscuit commented 2 weeks ago

In the meantime of course, anyone wanting these types without the domain-logic can still add bdk-ffi as a dependency, which will re-export (so far at least) all of the types in this library, and does so at no extra cost to the development team (we release anyway). This means you might add a few MB to your final binary but you're not out of options.

How do you feel about testing the waters with this library for a few months (say 6-8?) and seeing how things go and the library surface we cover before opening the box of releasing on all languages? At that point we can also discuss which languages might be good targets, or whether all of them is just as easy once you do it for one.

thunderbiscuit commented 2 weeks ago

Ok one last thing: @tnull feel free to push back on this! If you think adding and maintaining the release of separate libraries for bitcoin-ffi is worth doing right now and are willing to add the code to the codebase I'm not against it at all and don't want to be a blocker for a good library to get out there.

tnull commented 2 weeks ago

How do you feel about testing the waters with this library for a few months (say 6-8?) and seeing how things go and the library surface we cover before opening the box of releasing on all languages? At that point we can also discuss which languages might be good targets, or whether all of them is just as easy once you do it for one.

Right, as mentioned above we should only do it if more of the API surface is covered. But I'd rather not put a number in terms of months on it. Could be in two weeks, or more than the 6 months...

In any case, this discussion seems more or less unrelated to this PR. I guess we could slim it down a bit, but not sure if there is a lot of reason for it?

tnull commented 1 week ago

Addressed feedback in a fixup and rebased on main to resolve minor conflict.

Davidson-Souza commented 1 week ago

super nit, but 9d73f5a appears to have a loose f in the commit title.

Apart from that, ACK ca2ab10

tnull commented 1 week ago

super nit, but 9d73f5a appears to have a loose f in the commit title.

That is intentional, as it is a fixup intended to be squashed in after being reviewed. Will go ahead and do that now.

tnull commented 1 week ago

Squashed fixups without further changes:

> git diff-tree -U2 ca2ab10 69f6dd1
>

Will just land this as nothing changed since the two ACKs.