-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
feat: migrate to native primary currency #8720
Conversation
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. |
E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/0602c987-5a60-45c1-ac43-73c4a8d1b53a |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8720 +/- ##
==========================================
+ Coverage 43.74% 43.77% +0.02%
==========================================
Files 1273 1274 +1
Lines 31103 31124 +21
Branches 3161 3168 +7
==========================================
+ Hits 13606 13623 +17
- Misses 16684 16688 +4
Partials 813 813 ☔ View full report in Codecov by Sentry. |
0a2ac76
to
7af5342
Compare
E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/17d52963-c977-454a-9269-849bbfdbb795 |
7af5342
to
e35de21
Compare
E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/6bdf4db5-c5c2-469d-8f74-4bad5666bc11 |
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.
Hey @salimtb a few more test scenarios come to mind:
- Changing your currency conversion from USD to CAD and submit a txn
- Change your currency conversion from USD to DAI, ETC and submit a txn
Does this PR require migrations? If so here is another scenario to test:
- Test with a version of the app where you select Fiat as your primary currency
- Upgrade to the app with this version. Does the app break? try visiting the general view does the app break? Try to submit another txn.
e35de21
to
6bcf50d
Compare
hey @cortisiko , |
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.
@salimtb, I have reviewed your testing.
The idea of removing the fiat toggle makes sense to me. I'm concerned that omitting the fiat conversion of the total transaction amount on the confirmation screen could cause user confusion.
see this video at the 17-19 second mark
970bbbc
to
3ce37d9
Compare
3ce37d9
to
7f8e10b
Compare
Thanks for providing testing artifacts @salimtb. Everything looks ✅ from a quality perspective. |
7f8e10b
to
fe066be
Compare
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.
Left some comments
...ts/Views/confirmations/components/TransactionReview/TransactionReviewEIP1559Update/index.tsx
Outdated
Show resolved
Hide resolved
1f8f04f
to
4835a60
Compare
Quality Gate passedIssues Measures |
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!
Description
The primary currency toggle provides unnecessary overhead for every asset feature we build. The Extension team is removing it for Extension and we will do so as well for Mobile.
the default one on mobile will be Native.
Related issues
Fixes: #1846
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist