fix: wallet examples by storopoli · Pull Request #1442 · bitcoindevkit/bdk

storopoli · 2024-05-14T11:25:40Z

Description

Fix wallet examples.

Closes #1434

Notes to the reviewers

Print balances only in BTC
Uses Signet instead of Testnet
Fix wallet_esplora_async printing the KeychainKind twice, and forgets to print spk0
Change wallet examples to use mempool.space (Electrum) or BDK's signet (Esplora) to fix rate limiting issues, along with more conservative PARALLEL_REQUESTS
Standardize code for wallet_esplora_*
Uses the new BDK rustsqlite feature (in memory) instead of BDK's file store.

Changelog notice

fix BDK wallet example crates

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

Bugfixes:

This pull request breaks the existing API
I've added tests to reproduce the issue which are now passing
I'm linking the issue being fixed by this PR

example-crates/wallet_electrum/Cargo.toml

example-crates/wallet_esplora_blocking/src/main.rs

oleonardolima

ACK 214fa09

I'm just wondering if we should add a new dependency, but I don't see much problem as it's only on the example-crates

storopoli · 2024-05-14T13:55:59Z

ACK 214fa09

I'm just wondering if we should add a new dependency, but I don't see much problem as it's only on the example-crates

But then it adds extra compile time and further surface for conflicts and other errors in all CI tests and Clippy checks...

That's the overhead that I referred to.

ValuedMammal · 2024-05-16T00:18:16Z

I'm just wondering if we should add a new dependency, but I don't see much problem as it's only on the example-crates

But then it adds extra compile time and further surface for conflicts and other errors in all CI tests and Clippy checks...

That's the overhead that I referred to.

We could use Wallet::new_no_persist and forget about bringing in tempfile, but that would make the examples somewhat less illustrative. I recommended tempfile because otherwise the db file is left lingering in your system's tempdir which is kind of a nuisance #1045

ValuedMammal

I left a few comments. Additionally we should fix printing balances for wallet_rpc like you have in 96c0129, but besides that I think the rpc example is mostly fine. Also, what do you think about switching the examples to use Signet?

example-crates/wallet_esplora_blocking/src/main.rs

oleonardolima

utACK eeb3b7c

ValuedMammal

ACK eeb3b7c

oleonardolima

ACK cf22595

storopoli · 2024-05-23T15:27:42Z

We might need to revisit this after #1128 was merged.
I only changed the async esplora wallet example,
so we might need to change the explora blocking and elecrum wallet examples.

Since we are using the bdk_sqlite crate now, we don't need tempdir.

storopoli · 2024-05-27T07:36:37Z

We might need to revisit this after #1128 was merged. I only changed the async esplora wallet example, so we might need to change the explora blocking and elecrum wallet examples.

Since we are using the bdk_sqlite crate now, we don't need tempdir.

Done! We are using bdk_sqlite now instead of tempdir in all wallet_* examples.

Ready for a new quick round of reviews @ValuedMammal and @oleonardolima

example-crates/wallet_electrum/src/main.rs

oleonardolima

ACK 982756f

ValuedMammal

ACK 982756f

This is beautiful. I'm wondering if we should change the faucet address to one that we know will be able to recycle the sats, probably doesn't need to be done now though, and to be honest I usually don't broadcast when running the example code anyway.

oleonardolima · 2024-06-03T13:55:25Z

This is beautiful. I'm wondering if we should change the faucet address to one that we know will be able to recycle the sats, probably doesn't need to be done now though, and to be honest I usually don't broadcast when running the example code anyway.

That's a good point, maybe we could use one that's been used by @thunderbiscuit for the tests on the CI ? 🤔

storopoli · 2024-07-01T14:34:19Z

rebased

thunderbiscuit · 2024-07-01T14:52:00Z

FYI I'm in the process of securing signet satoshis for the foundation. Once I have them I'll let everyone know in the chat and I can send the to whatever wallets we need.

README.md

example-crates/wallet_esplora_async/src/main.rs

example-crates/wallet_rpc/src/main.rs

storopoli · 2024-07-03T16:06:13Z

Thanks @ValuedMammal. I've incorporated all of your suggestions, rebased and squashed everything into a single fix: wallet examples commit.

oleonardolima

tACK
besides the minor comment about the wallet_rpc README, it looks good to me.

example-crates/wallet_rpc/README.md

oleonardolima

ACK 3d08c0d

ValuedMammal · 2024-07-09T00:09:02Z

Just to recap I think there's a few things missing. Not sure if maybe something went wrong during a rebase. If we can fix these then I'd like to merge it soon to avoid more review churn.

Bring back the calls to wallet.take_staged() and db.write()
For electrum remember to use the new populate_tx_cache method
Fix typo in README, fix: wallet examples #1442 (comment)
For wallet_rpc I don't mind hardcoding the descriptors, just make sure the wallet_rpc/README matches the output of the help command cargo run --bin wallet_rpc -- --help

example-crates/wallet_rpc/src/main.rs

storopoli · 2024-07-09T16:05:10Z

Just to recap I think there's a few things missing. Not sure if maybe something went wrong during a rebase. If we can fix these then I'd like to merge it soon to avoid more review churn.

* Bring back the calls to `wallet.take_staged()` and `db.write()`

* For electrum remember to use the new `populate_tx_cache` method

* Fix typo in README, [fix: wallet examples #1442 (comment)](https://github.com/bitcoindevkit/bdk/pull/1442#discussion_r1663126729)

* For wallet_rpc I don't mind hardcoding the descriptors, just make sure the wallet_rpc/README matches the output of the help command `cargo run --bin wallet_rpc -- --help`

@ValuedMammal Thank you so much for going through this.
I made a mess in a rebase and all of these things were reverted back somehow.
They are properly fixed now.

ValuedMammal · 2024-07-09T22:11:28Z

self-ACK 31f8b95

@notmandatory This looks good to me

oleonardolima

tACK 31f8b95

storopoli · 2024-07-28T20:12:21Z

rebased

- Print balances only in BTC - Uses Signet instead of Testnet - Fix wallet_esplora_async printing the KeychainKind twice, and forgets to print spk0 - Change wallet examples to use mempool.space (Electrum) or BDK's signet (Esplora) to fix rate limiting issues, along with more conservative PARALLEL_REQUESTS - Standardize code for wallet_esplora_* - Uses the new BDK rustsqlite (in-memory) instead of BDK's file store. - Electrum: use the new populate_tx_cache method Co-authored-by: ValuedMammal <valuedmammal@protonmail.com>

ValuedMammal

Seeing that #1514 made significant improvements to the wallet examples, my feeling on this PR as of now is that most of the changes can be dropped except to fix these

remove "sats" from displayed balance
swap faucet address for BDK's address
electrum: fix inspect-spks

ValuedMammal · 2024-08-08T18:14:01Z

Remember to include the "rusqlite" feature in Cargo.toml for any example crates that use it

evanlinjin · 2024-08-13T09:09:44Z

Concept ACK, although changes in #1478 are going to somewhat conflict with this (expect a rebase). I would appreciate it if we merge #1478 before this one (since that one is a larger PR).

notmandatory · 2024-09-19T02:52:15Z

@storopoli we can close this one now right?

storopoli changed the title ~~Storopoli/fix-wallet-examples~~ fix: wallet examples May 14, 2024

storopoli commented May 14, 2024

View reviewed changes

example-crates/wallet_electrum/Cargo.toml Show resolved Hide resolved

storopoli commented May 14, 2024

View reviewed changes

example-crates/wallet_esplora_blocking/src/main.rs Outdated Show resolved Hide resolved

storopoli commented May 14, 2024

View reviewed changes

example-crates/wallet_esplora_blocking/src/main.rs Show resolved Hide resolved

oleonardolima approved these changes May 14, 2024

View reviewed changes

ValuedMammal reviewed May 16, 2024

View reviewed changes

example-crates/wallet_esplora_blocking/src/main.rs Outdated Show resolved Hide resolved

example-crates/wallet_esplora_blocking/src/main.rs Outdated Show resolved Hide resolved

notmandatory assigned storopoli May 16, 2024

notmandatory added the documentation Improvements or additions to documentation label May 16, 2024

notmandatory added this to the 1.0.0-beta milestone May 16, 2024

storopoli requested a review from ValuedMammal May 16, 2024 12:11

oleonardolima reviewed May 17, 2024

View reviewed changes

ValuedMammal approved these changes May 22, 2024

View reviewed changes

oleonardolima approved these changes May 23, 2024

View reviewed changes

storopoli force-pushed the storopoli/fix-wallet-examples branch 2 times, most recently from c2802fe to 982756f Compare May 27, 2024 07:35

storopoli requested review from ValuedMammal and oleonardolima May 27, 2024 07:36

oleonardolima reviewed May 27, 2024

View reviewed changes

example-crates/wallet_electrum/src/main.rs Outdated Show resolved Hide resolved

oleonardolima approved these changes May 27, 2024

View reviewed changes

ValuedMammal approved these changes May 30, 2024

View reviewed changes

storopoli mentioned this pull request Jul 1, 2024

example: Update example_cli and retire old nursery crates #1495

Merged

3 tasks

storopoli force-pushed the storopoli/fix-wallet-examples branch 2 times, most recently from 0e8124e to 3a0a1e2 Compare July 1, 2024 14:29

ValuedMammal reviewed Jul 2, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

example-crates/wallet_esplora_async/src/main.rs Show resolved Hide resolved

example-crates/wallet_rpc/src/main.rs Show resolved Hide resolved

storopoli force-pushed the storopoli/fix-wallet-examples branch from bd8daef to 1c9d131 Compare July 3, 2024 16:05

storopoli requested review from ValuedMammal and oleonardolima July 3, 2024 16:06

oleonardolima approved these changes Jul 4, 2024

View reviewed changes

example-crates/wallet_rpc/README.md Outdated Show resolved Hide resolved

storopoli force-pushed the storopoli/fix-wallet-examples branch from 1c9d131 to 3d08c0d Compare July 4, 2024 16:06

oleonardolima approved these changes Jul 4, 2024

View reviewed changes

ValuedMammal reviewed Jul 9, 2024

View reviewed changes

example-crates/wallet_rpc/src/main.rs Outdated Show resolved Hide resolved

storopoli force-pushed the storopoli/fix-wallet-examples branch 4 times, most recently from 00a7d32 to 0f4ab48 Compare July 9, 2024 16:02

storopoli mentioned this pull request Jul 15, 2024

refactor(wallet)!: Replace new_or_load() with load_with_descriptors() #1512

Closed

5 tasks

oleonardolima approved these changes Jul 16, 2024

View reviewed changes

storopoli force-pushed the storopoli/fix-wallet-examples branch from 31f8b95 to e282641 Compare July 28, 2024 20:11

storopoli force-pushed the storopoli/fix-wallet-examples branch from e282641 to dbd4f44 Compare July 28, 2024 20:13

ValuedMammal requested changes Aug 8, 2024

View reviewed changes

storopoli closed this Sep 19, 2024

storopoli deleted the storopoli/fix-wallet-examples branch September 19, 2024 07:02

Conversation

storopoli commented May 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Notes to the reviewers

Changelog notice

Checklists

All Submissions:

New Features:

Bugfixes:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oleonardolima left a comment

Choose a reason for hiding this comment

Uh oh!

storopoli commented May 14, 2024

Uh oh!

ValuedMammal commented May 16, 2024

Uh oh!

ValuedMammal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

oleonardolima left a comment

Choose a reason for hiding this comment

Uh oh!

ValuedMammal left a comment

Choose a reason for hiding this comment

Uh oh!

oleonardolima left a comment

Choose a reason for hiding this comment

Uh oh!

storopoli commented May 23, 2024

Uh oh!

storopoli commented May 27, 2024

Uh oh!

Uh oh!

oleonardolima left a comment

Choose a reason for hiding this comment

Uh oh!

ValuedMammal left a comment

Choose a reason for hiding this comment

Uh oh!

oleonardolima commented Jun 3, 2024

Uh oh!

storopoli commented Jul 1, 2024

Uh oh!

thunderbiscuit commented Jul 1, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

storopoli commented Jul 3, 2024

Uh oh!

oleonardolima left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

oleonardolima left a comment

Choose a reason for hiding this comment

Uh oh!

ValuedMammal commented Jul 9, 2024

Uh oh!

Uh oh!

storopoli commented Jul 9, 2024

Uh oh!

ValuedMammal commented Jul 9, 2024

Uh oh!

oleonardolima left a comment

Choose a reason for hiding this comment

Uh oh!

storopoli commented Jul 28, 2024

Uh oh!

ValuedMammal left a comment

Choose a reason for hiding this comment

Uh oh!

ValuedMammal commented Aug 8, 2024

storopoli commented May 14, 2024 •

edited

Loading