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

[UI] passphrase redesign #12731

Open
wants to merge 119 commits into
base: master
Choose a base branch
from

Conversation

wieslawsoltes
Copy link
Collaborator

@wieslawsoltes wieslawsoltes commented Mar 26, 2024

Fixes #12614

TODO:

  • Recovery words, pressing ENTER should tick the I have written down checkbox, and pressing ENTER again should execute the continue button. (We do the same in BuyAnything, check the code) 763e1a6, 8a16824
  • When recovering a wallet, the autocomplete feature should be enabled so they can press ENTER to accept the word
  • When recovering if all the words are entered but not valid, show an error message somewhere (we can polish it later).
  • Remove the custom derivation path from advanced wallet recovery. 1cf0a96
  • Add a back button to the advanced recovery word dialog. 27f2770
  • When recovering, the user should be edit to edit the previously filled words by clicking on them. 5257a80
  • When recovering, if the input files are empty and the user presses BACKSPACE, it focus should jump back to the previous word.
  • When recovering, after the first word is filled if you enter only when char then press TAB it goes to the next field (bug).
  • When recovering, clicking on a suggestion should immediately accept is the focus should go to the next field.
  • When recovering, if the user goes to the advanced view the filled words should be ported to the TagsBox in the advanced view
    • If some words got filled in the advanced view and the user goes back, the new words should be ported as well.
  • Add a Caption to the recovery screen that says Fill your 12 words in the correct order and enter the same passphrase that you used on wallet creation. If you have more words than 12 go to the advanced view. (Guys can polish it later) abc1789

Copy link
Collaborator

@yahiheb yahiheb left a comment

Choose a reason for hiding this comment

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

NACK

Why are we showing the passphrase dialog before the recovery words dialog?

As described in BIP39: a user may decide to protect their mnemonic with a passphrase, so the passphrase is an extension to the recovery words, it is a string added to the recovery words, so it should naturally be displayed after the recovery words not the other way around.

Years ago Wasabi 1.0 used to do exactly what this PR is doing; to ask for a passphrase after naming the wallet and before showing the recovery words and that got changed because the UX was bad and many users thought it is not part of the backup.

image

Here is the issue and the PR where we fixed that in Wasabi 2.0: #6930 & #8867. Why are we going back to this?

Last thing, why using 13th seed word is a bad idea:

(clarification: sometimes we use to say that the passphrase is the 13th word but that's just a mental shortcut for explaining the concept the newbies, the passphrase is not a word, or at least not necessarily, but multiple extra words separated by white spaces)

#5258 (reply in thread)

When I read 13th seed word, I think that the word should be in the BIP39 words list. It is not the case (I took a long time to understand that myself because you guys call it 13th seed word), therefore 13th word is not good, as it's not a 13th word. And I don't think it's relevant to say "13th word is used by some people", that's not an argument, if other people want to make a mistake: good for them, it doesn't mean we have to follow.

#12320 (comment)

@MaxHillebrand
Copy link
Member

I NACK the NACK

@yahiheb
Copy link
Collaborator

yahiheb commented Mar 27, 2024

I NACK the NACK

This doesn't answer the questions and concerns mentioned above.
NACKs should be accompanied by an explanation, otherwise they are not really helpful.

Here are some guidelines from the Bitcoin Core repo on how they should be used:

A NACK needs to include a rationale why the change is not worthwhile. NACKs without accompanying reasoning may be disregarded.

https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#conceptual-review

It is up to the PR author to convince the reviewers that the changes warrant the review effort, and if reviewers are "Concept NACK'ing" the PR, the author may need to present arguments and/or do research backing their suggested changes.

https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#peer-review

Side note: We can learn a lot from Bitcoin Core's contributing guidelines to improve our review process which is a big bottleneck in the development process of the project.

@molnard
Copy link
Collaborator

molnard commented Mar 27, 2024

The PR is not ready yet. Wait until it is finished.

Why are we showing the passphrase dialog before the recovery words dialog?

Because it will be displayed in the next dialog where you write down your recovery words.

many users thought it is not part of the backup.

The situation is the same now despite the fact we moved it after the recovery words. Why? It is not up to that. Those changes were made for different reasons.

why using 13th seed word is a bad idea

It won't be used. We will use passphrase everywhere and only passphrase. Please wait until the PR is ready to review.

@pull-request-size pull-request-size bot added size/L and removed size/M labels Mar 27, 2024
@molnard
Copy link
Collaborator

molnard commented Mar 29, 2024

GoDoItGIF

@pull-request-size pull-request-size bot added size/XL and removed size/L labels Apr 2, 2024
@wieslawsoltes
Copy link
Collaborator Author

I just checked the recovery workflow:

You can't go to the next box by clicking, you have to use Tab or enter.

You can use Shift+Tab to go to previous word.

@turbolay
Copy link
Collaborator

You can use Shift+Tab to go to previous word.

No, I'm trying to do this in the video

@molnard
Copy link
Collaborator

molnard commented Apr 24, 2024

The usage of 13th word and passphrase

Pinging: @Kruwed

Using both "passphrase" and "13th word" in the same instruction could potentially confuse a layman user, especially if they are not familiar with the specific jargon associated with bitcoin and cryptographic security.

  • Terminology Overload: The term "passphrase" often refers to a memorized secret consisting of a sequence of words or other text used to control access to a computer system, program, or data. Meanwhile, the "13th word" could be interpreted as a specific word within that passphrase or as something related to a seed phrase, which is a different concept commonly used in cryptocurrency wallets.
  • Assumption of Prior Knowledge: The instruction assumes that the user understands that their passphrase includes multiple words, and that they need to identify and use the 13th word specifically. For someone unfamiliar with the concept of a seed phrase (which typically consists of 12, 18, or 24 words), this can be very confusing.
  • Lack of Context: Without additional context or explanation, a user might not understand why they are being asked for the 13th word specifically. They may not even be sure if they are supposed to count the words in a phrase they have created or if it refers to something pre-generated like a recovery seed.

We need to use consistent terminology and avoid mixing it. We agreed to stick with the passphrase but we can reconsider it based on: we should use the one that is more commonly used.

  • Here is chatGPT answer to this question

In terms of usage:

  • Passphrase is the more widely used term in general security contexts and is understood in various tech-related fields, not just cryptocurrency.
  • 13th Word (or similarly, the "25th word") is much more specific and niche, used primarily by individuals who are familiar with the details of cryptocurrency wallet security protocols.

If you're targeting a broad audience, including less tech-savvy users, using "passphrase" with clear instructions is typically more accessible. However, if the context involves a specific action related to a seed phrase, it's important to clearly explain what is meant by "13th word" or consider using the more precise term "passphrase" as part of the seed extension, and provide additional guidance on how to use it.

So for this the text needs to be changed at the Add Passphrase dialog.

I like @yahiheb idea to change the dialog text to this:

image

@molnard
Copy link
Collaborator

molnard commented Apr 24, 2024

Recovery Words screen

  • This screen looks like this to me (see below). I see only 9 words.
  • I cannot click into the textboxes. I type in one word, select the suggestion and I am stuck. Only if I find the right pixel where to click I will be able to select the next word or press enter to move forward with the next word. It was almost perfect before. We don't need to ensure the order or anything - the words alignment is the same as when you create the wallet - that is the guarantee they will enter it perfectly.
  • I can enter wrong words like oiloiloil.
  • @turbolay 's feedback you could detect if there are 12 valid words on the clipboard and offer auto-paste. It could be a solution but please do not do it in this PR. Let that be out of scope.
  • Derivation Path: it was a bold move to bring that change into this PR. Please remove that as it is problematic. It can be considered regarding spacing but the actual implementation should happen in an upcoming PR. Let that be out of scope here.
  • Figure out something for Advanced Recovery Options related comments. Here is an idea from me:
    • We should support 24 words. How about having one more button. Do you have more then 12 words - click here. Then instead of the default 12 word control the user get back the good old recovery word entry box.
    • Advanced Recovery Option untouched it is the same as on master.
image

@molnard
Copy link
Collaborator

molnard commented Apr 24, 2024

"No added value in this design compared to the current one"

I went through carefully all of your comments @yahiheb. Here are my answers:

  • We simply moved the passphrase dialog before the recovery words which makes no sense
  • Anyone who tried this would immediately notice how bad the UX is compared to the very simply one we have now
    • This makes no sense. We have 2 critical issues with the UX what we have now - otherwise we won't be working on this at all. It is critical because ppl are literally loosing or believe that they lost their money. The details are here: Rename password - The ancient forgotten spell #12614.
  • Didn't we always aim to not overload the user with a lot of information in each dialog?
    • Yes that is the goal. Adding the passphrase to the recovery word screen is fundamental. At that screen the user flow is intentionally interrupted (with the checkbox) - to get his attention to write down everything which is needed for being able to recovery.
  • With this design the benefits are not clear so we better keep the current design.
    • I don't agree with this - the current design is broken (see issues), the new design is considered to be better by many ppl in the team (support, code, ux, UI).
    • Long before this PR was initiated, we held a focused meeting to figure out the changes to the UX and the reasons behind them. I understand that you have reservations about several aspects of the proposed solution. I've made efforts to address these concerns, though some suggestions will not be adopted, primarily because they conflict with the core concept of this change. The designs were created and evaluated in Figma, with significant contributions from many team members. I'd like to ask if you could phrase your insights in a more supportive and positive manner. This way, we can maintain a friendly and productive atmosphere that encourages everyone to contribute their best.

@soosr
Copy link
Collaborator

soosr commented Apr 25, 2024

@wieslawsoltes I gather the suggestion that makes sense from the other's comments. Here is the list to work on:

TODO:

  • Recovery words, pressing ENTER should tick the I have written down checkbox, and pressing ENTER again should execute the continue button. (We do the same in BuyAnything, check the code)
  • When recovering a wallet, the autocomplete feature should be enabled so they can press ENTER to accept the word
  • When recovering if all the words are entered but not valid, show an error message somewhere (we can polish it later).
  • Remove the custom derivation path from advanced wallet recovery.
  • Add a back button to the advanced recovery word dialog.
  • When recovering, the user should be edit to edit the previously filled words by clicking on them.
  • When recovering, if the input files are empty and the user presses BACKSPACE, it focus should jump back to the previous word.
  • When recovering, after the first word is filled if you enter only when char then press TAB it goes to the next field (bug).
  • When recovering, clicking on a suggestion should immediately accept is the focus should go to the next field.
  • When recovering, if the user goes to the advanced view the filled words should be ported to the TagsBox in the advanced view
    • If some words got filled in the advanced view and the user goes back, the new words should be ported as well.
  • Add a Caption to the recovery screen that says Fill your 12 words in the correct order and enter the same passphrase that you used on wallet creation. If you have more words than 12 go to the advanced view. (Guys can polish it later)

(There are still several issues, but fix this list first and we will do another round of testing)

@molnard
Copy link
Collaborator

molnard commented Apr 30, 2024

Say when...

WaitingWaitingPatientlyGIF

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename password - The ancient forgotten spell
8 participants