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.

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

[ABW-2214] Shared persona data not persisted in profile #734

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
105c690
Align naming with Android
maciek-rdx Sep 6, 2023
0cc3e11
Update RET
maciek-rdx Sep 6, 2023
76fb812
Update Gateway
maciek-rdx Sep 6, 2023
6fdbe71
Align names
maciek-rdx Sep 6, 2023
fd3eb27
Update the snapshot
maciek-rdx Sep 6, 2023
c1b8ac6
Update tests
maciek-rdx Sep 6, 2023
37bb0d9
downgrade RET to 0.12.1-293dd4c which is used by GW and Android. Bump…
CyonAlexRDX Sep 7, 2023
ecdd8bc
WIP
CyonAlexRDX Sep 7, 2023
8ffad72
WIP
CyonAlexRDX Sep 7, 2023
c9d18ec
WIP
CyonAlexRDX Sep 7, 2023
bd33190
WIP
CyonAlexRDX Sep 7, 2023
2069f8c
Merge branch 'main' into ABW-2214_shared_persona_data_not_persisted_i…
CyonAlexRDX Sep 8, 2023
0e04719
WIP
CyonAlexRDX Sep 8, 2023
d39b310
WIP
CyonAlexRDX Sep 8, 2023
39a568c
Merge branch 'main' into ABW-2214_shared_persona_data_not_persisted_i…
CyonAlexRDX Sep 8, 2023
df6331c
fix bad merge
CyonAlexRDX Sep 8, 2023
68e4f1f
revert profile test change
CyonAlexRDX Sep 8, 2023
18fdb22
WIP
CyonAlexRDX Sep 8, 2023
2d874a8
Merge branch 'main' into ABW-2214_shared_persona_data_not_persisted_i…
CyonAlexRDX Sep 11, 2023
f89fb8d
Delete references to shared entry if it gets deleted from persona
CyonAlexRDX Sep 11, 2023
729876c
WIP
CyonAlexRDX Sep 11, 2023
9d83bb5
WIP
CyonAlexRDX Sep 11, 2023
1efa8d5
Make sure IDs of persona data entries are NOT changed when editing pe…
CyonAlexRDX Sep 11, 2023
35bee56
WIP
CyonAlexRDX Sep 11, 2023
1ee5db9
WIP
CyonAlexRDX Sep 11, 2023
ae8a69e
WIP
CyonAlexRDX Sep 11, 2023
4b9b6e7
WIP
CyonAlexRDX Sep 11, 2023
ef07c89
WIP
CyonAlexRDX Sep 11, 2023
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
1 change: 1 addition & 0 deletions Package.swift
Original file line number Diff line number Diff line change
Expand Up @@ -775,6 +775,7 @@ package.addModules([
name: "PersonasClientLive",
dependencies: [
"PersonasClient",
"AuthorizedDappsClient", // Need to update persisted shared persona data if a persona data entry is deleted, so we do not have broken references.
"ProfileStore",
],
tests: .yes()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ extension AuthorizedDappsClient {
public typealias DetailsForAuthorizedDapp = @Sendable (Profile.Network.AuthorizedDapp) async throws -> Profile.Network.AuthorizedDappDetailed
public typealias AddAuthorizedDapp = @Sendable (Profile.Network.AuthorizedDapp) async throws -> Void
public typealias UpdateOrAddAuthorizedDapp = @Sendable (Profile.Network.AuthorizedDapp) async throws -> Void
public typealias ForgetAuthorizedDapp = @Sendable (Profile.Network.AuthorizedDapp.ID, NetworkID) async throws -> Void
public typealias ForgetAuthorizedDapp = @Sendable (Profile.Network.AuthorizedDapp.ID, NetworkID?) async throws -> Void
public typealias UpdateAuthorizedDapp = @Sendable (Profile.Network.AuthorizedDapp) async throws -> Void
public typealias DeauthorizePersonaFromDapp = @Sendable (Profile.Network.Persona.ID, Profile.Network.AuthorizedDapp.ID, NetworkID) async throws -> Void
}
Expand All @@ -57,4 +57,119 @@ extension AuthorizedDappsClient {
) async throws -> IdentifiedArrayOf<Profile.Network.AuthorizedDapp> {
try await getAuthorizedDapps().filter { $0.referencesToAuthorizedPersonas.ids.contains(id) }
}

public func removeBrokenReferencesToSharedPersonaData(
personaCurrent: Profile.Network.Persona,
personaUpdated: Profile.Network.Persona
) async throws {
guard personaCurrent.id == personaUpdated.id else {
struct PersonaIDMismatch: Swift.Error {}
throw PersonaIDMismatch()
}
let identityAddress = personaCurrent.address
let dApps = try await getAuthorizedDapps()

// We only care about the updated persona
let idsOfEntriesToKeep = Set(personaUpdated.personaData.entries.map(\.id))

for authorizedDapp in dApps {
var updatedAuthedDapp = authorizedDapp
for personaSimple in authorizedDapp.referencesToAuthorizedPersonas {
guard personaSimple.identityAddress == identityAddress else {
// irrelvant Persona
continue
}
// Relevant Persona => check if there are any old PersonaData entries that needs deleting
let idsOfEntriesToDelete = personaSimple.sharedPersonaData.entryIDs.subtracting(idsOfEntriesToKeep)

guard !idsOfEntriesToDelete.isEmpty else {
// No old entries needs to be deleted.
continue
}

loggerGlobal.notice("Pruning stale PersonaData entries with IDs: \(idsOfEntriesToDelete), for persona: \(personaUpdated.address) (\(personaUpdated.displayName.rawValue)), for Dapp: \(authorizedDapp)")
var authorizedPersonaSimple = personaSimple

authorizedPersonaSimple.sharedPersonaData.remove(ids: idsOfEntriesToDelete)

// Write back to `updatedAuthedDapp`
updatedAuthedDapp.referencesToAuthorizedPersonas[id: authorizedPersonaSimple.id] = authorizedPersonaSimple

// Soundness check
if
!Set(personaUpdated.personaData.entries.map(\.id))
.isSuperset(
of:
updatedAuthedDapp
.referencesToAuthorizedPersonas[id: authorizedPersonaSimple.id]!
.sharedPersonaData
.entryIDs
)
{
let errMsg = "Incorrect implementation, failed to prune stale PersonaData entries for authorizedDapp"
assertionFailure(errMsg)
loggerGlobal.error(.init(stringLiteral: errMsg))
}
}
if updatedAuthedDapp != authorizedDapp {
// Write back `updatedAuthedDapp` to Profile only if changes were needed
try await updateAuthorizedDapp(updatedAuthedDapp)
} else {
loggerGlobal.feature("nothing to do... skipped updating authorizedDapp")
}
}
}
}

extension Profile.Network.AuthorizedDapp.AuthorizedPersonaSimple.SharedPersonaData {
private mutating func remove(id: PersonaDataEntryID) {
func removeCollectionIfNeeded(
at keyPath: WritableKeyPath<Self, Profile.Network.AuthorizedDapp.AuthorizedPersonaSimple.SharedPersonaData.SharedCollection?>
) {
guard
var collection = self[keyPath: keyPath],
collection.ids.contains(id)
else { return }
collection.ids.remove(id)
switch collection.request.quantifier {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't we need to check the count in the exact case? And why can't we keep what we have, even if it's not enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because we KNOW it cannot pass, when exactly is required, we know we can delete the whole collection, because we just decreased the number of IDs by one, thus exactly cannot be fulfilled, right?

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 can add a clarifying comment

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok, presuming it has been correctly checked previously, which is probably a reasonable assumption, but in other places here we seem to be very defensive.

But what is the high level idea, why do we need to delete anything at all? What's the harm in keeping it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kugel3 we dont want the ids to contradict request ever, so we have to delete it if it does.

case .atLeast:
if collection.ids.count < collection.request.quantity {
// must delete whole collection since requested quantity is no longer fulfilled.
self[keyPath: keyPath] = nil
}
case .exactly:
// Must delete whole collection since requested quantity is no longer fulfilled,
// since we **just** deleted the id from `ids`.
self[keyPath: keyPath] = nil
}
}

func removeEntryIfNeeded(
at keyPath: WritableKeyPath<Self, PersonaDataEntryID?>
) {
guard self[keyPath: keyPath] == id else { return }
self[keyPath: keyPath] = nil
}

removeEntryIfNeeded(at: \.name)
removeEntryIfNeeded(at: \.dateOfBirth)
removeEntryIfNeeded(at: \.companyName)
removeCollectionIfNeeded(at: \.emailAddresses)
removeCollectionIfNeeded(at: \.phoneNumbers)
removeCollectionIfNeeded(at: \.urls)
removeCollectionIfNeeded(at: \.postalAddresses)
removeCollectionIfNeeded(at: \.creditCards)

// The only purpose of this switch is to make sure we get a compilation error when we add a new PersonaData.Entry kind, so
// we do not forget to handle it here.
switch PersonaData.Entry.Kind.fullName {
case .fullName, .dateOfBirth, .companyName, .emailAddress, .phoneNumber, .url, .postalAddress, .creditCard: break
}
Comment on lines +163 to +167
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally this should be covered by tests, instead of sprinkling this switch over the codebase...

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 agree 100% with you :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when all other tickets are merged today and soundness check of a binary looks promising I can start write tests, but ATM no time :/ :/ :/

}

mutating func remove(ids: Set<PersonaDataEntryID>) {
ids.forEach {
remove(id: $0)
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,10 @@ extension AuthorizedDappsClient: DependencyKey {
_ = try $0.addAuthorizedDapp(newDapp)
}
},
forgetAuthorizedDapp: { toForget, networkID in
try await getProfileStore().updating {
forgetAuthorizedDapp: { toForget, maybeNetworkID in
let currentNetworkID = await getProfileStore().profile.networkID
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're doing this in any case, what is the purpose of even allowing caller to submit a networkID? It's not performance, so is it the case that you might want to use some other networkID?

Either way, might as well do

let networkID = maybeNetworkID ?? await getProfileStore().profile.networkID

And I would also rename maybeNetworkID to explicitNetworkID, if that is the purpose.

Copy link
Contributor Author

@CyonAlexRDX CyonAlexRDX Sep 11, 2023

Choose a reason for hiding this comment

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

always safer to use explicit networkIDs, but I was in a context where we did not have it available.

And yes, ofc I tried that spelling :D but Xcode 14.3.1 is drunk, I get this compilation error, why I had to break it up :D
Screenshot 2023-09-11 at 15 57 38

let networkID = maybeNetworkID ?? currentNetworkID
return try await getProfileStore().updating {
_ = try $0.forgetAuthorizedDapp(toForget, on: networkID)
}
},
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import Prelude

// MARK: - P2P.Dapp.Request.OneTimeAccountsRequestItem
// MARK: - P2P.Dapp.Request.ResetRequestItem
extension P2P.Dapp.Request {
public struct ResetRequestItem: Sendable, Hashable, Decodable {
public let accounts: Bool
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ public struct AuthorizedDapps: Sendable, FeatureReducer {
case loadedDapps(TaskResult<Profile.Network.AuthorizedDapps>)
case loadedThumbnail(URL, dApp: Profile.Network.AuthorizedDapp.ID)
case presentDappDetails(DappDetails.State)
case failedToGetDetailsOfDapp(id: Profile.Network.AuthorizedDapp.ID)
}

public enum ChildAction: Sendable, Equatable {
Expand All @@ -56,17 +57,16 @@ public struct AuthorizedDapps: Sendable, FeatureReducer {
public func reduce(into state: inout State, viewAction: ViewAction) -> EffectTask<Action> {
switch viewAction {
case .appeared:
return .task {
await loadAuthorizedDapps()
}
return loadAuthorizedDapps()

case let .didSelectDapp(dAppID):
return .run { send in
let details = try await authorizedDappsClient.getDetailedDapp(dAppID)
let presentedDappState = DappDetails.State(dApp: details)
await send(.internal(.presentDappDetails(presentedDappState)))
} catch: { error, _ in
} catch: { error, send in
errorQueue.schedule(error)
await send(.internal(.failedToGetDetailsOfDapp(id: dAppID)))
}
}
}
Expand All @@ -86,6 +86,19 @@ public struct AuthorizedDapps: Sendable, FeatureReducer {
}
}
}

case let .failedToGetDetailsOfDapp(dappId):
#if DEBUG
return .run { _ in
CyonAlexRDX marked this conversation as resolved.
Show resolved Hide resolved
loggerGlobal.notice("DEBUG ONLY deleting authorized dapp since we failed to load detailed info about it")
try? await authorizedDappsClient.forgetAuthorizedDapp(dappId, nil)

}.concatenate(with: loadAuthorizedDapps())
#else
// FIXME: Should we have to handle this, this is a discrepancy bug..
return .none
#endif

case let .loadedDapps(.failure(error)):
errorQueue.schedule(error)
return .none
Expand All @@ -103,18 +116,19 @@ public struct AuthorizedDapps: Sendable, FeatureReducer {
case .presentedDapp(.presented(.delegate(.dAppForgotten))):
return .run { send in
await send(.child(.presentedDapp(.dismiss)))
await send(loadAuthorizedDapps())
}
}.concatenate(with: loadAuthorizedDapps())
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: move on next line.


case .presentedDapp:
return .none
}
}

private func loadAuthorizedDapps() async -> Action {
let result = await TaskResult {
try await authorizedDappsClient.getAuthorizedDapps()
private func loadAuthorizedDapps() -> EffectTask<Action> {
CyonAlexRDX marked this conversation as resolved.
Show resolved Hide resolved
.run { send in
let result = await TaskResult {
try await authorizedDappsClient.getAuthorizedDapps()
}
await send(.internal(.loadedDapps(result)))
}
return .internal(.loadedDapps(result))
}
}
Loading
Loading