bitcoindevkit / bdk-cli

A CLI wallet library and REPL tool to demo and test the BDK library
Other
111 stars 65 forks source link

Use async with esplora-reqwest #115

Closed danielabrozzoni closed 2 years ago

danielabrozzoni commented 2 years ago

Description

We previously had the esplora-reqwest feature, but it would use sync reqwest, as the "async-interface" feature in BDK wasn't set. This commit sets this feature so that using esplora-reqwest always uses async mode.

Notes to the reviewers

Checklists

All Submissions:

New Features:

rajarshimaitra commented 2 years ago

I am not sure if this is us or some problem with generic https calls with async-reqwest, but I am getting this error while trying out a wallet sync

$ ./target/debug/bdk-cli wallet -d "wpkh(tprv8ZgxMBicQKsPeuazF16EdPZw84eHj55AU8ZKgZgdhu3sXcHnFgjzskfDvZdTaAFHYNCbKqrurFo9onSaT7zGT1i3u3j7LKhVZF5sJA39WPN/86'/1'/0'/0/*)#40hv8z77" sync
[2022-08-29T10:12:21Z ERROR bdk_cli] Esplora(Reqwest(reqwest::Error { kind: Request, url: Url { scheme: "https", cannot_be_a_base: false, username: "", password: None, host: Some(Domain("blockstream.info")), port: None, path: "/testnet/api//scripthash/3149608bac07ddc9d628dd85fa0a275d88cc60b4efded5499230973557151e80/txs", query: None, fragment: None }, source: hyper::Error(Connect, "invalid URL, scheme is not http") }))

But ureq is working fine on the same esplora endpoint..

notmandatory commented 2 years ago

if we're not going to revert #114 then this PR also needs to be rebased. :-)

danielabrozzoni commented 2 years ago

Rebased :) @rajarshimaitra I'm ok with your change, but first, could you help me understand why the tests weren't failing? :( With which features did you compile?

notmandatory commented 2 years ago

@raj I'm ok with your change, but first, could you help me understand why the tests weren't failing? :( With which features did you compile?

This seems to be the issue if you use the esplora-reqwest features and try to connect to an https URL (which is default) then we get the below error:

% RUST_LOG=debug cargo run --features esplora-reqwest -- wallet --descriptor "wpkh(tpubEBr4i6yk5nf5DAaJpsi9N2pPYBeJ7fZ5Z9rmN4977iYLCGco1VyjB9tvvuvYtfZzjD5A8igzgw3HeWeeKFmanHYqksqZXYXGsw5zjnj7KM9/*)" sync
...
ERROR bdk_cli] Esplora(Reqwest(reqwest::Error { kind: Request, url: Url { scheme: "https", cannot_be_a_base: false, username: "", password: None, host: Some(Domain("blockstream.info")), port: None, path: "/testnet/api//scripthash/ca0948ef1c5bf5a023f22ed75275d7491c194476c11fbcfc63c0d25ed8234124/txs", query: None, fragment: None }, source: hyper::Error(Connect, "invalid URL, scheme is not http") }))

I confirmed that with @raj's suggestion this problem is fixed. We don't have any CI tests for this sort of thing so only found with the manual testing, not sure it's worth setting up integration tests for this project which is sort of meant to be used for manual testing (and other stuff).

rajarshimaitra commented 2 years ago

We don't have any CI tests for this sort of thing so only found with the manual testing, not sure it's worth setting up integration tests for this project which is sort of meant to be used for manual testing (and other stuff).

Its definitely worth setting up one, and now that we have automated backend we can use that to cook up a very simple integration test framework using std library only, where we can test out basic operations. This is included in this commit https://github.com/bitcoindevkit/bdk-cli/pull/102/commits/2302ff504bfe5bee72b5da47b5ccab7c5464d537 of #102 . Its still very basic but it works for Rpc and Electrum. Esplora is disabled for now because I was facing some issues with it..

rajarshimaitra commented 2 years ago

@rajarshimaitra I'm ok with your change, but first, could you help me understand why the tests weren't failing? :( With which features did you compile?

Yes as @notmandatory noted, we don't have functional tests yet for bdk-cli. We only check for clap structures and some few other functions..

danielabrozzoni commented 2 years ago

Thanks for the explanation, I just updated the PR 🙏🏻

notmandatory commented 2 years ago

@rajarshimaitra is it ok to merge this before #102? I don't think it will conflict much.

rajarshimaitra commented 2 years ago

@rajarshimaitra is it ok to merge this before https://github.com/bitcoindevkit/bdk-cli/pull/102? I don't think it will conflict much.

Yes it is.. I will rebase #102 once this is in..