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

fix: aggregated balance calculation #10404

Merged
merged 19 commits into from
Aug 14, 2024
Merged

Conversation

bergeron
Copy link
Contributor

@bergeron bergeron commented Jul 25, 2024

Description

Fixes the calculation for how much the aggregated balance changed in the last 1 day.

The fiat price 1 day ago was calculated incorrectly. For example it would calculate:

{"ethFiat1dAgo": 1.2011985977032296, "ethFiat": 1.28, "pricePercentChange1d": -6.156359554435198}

But this is backwards. If the price went down 6% in the last 24 hours, you would have had more fiat $ in the past, not less. This broke further calculations.

Related issues

#10406

Manual testing steps

  1. Open a wallet with just eth, no erc20 tokens (or hide them all)
  2. Verify the aggregated fiat balance % matches the balance % for ETH in the token list

Screenshots/Recordings

Before

Simplest case: a wallet with just eth. The aggregated balance should match ETH since it's the only token:

Screenshot 2024-07-24 at 6 46 20 PM

After

Screenshot 2024-07-24 at 6 45 54 PM

Matches extension now:

Screenshot 2024-07-24 at 6 41 16 PM

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@bergeron bergeron requested review from a team as code owners July 25, 2024 01:52
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@salimtb
Copy link
Contributor

salimtb commented Jul 25, 2024

i think we need the same fix here , my suggestion is :

// Get the percentage change in token price over the last day
          const tokenPriceChange1d =
            tokenExchangeRates?.[item.address as `0x${string}`]
              ?.pricePercentChange1d;
 // Calculate the token Fiat value from 1 day ago
         const tokenBalance1dAgo = tokenPriceChange1d !== undefined ?
              tokenBalanceFiat / (1 + tokenPriceChange1d / 100) : tokenBalanceFiat

@salimtb salimtb added Run Smoke E2E Triggers smoke e2e on Bitrise needs-assets-ux-review A shared label between the Assets and UX team to flag PRs ready for consolidated team review. labels Jul 25, 2024
Copy link
Contributor

github-actions bot commented Jul 25, 2024

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 8601ed6
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/c20cf4b5-baa3-4081-9401-b76e6ac23bff

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

@sahar-fehri
Copy link
Contributor

Thanks for the fix! 🙏
Tested this while looking at extension;
Noticed that extension total price percentage is not changing like mobile does as you import new tokens? Can someone else confirm?

test.mov

@salimtb
Copy link
Contributor

salimtb commented Jul 25, 2024

Thanks for the fix! 🙏 Tested this while looking at extension; Noticed that extension total price percentage is not changing like mobile does as you import new tokens? Can someone else confirm?

test.mov

hey @sahar-fehri , because the aggregated balance is only implemented on mobile for now

@darkwing
Copy link
Contributor

We need to add tests for this.

@bergeron
Copy link
Contributor Author

i think we need the same fix here ,

Should be there already, it's fixed for the erc20 token calculation too.

Copy link
Contributor

@brianacnguyen brianacnguyen left a comment

Choose a reason for hiding this comment

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

Not sure how DS was tagged to review this, as it does not touch DS

salimtb
salimtb previously approved these changes Jul 26, 2024
@salimtb salimtb added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Jul 26, 2024
Copy link
Contributor

github-actions bot commented Jul 26, 2024

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: ea93248
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/dc5282b1-702a-4c6a-95ba-f30a1951eca2

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

sahar-fehri
sahar-fehri previously approved these changes Jul 26, 2024
sahar-fehri
sahar-fehri previously approved these changes Jul 26, 2024
salimtb
salimtb previously approved these changes Jul 26, 2024
@chrisleewilcox chrisleewilcox added the No QA Needed Apply this label when your PR does not need any QA effort. label Jul 30, 2024
@bergeron
Copy link
Contributor Author

bergeron commented Jul 31, 2024

#10251 changed how accounts are represented. updated this PR to mock differently

@bergeron bergeron dismissed stale reviews from salimtb and sahar-fehri via 88c15f0 July 31, 2024 18:39
Copy link
Contributor

@gambinish gambinish left a comment

Choose a reason for hiding this comment

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

Approving with a couple comments out of curiosity ✅

@bergeron bergeron added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Aug 12, 2024
Copy link
Contributor

github-actions bot commented Aug 12, 2024

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 8936ed4
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/e99d17b2-5ac6-43ce-a004-453d178fd892

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

@bergeron bergeron added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Aug 12, 2024
Copy link

Copy link
Contributor

@Cal-L Cal-L left a comment

Choose a reason for hiding this comment

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

LGTM

@bergeron bergeron merged commit 1af0846 into main Aug 14, 2024
33 checks passed
@bergeron bergeron deleted the brian/price-diff-calculations branch August 14, 2024 16:24
@github-actions github-actions bot locked and limited conversation to collaborators Aug 14, 2024
@metamaskbot metamaskbot added the release-7.30.0 Issue or pull request that will be included in release 7.30.0 label Aug 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs-assets-ux-review A shared label between the Assets and UX team to flag PRs ready for consolidated team review. No QA Needed Apply this label when your PR does not need any QA effort. release-7.30.0 Issue or pull request that will be included in release 7.30.0 Run Smoke E2E Triggers smoke e2e on Bitrise team-assets
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

10 participants