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

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

Merged
merged 28 commits into from
Sep 11, 2023
Merged

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

merged 28 commits into from
Sep 11, 2023

Conversation

CyonAlexRDX
Copy link
Contributor

@CyonAlexRDX CyonAlexRDX commented Sep 8, 2023 •
edited
Loading

  • ensure we actually save Shared PersonaData in AuthDapp
  • try to autofill request with in Profile saved previously shared PersonaData
  • populate view with shared Persona data in AuthDapp -> Persona screen

Demo

See 8 min video on Slack

maciek-rdx and others added 17 commits September 6, 2023 17:15
@CyonAlexRDX CyonAlexRDX marked this pull request as ready for review September 11, 2023 06:23
GhenadieVP
GhenadieVP approved these changes Sep 11, 2023
View reviewed changes
Copy link
Contributor

@GhenadieVP GhenadieVP left a comment

Choose a reason for hiding this comment

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

LGTM,
added some suggestions, will perform some tests also.

Sources/Features/AuthorizedDAppsFeature/AuthorizedDApps/AuthorizedDApps.swift
@@ -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.

Sources/Features/DebugInspectProfile/InspectProfile+View.swift Show resolved Hide resolved
Sources/Features/DappInteractionFeature/Coordinator/DappInteractionFlow.swift Outdated
return nil
}
let personaDataEntries = personaData[keyPath: personaDataKeyPath]
let set = try OrderedSet<T>(validating: personaDataEntries.map(\.value))
Copy link
Contributor

Choose a reason for hiding this comment

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

better use something like personaDataEntriesSet

Copy link
Contributor

Choose a reason for hiding this comment

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

100% agree

Sources/Features/DappInteractionFeature/Coordinator/DappInteractionFlow.swift Outdated
Comment on lines 846 to 856
let expectedQuantity = numberOfRequestedElements.quantity
switch numberOfRequestedElements.quantifier {
case .atLeast:
guard set.count >= expectedQuantity else {
return nil
}
case .exactly:
guard set.count == expectedQuantity else {
return nil
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

could func satisfies(_ requestedNumber: RequestedNumber) be used instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

If not, it should be generalised, and either way it could operate on Collection rather than OrderedSet.

kugel3
kugel3 reviewed Sep 11, 2023
View reviewed changes
Sources/Clients/AuthorizedDappsClientLive/AuthorizedDappsClient+Live.swift
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 •
edited
Loading

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

kugel3
kugel3 reviewed Sep 11, 2023
View reviewed changes
Sources/Features/DappInteractionFeature/Coordinator/DappInteractionFlow.swift Outdated
Comment on lines 595 to 610
if let numberOfRequestedPhoneNumbers = personaDataRequested.numberOfRequestedPhoneNumbers {
guard
let phoneNumbers = responseItem.phoneNumbers,
phoneNumbers.satisfies(numberOfRequestedPhoneNumbers)
else {
throw RequiredPersonaDataFieldsNotPresentInResponse(
missingEntryKind: .phoneNumber
)
}
}

// FIXME: Handle dateOfBirth
// FIXME: Handle companyName
// FIXME: Handle urls
// FIXME: Handle postalAddresses
// FIXME: Handle creditCards
Copy link
Contributor

Choose a reason for hiding this comment

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

If the requirement we're checking here is always the number, we could abstract the check into a general function so that each field becomes a one-liner

kugel3
kugel3 reviewed Sep 11, 2023
View reviewed changes
Sources/Clients/AuthorizedDappsClient/AuthorizedDappsClient+Interface.swift
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.

kugel3
kugel3 reviewed Sep 11, 2023
View reviewed changes
Sources/Clients/AuthorizedDappsClient/AuthorizedDappsClient+Interface.swift
Comment on lines +138 to +142
// 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
}
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 :/ :/ :/

@CyonAlexRDX CyonAlexRDX merged commit 0aac52c into main Sep 11, 2023
5 checks passed
@CyonAlexRDX CyonAlexRDX deleted the ABW-2214_shared_persona_data_not_persisted_in_profile branch September 11, 2023 15:55
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kugel3 kugel3 kugel3 approved these changes

@GhenadieVP GhenadieVP GhenadieVP approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@CyonAlexRDX @GhenadieVP @kugel3 @maciek-rdx