From 6e78494846c9032fbf1264a0225c0df4df0867cb Mon Sep 17 00:00:00 2001 From: Frances Pangilinan Date: Fri, 16 Dec 2016 10:33:36 -0800 Subject: First pass at revision requests --- app/scripts/background.js | 14 +- app/scripts/keyring-controller.js | 101 +---------- app/scripts/lib/config-manager.js | 3 - app/scripts/lib/idStore.js | 261 ----------------------------- app/scripts/lib/provider-utils.js | 106 ------------ app/scripts/lib/tx-utils.js | 87 ++++++++++ app/scripts/metamask-controller.js | 38 +++-- app/scripts/transaction-manager.js | 247 ++++++++++++++++----------- library/controller.js | 8 +- mock-dev.js | 2 +- test/unit/idStore-test.js | 50 ------ test/unit/metamask-controller-test.js | 2 +- test/unit/tx-manager-test.js | 73 +++++--- ui/app/components/transaction-list-item.js | 4 +- 14 files changed, 320 insertions(+), 676 deletions(-) delete mode 100644 app/scripts/lib/provider-utils.js create mode 100644 app/scripts/lib/tx-utils.js diff --git a/app/scripts/background.js b/app/scripts/background.js index f476e89e4..a7e525999 100644 --- a/app/scripts/background.js +++ b/app/scripts/background.js @@ -17,7 +17,7 @@ const controller = new MetamaskController({ // User confirmation callbacks: showUnconfirmedMessage: triggerUi, unlockAccountMessage: triggerUi, - showUnconfirmedTx: triggerUi, + showUnapprovedTx: triggerUi, // Persistence Methods: setData, loadData, @@ -101,7 +101,7 @@ txManager.on('update', updateBadge) function updateBadge () { var label = '' - var unconfTxLen = controller.txManager.unConftxCount + var unconfTxLen = controller.txManager.unconfTxCount var unconfMsgs = messageManager.unconfirmedMsgs() var unconfMsgLen = Object.keys(unconfMsgs).length var count = unconfTxLen + unconfMsgLen @@ -112,16 +112,6 @@ function updateBadge () { extension.browserAction.setBadgeBackgroundColor({ color: '#506F8B' }) } -// txManger :: tx approvals and rejection cb's - -txManager.on('signed', function (txId) { - this.execOnTxDoneCb(txId, true) -}) - -txManager.on('rejected', function (txId) { - this.execOnTxDoneCb(txId, false) -}) - // data :: setters/getters function loadData () { diff --git a/app/scripts/keyring-controller.js b/app/scripts/keyring-controller.js index 37b3a947f..64c2ac933 100644 --- a/app/scripts/keyring-controller.js +++ b/app/scripts/keyring-controller.js @@ -100,8 +100,6 @@ module.exports = class KeyringController extends EventEmitter { isInitialized: (!!wallet || !!vault), isUnlocked: Boolean(this.password), isDisclaimerConfirmed: this.configManager.getConfirmedDisclaimer(), // AUDIT this.configManager.getConfirmedDisclaimer(), - transactions: this.txManager.getTxList(), - unconfTxs: this.txManager.getUnapprovedTxList(), unconfMsgs: messageManager.unconfirmedMsgs(), messages: messageManager.getMsgList(), selectedAccount: address, @@ -319,89 +317,10 @@ module.exports = class KeyringController extends EventEmitter { } - // SIGNING RELATED METHODS + // SIGNING METHODS // - // SIGN, SUBMIT TX, CANCEL, AND APPROVE. - // THIS SECTION INVOLVES THE REQUEST, STORING, AND SIGNING OF DATA - // WITH THE KEYS STORED IN THIS CONTROLLER. - - - // Add Unconfirmed Transaction - // @object txParams - // @function onTxDoneCb - // @function cb - // - // Calls back `cb` with @object txData = { txParams } - // Calls back `onTxDoneCb` with `true` or an `error` depending on result. - // - // Prepares the given `txParams` for final confirmation and approval. - // Estimates gas and other preparatory steps. - // Caches the requesting Dapp's callback, `onTxDoneCb`, for resolution later. - addUnconfirmedTransaction (txParams, onTxDoneCb, cb) { - const configManager = this.configManager - const txManager = this.txManager - // create txData obj with parameters and meta data - var time = (new Date()).getTime() - var txId = createId() - txParams.metamaskId = txId - txParams.metamaskNetworkId = this.getNetwork() - var txData = { - id: txId, - txParams: txParams, - time: time, - status: 'unapproved', - gasMultiplier: configManager.getGasMultiplier() || 1, - metamaskNetworkId: this.getNetwork(), - } - // keep the onTxDoneCb around for after approval/denial (requires user interaction) - // This onTxDoneCb fires completion to the Dapp's write operation. - txManager.txProviderUtils.analyzeGasUsage(txData, this.txDidComplete.bind(this, txData, onTxDoneCb, cb)) - // calculate metadata for tx - } - - txDidComplete (txData, onTxDoneCb, cb, err) { - if (err) return cb(err) - const txManager = this.txManager - txManager.addTx(txData, onTxDoneCb) - // signal update - this.emit('update') - // signal completion of add tx - cb(null, txData) - } - - // Cancel Transaction - // @string txId - // @function cb - // - // Calls back `cb` with no error if provided. - // - // Forgets any tx matching `txId`. - cancelTransaction (txId, cb) { - const txManager = this.txManager - txManager.setTxStatusRejected(txId) - - if (cb && typeof cb === 'function') { - cb() - } - } - - // Approve Transaction - // @string txId - // @function cb - // - // Calls back `cb` with no error always. - // - // Attempts to sign a Transaction with `txId` - // and submit it to the blockchain. - // - // Calls back the cached Dapp's confirmation callback, also. - approveTransaction (txId, cb) { - const txManager = this.txManager - txManager.setTxStatusSigned(txId) - this.emit('update') - cb() - } - + // This method signs tx and returns a promise for + // TX Manager to update the state after signing signTransaction (txParams, cb) { try { const address = normalize(txParams.from) @@ -420,20 +339,10 @@ module.exports = class KeyringController extends EventEmitter { txParams.data = normalize(txParams.data) txParams.gasLimit = normalize(txParams.gasLimit || txParams.gas) txParams.nonce = normalize(txParams.nonce) - const tx = new Transaction(txParams) return keyring.signTransaction(address, tx) - }) - .then((tx) => { - // Add the tx hash to the persisted meta-tx object - var txHash = ethUtil.bufferToHex(tx.hash()) - var metaTx = this.txManager.getTx(txParams.metamaskId) - metaTx.hash = txHash - this.txManager.updateTx(metaTx) - - // return raw serialized tx - var rawTx = ethUtil.bufferToHex(tx.serialize()) - cb(null, rawTx) + }).then((tx) => { + return {tx, txParams, cb} }) } catch (e) { cb(e) diff --git a/app/scripts/lib/config-manager.js b/app/scripts/lib/config-manager.js index 913a76a6e..a32842cc7 100644 --- a/app/scripts/lib/config-manager.js +++ b/app/scripts/lib/config-manager.js @@ -8,7 +8,6 @@ const normalize = require('./sig-util').normalize const TESTNET_RPC = MetamaskConfig.network.testnet const MAINNET_RPC = MetamaskConfig.network.mainnet const MORDEN_RPC = MetamaskConfig.network.morden -const txLimit = 40 /* The config-manager is a convenience object * wrapping a pojo-migrator. @@ -19,8 +18,6 @@ const txLimit = 40 */ module.exports = ConfigManager function ConfigManager (opts) { - this.txLimit = txLimit - // ConfigManager is observable and will emit updates this._subs = [] diff --git a/app/scripts/lib/idStore.js b/app/scripts/lib/idStore.js index 71bee8026..5c0f8d7f7 100644 --- a/app/scripts/lib/idStore.js +++ b/app/scripts/lib/idStore.js @@ -1,19 +1,12 @@ const EventEmitter = require('events').EventEmitter const inherits = require('util').inherits -const async = require('async') const ethUtil = require('ethereumjs-util') -const BN = ethUtil.BN -const EthQuery = require('eth-query') const KeyStore = require('eth-lightwallet').keystore const clone = require('clone') const extend = require('xtend') -const createId = require('./random-id') -const ethBinToOps = require('eth-bin-to-ops') const autoFaucet = require('./auto-faucet') -const messageManager = require('./message-manager') const DEFAULT_RPC = 'https://testrpc.metamask.io/' const IdManagement = require('./id-management') -const TxManager = require('../transaction-manager') module.exports = IdentityStore @@ -36,15 +29,7 @@ function IdentityStore (opts = {}) { selectedAddress: null, identities: {}, } - // not part of serilized metamask state - only kept in memory - this.txManager = new TxManager({ - TxListFromStore: opts.configManager.getTxList(), - setTxList: opts.configManager.setTxList.bind(opts.configManager), - txLimit: 40, - }) - this._unconfTxCbs = {} - this._unconfMsgCbs = {} } // @@ -94,7 +79,6 @@ IdentityStore.prototype.recoverFromSeed = function (password, seed, cb) { IdentityStore.prototype.setStore = function (store) { this._ethStore = store - this.txManager.setProvider(this._ethStore._query.currentProvider) } IdentityStore.prototype.clearSeedWordCache = function (cb) { @@ -105,17 +89,12 @@ IdentityStore.prototype.clearSeedWordCache = function (cb) { IdentityStore.prototype.getState = function () { const configManager = this.configManager - const TxManager = this.txManager var seedWords = this.getSeedIfUnlocked() return clone(extend(this._currentState, { isInitialized: !!configManager.getWallet() && !seedWords, isUnlocked: this._isUnlocked(), seedWords: seedWords, isDisclaimerConfirmed: configManager.getConfirmedDisclaimer(), - unconfTxs: TxManager.getUnapprovedTxList(), - transactions: TxManager.getTxList(), - unconfMsgs: messageManager.unconfirmedMsgs(), - messages: messageManager.getMsgList(), selectedAddress: configManager.getSelectedAccount(), shapeShiftTxList: configManager.getShapeShiftTxList(), currentFiat: configManager.getCurrentFiat(), @@ -214,245 +193,6 @@ IdentityStore.prototype.exportAccount = function (address, cb) { cb(null, privateKey) } -// -// Transactions -// - -// comes from dapp via zero-client hooked-wallet provider -IdentityStore.prototype.addUnconfirmedTransaction = function (txParams, onTxDoneCb, cb) { - const configManager = this.configManager - - var self = this - // create txData obj with parameters and meta data - var time = (new Date()).getTime() - var txId = createId() - txParams.metamaskId = txId - txParams.metamaskNetworkId = self._currentState.network - var txData = { - id: txId, - txParams: txParams, - time: time, - status: 'unconfirmed', - gasMultiplier: configManager.getGasMultiplier() || 1, - } - - console.log('addUnconfirmedTransaction:', txData) - - // keep the onTxDoneCb around for after approval/denial (requires user interaction) - // This onTxDoneCb fires completion to the Dapp's write operation. - self._unconfTxCbs[txId] = onTxDoneCb - - var provider = self._ethStore._query.currentProvider - var query = new EthQuery(provider) - - // calculate metadata for tx - async.parallel([ - analyzeForDelegateCall, - estimateGas, - ], didComplete) - - // perform static analyis on the target contract code - function analyzeForDelegateCall (cb) { - if (txParams.to) { - query.getCode(txParams.to, (err, result) => { - if (err) return cb(err.message || err) - var containsDelegateCall = self.checkForDelegateCall(result) - txData.containsDelegateCall = containsDelegateCall - cb() - }) - } else { - cb() - } - } - - function estimateGas (cb) { - var estimationParams = extend(txParams) - query.getBlockByNumber('latest', true, function(err, block){ - if (err) return cb(err) - // check if gasLimit is already specified - const gasLimitSpecified = Boolean(txParams.gas) - // if not, fallback to block gasLimit - if (!gasLimitSpecified) { - estimationParams.gas = block.gasLimit - } - // run tx, see if it will OOG - query.estimateGas(estimationParams, function(err, estimatedGasHex){ - if (err) return cb(err.message || err) - // all gas used - must be an error - if (estimatedGasHex === estimationParams.gas) { - txData.simulationFails = true - txData.estimatedGas = estimatedGasHex - txData.txParams.gas = estimatedGasHex - cb() - return - } - // otherwise, did not use all gas, must be ok - - // if specified gasLimit and no error, we're done - if (gasLimitSpecified) { - txData.estimatedGas = txParams.gas - cb() - return - } - - // try adding an additional gas buffer to our estimation for safety - const estimatedGasBn = new BN(ethUtil.stripHexPrefix(estimatedGasHex), 16) - const blockGasLimitBn = new BN(ethUtil.stripHexPrefix(block.gasLimit), 16) - const estimationWithBuffer = self.addGasBuffer(estimatedGasBn) - // added gas buffer is too high - if (estimationWithBuffer.gt(blockGasLimitBn)) { - txData.estimatedGas = estimatedGasHex - txData.txParams.gas = estimatedGasHex - // added gas buffer is safe - } else { - const gasWithBufferHex = ethUtil.intToHex(estimationWithBuffer) - txData.estimatedGas = gasWithBufferHex - txData.txParams.gas = gasWithBufferHex - } - cb() - return - }) - }) - } - - function didComplete (err) { - if (err) return cb(err.message || err) - configManager.addTx(txData) - // signal update - self._didUpdate() - // signal completion of add tx - cb(null, txData) - } -} - -IdentityStore.prototype.checkForDelegateCall = function (codeHex) { - const code = ethUtil.toBuffer(codeHex) - if (code !== '0x') { - const ops = ethBinToOps(code) - const containsDelegateCall = ops.some((op) => op.name === 'DELEGATECALL') - return containsDelegateCall - } else { - return false - } -} - -IdentityStore.prototype.addGasBuffer = function (gasBn) { - // add 20% to specified gas - const gasBuffer = gasBn.div(new BN('5', 10)) - const gasWithBuffer = gasBn.add(gasBuffer) - return gasWithBuffer -} - -// comes from metamask ui -IdentityStore.prototype.approveTransaction = function (txId, cb) { - const configManager = this.configManager - var approvalCb = this._unconfTxCbs[txId] || noop - - // accept tx - cb() - approvalCb(null, true) - // clean up - configManager.confirmTx(txId) - delete this._unconfTxCbs[txId] - this._didUpdate() -} - -// comes from metamask ui -IdentityStore.prototype.cancelTransaction = function (txId) { - const configManager = this.configManager - var approvalCb = this._unconfTxCbs[txId] || noop - - // reject tx - approvalCb(null, false) - // clean up - configManager.rejectTx(txId) - delete this._unconfTxCbs[txId] - this._didUpdate() -} - -// performs the actual signing, no autofill of params -IdentityStore.prototype.signTransaction = function (txParams, cb) { - try { - console.log('signing tx...', txParams) - var rawTx = this._idmgmt.signTx(txParams) - cb(null, rawTx) - } catch (err) { - cb(err) - } -} - -// -// Messages -// - -// comes from dapp via zero-client hooked-wallet provider -IdentityStore.prototype.addUnconfirmedMessage = function (msgParams, cb) { - // create txData obj with parameters and meta data - var time = (new Date()).getTime() - var msgId = createId() - var msgData = { - id: msgId, - msgParams: msgParams, - time: time, - status: 'unconfirmed', - } - messageManager.addMsg(msgData) - console.log('addUnconfirmedMessage:', msgData) - - // keep the cb around for after approval (requires user interaction) - // This cb fires completion to the Dapp's write operation. - this._unconfMsgCbs[msgId] = cb - - // signal update - this._didUpdate() - - return msgId -} - -// comes from metamask ui -IdentityStore.prototype.approveMessage = function (msgId, cb) { - var approvalCb = this._unconfMsgCbs[msgId] || noop - - // accept msg - cb() - approvalCb(null, true) - // clean up - messageManager.confirmMsg(msgId) - delete this._unconfMsgCbs[msgId] - this._didUpdate() -} - -// comes from metamask ui -IdentityStore.prototype.cancelMessage = function (msgId) { - var approvalCb = this._unconfMsgCbs[msgId] || noop - - // reject tx - approvalCb(null, false) - // clean up - messageManager.rejectMsg(msgId) - delete this._unconfTxCbs[msgId] - this._didUpdate() -} - -// performs the actual signing, no autofill of params -IdentityStore.prototype.signMessage = function (msgParams, cb) { - try { - console.log('signing msg...', msgParams.data) - var rawMsg = this._idmgmt.signMsg(msgParams.from, msgParams.data) - if ('metamaskId' in msgParams) { - var id = msgParams.metamaskId - delete msgParams.metamaskId - - this.approveMessage(id, cb) - } else { - cb(null, rawMsg) - } - } catch (err) { - cb(err) - } -} - -// // private // @@ -607,4 +347,3 @@ IdentityStore.prototype._autoFaucet = function () { // util -function noop () {} diff --git a/app/scripts/lib/provider-utils.js b/app/scripts/lib/provider-utils.js deleted file mode 100644 index d1678c964..000000000 --- a/app/scripts/lib/provider-utils.js +++ /dev/null @@ -1,106 +0,0 @@ -const async = require('async') -const EthQuery = require('eth-query') -const ethUtil = require('ethereumjs-util') -const BN = ethUtil.BN -const ethBinToOps = require('eth-bin-to-ops') - -module.exports = class txProviderUtils { - constructor (provider) { - this.provider = provider - this.query = new EthQuery(provider) - } - analyzeGasUsage (txData, cb) { - var self = this - this.query.getBlockByNumber('latest', true, (err, block) => { - if (err) return cb(err) - async.waterfall([ - self.estimateTxGas.bind(self, txData, block.gasLimit), - self.checkForTxGasError.bind(self, txData), - self.setTxGas.bind(self, txData, block.gasLimit), - ], cb) - }) - } - - // perform static analyis on the target contract code - analyzeForDelegateCall (txParams, cb) { - if (txParams.to) { - this.query.getCode(txParams.to, function (err, result) { - if (err) return cb(err) - - var code = ethUtil.toBuffer(result) - if (code !== '0x') { - var ops = ethBinToOps(code) - var containsDelegateCall = ops.some((op) => op.name === 'DELEGATECALL') - cb(containsDelegateCall) - } else { - cb() - } - }) - } else { - cb() - } - } - - estimateTxGas (txData, blockGasLimitHex, cb) { - const txParams = txData.txParams - // check if gasLimit is already specified - txData.gasLimitSpecified = Boolean(txParams.gas) - // if not, fallback to block gasLimit - if (!txData.gasLimitSpecified) { - txParams.gas = blockGasLimitHex - } - // run tx, see if it will OOG - this.query.estimateGas(txParams, cb) - } - - checkForTxGasError (txData, estimatedGasHex, cb) { - txData.estimatedGas = estimatedGasHex - // all gas used - must be an error - if (estimatedGasHex === txData.txParams.gas) { - txData.simulationFails = true - } - cb() - } - - handleFork (block) { - - } - - setTxGas (txData, blockGasLimitHex, cb) { - const txParams = txData.txParams - // if OOG, nothing more to do - if (txData.simulationFails) { - cb() - return - } - // if gasLimit was specified and doesnt OOG, - // use original specified amount - if (txData.gasLimitSpecified) { - txData.estimatedGas = txParams.gas - cb() - return - } - // if gasLimit not originally specified, - // try adding an additional gas buffer to our estimation for safety - const estimatedGasBn = new BN(ethUtil.stripHexPrefix(txData.estimatedGas), 16) - const blockGasLimitBn = new BN(ethUtil.stripHexPrefix(blockGasLimitHex), 16) - const estimationWithBuffer = new BN(this.addGasBuffer(estimatedGasBn), 16) - // added gas buffer is too high - if (estimationWithBuffer.gt(blockGasLimitBn)) { - txParams.gas = txData.estimatedGas - // added gas buffer is safe - } else { - const gasWithBufferHex = ethUtil.intToHex(estimationWithBuffer) - txParams.gas = gasWithBufferHex - } - cb() - return - } - - addGasBuffer (gas) { - const gasBuffer = new BN('100000', 10) - const bnGas = new BN(ethUtil.stripHexPrefix(gas), 16) - const correct = bnGas.add(gasBuffer) - return ethUtil.addHexPrefix(correct.toString(16)) - } -} diff --git a/app/scripts/lib/tx-utils.js b/app/scripts/lib/tx-utils.js new file mode 100644 index 000000000..a976173f5 --- /dev/null +++ b/app/scripts/lib/tx-utils.js @@ -0,0 +1,87 @@ +const async = require('async') +const EthQuery = require('eth-query') +const ethUtil = require('ethereumjs-util') +const BN = ethUtil.BN + +/* +tx-utils are utility methods for Transaction manager +its passed a provider and that is passed to ethquery +and used to do things like calculate gas of a tx. +*/ + +module.exports = class txProviderUtils { + constructor (provider) { + this.provider = provider + this.query = new EthQuery(provider) + } + analyzeGasUsage (txData, cb) { + var self = this + this.query.getBlockByNumber('latest', true, (err, block) => { + if (err) return cb(err) + async.waterfall([ + self.estimateTxGas.bind(self, txData, block.gasLimit), + self.checkForTxGasError.bind(self, txData), + self.setTxGas.bind(self, txData, block.gasLimit), + ], cb) + }) + } + + estimateTxGas (txData, blockGasLimitHex, cb) { + const txParams = txData.txParams + // check if gasLimit is already specified + txData.gasLimitSpecified = Boolean(txParams.gas) + // if not, fallback to block gasLimit + if (!txData.gasLimitSpecified) { + txParams.gas = blockGasLimitHex + } + // run tx, see if it will OOG + this.query.estimateGas(txParams, cb) + } + + checkForTxGasError (txData, estimatedGasHex, cb) { + txData.estimatedGas = estimatedGasHex + // all gas used - must be an error + if (estimatedGasHex === txData.txParams.gas) { + txData.simulationFails = true + } + cb() + } + + setTxGas (txData, blockGasLimitHex, cb) { + const txParams = txData.txParams + // if OOG, nothing more to do + if (txData.simulationFails) { + cb() + return + } + // if gasLimit was specified and doesnt OOG, + // use original specified amount + if (txData.gasLimitSpecified) { + txData.estimatedGas = txParams.gas + cb() + return + } + // if gasLimit not originally specified, + // try adding an additional gas buffer to our estimation for safety + const estimatedGasBn = new BN(ethUtil.stripHexPrefix(txData.estimatedGas), 16) + const blockGasLimitBn = new BN(ethUtil.stripHexPrefix(blockGasLimitHex), 16) + const estimationWithBuffer = new BN(this.addGasBuffer(estimatedGasBn), 16) + // added gas buffer is too high + if (estimationWithBuffer.gt(blockGasLimitBn)) { + txParams.gas = txData.estimatedGas + // added gas buffer is safe + } else { + const gasWithBufferHex = ethUtil.intToHex(estimationWithBuffer) + txParams.gas = gasWithBufferHex + } + cb() + return + } + + addGasBuffer (gas) { + const gasBuffer = new BN('100000', 10) + const bnGas = new BN(ethUtil.stripHexPrefix(gas), 16) + const correct = bnGas.add(gasBuffer) + return ethUtil.addHexPrefix(correct.toString(16)) + } +} diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 3b70f63db..d5b70c647 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -11,7 +11,6 @@ const extension = require('./lib/extension') const autoFaucet = require('./lib/auto-faucet') const nodeify = require('./lib/nodeify') - module.exports = class MetamaskController { constructor (opts) { @@ -19,12 +18,6 @@ module.exports = class MetamaskController { this.opts = opts this.listeners = [] this.configManager = new ConfigManager(opts) - this.txManager = new TxManager({ - TxListFromStore: this.configManager.getTxList(), - txLimit: this.configManager.txLimit, - setTxList: this.configManager.setTxList.bind(this.configManager), - }) - this.keyringController = new KeyringController({ configManager: this.configManager, txManager: this.txManager, @@ -33,9 +26,17 @@ module.exports = class MetamaskController { this.provider = this.initializeProvider(opts) this.ethStore = new EthStore(this.provider) this.keyringController.setStore(this.ethStore) - this.txManager.setProvider(this.provider) this.getNetwork() this.messageManager = messageManager + this.txManager = new TxManager({ + txList: this.configManager.getTxList(), + txHistoryLimit: 40, + setTxList: this.configManager.setTxList.bind(this.configManager), + getGasMultiplier: this.configManager.getGasMultiplier.bind(this.configManager), + getNetwork: this.getStateNetwork.bind(this), + provider: this.provider, + blockTracker: this.provider, + }) this.publicConfigStore = this.initPublicConfigStore() var currentFiat = this.configManager.getCurrentFiat() || 'USD' @@ -52,7 +53,8 @@ module.exports = class MetamaskController { this.state, this.ethStore.getState(), this.configManager.getConfig(), - this.keyringController.getState() + this.keyringController.getState(), + this.txManager.getState() ) } @@ -85,14 +87,14 @@ module.exports = class MetamaskController { exportAccount: nodeify(keyringController.exportAccount).bind(keyringController), // signing methods - approveTransaction: keyringController.approveTransaction.bind(keyringController), - cancelTransaction: keyringController.cancelTransaction.bind(keyringController), + approveTransaction: txManager.approveTransaction.bind(txManager), + cancelTransaction: txManager.cancelTransaction.bind(txManager), signMessage: keyringController.signMessage.bind(keyringController), cancelMessage: keyringController.cancelMessage.bind(keyringController), // forward directly to txManager - getUnapprovedTxList: txManager.getTxList.bind(txManager), - getFilterdTxList: txManager.getFilterdTxList.bind(txManager), + getUnapprovedTxList: txManager.getUnapprovedTxList.bind(txManager), + getFilteredTxList: txManager.getFilteredTxList.bind(txManager), // coinbase buyEth: this.buyEth.bind(this), // shapeshift @@ -150,7 +152,8 @@ module.exports = class MetamaskController { // tx signing approveTransaction: this.newUnsignedTransaction.bind(this), signTransaction: (...args) => { - keyringController.signTransaction(...args) + var signedTxPromise = keyringController.signTransaction(...args) + this.txManager.resolveSignedTransaction(signedTxPromise) this.sendUpdate() }, @@ -166,7 +169,6 @@ module.exports = class MetamaskController { var web3 = new Web3(provider) this.web3 = web3 keyringController.web3 = web3 - this.txManager.web3 = web3 provider.on('block', this.processBlock.bind(this)) provider.on('error', this.getNetwork.bind(this)) @@ -220,13 +222,13 @@ module.exports = class MetamaskController { } newUnsignedTransaction (txParams, onTxDoneCb) { - const keyringController = this.keyringController + const txManager = this.txManager const err = this.enforceTxValidations(txParams) if (err) return onTxDoneCb(err) - keyringController.addUnconfirmedTransaction(txParams, onTxDoneCb, (err, txData) => { + txManager.addUnapprovedTransaction(txParams, onTxDoneCb, (err, txData) => { if (err) return onTxDoneCb(err) this.sendUpdate() - this.opts.showUnconfirmedTx(txParams, txData, onTxDoneCb) + this.opts.showUnapprovedTx(txParams, txData, onTxDoneCb) }) } diff --git a/app/scripts/transaction-manager.js b/app/scripts/transaction-manager.js index d0226a600..6b3d1806f 100644 --- a/app/scripts/transaction-manager.js +++ b/app/scripts/transaction-manager.js @@ -1,15 +1,31 @@ const EventEmitter = require('events') const extend = require('xtend') -const TxProviderUtil = require('./lib/provider-utils') +const ethUtil = require('ethereumjs-util') +const TxProviderUtil = require('./lib/tx-utils') +const createId = require('./lib/random-id') module.exports = class TransactionManager extends EventEmitter { constructor (opts) { super() - this.txList = opts.TxListFromStore || [] - this._persistTxList = opts.setTxList + this.txList = opts.txList || [] + this._setTxList = opts.setTxList this._unconfTxCbs = {} - this.txLimit = opts.txLimit + this.txHistoryLimit = opts.txHistoryLimit + // txManager :: tx approvals and rejection cb's + this.provider = opts.provider + this.blockTracker = opts.blockTracker + this.txProviderUtils = new TxProviderUtil(this.provider) + this.blockTracker.on('block', this.checkForTxInBlock.bind(this)) + this.getGasMultiplier = opts.getGasMultiplier + this.getNetwork = opts.getNetwork + } + + getState () { + return { + transactions: this.getTxList(), + unconfTxs: this.getUnapprovedTxList(), + } } // Returns the tx list @@ -17,49 +33,49 @@ module.exports = class TransactionManager extends EventEmitter { return this.txList } - // Saves the new/updated txList. - // Function is intended only for internal use - _saveTxList (txList) { - this.txList = txList - this._persistTxList(txList) - } - // Adds a tx to the txlist - addTx (txData, onTxDoneCb) { + addTx (txMeta, onTxDoneCb = noop) { var txList = this.getTxList() - var txLimit = this.txLimit - if (txList.length > txLimit - 1) { - txList.shift() + var txHistoryLimit = this.txHistoryLimit + if (txList.length > txHistoryLimit - 1) { + var index = txList.findIndex((metaTx) => metaTx.status === 'confirmed' || metaTx.status === 'rejected') + index ? txList.splice(index, index) : txList.shift() } - txList.push(txData) + txList.push(txMeta) this._saveTxList(txList) - this.addOnTxDoneCb(txData.id, onTxDoneCb) - this.emit('unapproved', txData) + // keep the onTxDoneCb around in a listener + // for after approval/denial (requires user interaction) + // This onTxDoneCb fires completion to the Dapp's write operation. + this.once(`${txMeta.id}:signed`, function (txId) { + this.removeAllListeners(`${txMeta.id}:rejected`) + onTxDoneCb(null, true) + }) + this.once(`${txMeta.id}:rejected`, function (txId) { + this.removeAllListeners(`${txMeta.id}:signed`) + onTxDoneCb(null, false) + }) + this.emit('update') + this.emit(`${txMeta.id}:unapproved`, txMeta) } // gets tx by Id and returns it getTx (txId, cb) { var txList = this.getTxList() - var tx = txList.find((tx) => tx.id === txId) - return cb ? cb(tx) : tx + var txMeta = txList.find((txData) => txData.id === txId) + return cb ? cb(txMeta) : txMeta } // - updateTx (txData) { - var txId = txData.id + updateTx (txMeta) { + var txId = txMeta.id var txList = this.getTxList() - - var updatedTxList = txList.map((tx) => { - if (tx.id === txId) { - tx = txData - } - return tx - }) - this._saveTxList(updatedTxList) + var index = txList.findIndex((txData) => txData.id === txId) + txList[index] = txMeta + this._saveTxList(txList) } - get unConftxCount () { + get unconfTxCount () { return Object.keys(this.getUnapprovedTxList()).length } @@ -67,16 +83,66 @@ module.exports = class TransactionManager extends EventEmitter { return this.getTxsByMetaData('status', 'signed').length } + addUnapprovedTransaction (txParams, onTxDoneCb, cb) { + // create txData obj with parameters and meta data + var time = (new Date()).getTime() + var txId = createId() + txParams.metamaskId = txId + txParams.metamaskNetworkId = this.getNetwork() + var txData = { + id: txId, + txParams: txParams, + time: time, + status: 'unapproved', + gasMultiplier: this.getGasMultiplier() || 1, + metamaskNetworkId: this.getNetwork(), + } + this.txProviderUtils.analyzeGasUsage(txData, this.txDidComplete.bind(this, txData, onTxDoneCb, cb)) + // calculate metadata for tx + } + + txDidComplete (txMeta, onTxDoneCb, cb, err) { + if (err) return cb(err) + this.addTx(txMeta, onTxDoneCb) + cb(null, txMeta) + } + getUnapprovedTxList () { var txList = this.getTxList() - return txList.filter((tx) => { - return tx.status === 'unapproved' - }).reduce((result, tx) => { + return txList.filter((txMeta) => txMeta.status === 'unapproved') + .reduce((result, tx) => { result[tx.id] = tx return result }, {}) } + approveTransaction (txId, cb) { + this.setTxStatusSigned(txId) + cb() + } + + cancelTransaction (txId, cb) { + this.setTxStatusRejected(txId) + if (cb && typeof cb === 'function') { + cb() + } + } + + resolveSignedTransaction (txPromise) { + const self = this + + txPromise.then(({tx, txParams, cb}) => { + // Add the tx hash to the persisted meta-tx object + var txHash = ethUtil.bufferToHex(tx.hash()) + + var metaTx = self.getTx(txParams.metamaskId) + metaTx.hash = txHash + // return raw serialized tx + var rawTx = ethUtil.bufferToHex(tx.serialize()) + cb(null, rawTx) + }) + } + /* Takes an object of fields to search for eg: var thingsToLookFor = { @@ -92,7 +158,7 @@ module.exports = class TransactionManager extends EventEmitter { or for filltering for all txs from one account and that have been 'confirmed' */ - getFilterdTxList (opts) { + getFilteredTxList (opts) { var filteredTxList Object.keys(opts).forEach((key) => { filteredTxList = this.getTxsByMetaData(key, opts[key], filteredTxList) @@ -101,107 +167,96 @@ module.exports = class TransactionManager extends EventEmitter { } getTxsByMetaData (key, value, txList = this.getTxList()) { - return txList.filter((tx) => { - if (key in tx.txParams) { - return tx.txParams[key] === value + return txList.filter((txMeta) => { + if (key in txMeta.txParams) { + return txMeta.txParams[key] === value } else { - return tx[key] === value + return txMeta[key] === value } }) } -// keeps around the txCbs for later - addOnTxDoneCb (txId, cb) { - this._unconfTxCbs[txId] = cb || noop - } - - execOnTxDoneCb (txId, conf) { - var approvalCb = this._unconfTxCbs[txId] - - approvalCb(null, conf) - // clean up - delete this._unconfTxCbs[txId] - } - - // should return the tx - - // Should find the tx in the tx list and - // update it. - // should set the status in txData - // // - `'unapproved'` the user has not responded - // // - `'rejected'` the user has responded no! - // // - `'signed'` the tx is signed - // // - `'submitted'` the tx is sent to a server - // // - `'confirmed'` the tx has been included in a block. - setTxStatus (txId, status) { - var txData = this.getTx(txId) - txData.status = status - this.emit(status, txId) - this.updateTx(txData, status) - } - - // should return the status of the tx. getTxStatus (txId, cb) { - const txData = this.getTx(txId) - return cb ? cb(txData.staus) : txData.status + const txMeta = this.getTx(txId) + return cb ? cb(txMeta.staus) : txMeta.status } // should update the status of the tx to 'signed'. setTxStatusSigned (txId) { - this.setTxStatus(txId, 'signed') + this._setTxStatus(txId, 'signed') this.emit('update') } // should update the status of the tx to 'rejected'. setTxStatusRejected (txId) { - this.setTxStatus(txId, 'rejected') + this._setTxStatus(txId, 'rejected') this.emit('update') } setTxStatusConfirmed (txId) { - this.setTxStatus(txId, 'confirmed') + this._setTxStatus(txId, 'confirmed') } // merges txParams obj onto txData.txParams // use extend to ensure that all fields are filled updateTxParams (txId, txParams) { - var txData = this.getTx(txId) - txData.txParams = extend(txData, txParams) - this.updateTx(txData) - } - - // sets provider for provider utils and event listener - setProvider (provider) { - this.provider = provider - this.txProviderUtils = new TxProviderUtil(provider) - this.provider.on('block', this.checkForTxInBlock.bind(this)) + var txMeta = this.getTx(txId) + txMeta.txParams = extend(txMeta, txParams) + this.updateTx(txMeta) } // checks if a signed tx is in a block and // if included sets the tx status as 'confirmed' checkForTxInBlock () { - var signedTxList = this.getFilterdTxList({status: 'signed'}) + var signedTxList = this.getFilteredTxList({status: 'signed', err: undefined}) if (!signedTxList.length) return - var self = this signedTxList.forEach((tx) => { var txHash = tx.hash var txId = tx.id if (!txHash) return - // var d - this.txProviderUtils.query.getTransactionByHash(txHash, (err, txData) => { - if (err) { - tx - - return console.error(err) + this.txProviderUtils.query.getTransactionByHash(txHash, (err, txMeta) => { + if (err || !txMeta) { + tx.err = err || 'Tx could possibly have not submitted' + this.updateTx(tx) + return txMeta ? console.error(err) : console.debug(`txMeta is ${txMeta} for:`, tx) } - if (txData.blockNumber !== null) { - self.setTxStatusConfirmed(txId) + if (txMeta.blockNumber) { + this.setTxStatusConfirmed(txId) } }) }) } + + // Private functions + + // Saves the new/updated txList. + // Function is intended only for internal use + _saveTxList (txList) { + this.txList = txList + this._setTxList(txList) + } + + // should return the tx + + // Should find the tx in the tx list and + // update it. + // should set the status in txData + // - `'unapproved'` the user has not responded + // - `'rejected'` the user has responded no! + // - `'signed'` the tx is signed + // - `'submitted'` the tx is sent to a server + // - `'confirmed'` the tx has been included in a block. + _setTxStatus (txId, status) { + var txMeta = this.getTx(txId) + txMeta.status = status + this.emit(`${txMeta.id}:${status}`, txId) + this.updateTx(txMeta) + } + + } -function noop () {} + +const noop = () => console.warn('noop was used no cb provided') diff --git a/library/controller.js b/library/controller.js index 07419550a..5823287cc 100644 --- a/library/controller.js +++ b/library/controller.js @@ -21,7 +21,7 @@ function initializeZeroClient() { // User confirmation callbacks: showUnconfirmedMessage, unlockAccountMessage, - showUnconfirmedTx, + showUnapprovedTx, // Persistence Methods: setData, loadData, @@ -36,8 +36,8 @@ function initializeZeroClient() { console.log('notif stub - showUnconfirmedMessage') } - function showUnconfirmedTx (txParams, txData, onTxDoneCb) { - console.log('notif stub - showUnconfirmedTx') + function showUnapprovedTx (txParams, txData, onTxDoneCb) { + console.log('notif stub - showUnapprovedTx') } // @@ -156,4 +156,4 @@ function initializeZeroClient() { } } -} \ No newline at end of file +} diff --git a/mock-dev.js b/mock-dev.js index 84f2d234a..283bc2c79 100644 --- a/mock-dev.js +++ b/mock-dev.js @@ -46,7 +46,7 @@ const controller = new MetamaskController({ // User confirmation callbacks: showUnconfirmedMessage: noop, unlockAccountMessage: noop, - showUnconfirmedTx: noop, + showUnapprovedTx: noop, // Persistence Methods: setData, loadData, diff --git a/test/unit/idStore-test.js b/test/unit/idStore-test.js index 3ca89cd38..000c58a82 100644 --- a/test/unit/idStore-test.js +++ b/test/unit/idStore-test.js @@ -139,54 +139,4 @@ describe('IdentityStore', function() { }) }) }) - - describe('#addGasBuffer', function() { - it('formats the result correctly', function() { - const idStore = new IdentityStore({ - configManager: configManagerGen(), - ethStore: { - addAccount(acct) { accounts.push(ethUtil.addHexPrefix(acct)) }, - }, - }) - - const gas = '0x01' - const bnGas = new BN(gas, 16) - const bnResult = idStore.addGasBuffer(bnGas) - - assert.ok(bnResult.gt(gas), 'added more gas as buffer.') - }) - - it('buffers 20%', function() { - const idStore = new IdentityStore({ - configManager: configManagerGen(), - ethStore: { - addAccount(acct) { accounts.push(ethUtil.addHexPrefix(acct)) }, - }, - }) - - const gas = '0x04ee59' // Actual estimated gas example - const bnGas = new BN(ethUtil.stripHexPrefix(gas), 16) - const five = new BN('5', 10) - const correctBuffer = bnGas.div(five) - const correct = bnGas.add(correctBuffer) - - const bnResult = idStore.addGasBuffer(bnGas) - - assert(bnResult.gt(bnGas), 'Estimate increased in value.') - assert.equal(bnResult.sub(bnGas).toString(10), correctBuffer.toString(10), 'added 20% gas') - }) - }) - - describe('#checkForDelegateCall', function() { - const idStore = new IdentityStore({ - configManager: configManagerGen(), - ethStore: { - addAccount(acct) { accounts.push(ethUtil.addHexPrefix(acct)) }, - }, - }) - - var result = idStore.checkForDelegateCall(delegateCallCode) - assert.equal(result, true, 'no delegate call in provided code') - }) - }) diff --git a/test/unit/metamask-controller-test.js b/test/unit/metamask-controller-test.js index b87169ca2..414610404 100644 --- a/test/unit/metamask-controller-test.js +++ b/test/unit/metamask-controller-test.js @@ -9,7 +9,7 @@ describe('MetaMaskController', function() { let controller = new MetaMaskController({ showUnconfirmedMessage: noop, unlockAccountMessage: noop, - showUnconfirmedTx: noop, + showUnapprovedTx: noop, setData, loadData, }) diff --git a/test/unit/tx-manager-test.js b/test/unit/tx-manager-test.js index 0a7c5e83b..f09068a72 100644 --- a/test/unit/tx-manager-test.js +++ b/test/unit/tx-manager-test.js @@ -1,5 +1,6 @@ const assert = require('assert') const extend = require('xtend') +const EventEmitter = require('events') const STORAGE_KEY = 'metamask-persistance-key' const TransactionManager = require('../../app/scripts/transaction-manager') @@ -9,10 +10,11 @@ describe('Transaction Manager', function() { const onTxDoneCb = () => true beforeEach(function() { txManager = new TransactionManager ({ - TxListFromStore: [], + txList: [], setTxList: () => {}, provider: "testnet", - txLimit: 40, + txHistoryLimit: 10, + blockTracker: new EventEmitter(), }) }) @@ -38,7 +40,7 @@ describe('Transaction Manager', function() { describe('#addTx', function() { it('adds a tx returned in getTxList', function() { - var tx = { id: 1 } + var tx = { id: 1, status: 'confirmed',} txManager.addTx(tx, onTxDoneCb) var result = txManager.getTxList() assert.ok(Array.isArray(result)) @@ -47,15 +49,41 @@ describe('Transaction Manager', function() { }) it('cuts off early txs beyond a limit', function() { - const limit = txManager.txLimit + const limit = txManager.txHistoryLimit for (let i = 0; i < limit + 1; i++) { - let tx = { id: i, time: new Date()} + let tx = { id: i, time: new Date(), status: 'confirmed'} txManager.addTx(tx, onTxDoneCb) } var result = txManager.getTxList() assert.equal(result.length, limit, `limit of ${limit} txs enforced`) assert.equal(result[0].id, 1, 'early txs truncted') }) + + it('cuts off early txs beyond a limit weather or not it is confirmed or rejected', function() { + const limit = txManager.txHistoryLimit + for (let i = 0; i < limit + 1; i++) { + let tx = { id: i, time: new Date(), status: 'rejected'} + txManager.addTx(tx, onTxDoneCb) + } + var result = txManager.getTxList() + assert.equal(result.length, limit, `limit of ${limit} txs enforced`) + assert.equal(result[0].id, 1, 'early txs truncted') + }) + + it('cuts off early txs beyond a limit but does not cut unapproved txs', function() { + var unconfirmedTx = { id: 0, time: new Date(), status: 'unapproved'} + txManager.addTx(unconfirmedTx, onTxDoneCb) + const limit = txManager.txHistoryLimit + for (let i = 1; i < limit + 1; i++) { + let tx = { id: i, time: new Date(), status: 'confirmed'} + txManager.addTx(tx, onTxDoneCb) + } + var result = txManager.getTxList() + assert.equal(result.length, limit, `limit of ${limit} txs enforced`) + assert.equal(result[0].id, 0, 'first tx should still be their') + assert.equal(result[0].status, 'unapproved', 'first tx should be unapproved') + assert.equal(result[1].id, 2, 'early txs truncted') + }) }) describe('#setTxStatusSigned', function() { @@ -72,13 +100,10 @@ describe('Transaction Manager', function() { it('should emit a signed event to signal the exciton of callback', (done) => { this.timeout(10000) var tx = { id: 1, status: 'unapproved' } - txManager.on('signed', function (txId) { - var approvalCb = this._unconfTxCbs[txId] - assert(approvalCb(), 'txCb was retrieved') - assert.equal(txId, 1) - assert(true, 'event listener has been triggered') + let onTxDoneCb = function (err, txId) { + assert(true, 'event listener has been triggered and onTxDoneCb executed') done() - }) + } txManager.addTx(tx, onTxDoneCb) txManager.setTxStatusSigned(1) }) @@ -87,7 +112,7 @@ describe('Transaction Manager', function() { describe('#setTxStatusRejected', function() { it('sets the tx status to rejected', function() { var tx = { id: 1, status: 'unapproved' } - txManager.addTx(tx) + txManager.addTx(tx, onTxDoneCb) txManager.setTxStatusRejected(1) var result = txManager.getTxList() assert.ok(Array.isArray(result)) @@ -98,13 +123,10 @@ describe('Transaction Manager', function() { it('should emit a rejected event to signal the exciton of callback', (done) => { this.timeout(10000) var tx = { id: 1, status: 'unapproved' } - txManager.on('rejected', function (txId) { - var approvalCb = this._unconfTxCbs[txId] - assert(approvalCb(), 'txCb was retrieved') - assert.equal(txId, 1) - assert(true, 'event listener has been triggered') + let onTxDoneCb = function (err, txId) { + assert(true, 'event listener has been triggered and onTxDoneCb executed') done() - }) + } txManager.addTx(tx, onTxDoneCb) txManager.setTxStatusRejected(1) }) @@ -128,7 +150,6 @@ describe('Transaction Manager', function() { let result = txManager.getUnapprovedTxList() assert.equal(typeof result, 'object') assert.equal(result['1'].status, 'unapproved') - assert.equal(result['0'], undefined) assert.equal(result['2'], undefined) }) }) @@ -142,7 +163,7 @@ describe('Transaction Manager', function() { }) }) - describe('#getFilterdTxList', function() { + describe('#getFilteredTxList', function() { it('returns a tx with the requested data', function() { var foop = 0 var zoop = 0 @@ -157,12 +178,12 @@ describe('Transaction Manager', function() { }, onTxDoneCb) evryOther ? ++foop : ++zoop } - assert.equal(txManager.getFilterdTxList({status: 'confirmed', from: 'zoop'}).length, zoop) - assert.equal(txManager.getFilterdTxList({status: 'confirmed', to: 'foop'}).length, zoop) - assert.equal(txManager.getFilterdTxList({status: 'confirmed', from: 'foop'}).length, 0) - assert.equal(txManager.getFilterdTxList({status: 'confirmed'}).length, zoop) - assert.equal(txManager.getFilterdTxList({from: 'foop'}).length, foop) - assert.equal(txManager.getFilterdTxList({from: 'zoop'}).length, zoop) + assert.equal(txManager.getFilteredTxList({status: 'confirmed', from: 'zoop'}).length, zoop) + assert.equal(txManager.getFilteredTxList({status: 'confirmed', to: 'foop'}).length, zoop) + assert.equal(txManager.getFilteredTxList({status: 'confirmed', from: 'foop'}).length, 0) + assert.equal(txManager.getFilteredTxList({status: 'confirmed'}).length, zoop) + assert.equal(txManager.getFilteredTxList({from: 'foop'}).length, foop) + assert.equal(txManager.getFilteredTxList({from: 'zoop'}).length, zoop) }) }) diff --git a/ui/app/components/transaction-list-item.js b/ui/app/components/transaction-list-item.js index d1306549e..bb685abda 100644 --- a/ui/app/components/transaction-list-item.js +++ b/ui/app/components/transaction-list-item.js @@ -31,7 +31,7 @@ TransactionListItem.prototype.render = function () { var isMsg = ('msgParams' in transaction) var isTx = ('txParams' in transaction) - var isPending = transaction.status === 'unconfirmed' + var isPending = transaction.status === 'unapproved' let txParams if (isTx) { @@ -59,7 +59,7 @@ TransactionListItem.prototype.render = function () { }, [ h('.identicon-wrapper.flex-column.flex-center.select-none', [ - transaction.status === 'unconfirmed' ? h('i.fa.fa-ellipsis-h', { + transaction.status === 'unapproved' ? h('i.fa.fa-ellipsis-h', { style: { fontSize: '27px', }, -- cgit