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.

Sign up for GitHub

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

Jump to bottom

Create wallet dir only when necessary #116

Merged
merged 1 commit into from
Sep 23, 2022
Merged

Create wallet dir only when necessary #116

merged 1 commit into from
Sep 23, 2022

Conversation

danielabrozzoni
Copy link
Member

Description

Making sure to create the wallet dir only when necessary (database is either sqlite or key-value)

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

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@rajarshimaitra
Copy link
Contributor

rajarshimaitra commented Sep 5, 2022 •
edited
Loading

@danielabrozzoni I have modified this part of the code in #102 .. We needed to streamline the directory creation for many directories (includeing bitcoind and electrsd dirs along with wallet files) and current function flow wasn't ideal for it..

I don't know whats the good way to go, but all the new PRs in these locations are going to heavily conflict with #102.. Its always this problem of having big refactoring PR open for a long time. All downstream works gets conflicted..

Maybe we should push in #102 and #104 before making further updates? The resultant structure of the library might also guide approach for the following wasm changes.. The current state of the library is an "intermediate" one..

@danielabrozzoni
Copy link
Member Author

I understand if you don't want to rebase your PR on top of this one, but at least check that this same logic of "let's avoid creating the data directory if we don't need it" (which is a prerequisite for wasm) is still doable after #102, otherwise I have to re-refactor the whole code.

@danielabrozzoni
Copy link
Member Author

Also, how long will it take for #102 and #104? They are both huge, are they going to be merged soon?

@rajarshimaitra
Copy link
Contributor

I understand if you don't want to rebase your PR on top of this one, but at least check that this same logic of "let's avoid creating the data directory if we don't need it" (which is a prerequisite for wasm) is still doable after #102, otherwise I have to re-refactor the whole code.

The logic looks good to me.. 102 is meant to make wasm works easier, the handle_command function there only takes a CliOpts, so the wasm code should be easier to write, as you don't need to pass in a backend.. This PR can be done over 102 too, but probably over a different function.

Also, how long will it take for #102 and #104? They are both huge, are they going to be merged soon?

The code is ready.. No new modification would be required.. If small fixes remains can be done in future PRs.. We are just waiting for bdk 22.. After that it should be good to go..

@danielabrozzoni
Copy link
Member Author

The code is ready.. No new modification would be required.. If small fixes remains can be done in future PRs.. We are just waiting for bdk 22.. After that it should be good to go..

But there are still no ACKs, and the review process will take quite some time, so it will probably be merged in a couple of weeks? Does it make sense to wait weeks before merging this one?
As you wish, for me it's fine rebasing this one, it just doesn't seem worth it to make it wait so long...

@notmandatory
Copy link
Member

notmandatory commented Sep 6, 2022 •
edited
Loading

But there are still no ACKs, and the review process will take quite some time, so it will probably be merged in a couple of weeks? Does it make sense to wait weeks before merging this one? As you wish, for me it's fine rebasing this one, it just doesn't seem worth it to make it wait so long...

I just now added an ACK on #102 with a little explanation, I think it can be made ready to merge shortly after bdk 0.22 is released. My preference would be to then make the next bdk-cli release. With also this PR or similar added to #102.

@notmandatory notmandatory added this to the Release 0.6.0 milestone Sep 6, 2022
notmandatory
notmandatory approved these changes Sep 6, 2022
View reviewed changes
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.

tACK 74f6553

I used below command to confirm wallet worked and no dirs created as expected.

cargo run --no-default-features --features esplora-ureq,repl -- repl --descriptor "wpkh(tpubEBr4i6yk5nf5DAaJpsi9N2pPYBeJ7fZ5Z9rmN4977iYLCGco1VyjB9tvvuvYtfZzjD5A8igzgw3HeWeeKFmanHYqksqZXYXGsw5zjnj7KM9/*)"

@rajarshimaitra
Copy link
Contributor

Does it make sense to wait weeks before merging this one?

I am ok with merging this one.. But its getting really difficult to rebase 102 with more inb4 changes.. It could even require rewriting some changes totally, depending upon where it is being made..

For this one its straight forward so its fine to go in.. Probably the feature gate will moved onto a different function, but I can handle that at rebase.. But more intrusive changes at this stage in the crate would get difficult..

I am expecting 102 to go in as soon as the next bdk 22 release..

@danielabrozzoni
Copy link
Member Author

danielabrozzoni commented Sep 7, 2022 via email

This was referenced Sep 7, 2022
Merged
Merged
@notmandatory notmandatory mentioned this pull request Sep 13, 2022
Closed
26 tasks
@danielabrozzoni
Copy link
Member Author

After #102 this is quite difficult to do, but rebasing #117 I noticed that we might not need it for wasm...

@danielabrozzoni danielabrozzoni force-pushed the fix/wallet_dir branch 4 times, most recently from 714c76a to a4f58dc Compare September 14, 2022 12:11
@danielabrozzoni
Copy link
Member Author

Nevermind, I ended up needing it for #117. This is now ready for review :)

rajarshimaitra
rajarshimaitra reviewed Sep 15, 2022
View reviewed changes
Copy link
Contributor

@rajarshimaitra rajarshimaitra left a comment

Choose a reason for hiding this comment

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

tACK a4f58dc

Just one nit..

@danielabrozzoni
open_database avoids creating a wallet dir if...
bff2d37 
...not necessary

This is useful for the wasm integration, where we want to call the
`open_database` function directly, without wanting it to create any
directories.
Note that when using bdk-cli normally, it won't create a db directory
if it's useless, but it will still create a base directory (even if
empty). This is suboptimal, but fixing it would require a quite big
refactor to the code.
rajarshimaitra
rajarshimaitra approved these changes Sep 20, 2022
View reviewed changes
Copy link
Contributor

@rajarshimaitra rajarshimaitra left a comment

Choose a reason for hiding this comment

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

Thanks @danielabrozzoni

tACK bff2d37

LGTM.. Will wait for @notmandatory if he has anything to add. Or its good to go..

@notmandatory notmandatory merged commit 183e85e into bitcoindevkit:master Sep 23, 2022
rajarshimaitra added a commit that referenced this pull request Sep 26, 2022
@rajarshimaitra
Merge #117: Update bdk cli for wasm
b5f9177 
731dc74 Add wasm build in CI (Daniela Brozzoni)
b3469b4 Add wasm support (Daniela Brozzoni)

Pull request description:

  ### Description

  This PR adds a module to bdk-cli that exposes some structures/functions to WASM. You can see it in use here: bitcoindevkit/bitcoindevkit.org#118

  ### Notes to the reviewers

  Based on #116

  ### 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
  * [x] I've updated `CHANGELOG.md`

  #### Bugfixes:

  * [ ] This pull request breaks the existing API
  * [ ] I've added tests to reproduce the issue which are now passing
  * [ ] I'm linking the issue being fixed by this PR

ACKs for top commit:
  rajarshimaitra:
    ACK 731dc74

Tree-SHA512: 69011ab6cb421f7e6f7dd000f9eaf5581428b2288358e31ffaec4425996951443fa2fc79f99db7de9c030d6a3fda57d25eee11564cf2fab9c745da593324f763
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@notmandatory notmandatory notmandatory approved these changes

@rajarshimaitra rajarshimaitra rajarshimaitra approved these changes

Assignees

@danielabrozzoni danielabrozzoni

Labels
None yet
Projects
BDK-CLI Roadmap
Archived in project
Milestone
Release 0.6.0
Development

Successfully merging this pull request may close these issues.
3 participants
@danielabrozzoni @rajarshimaitra @notmandatory