-
Notifications
You must be signed in to change notification settings - Fork 279
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
base: master
Are you sure you want to change the base?
fix: wallet examples #1442
Conversation
There was a problem hiding this 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
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 |
There was a problem hiding this 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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK eeb3b7c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK eeb3b7c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK cf22595
We might need to revisit this after #1128 was merged. Since we are using the |
cf22595
to
c2802fe
Compare
c2802fe
to
982756f
Compare
Done! We are using 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")?; |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 982756f
There was a problem hiding this 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.
Description
Fix wallet examples.
Closes #1434
Notes to the reviewers
wallet_esplora_async
printing theKeychainKind
twice, and forgets to printspk0
PARALLEL_REQUESTS
wallet_esplora_*
Changelog notice
Checklists
All Submissions:
cargo fmt
andcargo clippy
before committingNew Features:
Bugfixes: