wallet: Allow user to navigate options while encrypting at creation

hernanmarino commented at 5:38 pm on March 16, 2023: contributor

This fixes #394. It adds a “Go back” button to the “Confirm wallet encryption” window, allowing the users to change the password if they want to. It also adds a Cancel button to the “Wallet to be encrypted” window. Prior to this change users had no option to alter the password, and were forced to either go ahead with wallet creation or cancel the whole process. Also, at the final window, they were shown a warning but with no option to cancel. The new workflow for wallet encryption and creation is similar to the following:

videoNavigation

DrahtBot commented at 5:38 pm on March 16, 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	alfonsoromanz, BrandonOdiwuor, hebasto
Concept ACK	luke-jr
Stale ACK	jarolrod, pablomartin4btc

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

pablomartin4btc approved

pablomartin4btc commented at 6:41 pm on March 16, 2023: contributor

Tested ACK. It works as expected according to the description of the PR and it fulfils the requirements of the related issue. Just as a reminder for other reviewers, if you want to verify if the wallet was encrypted or not, when a user creates an encrypted wallet, the password won’t be requested on loading or switching to the encrypted wallet, only on “send”; the private keys are encrypted not the wallet (.dat) itself.

in src/qt/askpassphrasedialog.cpp:110 in bd423555e9 outdated

109-                 QMessageBox::Yes|QMessageBox::Cancel,
110-                 QMessageBox::Cancel);
111+        QMessageBox msgBoxConfirm(QMessageBox::Question,
112+                                  tr("Confirm wallet encryption"),
113+                                  tr("Warning: If you encrypt your wallet and lose your passphrase, you will <b>LOSE ALL OF YOUR BITCOINS</b>!<br><br>"
114+                                     "Are you sure you wish to encrypt your wallet?"),

hebasto commented at 11:17 am on March 27, 2023:

Could we avoid changes in translatable strings in this PR?

hernanmarino commented at 3:49 pm on March 27, 2023:

Yes, will do.

hernanmarino commented at 2:15 pm on March 28, 2023:

I changed the code to reuse already existing strings, can you confirm it’s okey in this new way ?

in src/qt/askpassphrasedialog.cpp:113 in bd423555e9 outdated

112+                                  tr("Confirm wallet encryption"),
113+                                  tr("Warning: If you encrypt your wallet and lose your passphrase, you will <b>LOSE ALL OF YOUR BITCOINS</b>!<br><br>"
114+                                     "Are you sure you wish to encrypt your wallet?"),
115+                                  QMessageBox::Cancel | QMessageBox::Yes, this);
116+        msgBoxConfirm.button(QMessageBox::Yes)->setText(tr("Continue"));
117+        msgBoxConfirm.button(QMessageBox::Cancel)->setText(tr("Go back"));

hebasto commented at 11:19 am on March 27, 2023:

I’m not sure which label, “Go back” or just “Back”, is more expected in user experience.

ping @GBKS

GBKS commented at 11:41 am on March 27, 2023:

I think both are fine. Personally, I would go for “Back”, as the word “Go” doesn’t add any additional information.

pablomartin4btc commented at 2:03 pm on March 27, 2023:

I do agree with just “Back”.

hernanmarino commented at 2:14 pm on March 28, 2023:

Changed to “Back”

hebasto commented at 11:19 am on March 27, 2023: member

Concept ACK.

hebasto requested review from jarolrod on Mar 27, 2023

hebasto added the label Wallet on Mar 27, 2023

hernanmarino force-pushed on Mar 28, 2023

hernanmarino commented at 2:20 pm on March 28, 2023: contributor

Updated the code as per @hebasto suggestions.

pablomartin4btc approved

pablomartin4btc commented at 2:06 pm on May 5, 2023: contributor

re-ACK 78660e72001a2561c7ad3026367a69f65414dbd9. cc @jarolrod.

I seewallet_create_tx.py --descriptorstest is failing and you are re-running it, I don’t think it’s related with your change but let’s see.

jarolrod approved

jarolrod commented at 10:33 pm on May 19, 2023: member

ACK 78660e72001a2561c7ad3026367a69f65414dbd9

Tested for functionality and sanity checked the user flows between master and this PR.

in src/qt/askpassphrasedialog.cpp:161 in 78660e7200 outdated

157@@ -145,11 +158,7 @@ void AskPassphraseDialog::accept()
158                                      tr("The supplied passphrases do not match."));
159             }
160         }
161-        else
162-        {
163-            QDialog::reject(); // Cancelled
164-        }
165-        } break;
166+    } break;

hebasto commented at 12:16 pm on May 23, 2023:

Why this change?

hernanmarino commented at 1:39 am on June 17, 2023:

When originally testing, the reject() seemed redundant, because the window was also closing without this line. Since the next thing to happen was the method finishing / returning I assumed at that time that the default behaviour of QDialog class was being triggered. But now I was having second thoughts, and brought back the original behavior of calling an explicit reject.

hernanmarino force-pushed on Jun 17, 2023

hernanmarino commented at 1:43 am on June 17, 2023: contributor

After @hebasto ’s comment I decided to update the code to explicitly call reject() when cancelling the first dialog. This introduces one less change from the original code, and also makes the intention more explicit to developers / reviewers.

luke-jr commented at 1:57 am on June 23, 2023: member

Concept ACK

But shouldn’t the final confirmation dialog also have a “Go back” instead of “Cancel”?

(Better yet, why don’t we just show these warnings upfront in the password entry dialog?)

pablomartin4btc commented at 1:40 pm on June 23, 2023: contributor

But shouldn’t the final confirmation dialog also have a “Go back” instead of “Cancel”?

I think at that point, going back to the previous window, where you have to re-enter the password, won’t make much sense.

This is how it’s now:

pwd conf back

Now that I’ve re-tested it, once you have entered the passphrase, when you click on “Back”, doesn’t go back to anything, just close/ cancel the entire process, so if we agree on the use case I rather put back “Cancel” on the button label or I wouldn’t go back to the password either but to the “Create Wallet” window. Having said that I see in @jarolrod’s issue #394 that he wanted to go back to the “Encrypt wallet” window where you have to re-enter the password, so I’m not sure if we could add a “second” back, one to the creation wallet window and one for the encrypt wallet to re-enter the password, the reason behind going back to the “Create wallet window” is that perhaps you just realised you don’t want to bother, just create a regular (non encrypted) one.

So, same for the final confirmation @luke-jr, I would go “Back” to the “Create Wallet” window or just leave it as cancel.

And, I think I’d change button label “Cancel” on the “Encrypt wallet” and put “Back” there to go back to creation window, also I’d add “Create wallet -” (or “New wallet - #name) on the window’s title so you see the context if you alt-tab to another app or anything, also because I see this window is also used when you have a wallet selected and want to encrypt it (I’d add the name of the wallet in the title, perhaps to be consistent with when you create a new one) :

(Better yet, why don’t we just show these warnings upfront in the password entry dialog?)

I agree with this one, I’d add it when you enter the pwd (current “Encrypt wallet” window) and I’d leave it also on the confirmation pop-up. @GBKS, @jarolrod, what do you think? And others ofc.

pablomartin4btc commented at 1:50 pm on June 23, 2023: contributor

re-Tested nACK.

As I mentioned in the above response to @luke-jr, last change made the fix not to work anymore (see animated gif). Please consider my comments above if others also see them useful.

hernanmarino force-pushed on Aug 14, 2023

hernanmarino commented at 6:56 pm on August 14, 2023: contributor

Rolled back to undo changes detected by @pablomartin4btc . Now changes are at commit 78660e72001a2561c7ad3026367a69f65414dbd9 , the last one ACK ed by @jarolrod and others.

pablomartin4btc commented at 1:24 am on August 15, 2023: contributor

re-ACK https://github.com/bitcoin-core/gui/commit/78660e72001a2561c7ad3026367a69f65414dbd9 as previously here.

DrahtBot added the label CI failed on Aug 17, 2023

DrahtBot removed the label CI failed on Aug 21, 2023

DrahtBot removed the label Wallet on Aug 22, 2023

DrahtBot renamed this:
~~Wallet : Allow user to navigate options while encrypting at creation~~
wallet: Allow user to navigate options while encrypting at creation
on Aug 22, 2023

DrahtBot renamed this:
~~wallet: Allow user to navigate options while encrypting at creation~~
Wallet: Allow user to navigate options while encrypting at creation
on Aug 22, 2023

DrahtBot renamed this:
~~Wallet: Allow user to navigate options while encrypting at creation~~
Wallet: Allow user to navigate options while encrypting at creation
on Aug 22, 2023

DrahtBot added the label Wallet on Aug 22, 2023

maflcko removed the label Wallet on Aug 22, 2023

DrahtBot renamed this:
~~Wallet: Allow user to navigate options while encrypting at creation~~
wallet: Allow user to navigate options while encrypting at creation
on Aug 22, 2023

DrahtBot added the label Wallet on Aug 22, 2023

DrahtBot added the label CI failed on Sep 2, 2023

DrahtBot removed the label CI failed on Sep 4, 2023

DrahtBot added the label CI failed on Oct 21, 2023

BrandonOdiwuor commented at 4:46 pm on November 29, 2023: contributor

Concept ACK,

Allowing users to go back and update their passphrase before encrypting the wallet

DrahtBot requested review from luke-jr on Nov 29, 2023

DrahtBot requested review from hebasto on Nov 29, 2023

BrandonOdiwuor approved

BrandonOdiwuor commented at 6:07 pm on November 29, 2023: contributor

Tested ACK 78660e72001a2561c7ad3026367a69f65414dbd9

Screenshot from 2023-11-29 20-35-54

alfonsoromanz approved

alfonsoromanz commented at 3:13 pm on December 21, 2023: contributor

Tested ACK https://github.com/bitcoin-core/gui/pull/722/commits/78660e72001a2561c7ad3026367a69f65414dbd9

Wallet encrypt on create, allow to navigate options cccddc03f0

hernanmarino force-pushed on Feb 13, 2024

hernanmarino commented at 9:44 pm on February 13, 2024: contributor

Rebased for CI, no actual code changes in my code.

hernanmarino requested review from alfonsoromanz on Feb 13, 2024

hernanmarino requested review from BrandonOdiwuor on Feb 13, 2024

DrahtBot removed the label CI failed on Feb 13, 2024

alfonsoromanz approved

alfonsoromanz commented at 11:28 am on February 14, 2024: contributor

Re-Tested ACK https://github.com/bitcoin-core/gui/commit/cccddc03f0c625daeac7158eb20c1508aea5df39

test_wallet-encryption-navigation

BrandonOdiwuor approved

BrandonOdiwuor commented at 12:21 pm on February 14, 2024: contributor

re-Tested ACK cccddc03f0c625daeac7158eb20c1508aea5df39

DrahtBot requested review from jarolrod on Feb 14, 2024

DrahtBot requested review from pablomartin4btc on Feb 14, 2024

DrahtBot requested review from alfonsoromanz on Feb 14, 2024

DrahtBot removed review request from alfonsoromanz on Feb 14, 2024

DrahtBot removed review request from pablomartin4btc on Feb 14, 2024

DrahtBot requested review from pablomartin4btc on Feb 14, 2024

hebasto added this to the milestone 28.0 on Feb 15, 2024

hebasto approved

hebasto commented at 5:33 pm on May 15, 2024: member

ACK cccddc03f0c625daeac7158eb20c1508aea5df39, tested on Ubuntu 24.04.

hebasto added the label Needs release notes on May 15, 2024

hebasto merged this on May 15, 2024

hebasto closed this on May 15, 2024

maflcko commented at 2:01 pm on May 20, 2024: contributor

I don’t understand this change. It looks like a “Back” button is added to the warning pop-up, but it will only take you back to enter a different password, not to disable the password selection in the create wallet dialog. Wouldn’t it be better to add the “Back” button to the password dialog?

Also, wouldn’t it be better if the text from the two warning pop-ups was added to the top of the password dialog? Otherwise, it seems odd to first ask for a password from the user, then print two warnings about it, when the warnings could have been provided in one go?

maflcko commented at 2:03 pm on May 20, 2024: contributor

Also, this doesn’t need release notes, does it?

hebasto removed the label Needs release notes on May 20, 2024

hebasto commented at 4:54 pm on May 20, 2024: member

It looks like a “Back” button is added to the warning pop-up, but it will only take you back to enter a different password, not to disable the password selection in the create wallet dialog. Wouldn’t it be better to add the “Back” button to the password dialog?

Also, wouldn’t it be better if the text from the two warning pop-ups was added to the top of the password dialog? Otherwise, it seems odd to first ask for a password from the user, then print two warnings about it, when the warnings could have been provided in one go? @GBKS

What kind of a workflow will suit this part of the UI in your opinion?

GBKS commented at 12:28 pm on May 22, 2024: none

Good points. I think it could work well to combine the two warnings to a single dialog. It could be placed between the initial Create wallet dialog, and the password input.

Create wallet dialog: The user checks Encrypt wallet and clicks Continue.
Password warning dialog: The application informs the user about the risks of using a password and offers Back and Continue options. The user considers, decides to move forward with the password, and clicks Continue.
Password input dialog: The user enters their password and clicks Continue (the other option would be Back). It could be an option to have a I have stored this password in a safe place check box here. Another option is a password difficulty indicator.

This avoids the stack of dialog-on-dialog by putting them in a linear sequence with consistent Continue and Back options.

When dialogs are stacked like this and you mix buttons like Cancel and Back, it can be hard to know what they reference. Do you cancel the whole wallet creation or just the password option, or just the dialog? What does Back refer to in a dialog that sits on top of another dialog? Keeping the sequence linear makes this more obvious.

Just some thoughts, not sure if you even want to revisit this or not right after working through this PR.

hebasto commented at 4:07 pm on July 30, 2024: member

@hernanmarino

~Are you still working on this PR? If so,~ what do you think about GBKS’s suggestion?

hernanmarino commented at 4:18 pm on July 30, 2024: contributor

@hernanmarino

Are you still working on this PR? If so, what do you think about GBKS’s suggestion?

I like them. I can work on them and open a follow up PR soon.

wallet: Allow user to navigate options while encrypting at creation #722

Reviews