metamask-mobile
metamask-mobile copied to clipboard
Use converted input value if internal currency is FIAT
Description
This PR adds the following changes to Amount screen:
- Inserted amount is now validated correctly when Fiat is selected as primary currency in Settings;
- It should now provide converted Fiat values to all tokens that have conversion rate available, not only for mainnet;
- Improve safety while converting
numbertoBN; - Fix inconsistent decimal cases between native tokens and ERC-20 tokens when converted to Fiat;
What to test
-
Case 1 - Insufficient funds fix
- Switch to "Binance Smart Chain Mainet"
- Switch to "Fiat" as primary currency under "Settings > General"
- Go to Send view
- Send a value under your balance, it should proceed.
-
Case 2 - Insufficient funds fix
- Switch to "Binance Smart Chain Mainet"
- Switch to "Fiat" as primary currency under "Settings > General"
- Go to Send view
- Send a value above your balance, it should error "Insufficient funds".
-
Case 3 - Convert FIAT value for tokens (non-ETH)
- Switch to "Binance Smart Chain Mainet" (other networks can also be tested)
- Switch to "Fiat" as primary currency under "Settings > General"
- Go to send view
- Insert FIAT value
- It should show converted BNB value
Note: To validate correct conversion values please compare it with cryptocompare
Checklist
- [x] There is a related GitHub issue
- [x] Tests are included if applicable
- [ ] Any added code is fully documented
Issue
Resolves #3983
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.
This branch is a draft. Please update PR to show green open icon. Makes it easy to determine if a PR is ready for QA.
What do to when we don't have conversion rate available?
- Force the user to switch to crypto as native currency?
Discussed with @Fatxx, will work on some mocks and follow up.
Figma: https://www.figma.com/file/ad2Higmy1dBMonznFEV4hZ/Fiat-converssions-not-available?node-id=5%3A102
@Fatxx can you resolve the merge conflict?
@Fatxx can you merge in main and solve these merge conflicts? I will remove needs QA for now. cc @sethkfman
@cortisiko
I fixed Issue 1 since it's related to the Amount Screen, which was the initial screen that needed to be fixed in this PR.
I'll look at 4 and 5 since they are related to the overall transaction flow.
Issues 2 and 3 are out of scope and should be developed in separate issues.
@cortisiko
Regarding issue 4: The problem here is that the sender doesn't have the token currently on the token list and the controllers fetch the exchange rates from this token list, so when the sender opens the confirm screen from the QR code the conversion rate is not available.
What I did to fix this is to add the respective erc20 token to the token list, since there is no way to get this exchange rate directly from the controllers. cc @sethkfman
Recording with the fix: https://recordit.co/vK5sfaVBoc
Acceptable decision:
- If the user doesn't accept the transaction then remove the ERC20
@cortisiko
Regarding issue 5, I'm not able to reproduce it.
https://recordit.co/pXeQTh8SUY
Test case
- User receiving payment request
- User has 0 token balance (temporarily add during send flow, don't show token)
- User has more than 0 token balance but it doesn't display in wallet (add token and display)
@cortisiko
Issue 4 fixed
When I send funds, I get [TypeError: undefined is not an object (evaluating 'weiBalance.isZero')
to reproduce: send funds to an address notice the error.
@Fatxx sending this back your way.
Close in favour of https://github.com/MetaMask/metamask-mobile/pull/5777 (there's a bug with in one conversation, unable to resolve)