Skip to content
This repository has been archived by the owner on Nov 15, 2022. It is now read-only.

Update bdk-ffi to 0.5 (with TxBuilder APIs) #40

Merged
merged 7 commits into from Apr 6, 2022

Conversation

notmandatory
Copy link
Member

@notmandatory notmandatory commented Mar 29, 2022

Description

Update bdk-ffi to bitcoindevkit/bdk-ffi#123 and add new TxBuilder tests.

Notes to the reviewers

I also fixed tests to use network and addresses for TESTNET to match BlockchainConfig.

The tests must be a bit hacky until we have a way to use REGTEST for our Kotlin tests.

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature
  • I've updated CHANGELOG.md

@thunderbiscuit
Copy link
Collaborator

Looking at this right now. Should the submodule point to a tag on bdk-ffi? The 0.5.0 tag is 8a556d0ba0d5cd499b39dd65ca073229a45ffce2. Just not sure what our "official" procedure is or should be.

@thunderbiscuit thunderbiscuit self-requested a review April 4, 2022 20:05
@artfuldev
Copy link
Contributor

@thunderbiscuit I was gonna pick that up and complete upgrading to 0.5 but happy to take help.

Copy link
Collaborator

@thunderbiscuit thunderbiscuit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree using testnet for the tests is not ideal for now (the wallet will drain rather quickly if we always run tests), but I do think it's good enough for now. I suggested one small change to the error message for the walletTxBuilderBroadcast test. Otherwise everything worked well on my end.

jvm/src/test/kotlin/org/bitcoindevkit/JvmLibTest.kt Outdated Show resolved Hide resolved
@artfuldev
Copy link
Contributor

I'm yet to upgrade to 0.5 tag which will add 2 new APIs for testing.

@artfuldev artfuldev assigned artfuldev and unassigned notmandatory Apr 4, 2022
@notmandatory notmandatory marked this pull request as ready for review April 5, 2022 18:47
@notmandatory
Copy link
Member Author

notmandatory commented Apr 5, 2022

Until we can get a CI regtest setup (see bitcoindevkit/bdk-ffi#241) I think the best approach is first run the JVM tests locally to get addresses to pre-fund the two testnet wallets for the walletTxBuilderBroadcast and walletTxBuilderDrainWallet tests, then push your changes.

@notmandatory
Copy link
Member Author

notmandatory commented Apr 5, 2022

After discussing with @thunderbiscuit I commented out the walletTxBuilderDrainWallet test for now, it can be commented out and ran locally until we have a regtest CI setup working.

@artfuldev
Copy link
Contributor

Do we need to add documentation?

@artfuldev artfuldev changed the title Txbuilder Update bdk-ffi to 0.5 (with TxBuilder APIs) Apr 5, 2022
Copy link
Collaborator

@thunderbiscuit thunderbiscuit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested ACK a67b88a. @artfuldev was there anything else you were wanting to add? Yesterday I think you were saying you wanted to do a few more things. I don't want my ACK to mean "merge" if you're not ready.

@notmandatory
Copy link
Member Author

Rebased. Working on updating the docs.patch then will merge.

@notmandatory notmandatory merged commit 1bcacec into bitcoindevkit:master Apr 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants