Feature/make insufficient fee descriptive #2076

rickycodes · 2020-12-16T04:51:23Z

Description

Make 'Insufficient fee' error descriptive & add link to Buy ETH

Previously we were only displaying "Insufficient funds":

Now we display a more detailed error with the amount of ETH required along with a link to buy more ETH:

Checklist

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

Issue

Resolves https://github.com/MetaMask/mobile-planning/issues/7

rickycodes · 2020-12-19T20:04:27Z

app/components/UI/TransactionReview/TransactionReviewInformation/index.js

@@ -16,6 +16,9 @@ import {
 import { strings } from '../../../../../locales/i18n';
 import { getTicker, getNormalizedTxState } from '../../../../util/transactions';
 import TransactionReviewFeeCard from '../TransactionReviewFeeCard';
+import Analytics from '../../../../core/Analytics';
+import { ANALYTICS_EVENT_OPTS } from '../../../../util/analytics';
+import { withNavigation } from 'react-navigation';


rather than passing navigation through props we use withNavigation higher order component

rickycodes · 2020-12-21T21:07:24Z

app/components/Nav/Main/index.js

@@ -417,7 +417,11 @@ const Main = props => {

 	const renderDappTransactionModal = () =>
 		props.dappTransactionModalVisible && (
-			<Approval dappTransactionModalVisible toggleDappTransactionModal={props.toggleDappTransactionModal} />
+			<Approval
+				navigation={props.navigation}


this is unrelated, but Approval was missing navigation prop. it needs it for this: https://github.com/MetaMask/metamask-mobile/blob/develop/app/components/Views/Approval/index.js#L92-L96. I've made the prop isRequired on it now so we don't do this

But I think this pattern of passing navigation down through props is bad and we should consider using withNavigation instead

rickycodes · 2020-12-21T21:12:10Z

app/components/UI/TransactionReview/TransactionReviewInformation/index.js

+		const { onCancelPress, navigation } = this.props;
+		/* this is kinda weird, we have to reject the transaction to collapse the modal */
+		onCancelPress();
+		navigation.navigate('PaymentMethodSelector');


it would be nice to be able to leave the transaction up and be able to get the user to buy eth and come back to the aforementioned transaction but it's not really possible here. I think for this rejecting is fine and the user can just go through the trouble of setting up the transaction again

ibrahimtaveras00

Issue 1:

I'm unable to scroll to see any of the messaging in this approve modal

I got this view by using this deeplink:

https://metamask.app.link/approve/0x1d7a9cd8c67e3c74a84bb76cbc1b606e2e460db5@1/approve?address=0x1FDb169Ef12954F20A15852980e1F0C122BfC1D6&uint256=1

Issue 2:

I do see it on the Dapp confirm modal and contract deployment modal, however, I'm wondering, should we still show the option to buy ETH on testnets not supported?

For instance there's no option to buy ETH on Ropsten however, I see the link to buy ETH still

this is from https://metamask.github.io/test-dapp

Issue 3:

I'm not seeing this option when I come from a request, am I supposed to?
cc: @cjeria

I got here by using this deeplink:

https://metamask.app.link/send/0x1FDb169Ef12954F20A15852980e1F0C122BfC1D6@1?value=1e15

rickycodes · 2021-01-05T04:01:38Z

I'm not seeing this option when I come from a request, am I supposed to?

Ah good catch. yes we should also make this work for deep links (I can add that).

For instance there's no option to buy ETH on Ropsten however, I see the link to buy ETH still

Maybe in those cases we should have links to the faucets instead?

estebanmino

lgtm

Make the unknown network id 9999 for good measure

ibrahimtaveras00

Fixes look good, QA Passed 👍🏽

rickycodes force-pushed the feature/make-insufficient-fee-descriptive branch from 61b9fe2 to fe2b618 Compare December 16, 2020 19:59

rickycodes marked this pull request as ready for review December 17, 2020 16:29

rickycodes requested a review from a team as a code owner December 17, 2020 16:29

rickycodes added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Dec 17, 2020

rickycodes force-pushed the feature/make-insufficient-fee-descriptive branch from 9e62163 to 3c9e4d2 Compare December 17, 2020 17:48

rickycodes self-assigned this Dec 17, 2020

rickycodes marked this pull request as draft December 18, 2020 04:18

rickycodes marked this pull request as ready for review December 18, 2020 19:25

rickycodes force-pushed the feature/make-insufficient-fee-descriptive branch from 8519ffd to 0843d2b Compare December 18, 2020 19:26

rickycodes commented Dec 19, 2020

View reviewed changes

rickycodes commented Dec 21, 2020

View reviewed changes

omnat added the next release label Jan 5, 2021

ibrahimtaveras00 suggested changes Jan 5, 2021

View reviewed changes

ibrahimtaveras00 added QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Jan 5, 2021

estebanmino approved these changes Jan 5, 2021

View reviewed changes

estebanmino removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Jan 5, 2021

rickycodes force-pushed the feature/make-insufficient-fee-descriptive branch 2 times, most recently from 835ecf2 to e7dd03f Compare January 8, 2021 20:17

rickycodes added 8 commits January 8, 2021 17:17

Shift error location

577e3cb

Remove space

8f43946

Support over prop and get bold styles working properly

0b9b358

Get "Buy more ETH" link working

e423682

Add translation for "Buy more ETH" and only display message if over

fd4c662

Add conversionRate to test

5b32b32

Update snapshots

d6989f4

Only display details if the transaction hasn't gone over

7225744

rickycodes added 12 commits January 8, 2021 17:17

Get isMainNet and networkName functionality working

0e59ada

Update getNetworkName method

0eb031e

Add tests for networks

b5f849b

Make the unknown network id 9999 for good measure

Add format utility

1b7483e

Add Buy more ETH link to token approve

48f143a

Remove console.log

bff3a72

update snapshots

6697229

Disable buy more ETH when not on mainnet

d5c0690

Add isMainNet network utility

1257c79

Update .gitignore

9f2b1fd

Add some constants and some tests!

de735a8

constants++

05dcde4

rickycodes force-pushed the feature/make-insufficient-fee-descriptive branch from 511617a to 05dcde4 Compare January 8, 2021 22:18

rickycodes added 4 commits January 8, 2021 18:28

👋

a2420c9

Add buy eth link to Confirm

544f391

Update snapshot

7eec994

Get over styles working for totals

7a3813a

rickycodes added needs-qa Any New Features that needs a full manual QA prior to being added to a release. and removed QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed labels Jan 11, 2021

remove red text when you switch to account that holds enough eth

790b097

rickycodes force-pushed the feature/make-insufficient-fee-descriptive branch from 5e70c46 to 790b097 Compare January 11, 2021 21:16

rickycodes added 3 commits January 11, 2021 16:18

Reset other transactions

be77751

Fix error where str isn't defined

912151c

Undo

806b2e2

ibrahimtaveras00 approved these changes Jan 11, 2021

View reviewed changes

ibrahimtaveras00 added QA Passed A successful QA run through has been done and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Jan 11, 2021

rickycodes merged commit 7908f50 into develop Jan 11, 2021

rickycodes deleted the feature/make-insufficient-fee-descriptive branch January 11, 2021 22:30

rickycodes added a commit that referenced this pull request Jan 31, 2022

Feature/make insufficient fee descriptive (#2076)

655d881

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/make insufficient fee descriptive #2076

Feature/make insufficient fee descriptive #2076

rickycodes commented Dec 16, 2020 •

edited

Loading

rickycodes Dec 19, 2020 •

edited

Loading

rickycodes Dec 21, 2020 •

edited

Loading

estebanmino Jan 5, 2021

rickycodes Dec 21, 2020

ibrahimtaveras00 left a comment •

edited

Loading

rickycodes commented Jan 5, 2021

estebanmino left a comment

ibrahimtaveras00 left a comment

Feature/make insufficient fee descriptive #2076

Feature/make insufficient fee descriptive #2076

Conversation

rickycodes commented Dec 16, 2020 • edited Loading

rickycodes Dec 19, 2020 • edited Loading

Choose a reason for hiding this comment

rickycodes Dec 21, 2020 • edited Loading

Choose a reason for hiding this comment

estebanmino Jan 5, 2021

Choose a reason for hiding this comment

rickycodes Dec 21, 2020

Choose a reason for hiding this comment

ibrahimtaveras00 left a comment • edited Loading

Choose a reason for hiding this comment

rickycodes commented Jan 5, 2021

estebanmino left a comment

Choose a reason for hiding this comment

ibrahimtaveras00 left a comment

Choose a reason for hiding this comment

rickycodes commented Dec 16, 2020 •

edited

Loading

rickycodes Dec 19, 2020 •

edited

Loading

rickycodes Dec 21, 2020 •

edited

Loading

ibrahimtaveras00 left a comment •

edited

Loading