[WIP] Attempt empty password for keystore unlock before prompting for pass #548
Conversation
james-prado
changed the title
|
Looking at it again, the pause between uploading a password-empty encrypted file and loading the wallet can be more significant than just 1 - 2 seconds, up to 30 seconds sometimes. It looks like a lot of this time is waiting for the ethereumjs-wallet to generate the wallet with the empty passphrase. I can see similar wait time in your live version when I upload a password-empty encrypted keystore, but here it just looks unresponsive since we immediately try the empty passphrase. Perhaps a UI indicator would help to indicate the wallet is loading? |
|
Thanks for your contribution and interest @aitrean! Great point about the UI being un-responsive while decrypt is taking place. Unfortunately, the call itself is synchronous, and thus blocks the UI, so we aren't able to show a loading indicator (or at the very least, the loading indicator would be blocked as well). Let me do some research and decide if there is a way to ensure the UI is responsive while we attempt to decrypt the wallet. We definitely don't want to hurt overall UX by trying to fix a relatively un-common use-case. |
|
You could use |
|
In this case, we'd be showing a spinner to all users (while the decryption runs) who have uploaded a keystore, which is probably not the desired UX. Perhaps the alternative is to prevent the UI from being blocked, but also don't show any loading indicator, and simply login if the empty passphrase succeeds? This lets the more common user flow where users do input a password continue to function, while still providing a convenience to users who don't have a password. |
|
@HenryNguyen5 I'll look into that @dternyak Perhaps I'm misunderstanding, but would a spinner on the unblocked UI be detrimental to users with real passwords? Files encrypted with real passwords immediately reject the blank password, so they wouldn't see the spinner. Also, if you only unblock the UI, it will look like the interface didn't acknowledge your file change. If not a spinner, what about those notifications you have in the bottom of your live version when a file gets uploaded? I don't see them in this build. |
|
@aitrean Got it - as long as the existing flow for users with passwords doesn't change (e.g. they don't briefly see a loading indicator and then immediately disappear) that approach should be fine 👍 |
|
The issue is that there are encrypted keystores with empty passphrases, so I don't believe you can just check with |
|
Another (arguably cleaner and more scalable) solution would be to use webworkers for decryption tasks as it's CPU heavy |
|
@dternyak Gotcha. I like Henry's idea to use a webworker for handling the ethereumjs library decryption behaviour. To address the ultra-brief spinner appearance on password-encrypted files, could we set up a timer to wait for a webworker? If the webworker doesn't signal within x seconds, load the spinner so that the user doesn't think the UI has ignored them. |
|
@aitrean I haven't worked with web workers much before, but I think this should work in theory. @wbobeirne I think you may have had some web worker experience iirc - do you happen any thoughts? |
|
It definitely is a good use case for workers, but it's not exactly a very trivial refactor. Workers take in completely different files, so this would mean adding a new loader to webpack, and splitting out our decrypting logic into their own files that then get output and loaded in separately. I think all of this could be reasonably be tucked away into a wrapper async function, so there'd be little change on the component side. I think this'd be a great way to go, as it'd also close out #219. |
common/sagas/wallet.tsx
Outdated
| yield put(showNotification('danger', translate('ERROR_6'))); | ||
| if (password !== '') { | ||
| yield put(showNotification('danger', translate('ERROR_6'))); | ||
| } |
There was a problem hiding this comment.
Even if the password is blank, it could throw for other reasons (Invalid file?) We should ideally check if the password is blank and if it's a certain type of error.
There was a problem hiding this comment.
Woops! I should have seen that. Thanks for the feedback!
There was a problem hiding this comment.
Actually, testing this issue again without my changes, I see that there's no error at all for incorrect file uploads. I can upload jpgs to the keystore and it will give me an unlock button. Can't find this issue logged. Shall I open one?
Closes MyEtherWallet/MyEtherWallet#423
Hi guys. Since Henry and James were having so much fun with MEW I thought I'd try my hand at a few of your issues - if that's alright. This solution is for issue MyEtherWallet/MyEtherWallet#423 which I tested against some keystore files I generated with ethereumjs-wallet. It's working, however, there is a 1 - 2 second pause when we decrypt an empty-password encoded file. There doesn't seem to be any indicator that the wallet is being generated either, so there is a 1 -2 second period where it doesn't look like the UI is responsive. In V3, it looks like you guys use a notifier to indicate the file has been received, but I'm not seeing that notification in this build. Is that an issue?