-
Notifications
You must be signed in to change notification settings - Fork 65
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
Satoshi's Calculator. #102
Satoshi's Calculator. #102
Conversation
f0824ab
to
c3ef564
Compare
Pushed some more updates and refactoring.. Rearranged the commit history itself for easier reviews instead of new commits.. |
1204412
to
4b61e64
Compare
168554f
to
5d2556c
Compare
@notmandatory Thanks for the merge of #99. This one though needs a little more looks because a lot of codes and behaviors are updated.. This finishes up the remaining works of adding integration testing with auto deployed regtest nodes. Rebased on top of master. Updated the PR description.. This still lacks many crate level documentation, that will be completed in #104 which commits on top of this.. |
@notmandatory I feel like #104 could also be included in this PR itself.. That has no code changes but documentation updates only.. |
Hi @rajarshimaitra After playing with it for a few hours I noticed a few things:
Overall everything looks good, Satoshi's Calculator is pretty awesome for learning I enjoyed using it Thanks! |
Thanks for the look @waterst0ne .. Those are good suggestions, will update them soon.. |
e6e602c
to
831f339
Compare
Thanks @waterst0ne for the review.. Those were helpful.. I have addressed your comments on last 3 commits. e728577 - Instead of changing the command name, I elaborated the help doc. The command not only generate blocks but also funds the internal wallet. c1921a7 - Thanks for trying out the repl mode, and I agree having all the commands together is confusing.. I restructured the repl command and have them categorized in So repl now looks like this
And for each subcommand you can ask for
e6e602c - Adds a generic
We don't have these in our command list but we can use generic Things to figure out
|
I added a comment on #726. |
@notmandatory added a v0.22 update patch here 716efe8 pointing to the But for some reason the git link is not working in CI for bdk-reserve. It seems to work fine in my local.. Did the v0.22.0 update here instead of a new PR, because we have many modification over master here, and wanted to cover them. Not Much changes in the code due to update. But we might need to change the |
7e67ca3
to
6cb4acf
Compare
prelim ACK, @waterst0ne and I did testing on earlier versions in July of this and the main suggestion from that testing was addressed when @rajarshimaitra added sub-menus in the repl. I'd like to go through it again one more time once #118 is merged and this PR re-based on it. Plus we need the next bdk-reserves release including bitcoindevkit/bdk-reserves#11. Long story short, I think this PR is only missing dependency updates. |
- Update dependency - Disable `regtest-esplora-*` backends for further testing
- Add a new NodeCommand to handle backend node operation. - Fix the REPL mode to show commands via category - Other auxiliary fixes.
- Update the handlers to handle node commands - Main `handle_command()` function signature changed to only take in a `CliOpts`. This is helpful for wasm or any other binding produced on bdk-cli.
Add the backend Node mechanism.
Various fixes and moves in utility module
6cb4acf
to
322f6f1
Compare
Rebased on master and consolidated commit history.. |
33185d7
to
fa0454e
Compare
Everything looks good with the electrum backend and bdk client, I did a bunch of testing with repl mode too. I pushed a small commit so that in repl mode the backend node is only created once, otherwise it's too slow to use. I also found that with rpc mode the bdk wallet is unable to sync, though it looks like it works in the integration test in CI. I'm using a mac and the error I'm getting is below for cli or local integration testing. I think this is likely related to this issue in the ord project. I'll make an issue for what I found with the mac, I still want to confirm it's working on a linux system in cli and repl mode, if so then I'm ready to put this PR in.
|
Thanks @notmandatory for the report. Unfortunately I am not being able to create this in local. But the error message looks very familiar as Evan reported in this issue. bitcoindevkit/bdk#650 (comment) It probably means there's an bitcoind instance already running that is accessing the node data directory, thus the Also from the command
It seems you are running regular
I checked your changes too.. ACK on taking the backend creation to global.. Though I tested the work flow again in linux machine and all seems to be working okay.. |
Thanks for retesting on Linux, the problem I'm seeing must just be on MacOS. And for ACKing my global repl |
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.
tACK fa0454e
f8a5999 Minor grammar and puctuation fixes (Steve Myers) e7b6854 Update with Readme fixes (rajarshimaitra) 52e8c61 Add all `possible_values` to network command option (Leonardo Lima) 179618c Update crate documentation (rajarshimaitra) Pull request description: ### Description After #99 the previous documentation have been removed and new docs as per `structopts` documentation. This PR adds more documentation across the crate.. This PR is above #102 , to accommodate all the further refactoring changes. The Readme About section have been updated with more details.. Readme format made aligned with the BDK project itself.. The Readme file is used itself as the crate level documentation in docs.rs too.. ### Checklists #### All Submissions: * [x] I've signed all my commits * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk-cli/blob/master/CONTRIBUTING.md) * [x] I ran `cargo fmt` and `cargo clippy` before committing ACKs for top commit: notmandatory: ACK f8a5999 Tree-SHA512: 26c5b3903b0215aa9841c4d1079bbdeb9f9d9d458c7e27dddb625db24eb364b73ca978bb2018f486215878f3601b5572ca58d5c202cb74325c992f3e7107d850
Description
fixes #62
fixes #76
On the name :
If Bitcoin wallet devs had to have a "calculator", this is what it might look like. One single interface to do all wallet and node operations.
This PR does the following
nodes.rs
module to define basic node operation apis on theNodes
enum.node
command inCliSubCommand
which has its own subcommandNodeSubCommand
, These subcommands are essentiallybitcoin-cli
calls. And only the basic ones are included so far. We can also compose multiplebitcoin-cli
calls to create our own commands in future.handlers
to handle newly addedNodeSubCommands
.utils
:regtest-*
features.datadir
to. Note that nowdatadir
is a global app option, not just available forregtest
nodes. Defaultdatadir
is the previous~/.bdk-bitcoin
.tests/integration.rs
that runs a basic wallet operation with a bdk-cli wallet connected to auto deployed regtest-bitcoin/electrum.Notes to the reviewers
@sandipndev @krtk6160. I feel this PR is now ready to try out #87 .
Also looking for more integration test ideas too add into.
basic
node
usage looks like thisThe biggest benefit is I think in the
repl
mode. That now kinda becomes an integrated tool the like python interpreter to operate a bdk-cli and a bitcoin backend from one single interface.I am excited to see what kind of demonstrations with bdk-cli we can create with this..
Checklists
All Submissions:
cargo fmt
andcargo clippy
before committingNew Features:
CHANGELOG.md