From 4f0b4eef5030575e8ebdf35ca19fbc77376c70b9 Mon Sep 17 00:00:00 2001 From: Dan Date: Tue, 22 May 2018 12:46:53 -0230 Subject: [PATCH] Estimate gas using same algorithm as backend. --- ui/app/actions.js | 19 ++- ui/app/components/send_/send.component.js | 3 + ui/app/components/send_/send.constants.js | 3 + ui/app/components/send_/send.container.js | 5 +- ui/app/components/send_/send.selectors.js | 5 + ui/app/components/send_/send.utils.js | 65 +++++---- .../send_/tests/send-component.test.js | 2 + .../send_/tests/send-container.test.js | 7 +- .../send_/tests/send-selectors-test-data.js | 1 + .../send_/tests/send-selectors.test.js | 10 ++ .../components/send_/tests/send-utils.test.js | 126 +++++++++--------- ui/app/conversion-util.js | 5 +- 12 files changed, 153 insertions(+), 98 deletions(-) diff --git a/ui/app/actions.js b/ui/app/actions.js index 70ec3aed8..7a18b1c00 100644 --- a/ui/app/actions.js +++ b/ui/app/actions.js @@ -4,7 +4,6 @@ const getBuyEthUrl = require('../../app/scripts/lib/buy-eth-url') const { getTokenAddressFromTokenObject } = require('./util') const { calcGasTotal, - getParamsForGasEstimate, calcTokenBalance, estimateGas, estimateGasPriceFromRecentBlocks, @@ -725,12 +724,24 @@ function setGasTotal (gasTotal) { } } -function updateGasData ({ recentBlocks, selectedAddress, selectedToken, data }) { +function updateGasData ({ + blockGasLimit, + data, + recentBlocks, + selectedAddress, + selectedToken, + to, +}) { return (dispatch) => { - const estimateGasParams = getParamsForGasEstimate(selectedAddress, selectedToken, data) return Promise.all([ Promise.resolve(estimateGasPriceFromRecentBlocks(recentBlocks)), - estimateGas(estimateGasParams), + estimateGas({ + blockGasLimit, + data, + selectedAddress, + selectedToken, + to, + }), ]) .then(([gasPrice, gas]) => { dispatch(actions.setGasPrice(gasPrice)) diff --git a/ui/app/components/send_/send.component.js b/ui/app/components/send_/send.component.js index 438923ab3..0f82d3f19 100644 --- a/ui/app/components/send_/send.component.js +++ b/ui/app/components/send_/send.component.js @@ -18,6 +18,7 @@ export default class SendTransactionScreen extends PersistentForm { PropTypes.string, PropTypes.number, ]), + blockGasLimit: PropTypes.string, conversionRate: PropTypes.number, data: PropTypes.string, editingTransactionId: PropTypes.string, @@ -40,6 +41,7 @@ export default class SendTransactionScreen extends PersistentForm { updateGas () { const { + blockGasLimit, data, editingTransactionId, gasLimit, @@ -51,6 +53,7 @@ export default class SendTransactionScreen extends PersistentForm { } = this.props updateAndSetGasTotal({ + blockGasLimit, data, editingTransactionId, gasLimit, diff --git a/ui/app/components/send_/send.constants.js b/ui/app/components/send_/send.constants.js index e911a5510..df5dee371 100644 --- a/ui/app/components/send_/send.constants.js +++ b/ui/app/components/send_/send.constants.js @@ -35,6 +35,8 @@ const ONE_GWEI_IN_WEI_HEX = ethUtil.addHexPrefix(conversionUtil('0x1', { toNumericBase: 'hex', })) +const SIMPLE_GAS_COST = '0x5208' // Hex for 21000, cost of a simple send. + module.exports = { INSUFFICIENT_FUNDS_ERROR, INSUFFICIENT_TOKENS_ERROR, @@ -48,5 +50,6 @@ module.exports = { NEGATIVE_ETH_ERROR, ONE_GWEI_IN_WEI_HEX, REQUIRED_ERROR, + SIMPLE_GAS_COST, TOKEN_TRANSFER_FUNCTION_SIGNATURE, } diff --git a/ui/app/components/send_/send.container.js b/ui/app/components/send_/send.container.js index 879aef87c..3b72a3a5a 100644 --- a/ui/app/components/send_/send.container.js +++ b/ui/app/components/send_/send.container.js @@ -4,6 +4,7 @@ import { withRouter } from 'react-router-dom' import { compose } from 'recompose' import { getAmountConversionRate, + getBlockGasLimit, getConversionRate, getCurrentNetwork, getGasLimit, @@ -45,6 +46,7 @@ function mapStateToProps (state) { return { amount: getSendAmount(state), amountConversionRate: getAmountConversionRate(state), + blockGasLimit: getBlockGasLimit(state), conversionRate: getConversionRate(state), data: generateTokenTransferData(selectedAddress, selectedToken), editingTransactionId: getSendEditingTransactionId(state), @@ -66,6 +68,7 @@ function mapStateToProps (state) { function mapDispatchToProps (dispatch) { return { updateAndSetGasTotal: ({ + blockGasLimit, data, editingTransactionId, gasLimit, @@ -75,7 +78,7 @@ function mapDispatchToProps (dispatch) { selectedToken, }) => { !editingTransactionId - ? dispatch(updateGasData({ recentBlocks, selectedAddress, selectedToken, data })) + ? dispatch(updateGasData({ recentBlocks, selectedAddress, selectedToken, data, blockGasLimit })) : dispatch(setGasTotal(calcGasTotal(gasLimit, gasPrice))) }, updateSendTokenBalance: ({ selectedToken, tokenContract, address }) => { diff --git a/ui/app/components/send_/send.selectors.js b/ui/app/components/send_/send.selectors.js index 850328e10..7e7cfe2e9 100644 --- a/ui/app/components/send_/send.selectors.js +++ b/ui/app/components/send_/send.selectors.js @@ -12,6 +12,7 @@ const selectors = { // autoAddToBetaUI, getAddressBook, getAmountConversionRate, + getBlockGasLimit, getConversionRate, getConvertedCurrency, getCurrentAccountWithSendEtherInfo, @@ -89,6 +90,10 @@ function getAmountConversionRate (state) { : getConversionRate(state) } +function getBlockGasLimit (state) { + return state.metamask.currentBlockGasLimit +} + function getConversionRate (state) { return state.metamask.conversionRate } diff --git a/ui/app/components/send_/send.utils.js b/ui/app/components/send_/send.utils.js index 6055c98b1..4c731c91b 100644 --- a/ui/app/components/send_/send.utils.js +++ b/ui/app/components/send_/send.utils.js @@ -13,18 +13,19 @@ const { INSUFFICIENT_TOKENS_ERROR, NEGATIVE_ETH_ERROR, ONE_GWEI_IN_WEI_HEX, + SIMPLE_GAS_COST, } = require('./send.constants') +const EthQuery = require('ethjs-query') const abi = require('ethereumjs-abi') module.exports = { calcGasTotal, + calcTokenBalance, doesAmountErrorRequireUpdate, estimateGas, estimateGasPriceFromRecentBlocks, generateTokenTransferData, getAmountErrorObject, - getParamsForGasEstimate, - calcTokenBalance, isBalanceSufficient, isTokenBalanceSufficient, } @@ -142,24 +143,6 @@ function getAmountErrorObject ({ return { amount: amountError } } -function getParamsForGasEstimate (selectedAddress, selectedToken, data) { - const { symbol } = selectedToken || {} - const estimatedGasParams = { - from: selectedAddress, - gas: '746a528800', - } - - if (symbol) { - Object.assign(estimatedGasParams, { value: '0x0' }) - } - - if (data) { - Object.assign(estimatedGasParams, { data }) - } - - return estimatedGasParams -} - function calcTokenBalance ({ selectedToken, usersToken }) { const { decimals } = selectedToken || {} return calcTokenAmount(usersToken.balance.toString(), decimals) + '' @@ -182,15 +165,40 @@ function doesAmountErrorRequireUpdate ({ return amountErrorRequiresUpdate } -function estimateGas (params = {}) { - return new Promise((resolve, reject) => { - global.ethQuery.estimateGas(params, (err, data) => { - if (err) { - return reject(err) - } - return resolve(data) - }) +async function estimateGas ({ selectedAddress, selectedToken, data, blockGasLimit, to }) { + const ethQuery = new EthQuery(global.ethereumProvider) + const { symbol } = selectedToken || {} + const estimatedGasParams = { from: selectedAddress } + + if (symbol) { + Object.assign(estimatedGasParams, { value: '0x0' }) + } + + if (data) { + Object.assign(estimatedGasParams, { data }) + } + // if recipient has no code, gas is 21k max: + const hasRecipient = Boolean(to) + let code + if (hasRecipient) code = await ethQuery.getCode(to) + + if (hasRecipient && (!code || code === '0x')) { + return SIMPLE_GAS_COST + } + + estimatedGasParams.to = to + + // if not, fall back to block gasLimit + estimatedGasParams.gas = multiplyCurrencies(blockGasLimit, 0.95, { + multiplicandBase: 16, + multiplierBase: 10, + roundDown: '0', + toNumericBase: 'hex', }) + + // run tx + const estimatedGas = await ethQuery.estimateGas(estimatedGasParams) + return estimatedGas.toString(16) } function generateTokenTransferData (selectedAddress, selectedToken) { @@ -222,5 +230,6 @@ function estimateGasPriceFromRecentBlocks (recentBlocks) { .sort(hexComparator)[0] }) .sort(hexComparator) + return lowestPrices[Math.floor(lowestPrices.length / 2)] } diff --git a/ui/app/components/send_/tests/send-component.test.js b/ui/app/components/send_/tests/send-component.test.js index 780ee1046..3abff0d23 100644 --- a/ui/app/components/send_/tests/send-component.test.js +++ b/ui/app/components/send_/tests/send-component.test.js @@ -32,6 +32,7 @@ describe.only('Send Component', function () { wrapper = shallow( () => arg2() }, './send.selectors': { getAmountConversionRate: (s) => `mockAmountConversionRate:${s}`, + getBlockGasLimit: (s) => `mockBlockGasLimit:${s}`, getConversionRate: (s) => `mockConversionRate:${s}`, getCurrentNetwork: (s) => `mockNetwork:${s}`, getGasLimit: (s) => `mockGasLimit:${s}`, @@ -58,6 +59,7 @@ describe('send container', () => { assert.deepEqual(mapStateToProps('mockState'), { amount: 'mockAmount:mockState', amountConversionRate: 'mockAmountConversionRate:mockState', + blockGasLimit: 'mockBlockGasLimit:mockState', conversionRate: 'mockConversionRate:mockState', data: 'mockData:mockSelectedAddress:mockStatemockSelectedToken:mockState', editingTransactionId: 'mockEditingTransactionId:mockState', @@ -89,6 +91,7 @@ describe('send container', () => { describe('updateAndSetGasTotal()', () => { const mockProps = { + blockGasLimit: 'mockBlockGasLimit', data: '0x1', editingTransactionId: '0x2', gasLimit: '0x3', @@ -108,14 +111,14 @@ describe('send container', () => { }) it('should dispatch an updateGasData action when editingTransactionId is falsy', () => { - const { selectedAddress, selectedToken, data, recentBlocks } = mockProps + const { selectedAddress, selectedToken, data, recentBlocks, blockGasLimit } = mockProps mapDispatchToPropsObject.updateAndSetGasTotal( Object.assign({}, mockProps, {editingTransactionId: false}) ) assert(dispatchSpy.calledOnce) assert.deepEqual( actionSpies.updateGasData.getCall(0).args[0], - { selectedAddress, selectedToken, data, recentBlocks } + { selectedAddress, selectedToken, data, recentBlocks, blockGasLimit } ) }) }) diff --git a/ui/app/components/send_/tests/send-selectors-test-data.js b/ui/app/components/send_/tests/send-selectors-test-data.js index a1423675d..8f9c19314 100644 --- a/ui/app/components/send_/tests/send-selectors-test-data.js +++ b/ui/app/components/send_/tests/send-selectors-test-data.js @@ -22,6 +22,7 @@ module.exports = { 'name': 'Send Account 4', }, }, + 'currentBlockGasLimit': '0x4c1878', 'currentCurrency': 'USD', 'conversionRate': 1200.88200327, 'conversionDate': 1489013762, diff --git a/ui/app/components/send_/tests/send-selectors.test.js b/ui/app/components/send_/tests/send-selectors.test.js index 22e45afdb..152af8059 100644 --- a/ui/app/components/send_/tests/send-selectors.test.js +++ b/ui/app/components/send_/tests/send-selectors.test.js @@ -5,6 +5,7 @@ const { accountsWithSendEtherInfoSelector, // autoAddToBetaUI, getAddressBook, + getBlockGasLimit, getAmountConversionRate, getConversionRate, getConvertedCurrency, @@ -135,6 +136,15 @@ describe('send selectors', () => { }) }) + describe('getBlockGasLimit', () => { + it('should return the current block gas limit', () => { + assert.deepEqual( + getBlockGasLimit(mockState), + '0x4c1878' + ) + }) + }) + describe('getConversionRate()', () => { it('should return the eth conversion rate', () => { assert.deepEqual( diff --git a/ui/app/components/send_/tests/send-utils.test.js b/ui/app/components/send_/tests/send-utils.test.js index b5211a63d..a01ab4eba 100644 --- a/ui/app/components/send_/tests/send-utils.test.js +++ b/ui/app/components/send_/tests/send-utils.test.js @@ -3,6 +3,7 @@ import sinon from 'sinon' import proxyquire from 'proxyquire' import { ONE_GWEI_IN_WEI_HEX, + SIMPLE_GAS_COST, } from '../send.constants' const { addCurrencies, @@ -18,11 +19,19 @@ const stubs = { addCurrencies: sinon.stub().callsFake((a, b, obj) => a + b), conversionUtil: sinon.stub().callsFake((val, obj) => parseInt(val, 16)), conversionGTE: sinon.stub().callsFake((obj1, obj2) => obj1.value > obj2.value), - multiplyCurrencies: sinon.stub().callsFake((a, b) => a * b), + multiplyCurrencies: sinon.stub().callsFake((a, b) => `${a}x${b}`), calcTokenAmount: sinon.stub().callsFake((a, d) => 'calc:' + a + d), rawEncode: sinon.stub().returns([16, 1100]), } +const EthQuery = function () {} +EthQuery.prototype.estimateGas = sinon.stub().callsFake( + (data) => Promise.resolve({ toString: (n) => `mockToString:${n}` }) +) +EthQuery.prototype.getCode = sinon.stub().callsFake( + (address) => Promise.resolve(address.match(/isContract/) ? 'not-0x' : '0x') +) + const sendUtils = proxyquire('../send.utils.js', { '../../conversion-util': { addCurrencies: stubs.addCurrencies, @@ -34,6 +43,7 @@ const sendUtils = proxyquire('../send.utils.js', { 'ethereumjs-abi': { rawEncode: stubs.rawEncode, }, + 'ethjs-query': EthQuery, }) const { @@ -43,7 +53,6 @@ const { estimateGasPriceFromRecentBlocks, generateTokenTransferData, getAmountErrorObject, - getParamsForGasEstimate, calcTokenBalance, isBalanceSufficient, isTokenBalanceSufficient, @@ -54,7 +63,7 @@ describe('send utils', () => { describe('calcGasTotal()', () => { it('should call multiplyCurrencies with the correct params and return the multiplyCurrencies return', () => { const result = calcGasTotal(12, 15) - assert.equal(result, 180) + assert.equal(result, '12x15') const call_ = stubs.multiplyCurrencies.getCall(0).args assert.deepEqual( call_, @@ -145,41 +154,6 @@ describe('send utils', () => { }) }) - describe('getParamsForGasEstimate()', () => { - it('should return from and gas properties if no symbol or data', () => { - assert.deepEqual( - getParamsForGasEstimate('mockAddress'), - { - from: 'mockAddress', - gas: '746a528800', - } - ) - }) - - it('should return value property if selected token provided', () => { - assert.deepEqual( - getParamsForGasEstimate('mockAddress', { symbol: 'ABC' }), - { - from: 'mockAddress', - gas: '746a528800', - value: '0x0', - } - ) - }) - - it('should return data property if data provided', () => { - assert.deepEqual( - getParamsForGasEstimate('mockAddress', { symbol: 'ABC' }, 'somedata'), - { - from: 'mockAddress', - gas: '746a528800', - value: '0x0', - data: 'somedata', - } - ) - }) - }) - describe('calcTokenBalance()', () => { it('should return the calculated token blance', () => { assert.equal(calcTokenBalance({ @@ -271,38 +245,66 @@ describe('send utils', () => { }) describe('estimateGas', () => { - let tempEthQuery - beforeEach(() => { - tempEthQuery = global.ethQuery - global.ethQuery = { - estimateGas: sinon.stub().callsFake((data, cb) => { - return cb( - data.isMockErr ? 'mockErr' : null, - Object.assign(data, { estimateGasCalled: true }) - ) - }) - } - }) + const baseMockParams = { + blockGasLimit: '0x64', + selectedAddress: 'mockAddress', + to: '0xisContract', + } + const baseExpectedCall = { + from: 'mockAddress', + gas: '0x64x0.95', + to: '0xisContract', + } afterEach(() => { - global.ethQuery = tempEthQuery + EthQuery.prototype.estimateGas.resetHistory() + EthQuery.prototype.getCode.resetHistory() + }) + + it('should call ethQuery.estimateGas with the expected params', async () => { + const result = await estimateGas(baseMockParams) + assert.equal(EthQuery.prototype.estimateGas.callCount, 1) + assert.deepEqual( + EthQuery.prototype.estimateGas.getCall(0).args[0], + baseExpectedCall + ) + assert.equal(result, 'mockToString:16') + }) + + it('should call ethQuery.estimateGas with a value of 0x0 if the passed selectedToken has a symbol', async () => { + const result = await estimateGas(Object.assign({ selectedToken: { symbol: true } }, baseMockParams)) + assert.equal(EthQuery.prototype.estimateGas.callCount, 1) + assert.deepEqual( + EthQuery.prototype.estimateGas.getCall(0).args[0], + Object.assign({ value: '0x0' }, baseExpectedCall) + ) + assert.equal(result, 'mockToString:16') + }) + + it('should call ethQuery.estimateGas with data if data is passed', async () => { + const result = await estimateGas(Object.assign({ data: 'mockData' }, baseMockParams)) + assert.equal(EthQuery.prototype.estimateGas.callCount, 1) + assert.deepEqual( + EthQuery.prototype.estimateGas.getCall(0).args[0], + Object.assign({ data: 'mockData' }, baseExpectedCall) + ) + assert.equal(result, 'mockToString:16') }) - it('should call ethQuery.estimateGas and resolve that call\'s data', async () => { - const result = await estimateGas({ mockParam: 'someData' }) - assert.equal(global.ethQuery.estimateGas.callCount, 1) + it('should call ethQuery.estimateGas with data if data is passed', async () => { + const result = await estimateGas(Object.assign({ data: 'mockData' }, baseMockParams)) + assert.equal(EthQuery.prototype.estimateGas.callCount, 1) assert.deepEqual( - result, - { mockParam: 'someData', estimateGasCalled: true } + EthQuery.prototype.estimateGas.getCall(0).args[0], + Object.assign({ data: 'mockData' }, baseExpectedCall) ) + assert.equal(result, 'mockToString:16') }) - it('should reject with ethQuery.estimateGas error', async () => { - try { - await estimateGas({ mockParam: 'someData', isMockErr: true }) - } catch (err) { - assert.equal(err, 'mockErr') - } + it(`should return ${SIMPLE_GAS_COST} if ethQuery.getCode does not return '0x'`, async () => { + assert.equal(EthQuery.prototype.estimateGas.callCount, 0) + const result = await estimateGas(Object.assign({}, baseMockParams, { to: '0x123' })) + assert.equal(result, SIMPLE_GAS_COST) }) }) diff --git a/ui/app/conversion-util.js b/ui/app/conversion-util.js index d484ed16d..100402d95 100644 --- a/ui/app/conversion-util.js +++ b/ui/app/conversion-util.js @@ -11,7 +11,8 @@ * @param {string} [options.fromNumericBase = 'hex' | 'dec' | 'BN'] The numeric basic of the passed value. * @param {string} [options.toNumericBase = 'hex' | 'dec' | 'BN'] The desired numeric basic of the result. * @param {string} [options.fromDenomination = 'WEI'] The denomination of the passed value -* @param {number} [options.numberOfDecimals] The desired number of in the result +* @param {string} [options.numberOfDecimals] The desired number of decimals in the result +* @param {string} [options.roundDown] The desired number of decimals to round down to * @param {number} [options.conversionRate] The rate to use to make the fromCurrency -> toCurrency conversion * @returns {(number | string | BN)} * @@ -38,6 +39,7 @@ const BIG_NUMBER_GWEI_MULTIPLIER = new BigNumber('1000000000') // Individual Setters const convert = R.invoker(1, 'times') const round = R.invoker(2, 'round')(R.__, BigNumber.ROUND_HALF_DOWN) +const roundDown = R.invoker(2, 'round')(R.__, BigNumber.ROUND_DOWN) const invertConversionRate = conversionRate => () => new BigNumber(1.0).div(conversionRate) const decToBigNumberViaString = n => R.pipe(String, toBigNumber['dec']) @@ -104,6 +106,7 @@ const converter = R.pipe( whenPredSetWithPropAndSetter(fromAndToCurrencyPropsNotEqual, 'conversionRate', convert), whenPropApplySetterMap('toDenomination', toSpecifiedDenomination), whenPredSetWithPropAndSetter(R.prop('numberOfDecimals'), 'numberOfDecimals', round), + whenPredSetWithPropAndSetter(R.prop('roundDown'), 'roundDown', roundDown), whenPropApplySetterMap('toNumericBase', baseChange), R.view(R.lensProp('value')) )