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

Improve Mnemonic handling + UX, Add Advanced Mode #189

Merged
merged 9 commits into from
Sep 5, 2023

Conversation

JSKitty
Copy link
Member

@JSKitty JSKitty commented Sep 1, 2023

Abstract

This PR largely improves Seed Phrase UX, by:

The UX is simplified by removing the below functionality, outside of "Advanced Mode":

  • Hiding the "Optional Passphrase (BIP39)" from the New Wallet 'wizard'.
  • Hiding the "Optional Passphrase (BIP39)" from the "Import Wallet" flow.
  • Removing the "Are you sure?" popup on typo'd seeds, strictly denying their import.

"Advanced Mode" is an off-by-default, session persisted mode in which MPW's functionality is simplified, to improve the experience for less knowledgeable users that are prone to losing funds from accidental mistakes, such as mixing up BIP39 Passphrases with their MPW Password, hitting "Confirm" when MPW warns of Seed Phrase typos, etc.

image

In future PRs, Advanced Mode will be leveraged to also hide features such as "VanityGen", and any other non-primary features of MPW, to simplify the app as much as possible for the average user.

This feature allows for both New and Advanced users to benefit from MPW, without any drawbacks, both communities can eat their cake, rather than removing "complex" features in the name of simplicity, but denying advanced users their tools.


Testing

This PR can be tested in various ways, I'll outline a bunch of them below, each with their own "testing flow", feel free to try variations of these to maximise our testing reach!

1: Test Creating a new wallet in Normal and Advanced mode

  • (Optional) visit Settings and enable "Advanced Mode" in the Wallet options.
  • Visit Dashboard and Create a New Wallet.
    • If "Advanced Mode" is enabled, you'll see "Optional Passphrase" reveal itself.
    • If "Advanced Mode" is disabled, you should not see the "Optional Passphrase".

2: Test importing a Seed Phrase in various ways, in Normal or Advanced Mode

  • (Optional) visit Settings and enable "Advanced Mode" in the Wallet options.
  • Visit Dashboard and then "Access Wallet".
    • If "Advanced Mode" is enabled, when typing your Seed Phrase, you'll see the "Optional Passphrase" reveal itself.
    • If "Advanced Mode" is disabled, when typing your Seed Phrase, you should not see the "Optional Passphrase".

Now, try importing it in various ways, such as:

    • Import it as normal (all should go well).
    • Import it with a typo (with Advanced Mode, it will show a "confirm" popup, in Normal Mode, it will reject the typo entirely).
    • Import it with excess spaces at the start, end, or between existing words. (It should ignore the spaces, and if they break the words, it will reject the import).

If any errors are found, the PR works unexpectedly, or you have viable suggestions to improve the UX or functionality of the PR, let me know!


@JSKitty JSKitty added Bug This is either a bugfix (PR) or a bug (issue). Enhancement New feature or request labels Sep 1, 2023
@JSKitty JSKitty requested review from Liquid369, Duddino, BreadJS and a team September 1, 2023 19:02
@JSKitty JSKitty self-assigned this Sep 1, 2023
@JSKitty JSKitty added Awaiting Review This PR and/or issue is awaiting reviews before continuing. Review Reward: 50 PIV Reviewers of this Pull Request will receive a 50 PIV reward labels Sep 1, 2023
@JSKitty JSKitty linked an issue Sep 1, 2023 that may be closed by this pull request
Copy link

@floki008 floki008 left a comment

Choose a reason for hiding this comment

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

tack: tested on linux different inputs:
1- valid seedphrase passed
2- non-valid seedphrase was blocked
3- valid seedphrase with white spaces was stripped as expected

Copy link
Member

@Duddino Duddino left a comment

Choose a reason for hiding this comment

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

The PR works really well, i tried inserting a number of spaces in various places of the seed phrase and they all got removed as expected.
I've left some minors nitpicks

scripts/settings.js Outdated Show resolved Hide resolved
scripts/settings.js Show resolved Hide resolved
scripts/wallet.js Outdated Show resolved Hide resolved
scripts/wallet.js Outdated Show resolved Hide resolved
@JSKitty JSKitty requested a review from Duddino September 4, 2023 14:47
Copy link
Member

@Duddino Duddino left a comment

Choose a reason for hiding this comment

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

tACK

@JSKitty JSKitty merged commit d3335aa into master Sep 5, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Review This PR and/or issue is awaiting reviews before continuing. Bug This is either a bugfix (PR) or a bug (issue). Enhancement New feature or request Review Reward: 50 PIV Reviewers of this Pull Request will receive a 50 PIV reward
Projects
Development

Successfully merging this pull request may close these issues.

Create / Import Wallet potential issue with new BIP39 pw field
3 participants