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
Conversation
@@ -122,6 +135,8 @@ export default function RestoreCloudStep() { | |||
throw new Error('No backup file selected'); | |||
} | |||
|
|||
const prevWalletsState = await dispatch(walletsLoadState()); |
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
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 comment
The 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.
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)); |
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
this hurts my head, may need to huddle on monday
const hasSavedPassword = await getLocalBackupPassword(); | ||
if (!hasSavedPassword) { | ||
await saveLocalBackupPassword(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.
@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 comment
The 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
const hasSavedPassword = await getLocalBackupPassword(); | ||
if (!hasSavedPassword) { |
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.
@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 comment
The reason will be displayed to describe this comment to others. Learn more.
yeah i think that makes sense to me
await dispatch(setAllWalletsWithIdsAsBackedUp(walletIdsToUpdate, walletBackupTypes.cloud, filename)); | ||
// end mark |
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.
deleet
…ecks-language * 'develop' of github.com:rainbow-me/rainbow: (100 commits) [WIP]: Swaps v2 quote fetching (#5601) chore: app start up spring cleaning (#5622) fix remote config (#5627) would it kill you to log this only once (#5626) Recents (#5625) wc: improvements (#5616) Degen chain support (#5621) send: check contract address (#5586) tx requests: metadata (#5584) audit: phin (#5624) Fix: Wallets being marked as backed up by walletLoadState() (#5593) NFTs: filter instead of throw error when NFT has invalid network (#5537) Dapp Browser: Search (#5617) Browser: bug fixes, animation and UI improvements (#5618) cleanup file imports and duplicate types (#5619) requests: generalize analytics (#5589) browser: account icon clean up (#5612) Fix no tab and links (#5613) more e2e changes (#5558) . (#5615) ...
…eplink-add * 'develop' of github.com:rainbow-me/rainbow: (23 commits) Add SmoothPager (#5641) [APP-1370]: bump sentry sdk to latest (#5640) Browser refactor (#5638) bump version to v1.9.22 (#5634) Price Impact Warning (#5635) Disable welcome screen animations when IS_TESTING (#5637) init (#5495) [WIP]: Swaps v2 quote fetching (#5601) chore: app start up spring cleaning (#5622) fix remote config (#5627) would it kill you to log this only once (#5626) Recents (#5625) wc: improvements (#5616) Degen chain support (#5621) send: check contract address (#5586) tx requests: metadata (#5584) audit: phin (#5624) Fix: Wallets being marked as backed up by walletLoadState() (#5593) NFTs: filter instead of throw error when NFT has invalid network (#5537) Dapp Browser: Search (#5617) ...
Fixes APP-1296
What changed (plus any additional context for devs)
Screen recordings / screenshots
What to test