-
Notifications
You must be signed in to change notification settings - Fork 9
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-3762] Account lock claim status #1312
Conversation
RadixWallet/Clients/AccountLockersClient/AccountLockersClient+Live.swift
Outdated
Show resolved
Hide resolved
RadixWallet/Clients/AccountLockersClient/AccountLockersClient+Live.swift
Outdated
Show resolved
Hide resolved
RadixWallet/Clients/AccountLockersClient/AccountLockersClient+Interface.swift
Show resolved
Hide resolved
RadixWallet/Clients/AccountLockersClient/AccountLockersClient+Interface.swift
Outdated
Show resolved
Hide resolved
RadixWallet/Clients/AccountLockersClient/AccountLockersClient+Interface.swift
Outdated
Show resolved
Hide resolved
RadixWallet/Clients/AccountLockersClient/AccountLockersClient+Interface.swift
Outdated
Show resolved
Hide resolved
RadixWallet/Clients/AccountLockersClient/AccountLockersClient+Live.swift
Outdated
Show resolved
Hide resolved
RadixWallet/Clients/AccountLockersClient/AccountLockersClient+Live.swift
Outdated
Show resolved
Hide resolved
RadixWallet/Clients/GatewayAPI/GatewayAPIClient+Extension.swift
Outdated
Show resolved
Hide resolved
RadixWallet/Features/DappsAndPersonas/AuthorizedDApps/AuthorizedDApps.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, added an improvement suggestion.
|
||
public func reduce(into state: inout State, viewAction: ViewAction) -> Effect<Action> { | ||
switch viewAction { | ||
case .task: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At some point we did have the subscription for resources as well per account, but we did change that in favour of updating all of the rows from the Home reducer. Moving the subscription to accountClaims to HomeReducer would simplify some things and align it with resources loading, wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess that for resources loading it makes sense since we can optimize the requests. For account claims we are already making the requests for all of them at one place (the AccountLockerClient
), so it wouldn't make a difference in such sense.
I liked the idea of having each row making the suscription, and being independent from the parent (Home
) mutating its rows and modifying content directly (which doesn't align much with TCA architecture).
On the other hand, each subscription could bring a performance cost, but haven't seen anything noticeable while testing it.
What advantages do you see in such change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is exactly about reducing the number of subscription tasks, which don't always work as expected. Given that all account claims are updated at once, not individually, it makes even more sense to update the rows from the parent.
Also, we were thinking in terms of reducing the TCA bloat, to have eventually onky the Home be a reducer, and the children just vanilla swiftUI views with viewState.
Co-authored-by: danvleju-rdx <[email protected]> Co-authored-by: radixbot <[email protected]> Co-authored-by: Ghenadie Vasiliev-Pusca <[email protected]> Co-authored-by: Ghenadie <[email protected]>
Jira ticket: ABW-3762
Description
This PR adds the logic to check for pending claims on account lockers, and displaying them on the account rows at Home or inside the Approved dApps view.
Video
Link
This video shows two sessions. Simulator on the left shows a wallet with different dApps, while simulator on the right shows a regular consumer wallet with two accounts.
dApp1
, and are displayed for each account at Home.dApp1
, which hides the notifications from previous step.dApp2
intoDeny all
accountDeny all
accountdApp1
again