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-3762] Account lock claim status #1312

Merged
merged 33 commits into from
Sep 11, 2024
Merged

Conversation

matiasbzurovski
Copy link
Contributor

@matiasbzurovski matiasbzurovski commented Sep 3, 2024

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.

  • Consumer wallet starts and indicates there is a pending account locker claim for each account. These deposits correspond to dApp1, and are displayed for each account at Home.
  • User hides deposit claims for dApp1, which hides the notifications from previous step.
  • A new deposit is made from dApp2 into Deny all account
  • After a few seconds, the consumer app detects the deposit and displays it under Deny all account
  • User reverts change from second step, and shows deposits from dApp1 again
  • App now shows 1 pending deposit for first account, and 2 pending deposits for second account

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 an improvement suggestion.


public func reduce(into state: inout State, viewAction: ViewAction) -> Effect<Action> {
switch viewAction {
case .task:
Copy link
Contributor

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?

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 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?

Copy link
Contributor

@GhenadieVP GhenadieVP Sep 10, 2024

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.

@matiasbzurovski matiasbzurovski merged commit ec1ce73 into main Sep 11, 2024
6 checks passed
@matiasbzurovski matiasbzurovski deleted the ABW-3762-claim-status branch September 11, 2024 16:25
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.

3 participants