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

RPC backend implementation #36

Merged
merged 5 commits into from
Oct 19, 2021

Conversation

rajarshimaitra
Copy link
Contributor

@rajarshimaitra rajarshimaitra commented Aug 3, 2021

Description

This opens the RPC backend recently added in BDK bitcoindevkit/bdk#407.

This builds on top of BDK master, because RPC back end isn't released yet.

I committed the Cargo.lock file too understanding that we are suppose to commit this file also as per #27 (comment). Let me know if that's not appropriate.

This is now ready for review.

Notes to the reviewers

It seems there is some failure with esplora backend, probably something related to recent restructuring of esplora module in
bdk. Now that there are two different esplora configuration in BDK, we need to either chose one for bdk-cli, or have both options
as feature flag. I decided to go for use-explora-reqwest for now.

But in any case this error should not be there, as I can see bdk compiles with maybe-await!() macro in master just fine, with the reqwest esplora feature. Not sure what is the issue here. Any suggestion would be helpful.

Compiling bdk-cli v0.2.1-dev (/home/raj/github-repo/bdk-cli)
error[E0277]: the `?` operator can only be applied to values that implement `Try`
   --> src/lib.rs:984:13
    |
984 |             maybe_await!(wallet.sync(log_progress(), max_addresses))?;
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the `?` operator cannot be applied to type `impl Future`
    |
    = help: the trait `Try` is not implemented for `impl Future`
    = note: required by `into_result`

error[E0277]: the `?` operator can only be applied to values that implement `Try`
   --> src/lib.rs:999:24
    |
999 |             let txid = maybe_await!(wallet.broadcast(tx))?;
    |                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the `?` operator cannot be applied to type `impl Future`
    |
    = help: the trait `Try` is not implemented for `impl Future`
    = note: required by `into_result`

This is causing the test failures.

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

@rajarshimaitra rajarshimaitra force-pushed the rpc-backend branch 2 times, most recently from a504d71 to f752a1f Compare August 13, 2021 12:11
@rajarshimaitra rajarshimaitra marked this pull request as ready for review August 13, 2021 12:14
@rajarshimaitra rajarshimaitra changed the title Draft RPC backend implementation RPC backend implementation Aug 13, 2021
Cargo.toml Outdated Show resolved Hide resolved
@rajarshimaitra
Copy link
Contributor Author

Updated with v0.10.0. But cargo build --features esplora still fails.

@rajarshimaitra rajarshimaitra mentioned this pull request Aug 26, 2021
9 tasks
@rajarshimaitra
Copy link
Contributor Author

rajarshimaitra commented Aug 26, 2021

The above error is fixed (along with other updates for BDK v0.10.0) in https://github.com/bitcoindevkit/bdk-cli/pulls

Decided to use esplora ureq version. Removing all async from bdk-cli. cc @tcharding @notmandatory

This should be merged after the version update PR.

@rajarshimaitra
Copy link
Contributor Author

Rebased on esplora fix at #41 .. This should be merged after that PR.

@rajarshimaitra
Copy link
Contributor Author

rajarshimaitra commented Oct 5, 2021

Rebased on master.

@rajarshimaitra
Copy link
Contributor Author

There needs to be readme update after this. But doing it in this PR might cause conflicts with #44.

So putting that for a separate PR.

@notmandatory
Copy link
Member

notmandatory commented Oct 6, 2021

Please add rpc to the build.rs logic that prevents two blockchain features from being enabled together. Also maybe another rebase and then you can mention it as an option in the README too. I'd like to merge this one next, then #42. Thanks!

@notmandatory
Copy link
Member

Need to add feature = "rpc" also in src/lib.rs line 167

Expose the RPC backend feature via cli arg options.
RPC backend can be connected via all default parameters
without specifying any arg options.
@rajarshimaitra
Copy link
Contributor Author

  • Rebased,
  • updated build script,
  • fixed in lib.rs,
  • Updated Readme

Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

ACK 32a3a55

Looks good, thanks for the update. I'll give anyone else interested a chance to review and if no objections merge tomorrow.

@notmandatory notmandatory merged commit 32a3a55 into bitcoindevkit:master Oct 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants