Improve contract method data fetching (#6623)

* Remove async call from getTransactionActionKey() * Stop blocking confirm screen rendering on method data loading, and base screen route on transactionCategory * Remove use of withMethodData, fix use of knownMethodData, in relation to transaction-list-item.component * Load data contract method data progressively, making it non-blocking; requires simplifying conf-tx-base lifecycle logic. * Allow editing of gas price while loading on the confirm screen. * Fix transactionAction component and its unit tests. * Fix confirm transaction components for cases of route transitions within metamask. * Only call toString on id if truthy in getNavigateTxData()
MetaMask · Jun 17, 2019 · 919b784 · 919b784
1 parent 96e4d87
commit 919b784
Show file tree

Hide file tree

Showing 17 changed files with 242 additions and 270 deletions.
diff --git a/ui/app/components/app/transaction-action/tests/transaction-action.component.test.js b/ui/app/components/app/transaction-action/tests/transaction-action.component.test.js
@@ -18,33 +18,6 @@ describe('TransactionAction Component', () => {
       }
     })
 
-    it('should render -- when methodData is still fetching', () => {
-      const methodData = { data: {}, done: false, error: null }
-      const transaction = {
-        id: 1,
-        status: 'confirmed',
-        submittedTime: 1534045442919,
-        time: 1534045440641,
-        txParams: {
-          from: '0xc5ae6383e126f901dcb06131d97a88745bfa88d6',
-          gas: '0x5208',
-          gasPrice: '0x3b9aca00',
-          nonce: '0x96',
-          to: '0x50a9d56c2b8ba9a5c7f2c08c3d26e0499f23a706',
-          value: '0x2386f26fc10000',
-        },
-      }
-
-      const wrapper = shallow(<TransactionAction
-        methodData={methodData}
-        transaction={transaction}
-        className="transaction-action"
-      />, { context: { t }})
-
-      assert.equal(wrapper.find('.transaction-action').length, 1)
-      assert.equal(wrapper.text(), '--')
-    })
-
     it('should render Sent Ether', () => {
       const methodData = { data: {}, done: true, error: null }
       const transaction = {
@@ -75,15 +48,7 @@ describe('TransactionAction Component', () => {
 
     it('should render Approved', async () => {
       const methodData = {
-        data: {
-          name: 'Approve',
-          params: [
-            { type: 'address' },
-            { type: 'uint256' },
-          ],
-        },
-        done: true,
-        error: null,
+        name: 'Approve',
       }
       const transaction = {
         id: 1,
@@ -99,6 +64,7 @@ describe('TransactionAction Component', () => {
           value: '0x2386f26fc10000',
           data: '0x095ea7b300000000000000000000000050a9d56c2b8ba9a5c7f2c08c3d26e0499f23a7060000000000000000000000000000000000000000000000000000000000000003',
         },
+        transactionCategory: 'contractInteraction',
       }
 
       const wrapper = shallow(
@@ -111,23 +77,12 @@ describe('TransactionAction Component', () => {
       )
 
       assert.ok(wrapper)
-      assert.equal(wrapper.find('.test-class').length, 1)
-      await wrapper.instance().getTransactionAction()
-      assert.equal(wrapper.state('transactionAction'), 'approve')
+      assert.equal(wrapper.find('.transaction-action').length, 1)
+      assert.equal(wrapper.find('.transaction-action').text().trim(), 'Approve')
     })
 
-    it('should render Accept Fulfillment', async () => {
-      const methodData = {
-        data: {
-          name: 'AcceptFulfillment',
-          params: [
-            { type: 'address' },
-            { type: 'uint256' },
-          ],
-        },
-        done: true,
-        error: null,
-      }
+    it('should render contractInteraction', async () => {
+      const methodData = {}
       const transaction = {
         id: 1,
         status: 'confirmed',
@@ -142,6 +97,7 @@ describe('TransactionAction Component', () => {
           value: '0x2386f26fc10000',
           data: '0x095ea7b300000000000000000000000050a9d56c2b8ba9a5c7f2c08c3d26e0499f23a7060000000000000000000000000000000000000000000000000000000000000003',
         },
+        transactionCategory: 'contractInteraction',
       }
 
       const wrapper = shallow(
@@ -154,9 +110,8 @@ describe('TransactionAction Component', () => {
       )
 
       assert.ok(wrapper)
-      assert.equal(wrapper.find('.test-class').length, 1)
-      await wrapper.instance().getTransactionAction()
-      assert.equal(wrapper.state('transactionAction'), ' Accept Fulfillment')
+      assert.equal(wrapper.find('.transaction-action').length, 1)
+      assert.equal(wrapper.find('.transaction-action').text().trim(), 'contractInteraction')
     })
   })
 })
diff --git a/ui/app/components/app/transaction-action/transaction-action.component.js b/ui/app/components/app/transaction-action/transaction-action.component.js
@@ -15,43 +15,23 @@ export default class TransactionAction extends PureComponent {
     methodData: PropTypes.object,
   }
 
-  state = {
-    transactionAction: '',
-  }
-
-  componentDidMount () {
-    this.getTransactionAction()
-  }
-
-  componentDidUpdate () {
-    this.getTransactionAction()
-  }
-
-  async getTransactionAction () {
-    const { transactionAction } = this.state
+  getTransactionAction () {
     const { transaction, methodData } = this.props
-    const { data, done } = methodData
-    const { name = '' } = data
-
-    if (!done || transactionAction) {
-      return
-    }
+    const { name } = methodData
 
-    const actionKey = await getTransactionActionKey(transaction, data)
-    const action = actionKey
-      ? this.context.t(actionKey)
-      : camelCaseToCapitalize(name)
+    const actionKey = getTransactionActionKey(transaction)
+    const action = actionKey && this.context.t(actionKey)
+    const methodName = name && camelCaseToCapitalize(name)
 
-    this.setState({ transactionAction: action })
+    return methodName || action || ''
   }
 
   render () {
-    const { className, methodData: { done } } = this.props
-    const { transactionAction } = this.state
+    const { className } = this.props
 
     return (
       <div className={classnames('transaction-action', className)}>
-        { (done && transactionAction) || '--' }
+        { this.getTransactionAction() }
       </div>
     )
   }

diff --git a/ui/app/components/app/transaction-list-item/transaction-list-item.component.js b/ui/app/components/app/transaction-list-item/transaction-list-item.component.js
@@ -34,6 +34,8 @@ export default class TransactionListItem extends PureComponent {
     fetchBasicGasAndTimeEstimates: PropTypes.func,
     fetchGasEstimates: PropTypes.func,
     rpcPrefs: PropTypes.object,
+    data: PropTypes.string,
+    getContractMethodData: PropTypes.func,
   }
 
   static defaultProps = {
@@ -150,6 +152,12 @@ export default class TransactionListItem extends PureComponent {
       )
   }
 
+  componentDidMount () {
+    if (this.props.data) {
+      this.props.getContractMethodData(this.props.data)
+    }
+  }
+
   render () {
     const {
       assetImages,
@@ -214,7 +222,7 @@ export default class TransactionListItem extends PureComponent {
                 <TransactionListItemDetails
                   transactionGroup={transactionGroup}
                   onRetry={this.handleRetry}
-                  showRetry={showRetry && methodData.done}
+                  showRetry={showRetry}
                   onCancel={this.handleCancel}
                   showCancel={showCancel}
                   cancelDisabled={!hasEnoughCancelGas}

diff --git a/ui/app/components/app/transaction-list-item/transaction-list-item.container.js b/ui/app/components/app/transaction-list-item/transaction-list-item.container.js
@@ -1,9 +1,8 @@
 import { connect } from 'react-redux'
 import { withRouter } from 'react-router-dom'
 import { compose } from 'recompose'
-import withMethodData from '../../../helpers/higher-order-components/with-method-data'
 import TransactionListItem from './transaction-list-item.component'
-import { setSelectedToken, showModal, showSidebar, addKnownMethodData } from '../../../store/actions'
+import { setSelectedToken, showModal, showSidebar, getContractMethodData } from '../../../store/actions'
 import { hexToDecimal } from '../../../helpers/utils/conversions.util'
 import { getTokenData } from '../../../helpers/utils/transactions.util'
 import { getHexGasTotal, increaseLastGasPrice } from '../../../helpers/utils/confirm-tx.util'
@@ -14,15 +13,15 @@ import {
   setCustomGasPriceForRetry,
   setCustomGasLimit,
 } from '../../../ducks/gas/gas.duck'
-import { getIsMainnet, preferencesSelector, getSelectedAddress, conversionRateSelector } from '../../../selectors/selectors'
+import { getIsMainnet, preferencesSelector, getSelectedAddress, conversionRateSelector, getKnownMethodData } from '../../../selectors/selectors'
 import { isBalanceSufficient } from '../../../pages/send/send.utils'
 
 const mapStateToProps = (state, ownProps) => {
-  const { metamask: { knownMethodData, accounts, provider, frequentRpcListDetail } } = state
+  const { metamask: { accounts, provider, frequentRpcListDetail } } = state
   const { showFiatInTestnets } = preferencesSelector(state)
   const isMainnet = getIsMainnet(state)
   const { transactionGroup: { primaryTransaction } = {} } = ownProps
-  const { txParams: { gas: gasLimit, gasPrice } = {} } = primaryTransaction
+  const { txParams: { gas: gasLimit, gasPrice, data } = {} } = primaryTransaction
   const selectedAccountBalance = accounts[getSelectedAddress(state)].balance
   const selectRpcInfo = frequentRpcListDetail.find(rpcInfo => rpcInfo.rpcUrl === provider.rpcTarget)
   const { rpcPrefs } = selectRpcInfo || {}
@@ -38,7 +37,7 @@ const mapStateToProps = (state, ownProps) => {
   })
 
   return {
-    knownMethodData,
+    methodData: getKnownMethodData(state, data) || {},
     showFiat: (isMainnet || !!showFiatInTestnets),
     selectedAccountBalance,
     hasEnoughCancelGas,
@@ -51,7 +50,7 @@ const mapDispatchToProps = dispatch => {
     fetchBasicGasAndTimeEstimates: () => dispatch(fetchBasicGasAndTimeEstimates()),
     fetchGasEstimates: (blockTime) => dispatch(fetchGasEstimates(blockTime)),
     setSelectedToken: tokenAddress => dispatch(setSelectedToken(tokenAddress)),
-    addKnownMethodData: (fourBytePrefix, methodData) => dispatch(addKnownMethodData(fourBytePrefix, methodData)),
+    getContractMethodData: methodData => dispatch(getContractMethodData(methodData)),
     retryTransaction: (transaction, gasPrice) => {
       dispatch(setCustomGasPriceForRetry(gasPrice || transaction.txParams.gasPrice))
       dispatch(setCustomGasLimit(transaction.txParams.gas))
@@ -97,5 +96,4 @@ const mergeProps = (stateProps, dispatchProps, ownProps) => {
 export default compose(
   withRouter,
   connect(mapStateToProps, mapDispatchToProps, mergeProps),
-  withMethodData,
 )(TransactionListItem)
diff --git a/ui/app/ducks/app/app.js b/ui/app/ducks/app/app.js
@@ -79,6 +79,7 @@ function reduceApp (state, action) {
     lastSelectedProvider: null,
     networksTabSelectedRpcUrl: '',
     networksTabIsInAddMode: false,
+    loadingMethodData: false,
   }, state.appState)
 
   switch (action.type) {
@@ -763,6 +764,17 @@ function reduceApp (state, action) {
         networksTabIsInAddMode: action.value,
       })
 
+    case actions.LOADING_METHOD_DATA_STARTED:
+      return extend(appState, {
+        loadingMethodData: true,
+      })
+
+    case actions.LOADING_METHOD_DATA_FINISHED:
+      return extend(appState, {
+        loadingMethodData: false,
+      })
+
+
     default:
       return appState
   }

diff --git a/ui/app/ducks/confirm-transaction/confirm-transaction.duck.js b/ui/app/ducks/confirm-transaction/confirm-transaction.duck.js
@@ -1,4 +1,3 @@
-import log from 'loglevel'
 import {
   conversionRateSelector,
   currentCurrencySelector,
@@ -18,12 +17,9 @@ import {
 
 import {
   getTokenData,
-  getMethodData,
-  isSmartContractAddress,
   sumHexes,
 } from '../../helpers/utils/transactions.util'
 
-import { getSymbolAndDecimals } from '../../helpers/utils/token-util'
 import { conversionUtil } from '../../helpers/utils/conversion-util'
 import { addHexPrefix } from 'ethereumjs-util'
 
@@ -348,7 +344,7 @@ export function updateTxDataAndCalculate (txData) {
 }
 
 export function setTransactionToConfirm (transactionId) {
-  return async (dispatch, getState) => {
+  return (dispatch, getState) => {
     const state = getState()
     const unconfirmedTransactionsHash = unconfirmedTransactionsHashSelector(state)
     const transaction = unconfirmedTransactionsHash[transactionId]
@@ -364,34 +360,14 @@ export function setTransactionToConfirm (transactionId) {
       dispatch(updateTxDataAndCalculate(txData))
 
       const { txParams } = transaction
-      const { to } = txParams
 
       if (txParams.data) {
-        const { tokens: existingTokens } = state
-        const { data, to: tokenAddress } = txParams
-
-        dispatch(setFetchingData(true))
-        const methodData = await getMethodData(data)
-        dispatch(updateMethodData(methodData))
+        const { data } = txParams
 
-        try {
-          const toSmartContract = await isSmartContractAddress(to || '')
-          dispatch(updateToSmartContract(toSmartContract))
-        } catch (error) {
-          log.error(error)
-        }
-        dispatch(setFetchingData(false))
 
         const tokenData = getTokenData(data)
         dispatch(updateTokenData(tokenData))
 
-        try {
-          const tokenSymbolData = await getSymbolAndDecimals(tokenAddress, existingTokens) || {}
-          const { symbol: tokenSymbol = '', decimals: tokenDecimals = '' } = tokenSymbolData
-          dispatch(updateTokenProps({ tokenSymbol, tokenDecimals }))
-        } catch (error) {
-          dispatch(updateTokenProps({ tokenSymbol: '', tokenDecimals: '' }))
-        }
       }
 
       if (txParams.nonce) {

diff --git a/ui/app/ducks/confirm-transaction/confirm-transaction.duck.test.js b/ui/app/ducks/confirm-transaction/confirm-transaction.duck.test.js
@@ -630,7 +630,7 @@ describe('Confirm Transaction Duck', () => {
       storeActions.forEach((action, index) => assert.equal(action.type, expectedActions[index]))
     })
 
-    it('updates confirmTransaction transaction', done => {
+    it('updates confirmTransaction transaction', () => {
       const mockState = {
         metamask: {
           conversionRate: 468.58,
@@ -673,13 +673,10 @@ describe('Confirm Transaction Duck', () => {
       ]
 
       store.dispatch(actions.setTransactionToConfirm(2603411941761054))
-        .then(() => {
-          const storeActions = store.getActions()
-          assert.equal(storeActions.length, expectedActions.length)
+      const storeActions = store.getActions()
+      assert.equal(storeActions.length, expectedActions.length)
 
-          storeActions.forEach((action, index) => assert.equal(action.type, expectedActions[index]))
-          done()
-        })
+      storeActions.forEach((action, index) => assert.equal(action.type, expectedActions[index]))
     })
   })
 })