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 1 commit
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 @@ -57,4 +57,94 @@ 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 update persona
CyonAlexRDX marked this conversation as resolved.
Show resolved Hide resolved
let idsOfEntriesToKeep = Set(personaUpdated.personaData.entries.map(\.id))

for authedDapp in dApps {
CyonAlexRDX marked this conversation as resolved.
Show resolved Hide resolved
var updatedAuthedDapp = authedDapp
for personaSimple in authedDapp.referencesToAuthorizedPersonas {
guard personaSimple.identityAddress == identityAddress else {
continue
}
let sharedData = personaSimple.sharedPersonaData
CyonAlexRDX marked this conversation as resolved.
Show resolved Hide resolved
let idsOfEntriesToDelete = sharedData.entryIDs.subtracting(idsOfEntriesToKeep)

guard !idsOfEntriesToDelete.isEmpty else {
continue
}

var authorizedPersonaSimple = personaSimple

authorizedPersonaSimple.sharedPersonaData.remove(ids: idsOfEntriesToDelete)
updatedAuthedDapp.referencesToAuthorizedPersonas[id: authorizedPersonaSimple.id] = authorizedPersonaSimple
}
if updatedAuthedDapp != authedDapp {
try await updateAuthorizedDapp(updatedAuthedDapp)
}
}
}
}

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.
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)
}
}
}
6 changes: 6 additions & 0 deletions Sources/Features/EditPersonaFeature/EditPersona.swift
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import AuthorizedDappsClient
import FeaturePrelude
import PersonasClient
import Prelude
Expand Down Expand Up @@ -114,6 +115,7 @@ public struct EditPersona: Sendable, FeatureReducer {
public init() {}

@Dependency(\.dismiss) var dismiss
@Dependency(\.authorizedDappsClient) var authorizedDappsClient
@Dependency(\.personasClient) var personasClient
@Dependency(\.errorQueue) var errorQueue

Expand Down Expand Up @@ -159,6 +161,10 @@ public struct EditPersona: Sendable, FeatureReducer {
return .run { [state] send in
let updatedPersona = state.persona.updated(with: output)
loggerGlobal.critical("calling personasClient.updatePersona")
try await authorizedDappsClient.removeBrokenReferencesToSharedPersonaData(
personaCurrent: state.persona,
personaUpdated: updatedPersona
)
try await personasClient.updatePersona(updatedPersona)
loggerGlobal.critical("personasClient.updatePersona DONE => delegating")
await send(.delegate(.personaSaved(updatedPersona)))
Expand Down
18 changes: 9 additions & 9 deletions Sources/Profile/AuthorizedDapp/AuthorizedDapp.swift
Original file line number Diff line number Diff line change
Expand Up @@ -151,15 +151,15 @@ extension Profile.Network.AuthorizedDapp {
{
public typealias SharedCollection = Shared<PersonaDataEntryID>

public let name: PersonaDataEntryID?
public let dateOfBirth: PersonaDataEntryID?
public let companyName: PersonaDataEntryID?
public var name: PersonaDataEntryID?
public var dateOfBirth: PersonaDataEntryID?
public var companyName: PersonaDataEntryID?

public let emailAddresses: SharedCollection?
public let phoneNumbers: SharedCollection?
public let urls: SharedCollection?
public let postalAddresses: SharedCollection?
public let creditCards: SharedCollection?
public var emailAddresses: SharedCollection?
public var phoneNumbers: SharedCollection?
public var urls: SharedCollection?
public var postalAddresses: SharedCollection?
public var creditCards: SharedCollection?

public var entryIDs: Set<PersonaDataEntryID> {
var ids: [PersonaDataEntryID] = [
Expand Down Expand Up @@ -217,7 +217,7 @@ extension Profile.Network.AuthorizedDapp {
{
public typealias Number = RequestedNumber
public let request: Number
public private(set) var ids: OrderedSet<ID>
public var ids: OrderedSet<ID>

public init(
ids: OrderedSet<ID>,
Expand Down
Loading