Skip to content
This repository has been archived by the owner on Mar 14, 2023. It is now read-only.

RPC wallet syncing example with Functional tests #170

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rajarshimaitra
Copy link
Collaborator

@rajarshimaitra rajarshimaitra commented Feb 6, 2023

This PR adds the complete RPC syncing example with functional tests added in bdk_rpc_wallet_example/tests.

The RPC sync is by the "import into a core wallet" approach.

The functional tests covers 3 basic tests

  • Basic sync
  • Basic reorg situation
  • Sending transactions.

This can be used as a reference to use the bitcoind backend to run similar electrum and esplora functional tests.

More tests would be added in this PR or later.

@codecov-commenter
Copy link

Codecov Report

Merging #170 (cb9957b) into master (2a11787) will not change coverage.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@           Coverage Diff           @@
##           master     #170   +/-   ##
=======================================
  Coverage   56.74%   56.74%           
=======================================
  Files          10       10           
  Lines         252      252           
=======================================
  Hits          143      143           
  Misses        109      109           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@rajarshimaitra rajarshimaitra marked this pull request as ready for review February 15, 2023 07:18
@rajarshimaitra rajarshimaitra changed the title [test] RPC wallet syncing with BitcoinD RPC wallet syncing example with Functional tests Feb 15, 2023
@rajarshimaitra
Copy link
Collaborator Author

The latest update does the following:

Copy link
Owner

@LLFourn LLFourn left a comment

Choose a reason for hiding this comment

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

Thanks @rajarshimaitra. I think the design works. I suggest trimming down the scope of rpc.rs to only wallet_scan. Everything else seems like pure example code that can go in main.rs. The user can be left to make sure that all the spks they are interested in are loaded into the wallet. This allows them to use descriptors also. If we want to get more opinionated about this we can do it later.

The tests are not testing wallet_scan. I think they should only be testing the sparse chain output from wallet_scan.

I think we should make use of the label feature in bitcoin core rpc so wallet_scan takes a label to scan over (perhaps optional). In the example we should somehow expose the label.

Maybe call wallet_scan fetch_sparse_chain.

@rajarshimaitra
Copy link
Collaborator Author

Thanks @LLFourn for the review.. Updated as suggested.

  • Helper functions moved to main.rs.
  • wallet_sync moved to lib.rs.
  • Couldn't remove the struct RpcClient as bdk_cli::handle_commands expect a generic trait bound Broadcast and we can't impl foreign traits on foreign types.
  • Blockchain tests only testing for sparsechain structure.
  • We can't do create transaction test if we aren't fully syncing the tracker, which is the case currently as we are only getting the update_sparsechain from wallet_sync.

@LLFourn
Copy link
Owner

LLFourn commented Feb 22, 2023

Thanks @rajarshimaitra.

* Helper functions moved to `main.rs`.

* `wallet_sync` moved to `lib.rs`.

* Couldn't remove the struct `RpcClient` as `bdk_cli::handle_commands` expect a generic trait bound `Broadcast` and we can't impl foreign traits on foreign types.

We could fix this by getting rid of the Broadcast trait and just passing in a closure to the cli lib's main handler that takes a tx and broadcasts it. See #198

* Blockchain tests only testing for sparsechain structure.

* We can't do create transaction test if we aren't fully syncing the tracker, which is the case currently as we are only getting the `update_sparsechain` from `wallet_sync`.

This is good because this library doesn't provide create tx functionality.

@rajarshimaitra
Copy link
Collaborator Author

We could fix this by getting rid of the Broadcast trait and just passing in a closure to the cli lib's main handler that takes a tx and broadcasts it. See #198

That totally makes sense.. Will update accordingly.. Should I just build this over #198 or port the change?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants