Skip to content

Commit cfa916a

Browse files
danjmdanjm
authored
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()
1 parent a3475ee commit cfa916a

17 files changed

+242
-270
lines changed

‎ui/app/components/app/transaction-action/tests/transaction-action.component.test.js

+9-54
Original file line numberDiff line numberDiff line change
@@ -18,33 +18,6 @@ describe('TransactionAction Component', () => {
1818
}
1919
})
2020

21-
it('should render -- when methodData is still fetching', () => {
22-
const methodData = { data: {}, done: false, error: null }
23-
const transaction = {
24-
id: 1,
25-
status: 'confirmed',
26-
submittedTime: 1534045442919,
27-
time: 1534045440641,
28-
txParams: {
29-
from: '0xc5ae6383e126f901dcb06131d97a88745bfa88d6',
30-
gas: '0x5208',
31-
gasPrice: '0x3b9aca00',
32-
nonce: '0x96',
33-
to: '0x50a9d56c2b8ba9a5c7f2c08c3d26e0499f23a706',
34-
value: '0x2386f26fc10000',
35-
},
36-
}
37-
38-
const wrapper = shallow(<TransactionAction
39-
methodData={methodData}
40-
transaction={transaction}
41-
className="transaction-action"
42-
/>, { context: { t }})
43-
44-
assert.equal(wrapper.find('.transaction-action').length, 1)
45-
assert.equal(wrapper.text(), '--')
46-
})
47-
4821
it('should render Sent Ether', () => {
4922
const methodData = { data: {}, done: true, error: null }
5023
const transaction = {
@@ -75,15 +48,7 @@ describe('TransactionAction Component', () => {
7548

7649
it('should render Approved', async () => {
7750
const methodData = {
78-
data: {
79-
name: 'Approve',
80-
params: [
81-
{ type: 'address' },
82-
{ type: 'uint256' },
83-
],
84-
},
85-
done: true,
86-
error: null,
51+
name: 'Approve',
8752
}
8853
const transaction = {
8954
id: 1,
@@ -99,6 +64,7 @@ describe('TransactionAction Component', () => {
9964
value: '0x2386f26fc10000',
10065
data: '0x095ea7b300000000000000000000000050a9d56c2b8ba9a5c7f2c08c3d26e0499f23a7060000000000000000000000000000000000000000000000000000000000000003',
10166
},
67+
transactionCategory: 'contractInteraction',
10268
}
10369

10470
const wrapper = shallow(
@@ -111,23 +77,12 @@ describe('TransactionAction Component', () => {
11177
)
11278

11379
assert.ok(wrapper)
114-
assert.equal(wrapper.find('.test-class').length, 1)
115-
await wrapper.instance().getTransactionAction()
116-
assert.equal(wrapper.state('transactionAction'), 'approve')
80+
assert.equal(wrapper.find('.transaction-action').length, 1)
81+
assert.equal(wrapper.find('.transaction-action').text().trim(), 'Approve')
11782
})
11883

119-
it('should render Accept Fulfillment', async () => {
120-
const methodData = {
121-
data: {
122-
name: 'AcceptFulfillment',
123-
params: [
124-
{ type: 'address' },
125-
{ type: 'uint256' },
126-
],
127-
},
128-
done: true,
129-
error: null,
130-
}
84+
it('should render contractInteraction', async () => {
85+
const methodData = {}
13186
const transaction = {
13287
id: 1,
13388
status: 'confirmed',
@@ -142,6 +97,7 @@ describe('TransactionAction Component', () => {
14297
value: '0x2386f26fc10000',
14398
data: '0x095ea7b300000000000000000000000050a9d56c2b8ba9a5c7f2c08c3d26e0499f23a7060000000000000000000000000000000000000000000000000000000000000003',
14499
},
100+
transactionCategory: 'contractInteraction',
145101
}
146102

147103
const wrapper = shallow(
@@ -154,9 +110,8 @@ describe('TransactionAction Component', () => {
154110
)
155111

156112
assert.ok(wrapper)
157-
assert.equal(wrapper.find('.test-class').length, 1)
158-
await wrapper.instance().getTransactionAction()
159-
assert.equal(wrapper.state('transactionAction'), ' Accept Fulfillment')
113+
assert.equal(wrapper.find('.transaction-action').length, 1)
114+
assert.equal(wrapper.find('.transaction-action').text().trim(), 'contractInteraction')
160115
})
161116
})
162117
})

‎ui/app/components/app/transaction-action/transaction-action.component.js

+8-28
Original file line numberDiff line numberDiff line change
@@ -15,43 +15,23 @@ export default class TransactionAction extends PureComponent {
1515
methodData: PropTypes.object,
1616
}
1717

18-
state = {
19-
transactionAction: '',
20-
}
21-
22-
componentDidMount () {
23-
this.getTransactionAction()
24-
}
25-
26-
componentDidUpdate () {
27-
this.getTransactionAction()
28-
}
29-
30-
async getTransactionAction () {
31-
const { transactionAction } = this.state
18+
getTransactionAction () {
3219
const { transaction, methodData } = this.props
33-
const { data, done } = methodData
34-
const { name = '' } = data
35-
36-
if (!done || transactionAction) {
37-
return
38-
}
20+
const { name } = methodData
3921

40-
const actionKey = await getTransactionActionKey(transaction, data)
41-
const action = actionKey
42-
? this.context.t(actionKey)
43-
: camelCaseToCapitalize(name)
22+
const actionKey = getTransactionActionKey(transaction)
23+
const action = actionKey && this.context.t(actionKey)
24+
const methodName = name && camelCaseToCapitalize(name)
4425

45-
this.setState({ transactionAction: action })
26+
return methodName || action || ''
4627
}
4728

4829
render () {
49-
const { className, methodData: { done } } = this.props
50-
const { transactionAction } = this.state
30+
const { className } = this.props
5131

5232
return (
5333
<div className={classnames('transaction-action', className)}>
54-
{ (done && transactionAction) || '--' }
34+
{ this.getTransactionAction() }
5535
</div>
5636
)
5737
}

‎ui/app/components/app/transaction-list-item/transaction-list-item.component.js

+9-1
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,8 @@ export default class TransactionListItem extends PureComponent {
3434
fetchBasicGasAndTimeEstimates: PropTypes.func,
3535
fetchGasEstimates: PropTypes.func,
3636
rpcPrefs: PropTypes.object,
37+
data: PropTypes.string,
38+
getContractMethodData: PropTypes.func,
3739
}
3840

3941
static defaultProps = {
@@ -150,6 +152,12 @@ export default class TransactionListItem extends PureComponent {
150152
)
151153
}
152154

155+
componentDidMount () {
156+
if (this.props.data) {
157+
this.props.getContractMethodData(this.props.data)
158+
}
159+
}
160+
153161
render () {
154162
const {
155163
assetImages,
@@ -214,7 +222,7 @@ export default class TransactionListItem extends PureComponent {
214222
<TransactionListItemDetails
215223
transactionGroup={transactionGroup}
216224
onRetry={this.handleRetry}
217-
showRetry={showRetry && methodData.done}
225+
showRetry={showRetry}
218226
onCancel={this.handleCancel}
219227
showCancel={showCancel}
220228
cancelDisabled={!hasEnoughCancelGas}

‎ui/app/components/app/transaction-list-item/transaction-list-item.container.js

+6-8
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,8 @@
11
import { connect } from 'react-redux'
22
import { withRouter } from 'react-router-dom'
33
import { compose } from 'recompose'
4-
import withMethodData from '../../../helpers/higher-order-components/with-method-data'
54
import TransactionListItem from './transaction-list-item.component'
6-
import { setSelectedToken, showModal, showSidebar, addKnownMethodData } from '../../../store/actions'
5+
import { setSelectedToken, showModal, showSidebar, getContractMethodData } from '../../../store/actions'
76
import { hexToDecimal } from '../../../helpers/utils/conversions.util'
87
import { getTokenData } from '../../../helpers/utils/transactions.util'
98
import { getHexGasTotal, increaseLastGasPrice } from '../../../helpers/utils/confirm-tx.util'
@@ -14,15 +13,15 @@ import {
1413
setCustomGasPriceForRetry,
1514
setCustomGasLimit,
1615
} from '../../../ducks/gas/gas.duck'
17-
import { getIsMainnet, preferencesSelector, getSelectedAddress, conversionRateSelector } from '../../../selectors/selectors'
16+
import { getIsMainnet, preferencesSelector, getSelectedAddress, conversionRateSelector, getKnownMethodData } from '../../../selectors/selectors'
1817
import { isBalanceSufficient } from '../../../pages/send/send.utils'
1918

2019
const mapStateToProps = (state, ownProps) => {
21-
const { metamask: { knownMethodData, accounts, provider, frequentRpcListDetail } } = state
20+
const { metamask: { accounts, provider, frequentRpcListDetail } } = state
2221
const { showFiatInTestnets } = preferencesSelector(state)
2322
const isMainnet = getIsMainnet(state)
2423
const { transactionGroup: { primaryTransaction } = {} } = ownProps
25-
const { txParams: { gas: gasLimit, gasPrice } = {} } = primaryTransaction
24+
const { txParams: { gas: gasLimit, gasPrice, data } = {} } = primaryTransaction
2625
const selectedAccountBalance = accounts[getSelectedAddress(state)].balance
2726
const selectRpcInfo = frequentRpcListDetail.find(rpcInfo => rpcInfo.rpcUrl === provider.rpcTarget)
2827
const { rpcPrefs } = selectRpcInfo || {}
@@ -38,7 +37,7 @@ const mapStateToProps = (state, ownProps) => {
3837
})
3938

4039
return {
41-
knownMethodData,
40+
methodData: getKnownMethodData(state, data) || {},
4241
showFiat: (isMainnet || !!showFiatInTestnets),
4342
selectedAccountBalance,
4443
hasEnoughCancelGas,
@@ -51,7 +50,7 @@ const mapDispatchToProps = dispatch => {
5150
fetchBasicGasAndTimeEstimates: () => dispatch(fetchBasicGasAndTimeEstimates()),
5251
fetchGasEstimates: (blockTime) => dispatch(fetchGasEstimates(blockTime)),
5352
setSelectedToken: tokenAddress => dispatch(setSelectedToken(tokenAddress)),
54-
addKnownMethodData: (fourBytePrefix, methodData) => dispatch(addKnownMethodData(fourBytePrefix, methodData)),
53+
getContractMethodData: methodData => dispatch(getContractMethodData(methodData)),
5554
retryTransaction: (transaction, gasPrice) => {
5655
dispatch(setCustomGasPriceForRetry(gasPrice || transaction.txParams.gasPrice))
5756
dispatch(setCustomGasLimit(transaction.txParams.gas))
@@ -97,5 +96,4 @@ const mergeProps = (stateProps, dispatchProps, ownProps) => {
9796
export default compose(
9897
withRouter,
9998
connect(mapStateToProps, mapDispatchToProps, mergeProps),
100-
withMethodData,
10199
)(TransactionListItem)

‎ui/app/ducks/app/app.js

+12
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,7 @@ function reduceApp (state, action) {
7979
lastSelectedProvider: null,
8080
networksTabSelectedRpcUrl: '',
8181
networksTabIsInAddMode: false,
82+
loadingMethodData: false,
8283
}, state.appState)
8384

8485
switch (action.type) {
@@ -763,6 +764,17 @@ function reduceApp (state, action) {
763764
networksTabIsInAddMode: action.value,
764765
})
765766

767+
case actions.LOADING_METHOD_DATA_STARTED:
768+
return extend(appState, {
769+
loadingMethodData: true,
770+
})
771+
772+
case actions.LOADING_METHOD_DATA_FINISHED:
773+
return extend(appState, {
774+
loadingMethodData: false,
775+
})
776+
777+
766778
default:
767779
return appState
768780
}

‎ui/app/ducks/confirm-transaction/confirm-transaction.duck.js

+2-26
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,3 @@
1-
import log from 'loglevel'
21
import {
32
conversionRateSelector,
43
currentCurrencySelector,
@@ -18,12 +17,9 @@ import {
1817

1918
import {
2019
getTokenData,
21-
getMethodData,
22-
isSmartContractAddress,
2320
sumHexes,
2421
} from '../../helpers/utils/transactions.util'
2522

26-
import { getSymbolAndDecimals } from '../../helpers/utils/token-util'
2723
import { conversionUtil } from '../../helpers/utils/conversion-util'
2824
import { addHexPrefix } from 'ethereumjs-util'
2925

@@ -348,7 +344,7 @@ export function updateTxDataAndCalculate (txData) {
348344
}
349345

350346
export function setTransactionToConfirm (transactionId) {
351-
return async (dispatch, getState) => {
347+
return (dispatch, getState) => {
352348
const state = getState()
353349
const unconfirmedTransactionsHash = unconfirmedTransactionsHashSelector(state)
354350
const transaction = unconfirmedTransactionsHash[transactionId]
@@ -364,34 +360,14 @@ export function setTransactionToConfirm (transactionId) {
364360
dispatch(updateTxDataAndCalculate(txData))
365361

366362
const { txParams } = transaction
367-
const { to } = txParams
368363

369364
if (txParams.data) {
370-
const { tokens: existingTokens } = state
371-
const { data, to: tokenAddress } = txParams
372-
373-
dispatch(setFetchingData(true))
374-
const methodData = await getMethodData(data)
375-
dispatch(updateMethodData(methodData))
365+
const { data } = txParams
376366

377-
try {
378-
const toSmartContract = await isSmartContractAddress(to || '')
379-
dispatch(updateToSmartContract(toSmartContract))
380-
} catch (error) {
381-
log.error(error)
382-
}
383-
dispatch(setFetchingData(false))
384367

385368
const tokenData = getTokenData(data)
386369
dispatch(updateTokenData(tokenData))
387370

388-
try {
389-
const tokenSymbolData = await getSymbolAndDecimals(tokenAddress, existingTokens) || {}
390-
const { symbol: tokenSymbol = '', decimals: tokenDecimals = '' } = tokenSymbolData
391-
dispatch(updateTokenProps({ tokenSymbol, tokenDecimals }))
392-
} catch (error) {
393-
dispatch(updateTokenProps({ tokenSymbol: '', tokenDecimals: '' }))
394-
}
395371
}
396372

397373
if (txParams.nonce) {

‎ui/app/ducks/confirm-transaction/confirm-transaction.duck.test.js

+4-7
Original file line numberDiff line numberDiff line change
@@ -630,7 +630,7 @@ describe('Confirm Transaction Duck', () => {
630630
storeActions.forEach((action, index) => assert.equal(action.type, expectedActions[index]))
631631
})
632632

633-
it('updates confirmTransaction transaction', done => {
633+
it('updates confirmTransaction transaction', () => {
634634
const mockState = {
635635
metamask: {
636636
conversionRate: 468.58,
@@ -673,13 +673,10 @@ describe('Confirm Transaction Duck', () => {
673673
]
674674

675675
store.dispatch(actions.setTransactionToConfirm(2603411941761054))
676-
.then(() => {
677-
const storeActions = store.getActions()
678-
assert.equal(storeActions.length, expectedActions.length)
676+
const storeActions = store.getActions()
677+
assert.equal(storeActions.length, expectedActions.length)
679678

680-
storeActions.forEach((action, index) => assert.equal(action.type, expectedActions[index]))
681-
done()
682-
})
679+
storeActions.forEach((action, index) => assert.equal(action.type, expectedActions[index]))
683680
})
684681
})
685682
})

0 commit comments

Comments
 (0)