-
Notifications
You must be signed in to change notification settings - Fork 577
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
Fix: Wallets being marked as backed up by walletLoadState() #5593
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,14 @@ import { useDispatch } from 'react-redux'; | |
import WalletAndBackup from '@/assets/WalletsAndBackup.png'; | ||
import { KeyboardArea } from 'react-native-keyboard-area'; | ||
|
||
import { Backup, fetchBackupPassword, restoreCloudBackup, RestoreCloudBackupResultStates, saveBackupPassword } from '@/model/backup'; | ||
import { | ||
Backup, | ||
fetchBackupPassword, | ||
getLocalBackupPassword, | ||
restoreCloudBackup, | ||
RestoreCloudBackupResultStates, | ||
saveLocalBackupPassword, | ||
} from '@/model/backup'; | ||
import { cloudPlatform } from '@/utils/platform'; | ||
import { PasswordField } from '../fields'; | ||
import { Text } from '../text'; | ||
|
@@ -14,7 +21,13 @@ import { cloudBackupPasswordMinLength, isCloudBackupPasswordValid, normalizeAndr | |
import walletBackupTypes from '@/helpers/walletBackupTypes'; | ||
import { useDimensions, useInitializeWallet } from '@/hooks'; | ||
import { useNavigation } from '@/navigation'; | ||
import { addressSetSelected, setAllWalletsWithIdsAsBackedUp, walletsLoadState, walletsSetSelected } from '@/redux/wallets'; | ||
import { | ||
addressSetSelected, | ||
setAllWalletsWithIdsAsBackedUp, | ||
setWalletBackedUp, | ||
walletsLoadState, | ||
walletsSetSelected, | ||
} from '@/redux/wallets'; | ||
import Routes from '@/navigation/routesNames'; | ||
import styled from '@/styled-thing'; | ||
import { padding } from '@/styles'; | ||
|
@@ -122,6 +135,8 @@ export default function RestoreCloudStep() { | |
throw new Error('No backup file selected'); | ||
} | ||
|
||
const prevWalletsState = await dispatch(walletsLoadState()); | ||
|
||
const status = await restoreCloudBackup({ | ||
password, | ||
userData, | ||
|
@@ -130,34 +145,49 @@ export default function RestoreCloudStep() { | |
|
||
if (status === RestoreCloudBackupResultStates.success) { | ||
// Store it in the keychain in case it was missing | ||
await saveBackupPassword(password); | ||
const hasSavedPassword = await getLocalBackupPassword(); | ||
if (!hasSavedPassword) { | ||
await saveLocalBackupPassword(password); | ||
} | ||
Comment on lines
+148
to
+151
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @skylarbarrera does this make sense to do here? This is the new local backup function we wrote not sure if you remember. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this seems fine, i think the issue was that password could be undefined/empty? that issue seems diff |
||
|
||
InteractionManager.runAfterInteractions(async () => { | ||
const wallets = await dispatch(walletsLoadState()); | ||
const newWalletsState = await dispatch(walletsLoadState()); | ||
let filename = selectedBackup.name; | ||
if (IS_ANDROID && filename) { | ||
/** | ||
* We need to normalize the filename on Android, because sometimes | ||
* the filename is returned with the path used for Google Drive storage. | ||
* That is with REMOTE_BACKUP_WALLET_DIR included. | ||
*/ | ||
filename = normalizeAndroidBackupFilename(filename); | ||
} | ||
|
||
logger.info('Done updating backup state'); | ||
const walletIdsToUpdate = Object.keys(wallets); | ||
// NOTE: Marking the restored wallets as backed up | ||
// @ts-expect-error TypeScript doesn't play nicely with Redux types here | ||
const walletIdsToUpdate = Object.keys(newWalletsState || {}).filter(walletId => !(prevWalletsState || {})[walletId]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we want to mark all the wallets brought in from the restoration as backed up by cloud, so here is where we filter those down. |
||
logger.log('updating backup state of wallets with ids', { | ||
walletIds: JSON.stringify(walletIdsToUpdate), | ||
}); | ||
logger.log('backupSelected.name', { | ||
fileName: selectedBackup.name, | ||
}); | ||
|
||
await dispatch(setAllWalletsWithIdsAsBackedUp(walletIdsToUpdate, walletBackupTypes.cloud, filename)); | ||
|
||
const walletKeys = Object.keys(wallets || {}); | ||
const firstWallet = | ||
// @ts-expect-error TypeScript doesn't play nicely with Redux types here | ||
walletKeys.length > 0 ? (wallets || {})[walletKeys[0]] : undefined; | ||
const oldCloudIds: string[] = []; | ||
const oldManualIds: string[] = []; | ||
// NOTE: Looping over previous wallets and restoring backup state of that wallet | ||
Object.values(prevWalletsState || {}).forEach(wallet => { | ||
// NOTE: This handles cloud and manual backups | ||
if (wallet.backedUp && wallet.backupType === walletBackupTypes.cloud) { | ||
oldCloudIds.push(wallet.id); | ||
} else if (wallet.backedUp && wallet.backupType === walletBackupTypes.manual) { | ||
oldManualIds.push(wallet.id); | ||
} | ||
}); | ||
|
||
await dispatch(setAllWalletsWithIdsAsBackedUp(oldCloudIds, walletBackupTypes.cloud, filename)); | ||
await dispatch(setAllWalletsWithIdsAsBackedUp(oldManualIds, walletBackupTypes.manual, filename)); | ||
Comment on lines
+173
to
+186
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if there's a better way to do this, but we also need to handle previous wallets that were in a backed up state (whether manually or cloud),s o we loop over the prev state and grab those. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this hurts my head, may need to huddle on monday |
||
|
||
const walletKeys = Object.keys(newWalletsState || {}); | ||
// @ts-expect-error TypeScript doesn't play nicely with Redux types here | ||
const firstWallet = walletKeys.length > 0 ? (newWalletsState || {})[walletKeys[0]] : undefined; | ||
const firstAddress = firstWallet ? firstWallet.addresses[0].address : undefined; | ||
const p1 = dispatch(walletsSetSelected(firstWallet)); | ||
const p2 = dispatch(addressSetSelected(firstAddress)); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,7 +35,6 @@ export const useCreateBackup = ({ walletId, navigateToRoute }: UseCreateBackupPr | |
const walletCloudBackup = useWalletCloudBackup(); | ||
const { latestBackup, wallets } = useWallets(); | ||
const [loading, setLoading] = useState<useCreateBackupStateType>('none'); | ||
const [alreadyHasLocalPassword, setAlreadyHasLocalPassword] = useState(false); | ||
|
||
const [password, setPassword] = useState(''); | ||
|
||
|
@@ -49,7 +48,8 @@ export const useCreateBackup = ({ walletId, navigateToRoute }: UseCreateBackupPr | |
[setLoading] | ||
); | ||
const onSuccess = useCallback(async () => { | ||
if (!alreadyHasLocalPassword) { | ||
const hasSavedPassword = await getLocalBackupPassword(); | ||
if (!hasSavedPassword) { | ||
Comment on lines
+51
to
+52
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @skylarbarrera also think the main issue here was the state variable wasn't be persisted when we were fetching the password on the BackupCloudStep component. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah i think that makes sense to me |
||
await saveLocalBackupPassword(password); | ||
} | ||
analytics.track('Backup Complete', { | ||
|
@@ -58,7 +58,7 @@ export const useCreateBackup = ({ walletId, navigateToRoute }: UseCreateBackupPr | |
}); | ||
setLoadingStateWithTimeout('success'); | ||
fetchBackups(); | ||
}, [alreadyHasLocalPassword, setLoadingStateWithTimeout, fetchBackups, password]); | ||
}, [setLoadingStateWithTimeout, fetchBackups, password]); | ||
|
||
const onError = useCallback( | ||
(msg: string) => { | ||
|
@@ -115,7 +115,6 @@ export const useCreateBackup = ({ walletId, navigateToRoute }: UseCreateBackupPr | |
const getPassword = useCallback(async (): Promise<string | null> => { | ||
const password = await getLocalBackupPassword(); | ||
if (password) { | ||
setAlreadyHasLocalPassword(true); | ||
setPassword(password); | ||
return password; | ||
} | ||
|
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.
added this here because we need to diff the prev state with the new state once we receive a success response.
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.
didnt think this would return