arborwallet / arbor-wallet

Arbor Wallet is a light Chia wallet (SPV) with a focus on user privacy and security.
https://www.arborwallet.com
Apache License 2.0
11 stars 6 forks source link

Feature/restore wallet #2

Closed TobiCrackIT closed 3 years ago

TobiCrackIT commented 3 years ago
scotopic commented 3 years ago

Nice work so far! A couple of things:

  1. On the first Restore Wallet screen. "Scan a QR Code" and "Type Secret Phrase" need to be tappable (maybe InkWell widget ? ) not just the words ( at least on Android simulator tapping only works on the words/button Scan/Type...
scotopic commented 3 years ago
  1. On Android only: I'm seeing an unhandled error with the following steps: a. tap "I already have a wallet" a. tap "Type Secret Phrase" a. tap Next until you get "Restore" a. tap Restore
    • Expected: Message in UI or dialog
    • Actual: I/flutter ( 5014): Error E/flutter ( 5014): [ERROR:flutter/lib/ui/ui_dart_state.cc(199)] Unhandled Exception: Exception: Exception: type 'Null' is not a subtype of type 'String' E/flutter ( 5014): #0 RestoreWalletProvider.recoverWallet (package:arbor/core/providers/restore_wallet_provider.dart:260:7) E/flutter ( 5014): <asynchronous suspension>
  2. On iOS, in above scenario ( no password entered ) I get a successful password recovery O_o I'd expect to not be let press the "recover" button in the first place. Can you add a check that at least 1 character is required per box in all 12 boxes.

Perhaps disable the "next" button until all 4 words are typed in per screen to avoid empty passwords.

scotopic commented 3 years ago
  1. The password checking is restricted to the BIP39 standard. Please download https://github.com/bitcoin/bips/blob/master/bip-0039/english.txt and store in assets/bip39 then use this list for checking each word. If it's not in this list, the user shouldn't be able to leave the word entered.

Would it be possible to do "predictive text" ? where as they type a word a drop down appears with the possible words ( from the bip39 spec) and they tap to auto fill. I'm asking this only if easy and quick otherwise just showing something like "invalid word type" if the password word is not in the bip39 list of words is good enough.

scotopic commented 3 years ago
  1. On Android only: One of the screens slides up from the bottom but the rest appear to slide in from the right a. tap "I already have a wallet" a. tap "Type Secret Phrase" a. tap "Next" (slides from right ) a. tap "Next" (slides from right ) a. tap "Next" to see the "Restore" button (slides from bottom )

Any way to make the last view slide in from the right as well ?

TobiCrackIT commented 3 years ago
  1. On Android only: I'm seeing an unhandled error with the following steps: a. tap "I already have a wallet" a. tap "Type Secret Phrase" a. tap Next until you get "Restore" a. tap Restore

    • Expected: Message in UI or dialog
    • Actual: I/flutter ( 5014): Error E/flutter ( 5014): [ERROR:flutter/lib/ui/ui_dart_state.cc(199)] Unhandled Exception: Exception: Exception: type 'Null' is not a subtype of type 'String' E/flutter ( 5014): #0 RestoreWalletProvider.recoverWallet (package:arbor/core/providers/restore_wallet_provider.dart:260:7) E/flutter ( 5014): <asynchronous suspension>
  2. On iOS, in above scenario ( no password entered ) I get a successful password recovery O_o I'd expect to not be let press the "recover" button in the first place. Can you add a check that at least 1 character is required per box in all 12 boxes.

Perhaps disable the "next" button until all 4 words are typed in per screen to avoid empty passwords.

This some weird behaviour because I added validation for the password fields. I'll check it out

TobiCrackIT commented 3 years ago
  1. On Android only: One of the screens slides up from the bottom but the rest appear to slide in from the right a. tap "I already have a wallet" a. tap "Type Secret Phrase" a. tap "Next" (slides from right ) a. tap "Next" (slides from right ) a. tap "Next" to see the "Restore" button (slides from bottom )

Any way to make the last view slide in from the right as well ?

Noted

TobiCrackIT commented 3 years ago
  1. The password checking is restricted to the BIP39 standard. Please download https://github.com/bitcoin/bips/blob/master/bip-0039/english.txt and store in assets/bip39 then use this list for checking each word. If it's not in this list, the user shouldn't be able to leave the word entered.

Would it be possible to do "predictive text" ? where as they type a word a drop down appears with the possible words ( from the bip39 spec) and they tap to auto fill. I'm asking this only if easy and quick otherwise just showing something like "invalid word type" if the password word is not in the bip39 list of words is good enough.

Predicting passwords reduces security and aids guess work. Users should be able to type in their passwords

scotopic commented 3 years ago
  1. The password checking is restricted to the BIP39 standard. Please download https://github.com/bitcoin/bips/blob/master/bip-0039/english.txt and store in assets/bip39 then use this list for checking each word. If it's not in this list, the user shouldn't be able to leave the word entered.

Would it be possible to do "predictive text" ? where as they type a word a drop down appears with the possible words ( from the bip39 spec) and they tap to auto fill. I'm asking this only if easy and quick otherwise just showing something like "invalid word type" if the password word is not in the bip39 list of words is good enough.

Predicting passwords reduces security and aids guess work. Users should be able to type in their passwords

Normally, I agree, in this case that's not necessarily true. The word list is predefined (and we'll let everyone know it's the BIP39 standard) so the only thing we are doing is helping the user type the words in faster. We aren't storing the user password so we aren't predicting the user's password but giving them options about which words are in the dictionary.

scotopic commented 3 years ago

I'm seeing "invalid password" even though I've ended words from the dictionary. The validation text should show up only if the word is not in the dictionary:

Screen Shot 2021-09-08 at 8 51 41 AM
scotopic commented 3 years ago

On iOS (iPhone SE) the text doesn't fit always esp when the 4th word is being typed. Would it be possible to:

  1. Remove the "1 - 4" text above the first text box
  2. When the user taps in the first word box, for the keyboard not to block the 4th box
  3. When the user taps in the first word box, for the info text ( at the top ) not to be hidden ( low priority )
  4. Change the ellipsis color to white Screen Shot 2021-09-08 at 9 14 18 AM
scotopic commented 3 years ago

Android only: Any idea why in this branch we are now seeing this in the Console output:


Warning: Mapping new ns http://schemas.android.com/repository/android/common/02 to old ns http://schemas.android.com/repository/android/common/01
Warning: Mapping new ns http://schemas.android.com/repository/android/generic/02 to old ns http://schemas.android.com/repository/android/generic/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/addon2/02 to old ns http://schemas.android.com/sdk/android/repo/addon2/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/repository2/02 to old ns http://schemas.android.com/sdk/android/repo/repository2/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/sys-img2/02 to old ns http://schemas.android.com/sdk/android/repo/sys-img2/01``` ?
scotopic commented 3 years ago

When the user transitions to the wallet ui after successfully importing the wallet. Can you remove the "<" back button since the user shouldn't be able to go back to the previous UI ?

TobiCrackIT commented 3 years ago

On iOS (iPhone SE) the text doesn't fit always esp when the 4th word is being typed. Would it be possible to:

  1. Remove the "1 - 4" text above the first text box
  2. When the user taps in the first word box, for the keyboard not to block the 4th box
  3. When the user taps in the first word box, for the info text ( at the top ) not to be hidden ( low priority )
  4. Change the ellipsis color to white
Screen Shot 2021-09-08 at 9 14 18 AM

Do you want the instruction text to remain in one position when other items are scrolled?

TobiCrackIT commented 3 years ago

Android only: Any idea why in this branch we are now seeing this in the Console output:

Warning: Mapping new ns http://schemas.android.com/repository/android/common/02 to old ns http://schemas.android.com/repository/android/common/01
Warning: Mapping new ns http://schemas.android.com/repository/android/generic/02 to old ns http://schemas.android.com/repository/android/generic/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/addon2/02 to old ns http://schemas.android.com/sdk/android/repo/addon2/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/repository2/02 to old ns http://schemas.android.com/sdk/android/repo/repository2/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/sys-img2/02 to old ns http://schemas.android.com/sdk/android/repo/sys-img2/01``` ?

I don't have that output here. You probably have 2 different versions of Android build-tools installed. Invalidate cache on Android Studio and restart

scotopic commented 3 years ago

On iOS (iPhone SE) the text doesn't fit always esp when the 4th word is being typed. Would it be possible to:

  1. Remove the "1 - 4" text above the first text box
  2. When the user taps in the first word box, for the keyboard not to block the 4th box
  3. When the user taps in the first word box, for the info text ( at the top ) not to be hidden ( low priority )
  4. Change the ellipsis color to white
Screen Shot 2021-09-08 at 9 14 18 AM

Do you want the instruction text to remain in one position when other items are scrolled?

Yes. Ideally:

Again, not a big deal and doesn't have to be implemented if it's more than an hour of work. I'd say once you remove "1-4" and maybe just reduce the space between each text box that might be good enough on the smallest devices (for now).

scotopic commented 3 years ago

Latest changes looking good.

  1. Remove "1 - 4" text from the pages
  2. There's an bug in the word detection:
    • Enter valid words on all pages and get to the last page to restore the wallet
    • On the 12th word, type in "dust" and verify you can see "Restore" enabled
    • Remove the "t" from the end
    • EXPECTED: error says invalid AND the button is disabled
    • ACTUAL: error says invalid BUT the button is enabled. 3) Also please change the regex to not allow a space or any more than 1 word to be entered into 1 box.
scotopic commented 3 years ago

Looks good!