Skip to content

Commit

Permalink
Version 6.7.2 gas limit fix (#6786)
Browse files Browse the repository at this point in the history 
* Introduce delay for eth_estimateGas calls with in test

* Add test that fails when gas estimates of contract method calls without gas are too high.

* Get transaction gas data from unApprovedTxs instead of confirmTransaction

* Fix selection of gas data in gas-modal-page-container.container

* Lint changes related to Version-6.7.2-gasLimitFix

* Fix e2e tests on Version-6.7.2-gasLimitFix

* Fix unit and integration tests for changes from Version-6.7.2-gasLimitFix

* more e2e fixes

* Add assertions for transaction values on confirm screen

* Fix display of transaction amount on confirm screen.
  • Loading branch information
@danjm @danfinlay
danjm authored and danfinlay committed Jul 3, 2019
1 parent e768ed9 commit 632c9b2
Show file tree
Hide file tree
Showing 9 changed files with 273 additions and 85 deletions.
18 changes: 18 additions & 0 deletions app/scripts/controllers/network/createLocalhostClient.js
View file
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
View file
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
View file
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
63 changes: 48 additions & 15 deletions ui/app/components/app/gas-customization/advanced-gas-inputs/advanced-gas-inputs.component.js
View file
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)
}


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 })
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

0 comments on commit 632c9b2

Please sign in to comment.