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.

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

if no wallet name was provided, use one derived from the descriptor #57

Conversation

ulrichard
Copy link
Contributor

@ulrichard ulrichard commented Nov 25, 2021

Description

if no wallet name was provided, use one derived from the descriptor

Notes to the reviewers

Is there a better place to use wallet_name_from_descriptor so that the generated name shows up in the unit tests instead of the placeholder?

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

@ulrichard ulrichard force-pushed the feature/default-wallet-name-from-descriptor branch 4 times, most recently from fd0c3e3 to 7dbc74d Compare November 26, 2021 08:45
@notmandatory
Copy link
Member

notmandatory commented Nov 28, 2021

How about making WalletOpts.wallet an Option<String> ? Then you could avoid the "main_marker_will_be_replaced" default value and only do the replacement when WalletOpts.wallet is None; such as in the open_database function you can use unwrap_or to call your wallet_name_from_descriptor function.

@ulrichard ulrichard force-pushed the feature/default-wallet-name-from-descriptor branch from 7dbc74d to e7d5d27 Compare November 30, 2021 07:20
@ulrichard
Copy link
Contributor Author

@notmandatory Yes, of course. Sometimes the solution is so obvious that I have no clue why I didn't find it myself. Thanks.

@ulrichard ulrichard force-pushed the feature/default-wallet-name-from-descriptor branch 3 times, most recently from 5592a9d to 773f2db Compare December 2, 2021 07:40
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 773f2db. Code changes looks good to me.

Below are some minor nits.

I think this warrant an addition in changelog. This is probably a breaking change and we should document this.

src/bdk_cli.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/bdk_cli.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/bdk_cli.rs Outdated Show resolved Hide resolved
@ulrichard ulrichard force-pushed the feature/default-wallet-name-from-descriptor branch 4 times, most recently from e9ca941 to 025020d Compare December 15, 2021 07:38
@ulrichard ulrichard force-pushed the feature/default-wallet-name-from-descriptor branch from 025020d to adfdd8c Compare January 5, 2022 06:19
@ulrichard ulrichard marked this pull request as ready for review January 5, 2022 06:20
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 adfdd8c

Verified that each new descriptor is creating its own wallet directory. Updated checksum mismatch error message is being displayed for wallet with same name but different descriptor.

Just one single nit.

src/bdk_cli.rs Outdated Show resolved Hide resolved
@ulrichard ulrichard force-pushed the feature/default-wallet-name-from-descriptor branch from adfdd8c to 292e638 Compare January 6, 2022 12:41
@ulrichard ulrichard force-pushed the feature/default-wallet-name-from-descriptor branch from 292e638 to e52e819 Compare January 11, 2022 06:48
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.

ReACK e52e819

Tested it locally. All seem to be working fine.

There are rooms for few improvement but that can be done in a future PR.

This LGTM.

@notmandatory will wait for you take a look before merging.

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 e52e819

Works as expected. One suggestion for future PRs is to separate unrelated changes in to different PRs or at least separate commits. In this case the bdk version upgrade changes were mixed with the wallet name generation changes which made it a little harder to review. But anyway a good usability improvement!

@notmandatory notmandatory merged commit 49c9a9f into bitcoindevkit:master Jan 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants