The Great Reset #99

rajarshimaitra · 2022-06-15T22:18:26Z

Description

This is a massive refactoring PR that changes the whole structure of the crate. Previously it was written like a library
to be used to create the bdk-cli app. But eventually the crate itself became the app. This PR attempts to remove the remaining
lib like patterns in the code, and make it a pure binary crate.

This makes the code more modular and makes it look like a typical binary rust crate.

There was no real good way to structure the change into separate commits, so I made one single big one.. The best way to review is to look at the final structure of the code itself, not the change set.

The crate has following modules now

main : The main app runtime
commands: Includes all the structopt commands used by bdk-cli.
handlers: Include all the command handlers used buy the app.
utils: Include all the utility and helper functions
Backend : Defines the backend node process, and its related methods. (This will be filled more with Unleash the power of Bitcoin Core into bdk-cli #92).

Apart from the structure changes there are few other changes that took place

Almost all of the previous doc comments are removed. As they were written to use bdk-cli as a lib. Instead new structopts "comments" are added to describe the app functionality better. As a result the app --help commands are more elaborate and descriptive now. I have also removed few redundant description messages used before, that would mess up the help comments. And as a by product it solves Bug in help doc #93.
bdk is updated to v0.19.0
bdk-reserves is updated with current version pointing to bdk v0.19.0.
Default database is now sqlite.

Overall I think I managed not to break anything.

Currently this change will remove most of the previous documentation on the crate. But those aren't useful to context of bdk-cli after this change.. My proposal would be reproduce the README instructions itself in doc.rs landing page.

We also need to update the README to reflect these changes.. I will open that up in a separate PR.

I also haven't updated changelog yet.. Not sure yet how to describe the change in short.. Will do that once this is almost finalized..

Notes to the reviewers

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 · 2022-06-15T22:37:01Z

Currently trying to pass the CI.. Might require few more updates..

rajarshimaitra · 2022-06-15T23:06:51Z

I think I have hit a dead lock..

Our Previous CI had reserves with other blockchains features.. But the way reserve command is written it cannot be used with anything other than electrum. I am not sure why it wasn't failing before..

So Either we need to enforce that in build.rs or update the reserve command to work with any backend (not sure if possible, bdk-reserves uses the Electrum Api for the job)..

I have updated the CI file to keep only the reserve, electrum test..

But I think it won't be reflected in the CI of this PR.. So tests will now always fail here..

One easy way out now is to make another PR with the CI change, get that merged, and rebase this one on top of the new CI..

Any suggestion @notmandatory ?

Update: All other tests are passing except the reverses, esplora/comapct_filters.

rajarshimaitra · 2022-06-15T23:18:08Z

CI fix PR opened here #100

rajarshimaitra · 2022-06-17T08:47:25Z

cc @notmandatory

Waiting for some concept ack on this so I can start rebasing #92 on top of this.. That will take some no trivial refactoring..

rajarshimaitra · 2022-06-17T14:21:05Z

Rebased on top of #100 to make this dependent on it..

rajarshimaitra · 2022-06-17T14:25:02Z

Removed bdk-reserves patch after v0,19.0 update..

notmandatory · 2022-06-17T16:01:34Z

Concept ACK, but looks like some tests still broken. I'm going to be tied up with a PlebFi here in LA this weekend but can spend some time reviewing next week!

rajarshimaitra · 2022-06-17T17:24:10Z

No issues.. I will work on fixing the tests, and then move #92 on top of it too..

rajarshimaitra · 2022-06-20T08:56:05Z

Pushed some minor doc fixes..

rajarshimaitra · 2022-06-27T10:10:11Z

Added a commit to remove external base64 dependency + some clippy nits..

This PR moves all the components into different module. Checkout PR description for more details.

We don't need base64 because rust-bitcoin already exposes a version of that crate that we can use. Removing one more parallel dependency.

rajarshimaitra · 2022-06-27T18:37:19Z

Rebased on master and removed author list changes..

notmandatory

I didn't review all the code in details since most of it is existing code that was moved. But the general reorganization makes sense.

I tested the commands help and it looked OK. But the repl command doesn't seem to be working, it won't take any command including exit, it just shows the help.

src/backend.rs

Cargo.toml

notmandatory · 2022-06-28T04:45:04Z

My suggestion for the changelog is something like this, based on your PR comments:

Reorganize existing code into new modules
Rewrite relevant doc comments as structopt help documentation
Update bdk and bdk-reserves to v0.19.0
Change default database to sqlite

rajarshimaitra · 2022-07-01T11:26:21Z

Thanks @notmandatory for the look.. Sorry it took me some time to get back to this.. ACK on all the comments.. Updated with a new commit for easier review..

notmandatory · 2022-07-06T05:38:26Z

I pushed a commit to fix a couple little typos and a problem I found in repl mode with command parsing, probably wasn't a new issue but noticed it when testing, had to enable clap::AppSettings::NoBinaryName.

https://docs.rs/structopt/latest/structopt/trait.StructOpt.html#method.from_iter_safe

notmandatory

ACK 292dd1e

292dd1e Fix repl mode command parsing (Steve Myers) 073f1c3 Update with review comments (rajarshimaitra) 4e8f830 revert author list change (rajarshimaitra) b09c405 Remove base64 dependency (rajarshimaitra) 1e70ff9 Refactor everything (rajarshimaitra) Pull request description:  ### Description This is a massive refactoring PR that changes the whole structure of the crate. Previously it was written like a library to be used to create the bdk-cli app. But eventually the crate itself became the app. This PR attempts to remove the remaining lib like patterns in the code, and make it a pure binary crate. This makes the code more modular and makes it look like a typical binary rust crate. There was no real good way to structure the change into separate commits, so I made one single big one.. The best way to review is to look at the final structure of the code itself, not the change set. The crate has following modules now - `main` : The main app runtime - `commands`: Includes all the structopt commands used by bdk-cli. - `handlers`: Include all the command handlers used buy the app. - `utils`: Include all the utility and helper functions - `Backend` : Defines the backend node process, and its related methods. (This will be filled more with bitcoindevkit#92). Apart from the structure changes there are few other changes that took place - Almost all of the previous doc comments are removed. As they were written to use bdk-cli as a lib. Instead new structopts "comments" are added to describe the app functionality better. As a result the app `--help` commands are more elaborate and descriptive now. I have also removed few redundant description messages used before, that would mess up the help comments. And as a by product it solves bitcoindevkit#93. - bdk is updated to v0.19.0 - bdk-reserves is updated with current version pointing to bdk v0.19.0. - Default database is now sqlite. Overall I think I managed not to break anything. Currently this change will remove most of the previous documentation on the crate. But those aren't useful to context of bdk-cli after this change.. My proposal would be reproduce the README instructions itself in doc.rs landing page. We also need to update the README to reflect these changes.. I will open that up in a separate PR. I also haven't updated changelog yet.. Not sure yet how to describe the change in short.. Will do that once this is almost finalized.. ### Notes to the reviewers ### 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 #### New Features: * [ ] I've added tests for the new feature * [ ] I've added docs for the new feature * [ ] I've updated `CHANGELOG.md` ACKs for top commit: notmandatory: ACK 292dd1e Tree-SHA512: 895d8088bf93a481fd776e2ac5fe85926f13b7b4535f17b9edd3c0363a89dc3689e28c6e13dbcac3970bc00e3ff206f402e94406f3b3688c9e4a7f9d31b20e40

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

rajarshimaitra requested a review from notmandatory June 15, 2022 22:18

rajarshimaitra force-pushed the restructure branch 3 times, most recently from 0e07ab4 to 428e919 Compare June 15, 2022 22:36

rajarshimaitra force-pushed the restructure branch 3 times, most recently from 21a9b51 to 124356b Compare June 15, 2022 23:02

rajarshimaitra force-pushed the restructure branch from 124356b to b160f31 Compare June 17, 2022 14:20

rajarshimaitra force-pushed the restructure branch from b160f31 to 1f0b2f6 Compare June 17, 2022 14:24

rajarshimaitra force-pushed the restructure branch from 1f0b2f6 to 26e40bc Compare June 17, 2022 14:28

rajarshimaitra force-pushed the restructure branch from 26e40bc to 149f7e2 Compare June 17, 2022 17:21

rajarshimaitra force-pushed the restructure branch from 149f7e2 to e2e9e14 Compare June 20, 2022 08:55

rajarshimaitra mentioned this pull request Jun 20, 2022

Satoshi's Calculator. #102

Merged

6 tasks

notmandatory added this to the Release 0.6.0 milestone Jun 21, 2022

notmandatory mentioned this pull request Jun 21, 2022

Update bdk and bdk-reserves to 0.19.0 #98

Closed

9 tasks

notmandatory assigned rajarshimaitra Jun 21, 2022

notmandatory mentioned this pull request Jun 21, 2022

Fix stdweb audit advisotry RUSTSEC-2020-0056 #85

Closed

rajarshimaitra mentioned this pull request Jun 24, 2022

Remove some of the reserves and regtest features #100

Merged

3 tasks

rajarshimaitra force-pushed the restructure branch from e2e9e14 to b3d81b4 Compare June 24, 2022 12:43

rajarshimaitra force-pushed the restructure branch from 45df2e4 to 02fe4f8 Compare June 27, 2022 11:15

rajarshimaitra added 3 commits June 28, 2022 00:06

Refactor everything

1e70ff9

This PR moves all the components into different module. Checkout PR description for more details.

Remove base64 dependency

b09c405

We don't need base64 because rust-bitcoin already exposes a version of that crate that we can use. Removing one more parallel dependency.

revert author list change

4e8f830

rajarshimaitra force-pushed the restructure branch from 02fe4f8 to 4e8f830 Compare June 27, 2022 18:36

rajarshimaitra mentioned this pull request Jun 27, 2022

Update crate documentation #104

Merged

3 tasks

notmandatory reviewed Jun 28, 2022

View reviewed changes

src/backend.rs Outdated Show resolved Hide resolved

Cargo.toml Outdated Show resolved Hide resolved

Cargo.toml Outdated Show resolved Hide resolved

Update with review comments

073f1c3

notmandatory mentioned this pull request Jul 5, 2022

W27 BDK Library Team Call bitcoindevkit/.github#12

Closed

24 tasks

Fix repl mode command parsing

292dd1e

notmandatory approved these changes Jul 6, 2022

View reviewed changes

notmandatory merged commit d8e93ab into bitcoindevkit:master Jul 6, 2022

notmandatory added the enhancement New feature or request label Jul 14, 2022

rajarshimaitra mentioned this pull request Jul 19, 2022

Change default network to Regtest and default wallet db to Sqlite #84

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

The Great Reset #99

The Great Reset #99

rajarshimaitra commented Jun 15, 2022 •

edited

Loading

rajarshimaitra commented Jun 15, 2022

rajarshimaitra commented Jun 15, 2022 •

edited

Loading

rajarshimaitra commented Jun 15, 2022

rajarshimaitra commented Jun 17, 2022

rajarshimaitra commented Jun 17, 2022 •

edited

Loading

rajarshimaitra commented Jun 17, 2022

notmandatory commented Jun 17, 2022

rajarshimaitra commented Jun 17, 2022

rajarshimaitra commented Jun 20, 2022

rajarshimaitra commented Jun 27, 2022 •

edited

Loading

rajarshimaitra commented Jun 27, 2022

notmandatory left a comment

notmandatory commented Jun 28, 2022

rajarshimaitra commented Jul 1, 2022

notmandatory commented Jul 6, 2022 •

edited

Loading

notmandatory left a comment

The Great Reset #99

The Great Reset #99

Conversation

rajarshimaitra commented Jun 15, 2022 • edited Loading

Description

Notes to the reviewers

Checklists

All Submissions:

New Features:

rajarshimaitra commented Jun 15, 2022

rajarshimaitra commented Jun 15, 2022 • edited Loading

rajarshimaitra commented Jun 15, 2022

rajarshimaitra commented Jun 17, 2022

rajarshimaitra commented Jun 17, 2022 • edited Loading

rajarshimaitra commented Jun 17, 2022

notmandatory commented Jun 17, 2022

rajarshimaitra commented Jun 17, 2022

rajarshimaitra commented Jun 20, 2022

rajarshimaitra commented Jun 27, 2022 • edited Loading

rajarshimaitra commented Jun 27, 2022

notmandatory left a comment

Choose a reason for hiding this comment

notmandatory commented Jun 28, 2022

rajarshimaitra commented Jul 1, 2022

notmandatory commented Jul 6, 2022 • edited Loading

notmandatory left a comment

Choose a reason for hiding this comment

rajarshimaitra commented Jun 15, 2022 •

edited

Loading

rajarshimaitra commented Jun 15, 2022 •

edited

Loading

rajarshimaitra commented Jun 17, 2022 •

edited

Loading

rajarshimaitra commented Jun 27, 2022 •

edited

Loading

notmandatory commented Jul 6, 2022 •

edited

Loading