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-1278 Edit persona #449

Merged
merged 19 commits into from
Apr 25, 2023
Merged

ABW-1278 Edit persona #449

merged 19 commits into from
Apr 25, 2023

Conversation

kugel3
Copy link
Contributor

@kugel3 kugel3 commented Apr 20, 2023

Jira ticket: paste link here

Description

Adds persona editing to the persona list in settings as well as in Authorized Dapp details.

Notes

Also:
Refactors personas list and start using it in Authorized Dapps.
Start using push based updating of personas - letting the personaClient inform us of changes instead of relying on delegates.
Refactors PersonaDetails

RPReplay_Final1681994080.MP4

How to test

  • Edit persona details in both Settings > Personas > [some persona] and Settings > Authorized Dapps > [some dApp] > [some persona]
  • Changes should trickle back up to the "Persona details" view - the view that presents the edit view
  • Changes should also propagate to the list, when the name has been changed
  • Settings > Personas should show all personas
  • Settings > Personas > [some persona] should show all present fields
  • Settings > Authorized Dapps > [some dApp] > [some persona] should only show the shared fields
  • Settings > Authorized Dapps > [some dApp] > [some persona] > Edit persona should show all fields
  • Go back up from the edit view, you should again only see the shared fields
  • It should be possible to remove required fields, but then when using that dApp again it should request you to add the missing fields
  • If a shared field is removed and later re-added, it should automatically be re-shared
  • If you remove a shared field and then remove and reinstall the app, the wallet should not complain that a field is missing, it should still accept the profile

PR submission checklist

  • I have tested account to account transfer flow and have confirmed that it works

Comment on lines +87 to 90
guard let sharedFieldIDs = simple.sharedFieldIDs else { return nil }
let presentFields = sharedFieldIDs.compactMap { fieldID in
persona.fields.first { $0.id == fieldID }
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This changes the behaviour, on purpose: we no longer throw an error when a field is missing, we just skip it.

App/BabylonWallet.xcodeproj/project.pbxproj Outdated Show resolved Hide resolved
@@ -117,7 +117,7 @@ extension ProfileStore {
}

/// A multicasting replaying async sequence of distinct Personas for the currently selected network.
public func personaValues() async -> AnyAsyncSequence<Profile.Network.Personas> {
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI it was unnecessarily marked async for this reason: https://rdxworks.slack.com/archives/C03QFAWBRNX/p1680069940913459?thread_ts=1680027510.723299&cid=C03QFAWBRNX

happy to try without, but if we get weird compilation errors we will know what to revert.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aha. Either way it doesn't help, because the way it is wrapped we still introduce asynchronicity. Plus AnyAsyncSequence makes it throwing, which is also a shame. Makes the asynchronous for loops very ugly!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Speaking of which, I'm not sure cancellation behaves as expected, and I'm not sure the if !Task.isCancelled check works as intended.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok please have a look if you can fix the cancellation behaviour, it was my understanding that Task.isCancelled always should tell us the state of the cancellation.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah I actually though about adding reddavis/Asynchrone (repo) because it has nicer type erasure then sideffect-io/AsyncExtension, it has two variants:
AnyAsyncSequenceable
AnyThrowingAsyncSequenceable
where the first would not result in for try await .

But I deemed it not worth having THREE differnt Async libs... And we might get AsyncSequence<Element> into the language soon anyway!

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 recently asked about AsyncSequence<Element> and the reply I got didn't sound super hopeful...

Copy link
Contributor

@CyonAlexRDX CyonAlexRDX left a comment

Choose a reason for hiding this comment

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

Bravo, well done! looking forward to video demo in description, perhaps @nikola-milicevic or @GhenadieVP can help you out checking this branch out and test? OR did @umair-rdx ever setup local dev?

I'll give you a preemptive approve :)

@kugel3
Copy link
Contributor Author

kugel3 commented Apr 20, 2023

I suppose even without local dev, we could make an alpha for Umair?

@CyonAlexRDX
Copy link
Contributor

I suppose even without local dev, we could make an alpha for Umair?

Yes go ahead if not done so already

@kugel3 kugel3 merged commit 6af1209 into main Apr 25, 2023
@kugel3 kugel3 deleted the ABW-1278_Edit-persona branch April 25, 2023 09:06
@kugel3 kugel3 restored the ABW-1278_Edit-persona branch April 25, 2023 12:17
@CyonAlexRDX CyonAlexRDX deleted the ABW-1278_Edit-persona branch May 12, 2023 07:48
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants