Skip to content
This repository has been archived by the owner on May 10, 2024. It is now read-only.

Fix #5027: Slow portfolio balance calls can show balance incorrectly #5098

Merged
merged 3 commits into from
Mar 16, 2022
Merged

Fix #5027: Slow portfolio balance calls can show balance incorrectly #5098

merged 3 commits into from
Mar 16, 2022

Conversation

StephenHeaps
Copy link
Contributor

@StephenHeaps StephenHeaps commented Mar 11, 2022 •
edited
Loading

Summary of Changes

  • There was a race condition as update() would reset userVisibleAssets and call fetchBalances and fetchPrices which both directly modifier userVisibleAssets, but if update() is called >1 times prior to fetch calls responding we could double up balances.
  • Added PortfolioStoreTests to test against update()

This pull request fixes #5027

Submitter Checklist:

  • Unit Tests are updated to cover new or changed functionality
  • User-facing strings use NSLocalizableString()

Test Plan:

Best to reproduce this off development first as race conditions are somewhat tricky.

  1. Open and unlock wallet
  2. Turn on network link conditioner to simulate slow network; I used 100kb/s down & up, 10ms delay and 25% packet drop for a slow network.
  3. Rapidly change networks while loading
  4. Observe balance exceed actual balance

Screenshots:

Broken against development:
https://user-images.githubusercontent.com/5314553/157954951-c233204e-dbb5-4ac1-99d0-9c32f2125232.mov

Fixed against this bugfix/5027-update-race-condition branch:
https://user-images.githubusercontent.com/5314553/157961890-8db7a11b-473a-49f0-9061-db7017b9e3d6.mov

Reviewer Checklist:

  • Issues include necessary QA labels:
    • QA/(Yes|No)
    • bug / enhancement
  • Necessary security reviews have taken place.
  • Adequate unit test coverage exists to prevent regressions.
  • Adequate test plan exists for QA to validate (if applicable).
  • Issue and pull request is assigned to a milestone (should happen at merge time).

@StephenHeaps StephenHeaps requested review from kylehickinson, nuo-xu and a team March 11, 2022 20:20
nuo-xu
nuo-xu reviewed Mar 11, 2022
View reviewed changes
BraveWallet/Crypto/Stores/PortfolioStore.swift Outdated
Comment on lines 153 to 154
self.isLoadingBalances = true
self.rpcService.chainId { [self] chainId in
Copy link
Contributor

Choose a reason for hiding this comment

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

dont need self here

@StephenHeaps
- Fix #5027: Fix race condition in PortfolioStores update function …
0c1e513 
…which could cause us to double our balance if update is called repeatedly (changing networks, or choosing same network).
@StephenHeaps
Add PortfolioStoreTests for unit testing the PortfolioStore. Curr…
2508638 
…ently verifies `update()` will publish changes to it's various publishers correctly.
@StephenHeaps
Address PR comments: remove usage of self when not necessary
387f565
@StephenHeaps StephenHeaps force-pushed the bugfix/5027-update-race-condition branch from b460f6d to 387f565 Compare March 16, 2022 16:17
@StephenHeaps StephenHeaps merged commit 2997557 into development Mar 16, 2022
@StephenHeaps StephenHeaps deleted the bugfix/5027-update-race-condition branch March 16, 2022 16:21
@StephenHeaps StephenHeaps mentioned this pull request Mar 16, 2022
Merged
7 tasks
@iccub iccub added this to the 1.37 milestone Mar 17, 2022
@kjozwiak kjozwiak mentioned this pull request Apr 7, 2022
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@nuo-xu nuo-xu nuo-xu approved these changes

@kylehickinson kylehickinson Awaiting requested review from kylehickinson

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.37
Development

Successfully merging this pull request may close these issues.

Slow portfolio balance calls can cause balance to show incorrectly
3 participants
@StephenHeaps @nuo-xu @iccub