Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix FactoryReset to not remove CloudKit related cache #1136

Merged
merged 2 commits into from
May 28, 2024
Merged

Fix FactoryReset to not remove CloudKit related cache #1136

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
86e9d84
wip
GhenadieVP May 24, 2024
eb92839
remove unusued function
GhenadieVP May 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion RadixWallet/Clients/CacheClient/CacheClient+Interface.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,8 @@ extension Address {

extension CacheClient {
public enum Entry: Equatable {
static let root: String = "RadixWallet"

public init(address: some AddressProtocol) {
self = .onLedgerEntity(.init(address: address))
}
Expand Down Expand Up @@ -161,7 +163,7 @@ extension CacheClient {
case dateOfFirstTransaction(_ accountAddress: AccountAddress)

var filesystemFilePath: String {
switch self {
let path = switch self {
case let .onLedgerEntity(entity):
entity.filesystemFilePath
case let .networkName(url):
Expand All @@ -177,6 +179,8 @@ extension CacheClient {
case let .dateOfFirstTransaction(address):
"\(filesystemFolderPath)/account-\(address.address)"
}

return "\(Self.root)/\(path)"
}

var filesystemFolderPath: String {
Expand Down
2 changes: 1 addition & 1 deletion RadixWallet/Clients/CacheClient/CacheClient+Live.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ extension CacheClient: DependencyKey {
@Dependency(\.diskPersistenceClient) var diskPersistenceClient

do {
try diskPersistenceClient.removeAll()
try diskPersistenceClient.remove(Entry.root)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we delete the removeAll from DiskPersistenceClient?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did choose to still have it, in case we do really need to clear the whole caches directory.

loggerGlobal.debug("💾 Data successfully cleared from disk")
} catch {
loggerGlobal.debug("💾❌ Could not clear cached data from disk: \(error.localizedDescription)")
Expand Down
6 changes: 1 addition & 5 deletions RadixWallet/Clients/CloudBackupClient/CloudBackupClient+Interface.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ public struct CloudBackupClient: DependencyKey, Sendable {
public let lastBackup: LastBackup
public let loadProfile: LoadProfile
public let loadAllProfiles: LoadAllProfiles
public let backupProfile: BackupProfile

public init(
startAutomaticBackups: @escaping StartAutomaticBackups,
Expand All @@ -23,8 +22,7 @@ public struct CloudBackupClient: DependencyKey, Sendable {
checkAccountStatus: @escaping CheckAccountStatus,
lastBackup: @escaping LastBackup,
loadProfile: @escaping LoadProfile,
loadAllProfiles: @escaping LoadAllProfiles,
backupProfile: @escaping BackupProfile
loadAllProfiles: @escaping LoadAllProfiles
) {
self.startAutomaticBackups = startAutomaticBackups
self.loadDeviceID = loadDeviceID
Expand All @@ -34,7 +32,6 @@ public struct CloudBackupClient: DependencyKey, Sendable {
self.lastBackup = lastBackup
self.loadProfile = loadProfile
self.loadAllProfiles = loadAllProfiles
self.backupProfile = backupProfile
}
}

Expand All @@ -47,7 +44,6 @@ extension CloudBackupClient {
public typealias LastBackup = @Sendable (ProfileID) -> AnyAsyncSequence<BackupResult?>
public typealias LoadProfile = @Sendable (ProfileID) async throws -> BackedupProfile?
public typealias LoadAllProfiles = @Sendable () async throws -> [BackedupProfile]
public typealias BackupProfile = @Sendable () async throws -> CKRecord
}

// MARK: CloudBackupClient.BackedupProfile
Expand Down
7 changes: 0 additions & 7 deletions RadixWallet/Clients/CloudBackupClient/CloudBackupClient+Live.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,8 +152,6 @@ extension CloudBackupClient {
}

let savedRecord = try await uploadProfileSnapshotToICloud(profileSnapshot, id: profile.id, existingRecord: backedUpRecord)
// Migration completed, deleting old copy
Copy link
Contributor Author

@GhenadieVP GhenadieVP May 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep in mind that this code only applies to non-current profiles.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

try secureStorageClient.deleteProfile(profile.id)

return savedRecord
}
Expand All @@ -173,11 +171,6 @@ extension CloudBackupClient {
},
loadAllProfiles: {
try await fetchAllProfileRecords().map(extractProfile)
},
backupProfile: {
let profile = await profileStore.profile
let existingRecord = try? await fetchProfileRecord(.init(recordName: profile.id.uuidString))
return try await uploadProfileToICloud(profile, existingRecord: existingRecord)
}
)
}
Expand Down
6 changes: 2 additions & 4 deletions RadixWallet/Clients/CloudBackupClient/CloudBackupClient+Test.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,7 @@ extension CloudBackupClient: TestDependencyKey {
checkAccountStatus: { throw NoopError() },
lastBackup: { _ in AsyncLazySequence([]).eraseToAnyAsyncSequence() },
loadProfile: { _ in throw NoopError() },
loadAllProfiles: { throw NoopError() },
backupProfile: { throw NoopError() }
loadAllProfiles: { throw NoopError() }
)

public static let testValue = Self(
Expand All @@ -34,7 +33,6 @@ extension CloudBackupClient: TestDependencyKey {
checkAccountStatus: unimplemented("\(Self.self).checkAccountStatus"),
lastBackup: unimplemented("\(Self.self).lastBackup"),
loadProfile: unimplemented("\(Self.self).queryProfile"),
loadAllProfiles: unimplemented("\(Self.self).queryAllProfiles"),
backupProfile: unimplemented("\(Self.self).uploadProfile")
loadAllProfiles: unimplemented("\(Self.self).queryAllProfiles")
)
}
Loading