Fix FactoryReset to not remove CloudKit related cache #1136
Conversation
| @@ -152,8 +152,6 @@ extension CloudBackupClient { | |||
| } | |||
|
|
|||
| let savedRecord = try await uploadProfileSnapshotToICloud(profileSnapshot, id: profile.id, existingRecord: backedUpRecord) | |||
| // Migration completed, deleting old copy | |||
There was a problem hiding this comment.
@kugel3 Before merging this, I want to actually understand the reasoning behind deleting the profile from the secure storage.
IMO, the Wallet should be able to locate the profile in secure storage even after the "backup migration". To me, it is wrong to assume that backup migration means uploading the profile from the Keychain to the cloud container and then deleting it from the Keychain.
The SecureStorage(KeyChain) is the actual local storage of the Profile, and the profile should not be suddenly gone. A more correct interpretation of migration maybe is:
- Upload the profile from keychain to cloud.
- Disable keychain sync for the said profile.
Probably I am missing something as you have worked a lot more on this feature.
There was a problem hiding this comment.
I didn't see this comment before, but it was decided last week to not actually delete non-current profiles when they are "migrated". Matt still wants to delete the currently used profile from backup, but it just hit me that I am not sure we even can do that.
There was a problem hiding this comment.
Keep in mind that this code only applies to non-current profiles.
There was a problem hiding this comment.
Interesting that you mention non-current profiles. In the context of SelectBackup, aren't all profiles non-current? I don't see the migration function to be called from anywhere else than from SelectBackup.
I guess what we have to do to remove the backup from cloud, is to disable iCloud sync of the profile in SecureStorage, which we should do anyway.
| @@ -61,7 +61,7 @@ extension CacheClient: DependencyKey { | |||
| @Dependency(\.diskPersistenceClient) var diskPersistenceClient | |||
|
|
|||
| do { | |||
| try diskPersistenceClient.removeAll() | |||
| try diskPersistenceClient.remove(Entry.root) | |||
There was a problem hiding this comment.
Should we delete the removeAll from DiskPersistenceClient?
There was a problem hiding this comment.
I did choose to still have it, in case we do really need to clear the whole caches directory.
GhenadieVP
deleted the
fix/ABW-3342-i-os-pre-alpha-1-6-0-31-unable-to-see-cloud-backups-in-the-restore-wallet-from-backup-screen
branch
Jira ticket: https://radixdlt.atlassian.net/browse/ABW-3342
Jira ticket: https://radixdlt.atlassian.net/browse/ABW-3340
Description
Our CacheClient did create all of the cache folders/files at the root of
cachesdirectory, thus when it was needed to clear the cache on FactoryReset, the wholecachesdirectory was cleared out.CloudKit is using
cachesdirectory for its operations, and it would create specific folders. Thus when performing FactoryReset, those were deleted - this caused the bug of not being able to see the Cloud backups when restoring from backup immediately after FactoryReset.The error thrown by CloudKit was:
Which correlates well with the CloudKit directories being removed from
caches- CloudKit didn't know anymore where to store the temporary assets after downloading those from the container.The fix is about moving all of the CacheClient's entries under one and the same folder, thus when doing factory reset and calling
cacheClient.removeAll(), only the entries created by the CacheClient directly are removed.Video
RPReplay_Final1716529853.MP4