fix(multichain): fix showFiat option for test assets#26224
Merged
Conversation
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. |
7 tasks
Collaborator
Builds ready [3631381]
Page Load Metrics (199 ± 202 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
…fix tests balances and snapshots
…al + fix snapshots
…al and account-list-item tests
…ccount state + snapshot
edfb49b to
ef336a0
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #26224 +/- ##
===========================================
+ Coverage 69.96% 69.96% +0.01%
===========================================
Files 1410 1411 +1
Lines 49915 49990 +75
Branches 13802 13806 +4
===========================================
+ Hits 34919 34975 +56
- Misses 14996 15015 +19 ☔ View full report in Codecov by Sentry. |
Collaborator
Builds ready [bdbdc12]
Page Load Metrics (173 ± 173 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Collaborator
Builds ready [05dacfc]
Page Load Metrics (205 ± 200 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
ccharly
commented
Jul 31, 2024
| class="mm-box currency-display-component mm-box--display-flex mm-box--flex-wrap-wrap mm-box--align-items-center" | ||
| data-testid="first-currency-display" | ||
| title="$0.00 USD" | ||
| title="0 ETH" |
Contributor
Author
There was a problem hiding this comment.
I assume this snapshot was wrong for quite some time now.
The providerConfig on mock-state.json here is 0x5 (goerli), which is a testnet.
Test assets/accounts should not show any fiat values unless showFiatInTestnets is set to true (but it's currently false here)
You will see a bunch of snapshots changes in this PR, because of this ⬆️.
ccharly
commented
Jul 31, 2024
| <div | ||
| class="mm-box currency-display-component mm-box--display-flex mm-box--flex-wrap-wrap mm-box--align-items-center" | ||
| data-testid="first-currency-display" | ||
| title="$880.18 USD" |
Contributor
Author
There was a problem hiding this comment.
I did not track why the fiat balance was actually this low in the original snapshot.
However, based on this account's balance, the actual balance should be:
0x346ba7725f412cbfdb # Hex (WEI)
# Convert to decimal
966987986469506500000 # Decimal (WEI)
# Convert to ETH
966.9879864695065 # Decimal (ETH)
So here again, I guess the snapshot was wrong too. Also, the same comment applies here, so no more fiat.
danroc
reviewed
Aug 1, 2024
Contributor
I think we shouldn't truncate the unit, WDYT? |
Contributor
Author
|
I think we should not truncate no, but that's not something new, it seems to behave the same for Ethereum:
So I believe it should be fixed separately, is that ok for you? |
danroc
reviewed
Aug 1, 2024
Co-authored-by: Daniel Rocha <daniel.rocha@consensys.net>
|
danroc
approved these changes
Aug 1, 2024
Collaborator
Builds ready [24e70a6]
Page Load Metrics (62 ± 6 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
matthewwalsh0
approved these changes
Aug 1, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Since the introduction of the new Bitcoin accounts in the account-list-menu, we introduced a regression regarding the "Show conversion in testnets" flag (for both Bitcoin and Ethereum accounts).
This PR fixes that.
Related issues
Fixes:
Manual testing steps
yarn start:flaskScreenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist