Remove legacy wallet creation

furszy commented at 1:35 pm on October 6, 2023: member

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.

DrahtBot commented at 1:35 pm on October 6, 2023: contributor

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 commented at 1:36 pm on October 6, 2023: member

@furszy Thank you!

Concept ACK.

hebasto added this to the milestone 26.0 on Oct 6, 2023

hebasto renamed this:
~~gui: remove legacy wallet creation~~
Remove legacy wallet creation
on Oct 6, 2023

hebasto added the label Wallet on Oct 6, 2023

furszy force-pushed on Oct 6, 2023

hebasto commented at 2:09 pm on October 6, 2023: member

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

in src/qt/forms/createwalletdialog.ui:29 in 6b83663eb8 outdated

24+       <horstretch>0</horstretch>
25+       <verstretch>0</verstretch>
26+      </sizepolicy>
27+     </property>
28+     <property name="text">
29+      <string>&lt;html&gt;&lt;head/&gt;&lt;body&gt;&lt;p&gt;You are one step away from creating your new descriptors wallet!&lt;/p&gt;&lt;p&gt;Please enter the name and, if desired, enable any custom options&lt;/p&gt;&lt;/body&gt;&lt;/html&gt;</string>

hebasto commented at 2:20 pm on October 6, 2023:

HTML markups will go as a part translatable strings. Can it be avoided?

Is it better to replace “custom options” with “advanced options” as they are named in https://github.com/bitcoin-core/gui/blob/6b83663eb8ed06216351e76439b825de4840a481/src/qt/forms/createwalletdialog.ui#L111 ?

hebasto commented at 2:22 pm on October 6, 2023:

And “new descriptors wallet” –> “new descriptor wallet” probably?

furszy commented at 3:00 pm on October 6, 2023:

thx, pushed

hebasto commented at 2:29 pm on October 6, 2023: member

Approach ACK 6b83663eb8ed06216351e76439b825de4840a481.

Tested a build compiled with no SQLite support:

furszy force-pushed on Oct 6, 2023

hebasto approved

hebasto commented at 4:23 pm on October 6, 2023: member

ACK 038616b57e95197a421430cb179d967043de9999

hebasto commented at 4:24 pm on October 6, 2023: member

cc @achow101 @Sjors @luke-jr

in src/qt/forms/createwalletdialog.ui:29 in 038616b57e outdated

24+       <horstretch>0</horstretch>
25+       <verstretch>0</verstretch>
26+      </sizepolicy>
27+     </property>
28+     <property name="text">
29+      <string>You are one step away from creating your new descriptor wallet!</string>

Sjors commented at 5:03 pm on October 6, 2023:

This line just seems extra clutter. In any case please don’t use the scary word “descriptor”.

achow101 commented at 7:38 pm on October 6, 2023:

Agree that there’s no reason to mention “descriptor” here.

furszy commented at 8:33 pm on October 6, 2023:

Yeah sure. Word removed 👍🏼 .

About the extra line topic: Based on my experience, the GUI should be explicative on what is going to happen (and also friendly if possible). An actionable pop up with no description and only buttons is quite bad in terms of UX (unless the design is great, which isn’t the case here).

Still, this line is just a tiny tiny thing. I think this dialog is just bad. Would be nice to re-write it as a wizard and improve its design in the future.

in src/qt/forms/createwalletdialog.ui:42 in 038616b57e outdated

37+    </widget>
38+   </item>
39+   <item>
40+    <widget class="QLabel" name="label_subdescription">
41+     <property name="text">
42+      <string>Please enter the name and, if desired, enable any advanced options</string>

Sjors commented at 5:13 pm on October 6, 2023:

Please provide a name …

furszy commented at 8:33 pm on October 6, 2023:

Done, thx.

Sjors approved

Sjors commented at 5:13 pm on October 6, 2023: member

tACK 038616b57e95197a421430cb179d967043de9999

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)

gui: remove legacy wallet creation b442580ed2

furszy force-pushed on Oct 6, 2023

furszy commented at 8:38 pm on October 6, 2023: member

Updated per Sjors’s feedback. Thanks.

in src/qt/forms/createwalletdialog.ui:45 in b442580ed2

40+    <widget class="QLabel" name="label_subdescription">
41+     <property name="text">
42+      <string>Please provide a name and, if desired, enable any advanced options</string>
43+     </property>
44+    </widget>
45+   </item>

pablomartin4btc commented at 9:44 pm on October 6, 2023:

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.

pablomartin4btc approved

pablomartin4btc commented at 9:45 pm on October 6, 2023: contributor

tACK b442580ed2a6173f0cfb86f265887d783dde3ff8

DrahtBot requested review from Sjors on Oct 6, 2023

DrahtBot requested review from hebasto on Oct 6, 2023

achow101 commented at 10:53 pm on October 6, 2023: member

ACK b442580ed2a6173f0cfb86f265887d783dde3ff8

hebasto commented at 10:45 am on October 7, 2023: member

re-ACK b442580ed2a6173f0cfb86f265887d783dde3ff8

DrahtBot removed review request from hebasto on Oct 7, 2023

hebasto merged this on Oct 7, 2023

hebasto closed this on Oct 7, 2023

furszy deleted the branch on Oct 7, 2023

pablomartin4btc commented at 8:58 pm on October 7, 2023: contributor

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

bitcoin-core locked this on Oct 6, 2024

Remove legacy wallet creation #764

Reviews