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

Version 6.7.2 gas limit fix #6786

Merged
merged 10 commits into from
Jul 3, 2019
18 changes: 18 additions & 0 deletions app/scripts/controllers/network/createLocalhostClient.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,12 @@ const mergeMiddleware = require('json-rpc-engine/src/mergeMiddleware')
const createFetchMiddleware = require('eth-json-rpc-middleware/fetch')
const createBlockRefRewriteMiddleware = require('eth-json-rpc-middleware/block-ref-rewrite')
const createBlockTrackerInspectorMiddleware = require('eth-json-rpc-middleware/block-tracker-inspector')
const createAsyncMiddleware = require('json-rpc-engine/src/createAsyncMiddleware')
const providerFromMiddleware = require('eth-json-rpc-middleware/providerFromMiddleware')
const BlockTracker = require('eth-block-tracker')

const inTest = process.env.IN_TEST === 'true'

module.exports = createLocalhostClient

function createLocalhostClient () {
Expand All @@ -13,9 +16,24 @@ function createLocalhostClient () {
const blockTracker = new BlockTracker({ provider: blockProvider, pollingInterval: 1000 })

const networkMiddleware = mergeMiddleware([
createEstimateGasMiddleware(),
createBlockRefRewriteMiddleware({ blockTracker }),
createBlockTrackerInspectorMiddleware({ blockTracker }),
fetchMiddleware,
])
return { networkMiddleware, blockTracker }
}

function delay (time) {
return new Promise(resolve => setTimeout(resolve, time))
}


function createEstimateGasMiddleware () {
return createAsyncMiddleware(async (req, res, next) => {
if (req.method === 'eth_estimateGas' && inTest) {
await delay(2000)
}
return next()
})
}
3 changes: 2 additions & 1 deletion test/e2e/beta/metamask-beta-ui.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -528,7 +528,7 @@ describe('MetaMask', function () {
await delay(50)

await gasLimitInput.sendKeys('25000')
await delay(tinyDelayMs)
await delay(largeDelayMs * 2)

const confirmButton = await findElement(driver, By.xpath(`//button[contains(text(), 'Confirm')]`), 10000)
await confirmButton.click()
Expand Down Expand Up @@ -812,6 +812,7 @@ describe('MetaMask', function () {
await delay(regularDelayMs)

const [gasPriceInput, gasLimitInput] = await findElements(driver, By.css('.advanced-tab__gas-edit-row__input'))
assert(Number(gasLimitInput.getAttribute('value')) < 100000, 'Gas Limit too high')
await gasPriceInput.sendKeys(Key.chord(Key.CONTROL, 'a'))
await delay(50)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,21 @@ export default class AdvancedTabContent extends Component {
t: PropTypes.func,
}

constructor (props) {
super(props)
this.state = {
gasPrice: this.props.customGasPrice,
gasLimit: this.props.customGasLimit,
}
this.changeGasPrice = debounce(this.changeGasPrice, 500)
this.changeGasLimit = debounce(this.changeGasLimit, 500)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use the property syntax for the debounce? That is, wrap the property value in the debounce call directly. If we can remove theses lines we can move the state to a property as well and remove the constructor.

const tempSetState = this.setState.bind(this)
this.setState = (...args) => {
return tempSetState(...args)
}
}


static propTypes = {
updateCustomGasPrice: PropTypes.func,
updateCustomGasLimit: PropTypes.func,
Expand All @@ -20,15 +35,40 @@ export default class AdvancedTabContent extends Component {
showGasLimitInfoModal: PropTypes.func,
}

debouncedGasLimitReset = debounce((dVal) => {
if (dVal < 21000) {
componentDidUpdate (prevProps) {
const { customGasPrice: prevCustomGasPrice, customGasLimit: prevCustomGasLimit } = prevProps
const { customGasPrice, customGasLimit } = this.props
const { gasPrice, gasLimit } = this.state

if (customGasPrice !== prevCustomGasPrice && customGasPrice !== gasPrice) {
this.setState({ gasPrice: customGasPrice })
}
if (customGasLimit !== prevCustomGasLimit && customGasLimit !== gasLimit) {
this.setState({ gasLimit: customGasLimit })
}
}

onChangeGasLimit = (e) => {
this.setState({ gasLimit: e.target.value })
this.changeGasLimit({ target: { value: e.target.value } })
}

changeGasLimit = (e) => {
if (e.target.value < 21000) {
this.setState({ gasLimit: 21000 })
this.props.updateCustomGasLimit(21000)
} else {
this.props.updateCustomGasLimit(Number(e.target.value))
}
}, 1000, { trailing: true })
}

onChangeGasPrice = (e) => {
this.setState({ gasPrice: e.target.value })
Copy link
Member

Choose a reason for hiding this comment

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

Should e.target.value be converted to a Number here?

Edit: Likewise for onChangeGasLimit above

this.changeGasPrice({ target: { value: e.target.value } })
}

onChangeGasLimit = (val) => {
this.props.updateCustomGasLimit(val)
this.debouncedGasLimitReset(val)
changeGasPrice = (e) => {
this.props.updateCustomGasPrice(Number(e.target.value))
}

gasInputError ({ labelKey, insufficientBalance, customPriceIsSafe, isSpeedUp, value }) {
Expand Down Expand Up @@ -74,7 +114,7 @@ export default class AdvancedTabContent extends Component {
})}
type="number"
value={value}
onChange={event => onChange(Number(event.target.value))}
onChange={onChange}
/>
<div className={classnames('advanced-gas-inputs__gas-edit-row__input-arrows', {
'advanced-gas-inputs__gas-edit-row__input--error': isInError && errorType === 'error',
Expand Down Expand Up @@ -120,9 +160,6 @@ export default class AdvancedTabContent extends Component {

render () {
const {
customGasPrice,
updateCustomGasPrice,
customGasLimit,
insufficientBalance,
customPriceIsSafe,
isSpeedUp,
Expand All @@ -134,8 +171,8 @@ export default class AdvancedTabContent extends Component {
<div className="advanced-gas-inputs__gas-edit-rows">
{ this.renderGasEditRow({
labelKey: 'gasPrice',
value: customGasPrice,
onChange: updateCustomGasPrice,
value: this.state.gasPrice,
onChange: this.onChangeGasPrice,
insufficientBalance,
customPriceIsSafe,
showGWEI: true,
Expand All @@ -144,7 +181,7 @@ export default class AdvancedTabContent extends Component {
}) }
{ this.renderGasEditRow({
labelKey: 'gasLimit',
value: customGasLimit,
value: this.state.gasLimit,
onChange: this.onChangeGasLimit,
insufficientBalance,
customPriceIsSafe,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
hideSidebar,
updateSendAmount,
setGasTotal,
updateTransaction,
} from '../../../../store/actions'
import {
setCustomGasPrice,
Expand All @@ -22,9 +23,6 @@ import {
hideGasButtonGroup,
updateSendErrors,
} from '../../../../ducks/send/send.duck'
import {
updateGasAndCalculate,
} from '../../../../ducks/confirm-transaction/confirm-transaction.duck'
import {
conversionRateSelector as getConversionRate,
getCurrentCurrency,
Expand Down Expand Up @@ -118,6 +116,9 @@ const mapStateToProps = (state, ownProps) => {
conversionRate,
})

const { modalState: { props: modalProps } = {} } = state.appState.modal || {}
const { txData } = modalProps || {}

return {
hideBasic,
isConfirm: isConfirm(state),
Expand Down Expand Up @@ -151,6 +152,7 @@ const mapStateToProps = (state, ownProps) => {
transactionFee: addHexWEIsToRenderableEth('0x0', customGasTotal),
sendAmount,
},
transaction: txData || transaction,
isSpeedUp: transaction.status === 'submitted',
txId: transaction.id,
insufficientBalance,
Expand Down Expand Up @@ -179,10 +181,10 @@ const mapDispatchToProps = dispatch => {
dispatch(setGasLimit(newLimit))
dispatch(setGasPrice(newPrice))
},
updateConfirmTxGasAndCalculate: (gasLimit, gasPrice) => {
updateConfirmTxGasAndCalculate: (gasLimit, gasPrice, updatedTx) => {
updateCustomGasPrice(gasPrice)
dispatch(setCustomGasLimit(addHexPrefix(gasLimit.toString(16))))
return dispatch(updateGasAndCalculate({ gasLimit, gasPrice }))
return dispatch(updateTransaction(updatedTx))
},
createSpeedUpTransaction: (txId, gasPrice) => {
return dispatch(createSpeedUpTransaction(txId, gasPrice))
Expand Down Expand Up @@ -214,6 +216,7 @@ const mergeProps = (stateProps, dispatchProps, ownProps) => {
selectedToken,
tokenBalance,
customGasLimit,
transaction,
} = stateProps
const {
updateCustomGasPrice: dispatchUpdateCustomGasPrice,
Expand All @@ -234,7 +237,15 @@ const mergeProps = (stateProps, dispatchProps, ownProps) => {
...ownProps,
onSubmit: (gasLimit, gasPrice) => {
if (isConfirm) {
dispatchUpdateConfirmTxGasAndCalculate(gasLimit, gasPrice)
const updatedTx = {
...transaction,
txParams: {
...transaction.txParams,
gas: gasLimit,
gasPrice,
},
}
dispatchUpdateConfirmTxGasAndCalculate(gasLimit, gasPrice, updatedTx)
dispatchHideModal()
} else if (isSpeedUp) {
dispatchCreateSpeedUpTransaction(txId, gasPrice)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,9 @@ describe('gas-modal-page-container container', () => {
maxModeOn: false,
selectedToken: null,
tokenBalance: '0x0',
transaction: {
id: 34,
},
}
const baseMockOwnProps = { transaction: { id: 34 } }
const tests = [
Expand All @@ -168,7 +171,7 @@ describe('gas-modal-page-container container', () => {
mockOwnProps: Object.assign({}, baseMockOwnProps, {
transaction: { id: 34, status: 'submitted' },
}),
expectedResult: Object.assign({}, baseExpectedResult, { isSpeedUp: true }),
expectedResult: Object.assign({}, baseExpectedResult, { isSpeedUp: true, transaction: { id: 34, status: 'submitted' } }),
},
{
mockState: Object.assign({}, baseMockState, {
Expand Down Expand Up @@ -317,8 +320,10 @@ describe('gas-modal-page-container container', () => {
it('should dispatch a updateGasAndCalculate action with the correct props', () => {
mapDispatchToPropsObject.updateConfirmTxGasAndCalculate('ffff', 'aaaa')
assert.equal(dispatchSpy.callCount, 3)
assert(confirmTransactionActionSpies.updateGasAndCalculate.calledOnce)
assert.deepEqual(confirmTransactionActionSpies.updateGasAndCalculate.getCall(0).args[0], { gasLimit: 'ffff', gasPrice: 'aaaa' })
assert(actionSpies.setGasPrice.calledOnce)
assert(actionSpies.setGasLimit.calledOnce)
assert.equal(actionSpies.setGasLimit.getCall(0).args[0], 'ffff')
assert.equal(actionSpies.setGasPrice.getCall(0).args[0], 'aaaa')
})
})

Expand All @@ -337,6 +342,7 @@ describe('gas-modal-page-container container', () => {
},
isConfirm: true,
someOtherStateProp: 'baz',
transaction: {},
}
dispatchProps = {
updateCustomGasPrice: sinon.spy(),
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
import { connect } from 'react-redux'
import { compose } from 'recompose'
import { withRouter } from 'react-router-dom'
import ConfirmTokenTransactionBase from './confirm-token-transaction-base.component'
import {
contractExchangeRateSelector,
transactionFeeSelector,
} from '../../selectors/confirm-transaction'
import { tokenSelector } from '../../selectors/tokens'
import {
Expand All @@ -14,15 +17,21 @@ import {
} from '../../helpers/utils/token-util'


const mapStateToProps = (state) => {
const { confirmTransaction, metamask: { currentCurrency, conversionRate } } = state
const mapStateToProps = (state, ownProps) => {
const { match: { params = {} } } = ownProps
const { id: paramsTransactionId } = params
const { confirmTransaction, metamask: { currentCurrency, conversionRate, selectedAddressTxList } } = state

const {
txData: { txParams: { to: tokenAddress, data } = {} } = {},
fiatTransactionTotal,
ethTransactionTotal,
txData: { id: transactionId, txParams: { to: tokenAddress, data } = {} } = {},
} = confirmTransaction


const transaction = selectedAddressTxList.find(({ id }) => id === (Number(paramsTransactionId) || transactionId))
console.log('!!! * transaction', transaction)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: console log

const {
ethTransactionTotal,
fiatTransactionTotal,
} = transactionFeeSelector(state, transaction)
const tokens = tokenSelector(state)
const currentToken = tokens && tokens.find(({ address }) => tokenAddress === address)
const { decimals, symbol: tokenSymbol } = currentToken || {}
Expand All @@ -46,4 +55,7 @@ const mapStateToProps = (state) => {
}
}

export default connect(mapStateToProps)(ConfirmTokenTransactionBase)
export default compose(
withRouter,
connect(mapStateToProps)
)(ConfirmTokenTransactionBase)
Loading