LtbLightning / bdk-flutter

Bitcoin Development Kit - Flutter Package
MIT License
60 stars 27 forks source link

Align api with ffi #39

Closed BitcoinZavior closed 1 year ago

BitcoinZavior commented 1 year ago

Major upgrade to align api with bdk-ffi

1 - Updated API to align with bdk-ffi v0.11.0 2- Updated make file to ensure build on different laptop configs and resolve bugs. 3 - Update readme with build instructions in case someone wants to make changes to rust code and build. (Points 3 and 4 are not relevant and not required for projects who intend to use bdk-flutter, this is relevant and useful for contributors to the project) 4 - Comments added for auto doc generation, when the package gets published to pub.dev it will have complete api reference with mostly the same text and description as bdk. 5 - Updated test cases: https://github.com/LtbLightning/bdk-flutter/blob/align-api-with-ffi/test/bdk_flutter_test.dart 6- Updated Example App: https://github.com/LtbLightning/bdk-flutter/blob/align-api-with-ffi/example/lib/main.dart 7 - Kept MIT as the main license, this is to ensure pub. dev picks up the license information correctly. Kept Apache as an additional license.

Fixes #38 Fixes #37 Fixes #36 Fixes #35 Fixes #34 Fixes #33 Fixes #32 Fixes #27

BullishNode commented 1 year ago

Had a look at the readme. The api is much more powerful now. Many requested features available. Thx so much for your work 🧡

notmandatory commented 1 year ago

I took a quick look through ffi.rs and r_api.rs and this looks like a big step in the right direction. Your use of Opaque make this feel much more like the bdk-ffi apis. I do see you're using .panic() for error handling, eventually you should probably handle these cases with returned errors, but I understand if you want to get this out so users can start playing with it. One small nit is your naming of WalletInstance and BlockchainInstance I'd call them simply Wallet and Blockchain, but certainly either way works.

BitcoinZavior commented 1 year ago

Thanks @notmandatory

One small nit is your naming of WalletInstance and BlockchainInstance I'd call them simply Wallet and Blockchain, but certainly either way works.

Yes, I did change these names a couple of times as I couldn't decide on the most appropriate ones. I think your suggestion is a good one, I have just addressed it and updated all *Instance to *

I do see you're using .panic() for error handling, eventually you should probably handle these cases with returned errors, but I understand if you want to get this out so users can start playing with it.

rust_bridge converts it all into ffiexception, we catch and throw a custom exception, but its somewhat basic, so doesn't cover all the cases. Perhaps going through bdk docs and handling exceptions more efficiently will be a good exercise. Opening an issue for this to come up with a better approach, https://github.com/LtbLightning/bdk-flutter/issues/40.