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 139 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). 4875fb5, 36d796e
  • 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. c28e73c, 26ec0a9, 1b43631
  • 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
Collaborator

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
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