Commit af57390
authored
fix: New Persistence Improvements based on Abuse testing cp-7.59.0 (#21990)
<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->
## **Description**
<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->
When Redux state migrations fail, redux-persist resets the state to
defaults and updates the version number to the latest, preventing
migrations from re-running. This leaves users stuck on the onboarding
screen unable to access their wallets, even though their vault backup
exists in secure storage.
## Implementation
### 1. Vault Recovery Detection on Onboarding Screen
Added automatic detection when users land on the onboarding screen with
an existing vault backup:
- Detects migration failure scenario: `!existingUser` (Redux reset) +
vault backup exists
- Skips detection if user explicitly deleted their wallet
(`route.params.delete`)
- Automatically navigates to vault recovery screen
- Users can restore their wallet using their password
- Prevents data loss from failed migrations
**Files Changed:**
- `app/components/Views/Onboarding/index.js` - Added vault recovery
detection
- `app/components/Views/RestoreWallet/WalletRestored.tsx` - Sets
`existingUser` flag after restore
### 2. Fixed Vault Recovery Persistence Bug
Fixed critical bug where vault recovery didn't persist controller state
changes:
- Added `setupEnginePersistence()` call in `initializeVaultFromBackup()`
path
- Ensures controller state changes are saved to individual files after
vault recovery
- Previously caused infinite vault recovery loops after successful
restore
- Consolidated persistence setup in `initializeControllers()` for
consistency
**Files Changed:**
- `app/core/EngineService/EngineService.ts` - Fixed persistence setup in
vault recovery path
### 3. Fixed Race Condition in Wallet Deletion
Prevents temporary wallets (created during reset) from being backed up:
- Added `disableAutomaticVaultBackup` flag to temporarily disable vault
backups during wallet reset
- Clears all vault backups before creating temporary wallet
- Re-enables automatic backups in `finally` block for robustness
- Applies to both manual wallet deletion and OAuth error recovery
- Prevents false vault recovery prompts after intentional wallet resets
**Files Changed:**
- `app/core/Engine/Engine.ts` - Added circuit breaker flag
- `app/components/hooks/DeleteWallet/useDeleteWallet.ts` - Manual
deletion fix
- `app/core/Authentication/Authentication.ts` - Uncommented
`setExistingUser(true)` for new wallets
- `app/core/BackupVault/backupVault.ts` - Vault backup utilities
### 4. Fixed Migration Inflation/Deflation Logic
Corrected conditions for the new file-based persistence system
(migrations 104-105):
- **Inflation**: `> 106` - Only migrations 106+ need to inflate from
individual controller files
- **Deflation**: `>= 106` - Migration 106 is the first to deflate into
individual controller files
- Only future migrations require inflation, that will follow this PR
- Ensures smooth transition to new persistence system without breaking
app on restart
**Files Changed:**
- `app/store/migrations/index.ts` - Fixed inflation/deflation conditions
and removed debug logs
- `app/store/migrations/index.test.ts` - Updated tests to reflect
correct logic
### 5. Code Cleanup
Removed all debugging logs added during investigation:
- `app/components/Views/Onboarding/index.js` - Removed migration
detection logs
- `app/core/EngineService/EngineService.ts` - Removed persistence setup
logs
- `app/store/migrations/index.ts` - Removed KeyringController debug logs
- `app/core/BackupVault/backupVault.ts` - Simplified error handling
- `app/core/Engine/Engine.ts` - Removed vault backup logs
### 6. Test Compliance
Fixed unit test naming violations to comply with project guidelines:
- Updated 19 tests in `EngineService.test.ts` to remove "should" from
test names
- Ensured all tests follow action-oriented naming convention
## **Changelog**
<!--
If this PR is not End-User-Facing and should not show up in the
CHANGELOG, you can choose to either:
1. Write `CHANGELOG entry: null`
2. Label with `no-changelog`
If this PR is End-User-Facing, please write a short User-Facing
description in the past tense like:
`CHANGELOG entry: Added a new tab for users to see their NFTs`
`CHANGELOG entry: Fixed a bug that was causing some NFTs to flicker`
(This helps the Release Engineer do their job more quickly and
accurately)
-->
CHANGELOG entry:
## **Related issues**
Fixes:
## **Manual testing steps**
```gherkin
Feature: my feature name
Scenario: user [verb for user action]
Given [describe expected initial app state]
When user [verb for user action]
Then [describe expected outcome]
```
**Feature: Vault Recovery After Migration Failure**
Scenario: user restores wallet after migration failure
Given user has MetaMask installed with wallet data
And a state migration fails during app startup
And redux state is reset to defaults
And vault backup exists in secure storage
When user opens the app
Then user sees the vault recovery screen automatically
When user enters their wallet password
Then user successfully restores their wallet
And user can access their accounts and assets
**Feature: Manual Wallet Deletion**
Scenario: user deletes wallet without false vault recovery
Given user has MetaMask installed with an active wallet
And user is on the Settings screen
When user navigates to "Security & Privacy"
And user taps "Delete Wallet"
And user confirms the deletion
And user restarts the app
Then user sees the onboarding screen
And user does NOT see the vault recovery screen
**Feature: OAuth Login Error Recovery**
Feature: Vault Recovery After Migration Failure
Scenario: user restores wallet after migration failure
Given user has MetaMask installed with wallet data
And a state migration fails during app startup
And redux state is reset to defaults
And vault backup exists in secure storage
When user opens the app
Then user sees the vault recovery screen automatically
When user enters their wallet password
Then user successfully restores their wallet
And user can access their accounts and assets
**Feature: Manual Wallet Deletion**
Scenario: user deletes wallet without false vault recovery
Given user has MetaMask installed with an active wallet
And user is on the Settings screen
When user navigates to "Security & Privacy"
And user taps "Delete Wallet"
And user confirms the deletion
And user restarts the app
Then user sees the onboarding screen
And user does NOT see the vault recovery screen
**Feature: OAuth Login Error Recovery**
Scenario: user experiences OAuth backup failure without false vault
recovery
Given user is creating a new wallet with OAuth (Google/Apple login)
And local wallet creation succeeds
And cloud backup fails during OAuth process
When the error handler resets the wallet state
And user restarts the app
Then user sees the onboarding screen
And user does NOT see the vault recovery screen
And user can retry OAuth login or import with seed phrase
**Feature: Migration System Upgrade Path**
Scenario: user upgrades from version 103 to version 104+
Given user has MetaMask version with migrations up to 103
And controller data is stored in redux-persist
And user has an active wallet
When user upgrades to version 104 or higher
Then migrations 104+ run successfully
And controller data is deflated to individual files
And user's wallet remains accessible
And app functions normally after restart
Scenario: user upgrades from version 104 to version 105+
Given user has MetaMask version 104
And controller data is in individual files
And user has an active wallet
When user upgrades to version 105 or higher
Then controller data is inflated for migrations
And new migrations run successfully
And controller data is deflated back to individual files
And user's wallet remains accessible
And app functions normally after restart**: user experiences OAuth
backup failure without false vault recovery
Given user is creating a new wallet with OAuth (Google/Apple login)
And local wallet creation succeeds
And cloud backup fails during OAuth process
When the error handler resets the wallet state
And user restarts the app
Then user sees the onboarding screen
And user does NOT see the vault recovery screen
And user can retry OAuth login or import with seed phrase
**Feature: Migration System Upgrade Path**
Scenario: user upgrades from version 103 to version 104+
Given user has MetaMask version with migrations up to 103
And controller data is stored in redux-persist
And user has an active wallet
When user upgrades to version 104 or higher
Then migrations 104+ run successfully
And controller data is deflated to individual files
And user's wallet remains accessible
And app functions normally after restart
Scenario: user upgrades from version 104 to version 105+
Given user has MetaMask version 104
And controller data is in individual files
And user has an active wallet
When user upgrades to version 105 or higher
Then controller data is inflated for migrations
And new migrations run successfully
And controller data is deflated back to individual files
And user's wallet remains accessible
And app functions normally after restart
## **Screenshots/Recordings**
<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->
### **Before**
<!-- [screenshots/recordings] -->
### **After**
<!-- [screenshots/recordings] -->
## **Pre-merge author checklist**
- [x] I’ve followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile
Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.
## **Pre-merge reviewer checklist**
- [x] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [x] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
<!-- CURSOR_SUMMARY -->
---
> [!NOTE]
> Detects migration failure and routes to vault recovery, ensures
controller persistence after recovery, prevents backing up temporary
wallets during resets, and updates login/recovery flows with tests.
>
> - **Onboarding/Recovery**:
> - Detects migration failure on `Onboarding` by checking vault backup
and `existingUser`; skips in E2E and explicit delete, then navigates to
`Routes.VAULT_RECOVERY.RESTORE_WALLET`.
> - `WalletRestored` now routes to `Routes.ONBOARDING.LOGIN` with
`isVaultRecovery` instead of auto-auth; adds tests.
> - `Login` sets `existingUser` via `setExistingUser(true)` after
successful unlock when `isVaultRecovery` is true; adds tests.
> - **Engine/Authentication**:
> - Adds `Engine.disableAutomaticVaultBackup` and skips `backupVault`
when true.
> - Clears vault backups and temporarily disables auto-backup during
wallet reset and OAuth error recovery; always re-enables.
> - `EngineService` moves filesystem persistence setup to
`initializeControllers` and ensures it runs after vault recovery;
updates batching and tests.
> - **Hooks/Tests**:
> - `useDeleteWallet` clears backups first, disables auto-backup during
reset, re-enables in `finally`, and resets controllers; adds robust
tests.
> - Renames and tightens numerous test titles/expectations; snapshot
names updated.
>
> <sup>Written by [Cursor
Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit
ed1a8a7. This will update automatically
on new commits. Configure
[here](https://cursor.com/dashboard?tab=bugbot).</sup>
<!-- /CURSOR_SUMMARY -->1 parent 038133d commit af57390
File tree
14 files changed
+816
-123
lines changed- app
- components
- Views
- Login
- Onboarding
- __snapshots__
- RestoreWallet
- hooks/DeleteWallet
- core
- Authentication
- EngineService
- Engine
14 files changed
+816
-123
lines changed| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
25 | 25 | | |
26 | 26 | | |
27 | 27 | | |
| 28 | + | |
28 | 29 | | |
29 | 30 | | |
30 | 31 | | |
| |||
113 | 114 | | |
114 | 115 | | |
115 | 116 | | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
116 | 124 | | |
117 | 125 | | |
118 | 126 | | |
| |||
263 | 271 | | |
264 | 272 | | |
265 | 273 | | |
266 | | - | |
| 274 | + | |
267 | 275 | | |
268 | 276 | | |
269 | 277 | | |
| |||
296 | 304 | | |
297 | 305 | | |
298 | 306 | | |
299 | | - | |
| 307 | + | |
| 308 | + | |
300 | 309 | | |
301 | | - | |
| 310 | + | |
| 311 | + | |
302 | 312 | | |
303 | 313 | | |
| 314 | + | |
304 | 315 | | |
305 | 316 | | |
306 | 317 | | |
| |||
320 | 331 | | |
321 | 332 | | |
322 | 333 | | |
323 | | - | |
| 334 | + | |
324 | 335 | | |
325 | 336 | | |
326 | 337 | | |
| |||
354 | 365 | | |
355 | 366 | | |
356 | 367 | | |
357 | | - | |
358 | 368 | | |
359 | 369 | | |
360 | 370 | | |
| |||
375 | 385 | | |
376 | 386 | | |
377 | 387 | | |
378 | | - | |
| 388 | + | |
379 | 389 | | |
380 | 390 | | |
381 | 391 | | |
| |||
418 | 428 | | |
419 | 429 | | |
420 | 430 | | |
421 | | - | |
| 431 | + | |
422 | 432 | | |
423 | 433 | | |
424 | 434 | | |
| |||
462 | 472 | | |
463 | 473 | | |
464 | 474 | | |
465 | | - | |
| 475 | + | |
466 | 476 | | |
467 | 477 | | |
468 | 478 | | |
| |||
726 | 736 | | |
727 | 737 | | |
728 | 738 | | |
729 | | - | |
| 739 | + | |
| 740 | + | |
| 741 | + | |
| 742 | + | |
| 743 | + | |
| 744 | + | |
| 745 | + | |
| 746 | + | |
| 747 | + | |
| 748 | + | |
| 749 | + | |
| 750 | + | |
| 751 | + | |
| 752 | + | |
| 753 | + | |
| 754 | + | |
| 755 | + | |
| 756 | + | |
| 757 | + | |
| 758 | + | |
| 759 | + | |
| 760 | + | |
| 761 | + | |
| 762 | + | |
| 763 | + | |
| 764 | + | |
| 765 | + | |
| 766 | + | |
| 767 | + | |
| 768 | + | |
| 769 | + | |
| 770 | + | |
| 771 | + | |
| 772 | + | |
| 773 | + | |
| 774 | + | |
| 775 | + | |
| 776 | + | |
| 777 | + | |
| 778 | + | |
| 779 | + | |
| 780 | + | |
| 781 | + | |
| 782 | + | |
| 783 | + | |
| 784 | + | |
| 785 | + | |
| 786 | + | |
| 787 | + | |
| 788 | + | |
| 789 | + | |
| 790 | + | |
| 791 | + | |
| 792 | + | |
| 793 | + | |
| 794 | + | |
| 795 | + | |
| 796 | + | |
| 797 | + | |
| 798 | + | |
| 799 | + | |
| 800 | + | |
| 801 | + | |
| 802 | + | |
| 803 | + | |
| 804 | + | |
| 805 | + | |
| 806 | + | |
| 807 | + | |
| 808 | + | |
| 809 | + | |
| 810 | + | |
| 811 | + | |
| 812 | + | |
| 813 | + | |
| 814 | + | |
| 815 | + | |
| 816 | + | |
| 817 | + | |
| 818 | + | |
| 819 | + | |
| 820 | + | |
| 821 | + | |
| 822 | + | |
| 823 | + | |
| 824 | + | |
| 825 | + | |
| 826 | + | |
| 827 | + | |
| 828 | + | |
| 829 | + | |
| 830 | + | |
| 831 | + | |
| 832 | + | |
| 833 | + | |
| 834 | + | |
| 835 | + | |
730 | 836 | | |
731 | 837 | | |
732 | 838 | | |
| |||
795 | 901 | | |
796 | 902 | | |
797 | 903 | | |
798 | | - | |
| 904 | + | |
799 | 905 | | |
800 | 906 | | |
801 | 907 | | |
| |||
860 | 966 | | |
861 | 967 | | |
862 | 968 | | |
863 | | - | |
| 969 | + | |
| 970 | + | |
864 | 971 | | |
865 | 972 | | |
866 | 973 | | |
| |||
880 | 987 | | |
881 | 988 | | |
882 | 989 | | |
| 990 | + | |
883 | 991 | | |
884 | 992 | | |
885 | 993 | | |
886 | 994 | | |
887 | 995 | | |
888 | 996 | | |
889 | 997 | | |
| 998 | + | |
890 | 999 | | |
891 | | - | |
| 1000 | + | |
892 | 1001 | | |
893 | 1002 | | |
894 | 1003 | | |
| |||
910 | 1019 | | |
911 | 1020 | | |
912 | 1021 | | |
913 | | - | |
| 1022 | + | |
914 | 1023 | | |
915 | 1024 | | |
916 | 1025 | | |
| |||
940 | 1049 | | |
941 | 1050 | | |
942 | 1051 | | |
943 | | - | |
| 1052 | + | |
944 | 1053 | | |
945 | 1054 | | |
946 | 1055 | | |
| |||
962 | 1071 | | |
963 | 1072 | | |
964 | 1073 | | |
965 | | - | |
| 1074 | + | |
966 | 1075 | | |
967 | 1076 | | |
968 | 1077 | | |
| |||
990 | 1099 | | |
991 | 1100 | | |
992 | 1101 | | |
993 | | - | |
| 1102 | + | |
994 | 1103 | | |
995 | 1104 | | |
996 | 1105 | | |
| |||
1012 | 1121 | | |
1013 | 1122 | | |
1014 | 1123 | | |
1015 | | - | |
| 1124 | + | |
1016 | 1125 | | |
1017 | 1126 | | |
1018 | 1127 | | |
| |||
1040 | 1149 | | |
1041 | 1150 | | |
1042 | 1151 | | |
1043 | | - | |
| 1152 | + | |
1044 | 1153 | | |
1045 | 1154 | | |
1046 | 1155 | | |
| |||
1054 | 1163 | | |
1055 | 1164 | | |
1056 | 1165 | | |
1057 | | - | |
| 1166 | + | |
1058 | 1167 | | |
1059 | 1168 | | |
1060 | 1169 | | |
| |||
1070 | 1179 | | |
1071 | 1180 | | |
1072 | 1181 | | |
1073 | | - | |
| 1182 | + | |
1074 | 1183 | | |
1075 | 1184 | | |
1076 | 1185 | | |
1077 | 1186 | | |
1078 | 1187 | | |
1079 | | - | |
| 1188 | + | |
1080 | 1189 | | |
1081 | 1190 | | |
1082 | 1191 | | |
| |||
1100 | 1209 | | |
1101 | 1210 | | |
1102 | 1211 | | |
1103 | | - | |
| 1212 | + | |
1104 | 1213 | | |
1105 | 1214 | | |
1106 | 1215 | | |
| |||
1131 | 1240 | | |
1132 | 1241 | | |
1133 | 1242 | | |
1134 | | - | |
| 1243 | + | |
1135 | 1244 | | |
1136 | 1245 | | |
1137 | 1246 | | |
| |||
1220 | 1329 | | |
1221 | 1330 | | |
1222 | 1331 | | |
1223 | | - | |
| 1332 | + | |
| 1333 | + | |
1224 | 1334 | | |
1225 | 1335 | | |
1226 | 1336 | | |
| |||
1239 | 1349 | | |
1240 | 1350 | | |
1241 | 1351 | | |
| 1352 | + | |
1242 | 1353 | | |
1243 | 1354 | | |
1244 | 1355 | | |
1245 | 1356 | | |
| 1357 | + | |
1246 | 1358 | | |
1247 | 1359 | | |
1248 | | - | |
1249 | 1360 | | |
1250 | 1361 | | |
1251 | 1362 | | |
| |||
0 commit comments