metamask-mobile icon indicating copy to clipboard operation
metamask-mobile copied to clipboard

Use converted input value if internal currency is FIAT

Open Fatxx opened this issue 3 years ago • 4 comments

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 number to BN;
  • 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

Fatxx avatar Mar 30 '22 19:03 Fatxx

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.

github-actions[bot] avatar Mar 30 '22 19:03 github-actions[bot]

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.

chrisleewilcox avatar Apr 12 '22 15:04 chrisleewilcox

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.

jakehaugen avatar May 11 '22 22:05 jakehaugen

Figma: https://www.figma.com/file/ad2Higmy1dBMonznFEV4hZ/Fiat-converssions-not-available?node-id=5%3A102

Fatxx avatar Jun 07 '22 14:06 Fatxx

@Fatxx can you resolve the merge conflict?

chrisleewilcox avatar Oct 15 '22 01:10 chrisleewilcox

@Fatxx can you merge in main and solve these merge conflicts? I will remove needs QA for now. cc @sethkfman

cortisiko avatar Dec 16 '22 15:12 cortisiko

@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.

Fatxx avatar Dec 29 '22 15:12 Fatxx

@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

Fatxx avatar Dec 29 '22 20:12 Fatxx

@cortisiko

Regarding issue 5, I'm not able to reproduce it.

https://recordit.co/pXeQTh8SUY

Fatxx avatar Dec 29 '22 20:12 Fatxx

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)

sethkfman avatar Jan 13 '23 18:01 sethkfman

@cortisiko

Issue 4 fixed

Fatxx avatar Jan 17 '23 19:01 Fatxx

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.

cortisiko avatar Feb 09 '23 23:02 cortisiko

Close in favour of https://github.com/MetaMask/metamask-mobile/pull/5777 (there's a bug with in one conversation, unable to resolve)

Fatxx avatar Feb 17 '23 02:02 Fatxx