bitcoindevkit / bdk-cli

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

The Great Reset #99

Closed rajarshimaitra closed 2 years ago

rajarshimaitra commented 2 years ago

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

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

Notes to the reviewers

Checklists

All Submissions:

New Features:

rajarshimaitra commented 2 years ago

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

rajarshimaitra commented 2 years ago

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 commented 2 years ago

CI fix PR opened here https://github.com/bitcoindevkit/bdk-cli/pull/100

rajarshimaitra commented 2 years ago

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 commented 2 years ago

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

rajarshimaitra commented 2 years ago

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

notmandatory commented 2 years ago

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 commented 2 years ago

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

rajarshimaitra commented 2 years ago

Pushed some minor doc fixes..

rajarshimaitra commented 2 years ago

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

rajarshimaitra commented 2 years ago

Rebased on master and removed author list changes..

notmandatory commented 2 years ago

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

rajarshimaitra commented 2 years ago

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 commented 2 years ago

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