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

[5.3.0][FIX] Prevent crash when funds warning is pressed #4178

Merged
merged 19 commits into from
May 26, 2022

Conversation

tommasini
Copy link
Contributor

@tommasini tommasini commented Apr 28, 2022

Description
The app is crashing when we do not have enough funds to do a transaction on testnet

Proposed Solution
The app will redirect to the MetaMask faucet when it's testnet
The app will redirect to onRamp with the crypto we are trying to transact

Code Impact
Medium, was changed in all transaction confirmations screens/modals:

  • Confirm screen of send flow
  • Modal of transaction Review
  • Modal of transaction approval

Test Cases
(For mainnet, avax and rinkeby)
Case 1:

  • Press the warning on the confirm screen of send flow
    Case 2:
  • Press the warning on transaction modal of deeplinks
    Case 3:
  • Press the warning on transaction modal of webview

Screenshots/Recordings
Bug:
http://recordit.co/wpPgIUiPcG

Solution:
https://recordit.co/KR5KHCtYOV

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

Issue

Progresses #4177

@tommasini tommasini requested a review from a team as a code owner April 28, 2022 18:37
@github-actions
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.

Copy link
Member

@gantunesr gantunesr left a comment

Choose a reason for hiding this comment

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

lgtm

@gantunesr gantunesr changed the base branch from main to release/5.1.0 April 28, 2022 19:29
Copy link
Member

@cortisiko cortisiko left a comment

Choose a reason for hiding this comment

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

Some custom networks have the ability to onramp. for example BSC, Polygon, Avalanche.
With that being said if i were to be on any of those custom networks (in my case i was on polygon) & i tap the warning message I am taken the faucet page

To reproduce:

  • with an account with no funds add a custom network (for example BSC, Polygon, Avalanche.)
  • go to https://metamask.github.io/test-dapp/
  • connect your wallet
  • tap on deploy contract, or create token
  • now tap on the warning message
  • notice the error

@cortisiko cortisiko added the QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed label Apr 28, 2022
app/util/networks/index.js Outdated Show resolved Hide resolved
@tommasini
Copy link
Contributor Author

  • Replaced all the places where transaction needs to be confirmed
  • The message is according to the coin that you are trying to transact
  • New coins added, BSC, Polygon, Avalanche, Celo and Fantom

https://images.zenhubusercontent.com/141427485/57f78fe9-f2e6-4858-9eee-d690f317477d/simulator_screen_recording___iphone_11_pro___2022_05_02_at_18_15_01.mp4

@tommasini tommasini added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label May 2, 2022
@gantunesr gantunesr changed the title The app does not crash anymore when the transaction funds warning is pressed [FIX] Prevent crash when funds warning is pressed May 5, 2022
@tommasini tommasini changed the base branch from release/5.1.0 to main May 10, 2022 10:27
app/util/networks/index.js Outdated Show resolved Hide resolved
app/components/UI/ApproveTransactionReview/index.js Outdated Show resolved Hide resolved
app/components/Views/SendFlow/Confirm/index.js Outdated Show resolved Hide resolved
app/components/Views/SendFlow/Confirm/index.js Outdated Show resolved Hide resolved
app/constants/network.js Outdated Show resolved Hide resolved
locales/languages/en.json Outdated Show resolved Hide resolved
@cortisiko
Copy link
Member

@tommasini The app no longer crashes but we have an issue. The custom networks (BSC, Polygon, Avalanche, Celo and Fantom) were hard coded into the networks list: http://recordit.co/NfLmFZAph1

This should not be the case. Only mainnet & testnets should be hard coded in the network lists.

To reproduce:
launch the app
create/import your wallet
on the wallet view tap on the network name in the top nav.
notice (BSC, Polygon, Avalanche, Celo and Fantom) is automatically added.

Furthermore If I were to select any of the hard coded networks I get Error: Unrecognized network type: 'polygon'

see here: http://recordit.co/vCFebnF6qM

@cortisiko cortisiko added QA Passed A successful QA run through has been done and removed QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed labels May 12, 2022
Copy link
Member

@cortisiko cortisiko left a comment

Choose a reason for hiding this comment

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

🌮 🌮 🌮 🌮

app/util/networks/index.js Outdated Show resolved Hide resolved
Copy link
Contributor

@Fatxx Fatxx left a comment

Choose a reason for hiding this comment

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

LGTM

@tommasini tommasini changed the title [FIX] Prevent crash when funds warning is pressed [5.3.0][FIX] Prevent crash when funds warning is pressed May 26, 2022
@tommasini tommasini merged commit be47142 into main May 26, 2022
@tommasini tommasini deleted the fix/4177-fix-crash-on-press-transaction-warning branch May 26, 2022 10:00
@github-actions github-actions bot locked and limited conversation to collaborators May 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) QA Passed A successful QA run through has been done
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants