Dapp Transaction Confirmation Re-designs #1559

EtDu · 2020-05-12T07:03:46Z

Redesigns for Approval or dApp confirmation flow for transactions.

Checklist

Approval: change to Modal
Implement modal design
Ensure transactions work properly after redesign
Tie in Custom Gas

Issue

Resolves #1501
Resolves: #1499

EtDu · 2020-05-19T11:14:13Z

EtDu · 2020-05-22T11:18:25Z

@estebanmino please let me know if this implementation is okay. I haven't chopped any components, although it's possible that there is some lingering redundant code.

omnat · 2020-05-22T16:19:08Z

Confirm transactions

estebanmino · 2020-05-26T20:31:48Z

app/components/Nav/Main/index.js

@@ -1143,6 +1155,7 @@ class Main extends PureComponent {
 				{this.renderSigningModal()}
 				{this.renderWalletConnectSessionRequestModal()}
 				{this.renderWalletConnectReturnModal()}
+				{this.state.dappTransactionModalVisible && this.renderDappTransactionModal()}


isn't this hiding the component as https://github.com/MetaMask/metamask-mobile/pull/1559/files#diff-bf0cd540666d1b6f43322fe31ec18ffbR1132 ?

Yes, looks like it's doing the same thing

estebanmino · 2020-05-26T20:38:49Z

app/components/UI/CustomGas/index.js

+		const fromAccount = this.props.accounts[checksummedFrom];
+		if (fromAccount && isBN(gas) && isBN(gasPrice) && hexToBN(fromAccount.balance).lt(gas.mul(gasPrice)))
+			return strings('transaction.insufficient');
+		if (hexToBN(fromAccount.balance).lt(gas.mul(gasPrice).add(value))) return strings('transaction.insufficient');


I believe hexToBN(fromAccount.balance).lt(gas.mul(gasPrice).add(value)) includes hexToBN(fromAccount.balance).lt(gas.mul(gasPrice))) from this line right?
I mean if gas.mul(gasPrice).add(value) therefore gas.mul(gasPrice), we are comparing to times this, converting and comparing BN

Right, it's redundant

estebanmino · 2020-05-26T20:40:40Z

app/components/UI/CustomGas/index.js

+						style={styles.gasInput}
+						onChangeText={this.onGasLimitChange}
+						//useing BN here due to a glitch that causes it to sometimes render as x.00000001
+						value={customGasLimitBN.toString()}


I think I have many problems with converting BN to strings or number before, you didn't have any issue?

No problems at all

estebanmino · 2020-05-26T20:41:17Z

app/components/UI/CustomGas/index.js

+						keyboardType="numeric"
+						style={styles.gasInput}
+						onChangeText={this.onGasPriceChange}
+						value={customGasPrice.toString()}


same comment above, assuming is a BN

worked fine unless I've missed something

estebanmino · 2020-05-26T20:43:23Z

app/components/UI/CustomGas/index.js

+	renderGasError = () => {
+		const { warningGasLimit, warningGasPrice, warningSufficientFunds } = this.state;
+		const { gasError } = this.props;
+		const hideError = !warningGasLimit && !warningGasPrice && !warningSufficientFunds && !gasError;


isn't this equal to ! gasErrorMessage from the line below? you could use it directly here without crating a const

Indeed, fixed

estebanmino · 2020-05-26T20:45:14Z

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

 						<View style={styles.label}>
-							<Text style={styles.labelText}>{strings('transaction.review_function_type')}</Text>
-							<Text style={styles.functionType}>{actionKey}</Text>
+							<Text style={styles.labelText}>{strings('transaction.review_function')}: </Text>


I think you missed the : at the end

Seems to work fine

estebanmino

Overall looks great. Left some small comments and one related to BN numbers being handled on the render method, I'm almost sure I've has issues with that before, but not completely sure so I wanted to ask

… button press

…rror

…ISF), unlocks going back from custom gas modals if error is present

…tal value now passed down as view or text component

…lVisible to redux, use in Send to close dapp tx modal (if it's open) to fix send deeplink tx data getting wiped

* Nav/Main: dappTransactionModalVisible in state & toggleDappTransactionModal function, render Approve directly instead of using navigation * Approval: stop using navigation API, instead hide or show the modal. Render a modal with TransactionEditor & TransactionHeader, getUrlFromBrowser helper added for TransactionHeader * TransactionEditor: use react fragment in place of view, remove styles * Approval: move out TransactionHeader and root style * TransactionEditor: move in root styles, different for txEdit and txReview * TransactionReview: Use TransactionHeader, remove ActionView * TransactionReviewSummary: use WarningMessage component * TransactionReviewSummary: styling for function indicator and amount summary * TransactionReviewSummary: remove flex style, fix paddings * TransactionReview: use AccountInfoCard * Fix merge glitches * TransactionReview: take state and functionality from TransactionReviewSummary, pass props * TransactionReviewFeeCard: use padding instead of width percentage * TransactionReview: remove renderTransactionDetails, render TransactionReviewInformation directly * TransactionReviewSummary: fix proptypes * TransactionReviewInformation: remove overview style, fix styles for error, add assetAmount, fiatValue & primaryCurrency props, render TransactionReviewFeeCard * TransactionReviewInformation: add View Data button * TransactionReview: toggleDataView & dataVisible in state, render review info if dataVisible false, else render empty view for now * TransactionReview: change toggleDataView to arrow fn * Locales: add transaction.view_data * TransactionReviewInformation: fix touch area for view data button * TransactionReview: re-add ActionView, fix it's height based on screen size, fix error message * TransactionEditor: ensure dynamic modal height * Locales: add review_data, data, review_function * TransactionReviewData: implement design, use toggleDataView from props to go back * TransactionReview: use TransactionReviewData, pass toggleDataView to props * TransactionEdit: strip down, remove everything and render CustomGas with props * UI/CustomGas: fix styling + logic to mimic SendFlow/CustomGas, only handle selector gas price change upon clicking save, use renderGasError * CustomGas: fix loader * SendFlow/CustomGas: Add comment for fix in transaction-components * locales: add custom_gas.total * UI/CustomGas: use local error validation, only handle gas fee on save button press * snapshot/test updates * Approval: fix lint error * TransactionReviewSummary: change primary value based on primary currency * TransactionReview: pass props to TransactionReviewSummary * snapshots & tests * UI/CustomGas: fix custom gas selector button locking if initial NSF error * snapshot * TransactionEdit: can change mode to review even if gas error (namely ISF), unlocks going back from custom gas modals if error is present * snapshot gravy * Feedback updates * Design fixes * locales: change data description text * snapshot * TransactionReviewData: add Hex data title * locales: change hex data * snapshot update yo * CustomGas: remove redundant setstate calls * TransactionReview: tweak style of hex data and add bold text * TransactionReviewFeeCard: fix rendering problem with ERC20 tokens, total value now passed down as view or text component * snapshot updatessss * Send, Nav/Main: move toggleDappTransactionModal & dappTransactionModalVisible to redux, use in Send to close dapp tx modal (if it's open) to fix send deeplink tx data getting wiped * Fix stubborn tab/spacing lint error

EtDu changed the base branch from develop to transaction-components May 12, 2020 07:04

EtDu marked this pull request as draft May 12, 2020 13:11

EtDu force-pushed the transaction-components branch from 8a38295 to dd4ed7a Compare May 14, 2020 06:41

EtDu force-pushed the dapp-confirmation-designs branch from 12d3484 to 5443d20 Compare May 14, 2020 13:41

EtDu force-pushed the transaction-components branch from 90a451b to d48dc94 Compare May 18, 2020 09:53

EtDu force-pushed the dapp-confirmation-designs branch from a10507b to 14915b1 Compare May 18, 2020 10:03

EtDu marked this pull request as ready for review May 19, 2020 13:31

EtDu force-pushed the transaction-components branch from d48dc94 to e7c03ea Compare May 22, 2020 11:25

EtDu force-pushed the dapp-confirmation-designs branch from 892be43 to ec866d1 Compare May 22, 2020 11:34

omnat added next release and removed next release labels May 26, 2020

estebanmino reviewed May 26, 2020

View reviewed changes

EtDu force-pushed the transaction-components branch from 8d783bb to 18a47ff Compare May 28, 2020 11:00

EtDu force-pushed the dapp-confirmation-designs branch from ec866d1 to 2fdb65a Compare May 28, 2020 12:27

EtDu requested a review from a team as a code owner May 28, 2020 12:27

estebanmino added the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label May 28, 2020

estebanmino requested a review from ibrahimtaveras00 May 28, 2020 14:21

EtDu force-pushed the transaction-components branch from 1900922 to 7b77976 Compare June 2, 2020 08:52

EtDu force-pushed the dapp-confirmation-designs branch from f3a316d to ae7579c Compare June 2, 2020 09:01

Base automatically changed from transaction-components to develop June 9, 2020 05:25

EtDu force-pushed the dapp-confirmation-designs branch from ae7579c to f6d7eb1 Compare June 9, 2020 05:45

EtDu added 24 commits July 5, 2020 12:22

locales: add custom_gas.total

746bcc6

UI/CustomGas: use local error validation, only handle gas fee on save…

6abc54c

… button press

snapshot/test updates

fce26c1

Approval: fix lint error

6da42f7

TransactionReviewSummary: change primary value based on primary currency

6fc47bd

TransactionReview: pass props to TransactionReviewSummary

54813da

snapshots & tests

9716ff1

UI/CustomGas: fix custom gas selector button locking if initial NSF e…

1bdced0

…rror

snapshot

87b422a

TransactionEdit: can change mode to review even if gas error (namely …

8ade2c1

…ISF), unlocks going back from custom gas modals if error is present

snapshot gravy

fdbdd48

Feedback updates

f76d6d6

Design fixes

bb3e228

locales: change data description text

1b82428

snapshot

3a126d8

TransactionReviewData: add Hex data title

c5bf08b

locales: change hex data

1702023

snapshot update yo

2554214

CustomGas: remove redundant setstate calls

47887a3

TransactionReview: tweak style of hex data and add bold text

13dceb6

TransactionReviewFeeCard: fix rendering problem with ERC20 tokens, to…

f8b9663

…tal value now passed down as view or text component

snapshot updatessss

7e5f4b4

Send, Nav/Main: move toggleDappTransactionModal & dappTransactionModa…

e1c0f24

…lVisible to redux, use in Send to close dapp tx modal (if it's open) to fix send deeplink tx data getting wiped

Fix stubborn tab/spacing lint error

4f6d250

EtDu force-pushed the dapp-confirmation-designs branch 2 times, most recently from 4f6d250 to e1c0f24 Compare July 5, 2020 04:56

EtDu merged commit 0c7a192 into develop Jul 7, 2020

EtDu deleted the dapp-confirmation-designs branch July 7, 2020 03:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dapp Transaction Confirmation Re-designs #1559

Dapp Transaction Confirmation Re-designs #1559

EtDu commented May 12, 2020 •

edited

Loading

EtDu commented May 19, 2020 •

edited

Loading

EtDu commented May 22, 2020

omnat commented May 22, 2020

estebanmino May 26, 2020

EtDu May 28, 2020

estebanmino May 26, 2020

EtDu May 28, 2020

estebanmino May 26, 2020

EtDu May 28, 2020

estebanmino May 26, 2020

EtDu May 28, 2020

estebanmino May 26, 2020

EtDu May 28, 2020

estebanmino May 26, 2020

EtDu May 28, 2020

estebanmino left a comment

Dapp Transaction Confirmation Re-designs #1559

Dapp Transaction Confirmation Re-designs #1559

Conversation

EtDu commented May 12, 2020 • edited Loading

EtDu commented May 19, 2020 • edited Loading

EtDu commented May 22, 2020

omnat commented May 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estebanmino left a comment

Choose a reason for hiding this comment

EtDu commented May 12, 2020 •

edited

Loading

EtDu commented May 19, 2020 •

edited

Loading