-
Notifications
You must be signed in to change notification settings - Fork 728
Improve damaged wallet error handling #6935
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
Conversation
a0c36bd to
4c2f56a
Compare
|
/perf |
ibrahimtaveras00
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issues during my testing, posted here
|
🧪 Flashlight Performance Report (AWS Device Farm) 🔀 Commit: 1d15ec1
|
| <> | ||
| <CopyFloatingEmojis textToCopy={accountAddress}> | ||
| <ActionButton onPress={handlePressCopy} icon="" testID="receive-button"> | ||
| <CopyFloatingEmojis textToCopy={accountAddress} onPress={handlePressCopy} disabled={isDamagedWallet}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was an issue on Android where the copied toast didn't work because only the onPress on CopyFloatingEmojis was getting called.
|
|
||
| switch (action) { | ||
| case 'import': | ||
| await savePIN('1111'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that in e2e tests the PIN screen would always ask to create a PIN since we never saved it when using the deep link handler here.
| validPin = await getExistingPIN(); | ||
| // eslint-disable-next-line no-empty | ||
| } catch (e) {} | ||
| if (!validPin) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fixes the create pin screen showing up when the user doesn't have a PIN. Places where pin creation is possible already use authenticateWithPINAndCreateIfNeeded which handle this case.
This is needed since if keychain is missing we no longer have the user pin. When that happens we should not ask them to create a new PIN, they should restore from backup.
| "no_price": "الأرصدة الصغيرة" | ||
| }, | ||
| "authenticate": { | ||
| "alert": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cleanup some unused messages.
| name: DEFAULT_WALLET_NAME, | ||
| primary: true, | ||
| type: WalletTypes.mnemonic, | ||
| encryptionType: EncryptionType.keychain, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This field was added to know the type of encryption used on the wallet. This allows us to only check wallets saved in keychain.
ibrahimtaveras00
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All issues have been resolved, QA Passed 👍🏽
| }); | ||
|
|
||
| const validateRecipient = (toAddress?: string, tokenAddress?: string) => { | ||
| if (!toAddress || toAddress?.toLowerCase() === tokenAddress?.toLowerCase()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separated this in 2 different checks since we want to show the sheet only in the case that the validation fails because the recipient wallet is in damaged state.
|
|
||
| checkKeychainIntegrity: async () => { | ||
| try { | ||
| const startTime = Date.now(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a complete rewrite of checkKeychainIntegrity with our current requirements.
|
|
||
| export const loadWallet = async <S extends Screen>({ | ||
| address, | ||
| showErrorIfNotLoaded = true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't see any cases where we wouldn't want to show the error sheet when loading wallet, so removed this option.
Fixes APP-3214
What changed (plus any additional context for devs)
Screen recordings / screenshots
TODO
What to test
iOS
Note: Must use a real device
Passcode removed
Device transfer
Android
Passcode removed