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, _, next) => {
if (req.method === 'eth_estimateGas' && inTest) {
await delay(2000)
}
return next()
})
}
22 changes: 21 additions & 1 deletion development/states/send-edit.json
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
{
"confirmTransaction": {
"txData": {
"id": 4768706228115573
}
},
"metamask": {
"completedOnboarding": true,
"isInitialized": true,
Expand Down Expand Up @@ -66,7 +71,22 @@
],
"tokens": [],
"transactions": {},
"selectedAddressTxList": [],
"selectedAddressTxList": [{
"id": 4768706228115573,
"time": 1487363153561,
"status": "unapproved",
"gasMultiplier": 1,
"metamaskNetworkId": "3",
"txParams": {
"from": "0xc5b8dbac4c1d3f152cdeb400e2313f309c410acb",
"to": "0x2f8d4a878cfa04a6e60d46362f5644deab66572d",
"value": "0x1bc16d674ec80000",
"metamaskId": 4768706228115573,
"metamaskNetworkId": "3",
"gas": "0xea60",
"gasPrice": "0xba43b7400"
}
}],
"unapprovedTxs": {
"4768706228115573": {
"id": 4768706228115573,
Expand Down
26 changes: 19 additions & 7 deletions test/e2e/beta/metamask-beta-ui.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,7 @@ describe('MetaMask', function () {
it('confirms the transaction', async function () {
const confirmButton = await findElement(driver, By.xpath(`//button[contains(text(), 'Confirm')]`))
await confirmButton.click()
await delay(largeDelayMs)
await delay(largeDelayMs * 2)
})

it('finds the transaction in the transactions list', async function () {
Expand Down Expand Up @@ -428,6 +428,10 @@ describe('MetaMask', function () {
})

it('confirms the transaction', async function () {
const transactionAmounts = await findElements(driver, By.css('.currency-display-component__text'))
const transactionAmount = transactionAmounts[0]
assert.equal(await transactionAmount.getText(), '1')

const confirmButton = await findElement(driver, By.xpath(`//button[contains(text(), 'Confirm')]`))
await confirmButton.click()
await delay(largeDelayMs)
Expand Down Expand Up @@ -528,7 +532,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 @@ -685,11 +689,13 @@ describe('MetaMask', function () {
})

it('confirms a transaction', async () => {
await delay(tinyDelayMs)
const confirmButton = await findElement(driver, By.xpath(`//button[contains(text(), 'Confirm')]`), 10000)
await confirmButton.click()
await delay(regularDelayMs)
await delay(largeDelayMs * 2)

const navigationElement = await findElement(driver, By.css('.confirm-page-container-navigation'))
await delay(tinyDelayMs)
const navigationText = await navigationElement.getText()
assert.equal(navigationText.includes('4'), true, 'transaction confirmed')
})
Expand Down Expand Up @@ -792,7 +798,7 @@ describe('MetaMask', function () {
await driver.wait(until.elementTextMatches(contractStatus, /Deposit\sinitiated/), 10000)

await driver.switchTo().window(extension)
await delay(largeDelayMs)
await delay(largeDelayMs * 2)

await findElements(driver, By.css('.transaction-list-item'))
const [txListValue] = await findElements(driver, By.css('.transaction-list-item__amount--primary'))
Expand All @@ -812,6 +818,8 @@ describe('MetaMask', function () {
await delay(regularDelayMs)

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

Expand Down Expand Up @@ -870,7 +878,7 @@ describe('MetaMask', function () {
await delay(regularDelayMs)

await driver.switchTo().window(extension)
await delay(regularDelayMs)
await delay(largeDelayMs * 2)

const txListItem = await findElement(driver, By.css('.transaction-list-item'))
await txListItem.click()
Expand Down Expand Up @@ -1102,6 +1110,10 @@ describe('MetaMask', function () {
await txListValue.click()
await delay(regularDelayMs)

const transactionAmounts = await findElements(driver, By.css('.currency-display-component__text'))
const transactionAmount = transactionAmounts[0]
assert(await transactionAmount.getText(), '1.5 TST')

// Set the gas limit
const configureGas = await driver.wait(until.elementLocated(By.css('.confirm-detail-row__header-text--edit')), 10000)
await configureGas.click()
Expand Down Expand Up @@ -1340,10 +1352,10 @@ describe('MetaMask', function () {
})

it('submits the transaction', async function () {
await delay(regularDelayMs)
await delay(largeDelayMs * 2)
const confirmButton = await findElement(driver, By.xpath(`//button[contains(text(), 'Confirm')]`))
await confirmButton.click()
await delay(regularDelayMs)
await delay(largeDelayMs * 2)
})

it('finds the transaction in the transactions list', async function () {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,17 @@ 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.

}


static propTypes = {
updateCustomGasPrice: PropTypes.func,
updateCustomGasLimit: PropTypes.func,
Expand All @@ -20,15 +31,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,21 +110,21 @@ 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',
'advanced-gas-inputs__gas-edit-row__input--warning': isInError && errorType === 'warning',
})}>
<div
className="advanced-gas-inputs__gas-edit-row__input-arrows__i-wrap"
onClick={() => onChange(value + 1)}
onClick={() => onChange({ target: { value: value + 1 } })}
>
<i className="fa fa-sm fa-angle-up" />
</div>
<div
className="advanced-gas-inputs__gas-edit-row__input-arrows__i-wrap"
onClick={() => onChange(Math.max(value - 1, 0))}
onClick={() => onChange({ target: { value: Math.max(value - 1, 0) } })}
>
<i className="fa fa-sm fa-angle-down" />
</div>
Expand Down Expand Up @@ -120,9 +156,6 @@ export default class AdvancedTabContent extends Component {

render () {
const {
customGasPrice,
updateCustomGasPrice,
customGasLimit,
insufficientBalance,
customPriceIsSafe,
isSpeedUp,
Expand All @@ -134,8 +167,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 +177,7 @@ export default class AdvancedTabContent extends Component {
}) }
{ this.renderGasEditRow({
labelKey: 'gasLimit',
value: customGasLimit,
value: this.state.gasLimit,
onChange: this.onChangeGasLimit,
insufficientBalance,
customPriceIsSafe,
Expand Down
Loading