Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: wallet examples #1442

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

storopoli
Copy link
Contributor

@storopoli storopoli commented May 14, 2024

Description

Fix wallet examples.

Closes #1434

Notes to the reviewers

  • Print balances only in BTC
  • 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_*

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

@storopoli storopoli changed the title Storopoli/fix-wallet-examples fix: wallet examples May 14, 2024
Copy link
Contributor

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

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
Copy link
Contributor

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

Copy link
Contributor

@ValuedMammal ValuedMammal left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
example-crates/wallet_esplora_blocking/src/main.rs Outdated Show resolved Hide resolved
@notmandatory notmandatory added the documentation Improvements or additions to documentation label May 16, 2024
@notmandatory notmandatory added this to the 1.0.0-beta milestone May 16, 2024
Copy link
Contributor

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

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

utACK eeb3b7c

Copy link
Contributor

@ValuedMammal ValuedMammal left a comment

Choose a reason for hiding this comment

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

ACK eeb3b7c

Copy link
Contributor

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

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

ACK cf22595

@storopoli
Copy link
Contributor Author

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 storopoli force-pushed the storopoli/fix-wallet-examples branch from cf22595 to c2802fe Compare May 27, 2024 07:09
@storopoli storopoli force-pushed the storopoli/fix-wallet-examples branch from c2802fe to 982756f Compare May 27, 2024 07:35
@storopoli
Copy link
Contributor Author

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


print!("Syncing...");
let client = electrum_client::Client::new("ssl://electrum.blockstream.info:60002")?;
let client = electrum_client::Client::new("ssl://mempool.space:60602")?;
Copy link
Contributor

Choose a reason for hiding this comment

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

@storopoli It didn't work for me when using the mempool.space, only with blockstream's one 🤔, was it working for you ?

Error: Made one or multiple attempts, all errored:
	- Socket is not connected (os error 57)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I've successfully run it twice behind a mullvad VPN by the way

Copy link
Contributor

Choose a reason for hiding this comment

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

Weird, it only worked for me when I disabled the VPN 👀

Copy link
Contributor

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

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

ACK 982756f

Copy link
Contributor

@ValuedMammal ValuedMammal left a comment

Choose a reason for hiding this comment

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
Status: Needs Review
Development

Successfully merging this pull request may close these issues.

example: Fix wallet examples
4 participants