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

Recovery seed #1745

Merged
merged 7 commits into from Jul 22, 2020
Merged

Conversation

dennisreimann
Copy link
Member

@dennisreimann dennisreimann commented Jul 15, 2020

Addresses #1645 by using a separate page for the recovery seed.

Also sprinkeled some UI improvements on the UpdateStore, DerivationScheme and Wallet Settings views.
I got some more ideas on how we could improve the UI/flow further, but we can tackle that in another PR after this one.

Things that should be discussed:

  • Does this belong in HomeController? Because of the required policies I couldn't make it work in the WalletsController.
  • Should the passphrase also be shown?

light

dark

@dennisreimann dennisreimann requested a review from Kukks Jul 15, 2020
@Eskyee
Copy link
Contributor

Eskyee commented Jul 15, 2020

very nice @dennisreimann

@dennisreimann dennisreimann marked this pull request as ready for review Jul 15, 2020
@bolatovumar
Copy link
Contributor

bolatovumar commented Jul 16, 2020

Couple of minor comments:

  1. Regarding "non air-gapped digital format" phrase. Novice users may not understand what this means at all. Probably safer to use really simple language here like "do not store this phrase on an internet-connected device" or something like that.
  2. Because the distance between columns is larger than between rows, my eye wants to read from top to bottom and then to left (see below) but the numbers (1, 2, 3, etc...) go across, not down.

87590541-38264a80-c6e7-11ea-8018-aa201c71066d

See how Trezor does it, for example:

Seed_card_example

@dennisreimann
Copy link
Member Author

dennisreimann commented Jul 16, 2020

@bolatovumar good points. I'm with you on both, so let's see if we can improve the explanation and I'll tinker with the word list.

@NicolasDorier
Copy link
Contributor

NicolasDorier commented Jul 16, 2020

so happy about this one.

@pavlenex
Copy link
Contributor

pavlenex commented Jul 16, 2020

Should the passphrase also be shown?

On this page? I don't think so, we need to improve the UX on the generate wallet first, then warn users when they're populating the passphrase field that in case they lose it they're most likely rekt with a warring. Then I guess we can mention it on this page in form of text, but not show it.

I would add, if wallet is created and not a hot wallet, let user know that the seed will be wiped from the server. This is really important.

Regarding explanation language, I agree it needs to be simple. I like how Monero does it #1645 (comment)

Suggestions:

  1. Title Save your recovery phrase should be renamed to something actionable Write down your recovery phrase
  2. Rename I have saved.... to I have written down my recovery phrase and stored it in a secure location saved implies digital saving.
  3. I think it's important to mention that the words need to be written down in particular order.

@GBKS take a look at this PR, feedback appreciated.

@dennisreimann
Copy link
Member Author

dennisreimann commented Jul 16, 2020

I would add, if wallet is created and not a hot wallet, let user know that the seed will be wiped from the server. This is really important.

yep, this is already part of the template.

@GBKS
Copy link

GBKS commented Jul 16, 2020

Looking good. I attached a rough mockup with some suggestions:

  1. Remove the top banner (if the extra statement about the wallet being generated is important, maybe implement it into the body text?)
  2. Replace warning/error icon with an info icon (nothing is broken on this page, but there's important information)
  3. Center the layout consistently

btcpay-seed-200716

I also like the suggestions about simplifying the language and flipping rows and columns.

@pavlenex
Copy link
Contributor

pavlenex commented Jul 16, 2020

On a side note, I noticed this tackles derivation scheme import. I'm unsure if changing position brings UX improvement (but would require us to modify docs) but I would probably make button more prominent, right now it's grey. Would be good to keep changes like this in separate PR's in the future. I tested the workflow of seed generation and I think it's very nice, great improvement.

@dennisreimann
Copy link
Member Author

dennisreimann commented Jul 16, 2020

@pavlenex You mean the screenshots like on this page, right?
We can separate out the changes unrelated to the workflow from this PR – as i said I got some more ideas for improvements.

All in all, the terms and explanations on the wallet-related screens are too technical imho. There's hardly something that feels self explanatory or doesn't need further information. I think it's also a smell that we have to rely so heavily on screenshots in the docs. I'd love to go through this and clean it up as best as we can, because using so many screenshots will also prevent us from doing incremental improvements there.

@pavlenex
Copy link
Contributor

pavlenex commented Jul 16, 2020

Yes. It's well documented for now and changes like this barely bring anything in terms of UX improvement. Our long-term goal is to completely re-work the workflow of setting up a store/wallet, and I'd leave these changes for that time.

Let's focus on recovery seed for now. Wallet page is another story that again first needs designers and UX view before it can be tackled.

For this particular PR, it seems the consensus on the modifications is: (if I missed anything anybody feel free to correct me)

  • Improve the text on the main body to be more human-friendly without too much technical terms.
  • Reduce padding between columns
  • Use info icon instead of warring
  • Center the layout

Optional suggestions:

  • Use write down instead of save in the headline
  • Use write down instead of save in the body
  • Top banner modification (I actually think it's okay to leave it here because it makes the transition more natural and user is aware that his action is complete so my suggestion is we leave top banner for now)

Here's my suggestion for main body text, would be good if we can brainstorm and determine what's best.

Title: Secure your recovery phrase

Body: The words below are called your recovery phrase (mnemonic seed). Write them down on a piece of paper in the exact order.

They are the only backup of your private keys and allow you to restore your wallet. If you lose or write down the recovery phrase incorrectly you will permanently lose access of your funds. Double-check the spelling and position of each recovery phrase word.
Do not photograph the recovery seed. Do not keep it a digital format. Do not share them with anybody.

We'll probably need two versions of paragraph 2. When hot wallet is enabled and when it's not.

@dennisreimann
Copy link
Member Author

dennisreimann commented Jul 16, 2020

Our long-term goal is to completely re-work the workflow of setting up a store/wallet, and I'd leave these changes for that time.

Ok, I removed the commit with general changes and added another one that incorporates the suggested changes. Thanks for the valuable feedback everyone! 👍

The word list columns depend on the different screensizes – I've made it so that there's enough space to get that this is meant to be read in columns. See the screenshots below. (Mobile is just one column, left it out here)

We'll probably need two versions of paragraph 2. When hot wallet is enabled and when it's not.

This last screenshot shows how it looks in casee the seed isn't stored. There's a separate paragraph in that case.


2


3


4

@pavlenex
Copy link
Contributor

pavlenex commented Jul 16, 2020

@dennisreimann I think we need a better distinction between a hot wallet vs not a hot wallet messages, will try to think of something tomorrow. As mentioned, avoid air-gaped nobody knows what that means, it's hard to translate once we add localization, simple language is better.

@pavlenex
Copy link
Contributor

pavlenex commented Jul 18, 2020

Posting screenshots again, since Dennis, double-posted the same one. Others are my suggestions.

Current state

Wallet Generation when hot wallet is disabled

recoveryseed

Wallet Generation when hot wallet is enabled

recoveryseed2

My suggestions:

In general, it's great progress so far. I really like the design and in particular I'm found of us having 2 separate messages for hot wallet vs non hot wallet, because I think it's essential piece of info. I feel like we maybe have way too much text or at lest we should better position it so that mnemonic words are in the center of the attention.

Wallet Generation when hot wallet is disabled

  • I think the biggest mistake people can make with wallet generation is generate a wallet, do not back up or lose backup, receive payments, - they are rekt. We need to prevent this with very precise message. So calling it a backup on this page may be a mistake.
    The text I suggest is:
The recovery seed allows you to access and restore your wallet. 
If you lose it or write it down incorrectly, you will permanently lose access to your funds. 
Do not photograph the recovery seed and do not store it digitally.

The recovery phrase will permanently be erased from the server.

The emphasis is on "access and restore your wallet".

1recoveryseed

Wallet Generation when hot wallet is enabled

  • Here I wanted to emphasize the word "backup" so people are aware that if their server goes down or crashes they MAY loose their funds if they don't have a backup.
    The text I suggest is:
The recovery phrase is a backup that allows you to restore your wallet in case of a server crash.
If you lose it or write it down incorrectly, you may permanently lose access to your funds.
Do not photograph it. Do not store it digitally.

The recovery phrase will also be stored on a server as a hot wallet.

It would be great if we can somehow find appropriate human-readable language instead of a hot wallet. Should we point them to our docs maybe? Should we on this page emphasize the risks of a hot wallet or should we do it on wallet generation page, I assume people who enabled hot wallet already know the repercussions of using it.

2recoveryseed

As you can see my suggested layout re-positions text and modifies text slightly to better fit each use case. It's not perfect so any feedback regarding text and how to better convey a message is really appreciated, I think we really need to think careful about this, since user-funds are at risk if we don't coney the importance of each use-case.

@dennisreimann
Copy link
Member Author

dennisreimann commented Jul 18, 2020

@pavlenex I like the repositioning and think the text is good as it is. If there's no other feedback I'd implement it as suggested and we're good to go in my opinion.

All in all this is a major improvement on what we currently have.

@pavlenex pavlenex added this to In progress in 0.08 Miles-stone Jul 19, 2020
@dennisreimann
Copy link
Member Author

dennisreimann commented Jul 19, 2020

Updated and pushed.

Hot Wallet enabled

hot

Hot Wallet disabled

non-hot

@pavlenex
Copy link
Contributor

pavlenex commented Jul 20, 2020

tACK. This looks good to me. I'll leave it to others to submit feedback if any. Great work so far everyone! 🚀
Edit: Unsure why the tests are failing.

@dennisreimann
Copy link
Member Author

dennisreimann commented Jul 20, 2020

ok, let's first get #1752 done, then I'll take care of the tests and rebase here. I think they are failing because some layout stuff changed and it'll be easier to fix this one the login PR is merged. (It contains updates for the same layout files involved here)

@dennisreimann
Copy link
Member Author

dennisreimann commented Jul 20, 2020

Updated the views, which fixed the tests – nevertheless, let's first merge the login PR, then rebase this one and we're good to go.

@NicolasDorier
Copy link
Contributor

NicolasDorier commented Jul 22, 2020

no rebase yolo merge

@NicolasDorier NicolasDorier merged commit 27d7d03 into btcpayserver:master Jul 22, 2020
3 checks passed
0.08 Miles-stone automation moved this from In progress to Done Jul 22, 2020
@dennisreimann dennisreimann deleted the recovery-seed branch Jul 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants