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

Remove legacy wallet creation #764

Merged

Conversation

furszy
Copy link
Member

@furszy furszy commented Oct 6, 2023

Fixes #763

Preventing users from creating a legacy wallet prior to its deprecation in the upcoming releases.

Note:
This is still available using the createwallet RPC command.

Future Note:
Would be nice to re-write this modal as a wizard. And improve the design.

Pre-Changes Screenshot Screenshot 2023-10-06 at 11 30 14
Post-Changes Screenshot Screenshot 2023-10-06 at 11 32 58

@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 6, 2023

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK pablomartin4btc, achow101, hebasto
Stale ACK Sjors

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@hebasto
Copy link
Member

hebasto commented Oct 6, 2023

@furszy Thank you!

Concept ACK.

@hebasto hebasto added this to the 26.0 milestone Oct 6, 2023
@hebasto hebasto changed the title gui: remove legacy wallet creation Remove legacy wallet creation Oct 6, 2023
@hebasto hebasto added the Wallet label Oct 6, 2023
@furszy furszy force-pushed the 2023_gui_remove_legacy_wallet_creation branch from 9702028 to 6b83663 Compare October 6, 2023 14:04
@hebasto
Copy link
Member

hebasto commented Oct 6, 2023

It would be nice to have screenshots "before" and "after" in the PR description.

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

Approach ACK 6b83663.

Tested a build compiled with no SQLite support:

image

@furszy furszy force-pushed the 2023_gui_remove_legacy_wallet_creation branch 2 times, most recently from 3336abb to 038616b Compare October 6, 2023 14:56
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK 038616b

@hebasto
Copy link
Member

hebasto commented Oct 6, 2023

cc @achow101 @Sjors @luke-jr

Copy link
Member

@Sjors Sjors left a comment

Choose a reason for hiding this comment

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

tACK 038616b

I also checked the wallet creation error message when sqlite is not compiled (./configure --without-sqlite).

(text can always be changed later, so don't let my remarks block a merge)

src/qt/forms/createwalletdialog.ui Outdated Show resolved Hide resolved
src/qt/forms/createwalletdialog.ui Outdated Show resolved Hide resolved
@furszy furszy force-pushed the 2023_gui_remove_legacy_wallet_creation branch from 038616b to b442580 Compare October 6, 2023 20:22
@furszy
Copy link
Member Author

furszy commented Oct 6, 2023

Updated per Sjors's feedback. Thanks.

Copy link
Contributor

@pablomartin4btc pablomartin4btc left a comment

Choose a reason for hiding this comment

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

tACK b442580

Comment on lines +39 to +45
<item>
<widget class="QLabel" name="label_subdescription">
<property name="text">
<string>Please provide a name and, if desired, enable any advanced options</string>
</property>
</widget>
</item>
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: is this not implicit? Perhaps we can set it as the tool-tip of the "Create" button? Also I think extracomment attribute for the <string> tag would be needed for the translation.

Suggested change
<item>
<widget class="QLabel" name="label_subdescription">
<property name="text">
<string>Please provide a name and, if desired, enable any advanced options</string>
</property>
</widget>
</item>

@DrahtBot DrahtBot requested review from Sjors and hebasto October 6, 2023 21:45
@achow101
Copy link
Member

achow101 commented Oct 6, 2023

ACK b442580

@hebasto
Copy link
Member

hebasto commented Oct 7, 2023

re-ACK b442580

@DrahtBot DrahtBot removed the request for review from hebasto October 7, 2023 10:45
@hebasto hebasto merged commit d2b8c5e into bitcoin-core:master Oct 7, 2023
16 checks passed
@furszy furszy deleted the 2023_gui_remove_legacy_wallet_creation branch October 7, 2023 12:29
Copy link
Contributor

@pablomartin4btc pablomartin4btc left a comment

Choose a reason for hiding this comment

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

post-merge, error message displayed as expected when sqlite is not available (forced with ./configure --without-sqlite)

image

@bitcoin-core bitcoin-core locked and limited conversation to collaborators Oct 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove BDB checkbox in wallet creation dialog
6 participants