From 451845142ed40d1e10bd993cedca1b28e59baba1 Mon Sep 17 00:00:00 2001 From: Frankie Date: Fri, 27 Jan 2017 14:16:35 -0800 Subject: [PATCH 1/6] Rewrite message controller to fit controller pattern --- app/scripts/lib/message-manager.js | 113 ++++++++++++++--------------- 1 file changed, 55 insertions(+), 58 deletions(-) diff --git a/app/scripts/lib/message-manager.js b/app/scripts/lib/message-manager.js index b609b820e..379f38917 100644 --- a/app/scripts/lib/message-manager.js +++ b/app/scripts/lib/message-manager.js @@ -1,61 +1,58 @@ -module.exports = new MessageManager() +const EventEmitter = require('events') -function MessageManager (opts) { - this.messages = [] -} - -MessageManager.prototype.getMsgList = function () { - return this.messages -} - -MessageManager.prototype.unconfirmedMsgs = function () { - var messages = this.getMsgList() - return messages.filter(msg => msg.status === 'unconfirmed') - .reduce((result, msg) => { result[msg.id] = msg; return result }, {}) -} - -MessageManager.prototype._saveMsgList = function (msgList) { - this.messages = msgList -} - -MessageManager.prototype.addMsg = function (msg) { - var messages = this.getMsgList() - messages.push(msg) - this._saveMsgList(messages) -} - -MessageManager.prototype.getMsg = function (msgId) { - var messages = this.getMsgList() - var matching = messages.filter(msg => msg.id === msgId) - return matching.length > 0 ? matching[0] : null -} - -MessageManager.prototype.confirmMsg = function (msgId) { - this._setMsgStatus(msgId, 'confirmed') -} - -MessageManager.prototype.rejectMsg = function (msgId) { - this._setMsgStatus(msgId, 'rejected') -} - -MessageManager.prototype._setMsgStatus = function (msgId, status) { - var msg = this.getMsg(msgId) - if (msg) msg.status = status - this.updateMsg(msg) -} - -MessageManager.prototype.updateMsg = function (msg) { - var messages = this.getMsgList() - var found, index - messages.forEach((otherMsg, i) => { - if (otherMsg.id === msg.id) { - found = true - index = i - } - }) - if (found) { - messages[index] = msg +module.exports = class MessageManager extends EventEmitter{ + constructor (opts) { + super() + this.messages = [] } - this._saveMsgList(messages) -} + getMsgList () { + return this.messages + } + + unconfirmedMsgs () { + let messages = this.getMsgList() + return messages.filter(msg => msg.status === 'unconfirmed') + .reduce((result, msg) => { result[msg.id] = msg; return result }, {}) + } + + _saveMsgList (msgList) { + this.messages = msgList + } + + addMsg (msg) { + let messages = this.getMsgList() + messages.push(msg) + this._saveMsgList(messages) + } + + getMsg (msgId) { + let messages = this.getMsgList() + let matching = messages.filter(msg => msg.id === msgId) + return matching.length > 0 ? matching[0] : null + } + + confirmMsg (msgId) { + this._setMsgStatus(msgId, 'confirmed') + } + + rejectMsg (msgId) { + this._setMsgStatus(msgId, 'rejected') + } + + _setMsgStatus (msgId, status) { + let msg = this.getMsg(msgId) + if (msg) msg.status = status + this.updateMsg(msg) + } + + updateMsg (msg) { + let messages = this.getMsgList() + let index = messages.findIndex((message) => message.id === msg.id) + if (index !== -1) { + this.emit('update', msg.id) + messages[index] = msg + } + this._saveMsgList(messages) + } +} From 8be68575bbef1dcc89b51355abaee90dbf018387 Mon Sep 17 00:00:00 2001 From: Frankie Date: Fri, 27 Jan 2017 16:11:59 -0800 Subject: [PATCH 2/6] Clean up message manger includes: Provider egine bump Remove presence of message manger in keyring controller Change the status wording fom conf to approved make Message manager a class fix messages not being apart of the badge re write message manger to better reflect controller pattern --- app/scripts/background.js | 9 +- app/scripts/keyring-controller.js | 105 ++++-------------- app/scripts/lib/message-manager.js | 77 +++++++++++-- app/scripts/metamask-controller.js | 62 +++++++---- app/scripts/transaction-manager.js | 2 +- package.json | 2 +- test/unit/actions/tx_test.js | 6 +- ui/app/account-detail.js | 6 +- ui/app/accounts/index.js | 10 +- ui/app/app.js | 4 +- .../components/transaction-list-item-icon.js | 8 +- ui/app/components/transaction-list-item.js | 1 - ui/app/components/transaction-list.js | 4 +- ui/app/conf-tx.js | 10 +- ui/app/css/index.css | 10 ++ ui/app/reducers/app.js | 18 +-- ui/app/reducers/metamask.js | 14 +-- ui/example.js | 6 +- ui/index.js | 4 +- ui/lib/tx-helper.js | 6 +- 20 files changed, 185 insertions(+), 179 deletions(-) diff --git a/app/scripts/background.js b/app/scripts/background.js index da9c4f24b..2e5a992b9 100644 --- a/app/scripts/background.js +++ b/app/scripts/background.js @@ -8,7 +8,6 @@ const Migrator = require('./lib/migrator/') const migrations = require('./migrations/') const PortStream = require('./lib/port-stream.js') const notification = require('./lib/notifications.js') -const messageManager = require('./lib/message-manager') const MetamaskController = require('./metamask-controller') const extension = require('./lib/extension') const firstTimeState = require('./first-time-state') @@ -112,14 +111,14 @@ function setupController (initState) { updateBadge() controller.txManager.on('updateBadge', updateBadge) + controller.messageManager.on('updateBadge', updateBadge) // plugin badge text function updateBadge () { var label = '' var unapprovedTxCount = controller.txManager.unapprovedTxCount - var unconfMsgs = messageManager.unconfirmedMsgs() - var unconfMsgLen = Object.keys(unconfMsgs).length - var count = unapprovedTxCount + unconfMsgLen + var unapprovedMsgCount = controller.messageManager.unapprovedMsgCount + var count = unapprovedTxCount + unapprovedMsgCount if (count) { label = String(count) } @@ -145,4 +144,4 @@ extension.runtime.onInstalled.addListener(function (details) { if ((details.reason === 'install') && (!METAMASK_DEBUG)) { extension.tabs.create({url: 'https://metamask.io/#how-it-works'}) } -}) \ No newline at end of file +}) diff --git a/app/scripts/keyring-controller.js b/app/scripts/keyring-controller.js index a4bee8ae1..b6c9aa3a7 100644 --- a/app/scripts/keyring-controller.js +++ b/app/scripts/keyring-controller.js @@ -5,7 +5,6 @@ const filter = require('promise-filter') const encryptor = require('browser-passworder') const normalize = require('./lib/sig-util').normalize -const messageManager = require('./lib/message-manager') const BN = ethUtil.BN // Keyrings: @@ -16,8 +15,6 @@ const keyringTypes = [ HdKeyring, ] -const createId = require('./lib/random-id') - module.exports = class KeyringController extends EventEmitter { // PUBLIC METHODS @@ -35,9 +32,6 @@ module.exports = class KeyringController extends EventEmitter { this.keyringTypes = keyringTypes this.keyrings = [] this.identities = {} // Essentially a name hash - - this._unconfMsgCbs = {} - this.getNetwork = opts.getNetwork } @@ -84,8 +78,6 @@ module.exports = class KeyringController extends EventEmitter { isInitialized: (!!wallet || !!vault), isUnlocked: Boolean(this.password), isDisclaimerConfirmed: this.configManager.getConfirmedDisclaimer(), - unconfMsgs: messageManager.unconfirmedMsgs(), - messages: messageManager.getMsgList(), selectedAccount: address, shapeShiftTxList: this.configManager.getShapeShiftTxList(), currentFiat: this.configManager.getCurrentFiat(), @@ -154,6 +146,17 @@ module.exports = class KeyringController extends EventEmitter { .then(this.fullUpdate.bind(this)) } + // ClearSeedWordCache + // + // returns Promise( @string currentSelectedAccount ) + // + // Removes the current vault's seed words from the UI's state tree, + // ensuring they are only ever available in the background process. + clearSeedWordCache () { + this.configManager.setSeedWords(null) + return Promise.resolve(this.configManager.getSelectedAccount()) + } + // Set Locked // returns Promise( @object state ) // @@ -204,8 +207,8 @@ module.exports = class KeyringController extends EventEmitter { this.keyrings.push(keyring) return this.setupAccounts(accounts) }) - .then(() => this.persistAllKeyrings()) - .then(() => this.fullUpdate()) + .then(() => { return this.password }) + .then(this.persistAllKeyrings.bind(this)) .then(() => { return keyring }) @@ -287,86 +290,19 @@ module.exports = class KeyringController extends EventEmitter { return keyring.signTransaction(fromAddress, ethTx) }) } - // Add Unconfirmed Message - // @object msgParams - // @function cb - // - // Does not call back, only emits an `update` event. - // - // Adds the given `msgParams` and `cb` to a local cache, - // for displaying to a user for approval before signing or canceling. - addUnconfirmedMessage (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.emit('update') - return msgId - } - - // Cancel Message - // @string msgId - // @function cb (optional) - // - // Calls back to cached `unconfMsgCb`. - // Calls back to `cb` if provided. - // - // Forgets any messages matching `msgId`. - cancelMessage (msgId, cb) { - var approvalCb = this._unconfMsgCbs[msgId] || noop - - // reject tx - approvalCb(null, false) - // clean up - messageManager.rejectMsg(msgId) - delete this._unconfTxCbs[msgId] - - if (cb && typeof cb === 'function') { - cb() - } - } // Sign Message // @object msgParams - // @function cb // // returns Promise(@buffer rawSig) - // calls back @function cb with @buffer rawSig - // calls back cached Dapp's @function unconfMsgCb. // // Attempts to sign the provided @object msgParams. - signMessage (msgParams, cb) { - try { - const msgId = msgParams.metamaskId - delete msgParams.metamaskId - const approvalCb = this._unconfMsgCbs[msgId] || noop - - const address = normalize(msgParams.from) - return this.getKeyringForAccount(address) - .then((keyring) => { - return keyring.signMessage(address, msgParams.data) - }).then((rawSig) => { - cb(null, rawSig) - approvalCb(null, true) - messageManager.confirmMsg(msgId) - return rawSig - }) - } catch (e) { - cb(e) - } + signMessage (msgParams) { + const address = normalize(msgParams.from) + return this.getKeyringForAccount(address) + .then((keyring) => { + return keyring.signMessage(address, msgParams.data) + }) } // PRIVATE METHODS @@ -643,6 +579,3 @@ module.exports = class KeyringController extends EventEmitter { } } - - -function noop () {} diff --git a/app/scripts/lib/message-manager.js b/app/scripts/lib/message-manager.js index 379f38917..bc9a9e6c8 100644 --- a/app/scripts/lib/message-manager.js +++ b/app/scripts/lib/message-manager.js @@ -1,23 +1,61 @@ const EventEmitter = require('events') +const ObservableStore = require('obs-store') +const createId = require('./random-id') + module.exports = class MessageManager extends EventEmitter{ constructor (opts) { super() - this.messages = [] + this.memStore = new ObservableStore({ messages: [] }) + } + + getState() { + return { + unapprovedMsgs: this.unapprovedMsgs(), + messages: this.getMsgList(), + } } getMsgList () { - return this.messages + return this.memStore.getState().messages } - unconfirmedMsgs () { + get unapprovedMsgCount () { + return Object.keys(this.unapprovedMsgs()).length + } + + unapprovedMsgs () { let messages = this.getMsgList() - return messages.filter(msg => msg.status === 'unconfirmed') + return messages.filter(msg => msg.status === 'unapproved') .reduce((result, msg) => { result[msg.id] = msg; return result }, {}) } _saveMsgList (msgList) { - this.messages = msgList + this.emit('updateBadge') + let state = this.memStore.getState() + state.messages = msgList + this.memStore.putState(state) + } + + addUnapprovedMessage (msgParams) { + // 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: 'unapproved', + } + this.addMsg(msgData) + console.log('addUnapprovedMessage:', msgData) + + // keep the cb around for after approval (requires user interaction) + // This cb fires completion to the Dapp's write operation. + + // signal update + this.emit('update') + return msgId } addMsg (msg) { @@ -32,8 +70,28 @@ module.exports = class MessageManager extends EventEmitter{ return matching.length > 0 ? matching[0] : null } - confirmMsg (msgId) { - this._setMsgStatus(msgId, 'confirmed') + brodcastMessage (rawSig, msgId, status) { + this.emit(`${msgId}:finished`, {status, rawSig}) + } + + approveMessage (msgParams) { + this.setMessageApproved(msgParams.metamaskId) + return this.prepMsgForSigning(msgParams) + } + + setMessageApproved (msgId) { + this._setMsgStatus(msgId, 'approved') + } + prepMsgForSigning (msgParams) { + delete msgParams.metamaskId + return Promise.resolve(msgParams) + } + + cancelMessage (msgId) { + // reject tx + // clean up + this.brodcastMessage(null, msgId, 'rejected') + this.rejectMsg(msgId) } rejectMsg (msgId) { @@ -43,14 +101,13 @@ module.exports = class MessageManager extends EventEmitter{ _setMsgStatus (msgId, status) { let msg = this.getMsg(msgId) if (msg) msg.status = status - this.updateMsg(msg) + this._updateMsg(msg) } - updateMsg (msg) { + _updateMsg (msg) { let messages = this.getMsgList() let index = messages.findIndex((message) => message.id === msg.id) if (index !== -1) { - this.emit('update', msg.id) messages[index] = msg } this._saveMsgList(messages) diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 3ce9c2373..571968b65 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -12,7 +12,7 @@ const MetaMaskProvider = require('web3-provider-engine/zero.js') const setupMultiplex = require('./lib/stream-utils.js').setupMultiplex const KeyringController = require('./keyring-controller') const NoticeController = require('./notice-controller') -const messageManager = require('./lib/message-manager') +const MessageManager = require('./lib/message-manager') const TxManager = require('./transaction-manager') const ConfigManager = require('./lib/config-manager') const extension = require('./lib/extension') @@ -32,7 +32,7 @@ module.exports = class MetamaskController extends EventEmitter { // observable state store this.store = new ObservableStore(opts.initState) - + // config manager this.configManager = new ConfigManager({ store: this.store, @@ -47,7 +47,7 @@ module.exports = class MetamaskController extends EventEmitter { // eth data query tools this.ethQuery = new EthQuery(this.provider) this.ethStore = new EthStore(this.provider) - + // key mgmt this.keyringController = new KeyringController({ ethStore: this.ethStore, @@ -70,7 +70,7 @@ module.exports = class MetamaskController extends EventEmitter { provider: this.provider, blockTracker: this.provider, }) - + // notices this.noticeController = new NoticeController({ configManager: this.configManager, @@ -80,7 +80,7 @@ module.exports = class MetamaskController extends EventEmitter { // this.noticeController.startPolling() this.getNetwork() - this.messageManager = messageManager + this.messageManager = new MessageManager() this.publicConfigStore = this.initPublicConfigStore() this.checkTOSChange() @@ -96,6 +96,7 @@ module.exports = class MetamaskController extends EventEmitter { this.ethStore.on('update', this.sendUpdate.bind(this)) this.keyringController.on('update', this.sendUpdate.bind(this)) this.txManager.on('update', this.sendUpdate.bind(this)) + this.messageManager.on('update', this.sendUpdate.bind(this)) } // @@ -118,11 +119,7 @@ module.exports = class MetamaskController extends EventEmitter { // tx signing processTransaction: (txParams, cb) => this.newUnapprovedTransaction(txParams, cb), // msg signing - approveMessage: this.newUnsignedMessage.bind(this), - signMessage: (...args) => { - this.keyringController.signMessage(...args) - this.sendUpdate() - }, + processMessage: this.newUnsignedMessage.bind(this), }) return provider } @@ -163,6 +160,7 @@ module.exports = class MetamaskController extends EventEmitter { this.ethStore.getState(), this.configManager.getConfig(), this.txManager.getState(), + this.messageManager.getState(), keyringControllerState, this.noticeController.getState(), { lostAccounts: this.configManager.getLostAccounts(), @@ -178,6 +176,7 @@ module.exports = class MetamaskController extends EventEmitter { getApi () { const keyringController = this.keyringController const txManager = this.txManager + const messageManager = this.messageManager const noticeController = this.noticeController return { @@ -197,7 +196,7 @@ module.exports = class MetamaskController extends EventEmitter { buyEth: this.buyEth.bind(this), // shapeshift createShapeShiftTx: this.createShapeShiftTx.bind(this), - + // primary HD keyring management addNewAccount: this.addNewAccount.bind(this), placeSeedWords: this.placeSeedWords.bind(this), @@ -219,8 +218,8 @@ module.exports = class MetamaskController extends EventEmitter { // signing methods approveTransaction: txManager.approveTransaction.bind(txManager), cancelTransaction: txManager.cancelTransaction.bind(txManager), - signMessage: keyringController.signMessage.bind(keyringController), - cancelMessage: keyringController.cancelMessage.bind(keyringController), + signMessage: this.signMessage.bind(this), + cancelMessage: messageManager.cancelMessage.bind(messageManager), // notices checkNotices: noticeController.updateNoticesList.bind(noticeController), @@ -358,20 +357,35 @@ module.exports = class MetamaskController extends EventEmitter { } newUnsignedMessage (msgParams, cb) { - var state = this.keyringController.getState() - if (!state.isUnlocked) { - this.keyringController.addUnconfirmedMessage(msgParams, cb) - this.opts.unlockAccountMessage() - } else { - this.addUnconfirmedMessage(msgParams, cb) + this.keyringController.getState() + .then((state) => { + let msgId = this.messageManager.addUnapprovedMessage(msgParams) this.sendUpdate() - } + state.isUnlocked ? this.opts.unlockAccountMessage() : this.opts.showUnconfirmedMessage() + this.messageManager.once(`${msgId}:finished`, (data) => { + switch (data.status) { + case 'approved': + return cb(null, data.rawSig) + case 'rejected': + return cb(new Error('MetaMask Tx Signature: User denied transaction signature.')) + default: + return cb(new Error(`MetaMask Tx Signature: Unknown problem: ${JSON.stringify(msgParams)}`)) + } + }) + }) } - addUnconfirmedMessage (msgParams, cb) { - const keyringController = this.keyringController - const msgId = keyringController.addUnconfirmedMessage(msgParams, cb) - this.opts.showUnconfirmedMessage(msgParams, msgId) + signMessage (msgParams, cb) { + const msgId = msgParams.metamaskId + return this.messageManager.approveMessage(msgParams) + .then((cleanMsgParams) => { + return this.keyringController.signMessage(cleanMsgParams) + }) + .then((rawSig) => { + this.messageManager.brodcastMessage(rawSig, msgId, 'approved') + }).then(() => { + cb() + }).catch((err) => cb(err)) } setupPublicConfig (outStream) { diff --git a/app/scripts/transaction-manager.js b/app/scripts/transaction-manager.js index 6d0121afd..153b8bc28 100644 --- a/app/scripts/transaction-manager.js +++ b/app/scripts/transaction-manager.js @@ -28,7 +28,7 @@ module.exports = class TransactionManager extends EventEmitter { var selectedAccount = this.getSelectedAccount() return { transactions: this.getTxList(), - unconfTxs: this.getUnapprovedTxList(), + unapprovedTxs: this.getUnapprovedTxList(), selectedAccountTxList: this.getFilteredTxList({metamaskNetworkId: this.getNetwork(), from: selectedAccount}), } } diff --git a/package.json b/package.json index bf4b3986c..005da5c89 100644 --- a/package.json +++ b/package.json @@ -102,7 +102,7 @@ "valid-url": "^1.0.9", "vreme": "^3.0.2", "web3": "0.17.0-beta", - "web3-provider-engine": "^8.4.0", + "web3-provider-engine": "^8.5.0", "web3-stream-provider": "^2.0.6", "xtend": "^4.0.1" }, diff --git a/test/unit/actions/tx_test.js b/test/unit/actions/tx_test.js index 1f06b1120..7ded5b1ef 100644 --- a/test/unit/actions/tx_test.js +++ b/test/unit/actions/tx_test.js @@ -31,7 +31,7 @@ describe('tx confirmation screen', function() { }, }, metamask: { - unconfTxs: { + unapprovedTxs: { '1457634084250832': { id: 1457634084250832, status: "unconfirmed", @@ -119,7 +119,7 @@ describe('tx confirmation screen', function() { }, }, metamask: { - unconfTxs: { + unapprovedTxs: { '1457634084250832': { id: 1457634084250832, status: "unconfirmed", @@ -162,7 +162,7 @@ describe('tx confirmation screen', function() { }); function getUnconfirmedTxCount(state) { - var txs = state.metamask.unconfTxs + var txs = state.metamask.unapprovedTxs var count = Object.keys(txs).length return count } diff --git a/ui/app/account-detail.js b/ui/app/account-detail.js index 387a1720a..7f6f155f3 100644 --- a/ui/app/account-detail.js +++ b/ui/app/account-detail.js @@ -27,7 +27,7 @@ function mapStateToProps (state) { address: state.metamask.selectedAccount, accountDetail: state.appState.accountDetail, network: state.metamask.network, - unconfMsgs: valuesFor(state.metamask.unconfMsgs), + unapprovedMsgs: valuesFor(state.metamask.unapprovedMsgs), shapeShiftTxList: state.metamask.shapeShiftTxList, transactions: state.metamask.selectedAccountTxList || [], } @@ -245,11 +245,11 @@ AccountDetailScreen.prototype.subview = function () { } AccountDetailScreen.prototype.transactionList = function () { - const {transactions, unconfMsgs, address, network, shapeShiftTxList } = this.props + const {transactions, unapprovedMsgs, address, network, shapeShiftTxList } = this.props return h(TransactionList, { transactions: transactions.sort((a, b) => b.time - a.time), network, - unconfMsgs, + unapprovedMsgs, address, shapeShiftTxList, viewPendingTx: (txId) => { diff --git a/ui/app/accounts/index.js b/ui/app/accounts/index.js index e6f376735..db380cd39 100644 --- a/ui/app/accounts/index.js +++ b/ui/app/accounts/index.js @@ -10,15 +10,15 @@ const AccountListItem = require('./account-list-item') module.exports = connect(mapStateToProps)(AccountsScreen) function mapStateToProps (state) { - const pendingTxs = valuesFor(state.metamask.unconfTxs) + const pendingTxs = valuesFor(state.metamask.unapprovedTxs) .filter(tx => tx.txParams.metamaskNetworkId === state.metamask.network) - const pendingMsgs = valuesFor(state.metamask.unconfMsgs) + const pendingMsgs = valuesFor(state.metamask.unapprovedMsgs) const pending = pendingTxs.concat(pendingMsgs) return { accounts: state.metamask.accounts, identities: state.metamask.identities, - unconfTxs: state.metamask.unconfTxs, + unapprovedTxs: state.metamask.unapprovedTxs, selectedAccount: state.metamask.selectedAccount, scrollToBottom: state.appState.scrollToBottom, pending, @@ -35,7 +35,7 @@ AccountsScreen.prototype.render = function () { const props = this.props const { keyrings } = props const identityList = valuesFor(props.identities) - const unconfTxList = valuesFor(props.unconfTxs) + const unapprovedTxList = valuesFor(props.unapprovedTxs) return ( @@ -107,7 +107,7 @@ AccountsScreen.prototype.render = function () { h('hr.horizontal-line'), ]), - unconfTxList.length ? ( + unapprovedTxList.length ? ( h('.unconftx-link.flex-row.flex-center', { onClick: this.navigateToConfTx.bind(this), diff --git a/ui/app/app.js b/ui/app/app.js index d8dedd397..3bc4897c8 100644 --- a/ui/app/app.js +++ b/ui/app/app.js @@ -52,8 +52,8 @@ function mapStateToProps (state) { activeAddress: state.appState.activeAddress, transForward: state.appState.transForward, seedWords: state.metamask.seedWords, - unconfTxs: state.metamask.unconfTxs, - unconfMsgs: state.metamask.unconfMsgs, + unapprovedTxs: state.metamask.unapprovedTxs, + unapprovedMsgs: state.metamask.unapprovedMsgs, menuOpen: state.appState.menuOpen, network: state.metamask.network, provider: state.metamask.provider, diff --git a/ui/app/components/transaction-list-item-icon.js b/ui/app/components/transaction-list-item-icon.js index 353401099..90b4ec094 100644 --- a/ui/app/components/transaction-list-item-icon.js +++ b/ui/app/components/transaction-list-item-icon.js @@ -15,15 +15,9 @@ TransactionIcon.prototype.render = function () { const { transaction, txParams, isMsg } = this.props switch (transaction.status) { case 'unapproved': - return h('.unapproved-tx', { + return h( !isMsg ? '.unapproved-tx-icon' : 'i.fa.fa-certificate.fa-lg', { style: { width: '24px', - height: '24px', - background: '#4dffff', - border: 'solid', - borderColor: '#AEAEAE', - borderWidth: '0.5px', - borderRadius: '13px', }, }) diff --git a/ui/app/components/transaction-list-item.js b/ui/app/components/transaction-list-item.js index 95e850264..44d2dc587 100644 --- a/ui/app/components/transaction-list-item.js +++ b/ui/app/components/transaction-list-item.js @@ -33,7 +33,6 @@ TransactionListItem.prototype.render = function () { var isMsg = ('msgParams' in transaction) var isTx = ('txParams' in transaction) var isPending = transaction.status === 'unapproved' - let txParams if (isTx) { txParams = transaction.txParams diff --git a/ui/app/components/transaction-list.js b/ui/app/components/transaction-list.js index b055ca9d5..3ae953637 100644 --- a/ui/app/components/transaction-list.js +++ b/ui/app/components/transaction-list.js @@ -13,13 +13,13 @@ function TransactionList () { } TransactionList.prototype.render = function () { - const { transactions, network, unconfMsgs } = this.props + const { transactions, network, unapprovedMsgs } = this.props var shapeShiftTxList if (network === '1') { shapeShiftTxList = this.props.shapeShiftTxList } - const txsToRender = !shapeShiftTxList ? transactions.concat(unconfMsgs) : transactions.concat(unconfMsgs, shapeShiftTxList) + const txsToRender = !shapeShiftTxList ? transactions.concat(unapprovedMsgs) : transactions.concat(unapprovedMsgs, shapeShiftTxList) .sort((a, b) => b.time - a.time) return ( diff --git a/ui/app/conf-tx.js b/ui/app/conf-tx.js index 1bd69f7d9..f4fea03e9 100644 --- a/ui/app/conf-tx.js +++ b/ui/app/conf-tx.js @@ -20,8 +20,8 @@ function mapStateToProps (state) { identities: state.metamask.identities, accounts: state.metamask.accounts, selectedAccount: state.metamask.selectedAccount, - unconfTxs: state.metamask.unconfTxs, - unconfMsgs: state.metamask.unconfMsgs, + unapprovedTxs: state.metamask.unapprovedTxs, + unapprovedMsgs: state.metamask.unapprovedMsgs, index: state.appState.currentView.context, warning: state.appState.warning, network: state.metamask.network, @@ -39,10 +39,10 @@ ConfirmTxScreen.prototype.render = function () { var network = state.network var provider = state.provider - var unconfTxs = state.unconfTxs - var unconfMsgs = state.unconfMsgs + var unapprovedTxs = state.unapprovedTxs + var unapprovedMsgs = state.unapprovedMsgs - var unconfTxList = txHelper(unconfTxs, unconfMsgs, network) + var unconfTxList = txHelper(unapprovedTxs, unapprovedMsgs, network) var index = state.index !== undefined && unconfTxList[index] ? state.index : 0 var txData = unconfTxList[index] || {} var txParams = txData.params || {} diff --git a/ui/app/css/index.css b/ui/app/css/index.css index 16e1dbe7e..c3beacef2 100644 --- a/ui/app/css/index.css +++ b/ui/app/css/index.css @@ -408,6 +408,16 @@ input.large-input { .name-label{ } + +.unapproved-tx-icon { + height: 24px; + background: #4dffff; + border: solid; + borderColor: #AEAEAE; + borderWidth: 0.5px; + borderRadius: 13px; +} + .edit-text { height: 100%; visibility: hidden; diff --git a/ui/app/reducers/app.js b/ui/app/reducers/app.js index 6a2c93f78..3b960e4c1 100644 --- a/ui/app/reducers/app.js +++ b/ui/app/reducers/app.js @@ -307,11 +307,11 @@ function reduceApp (state, action) { }) case actions.COMPLETED_TX: - var unconfTxs = state.metamask.unconfTxs - var unconfMsgs = state.metamask.unconfMsgs + var unapprovedTxs = state.metamask.unapprovedTxs + var unapprovedMsgs = state.metamask.unapprovedMsgs var network = state.metamask.network - var unconfTxList = txHelper(unconfTxs, unconfMsgs, network) + var unconfTxList = txHelper(unapprovedTxs, unapprovedMsgs, network) .filter(tx => tx !== tx.id) if (unconfTxList && unconfTxList.length > 0) { @@ -572,18 +572,18 @@ function reduceApp (state, action) { } function hasPendingTxs (state) { - var unconfTxs = state.metamask.unconfTxs - var unconfMsgs = state.metamask.unconfMsgs + var unapprovedTxs = state.metamask.unapprovedTxs + var unapprovedMsgs = state.metamask.unapprovedMsgs var network = state.metamask.network - var unconfTxList = txHelper(unconfTxs, unconfMsgs, network) + var unconfTxList = txHelper(unapprovedTxs, unapprovedMsgs, network) return unconfTxList.length > 0 } function indexForPending (state, txId) { - var unconfTxs = state.metamask.unconfTxs - var unconfMsgs = state.metamask.unconfMsgs + var unapprovedTxs = state.metamask.unapprovedTxs + var unapprovedMsgs = state.metamask.unapprovedMsgs var network = state.metamask.network - var unconfTxList = txHelper(unconfTxs, unconfMsgs, network) + var unconfTxList = txHelper(unapprovedTxs, unapprovedMsgs, network) let idx unconfTxList.forEach((tx, i) => { if (tx.id === txId) { diff --git a/ui/app/reducers/metamask.js b/ui/app/reducers/metamask.js index 8679ab062..5cf249197 100644 --- a/ui/app/reducers/metamask.js +++ b/ui/app/reducers/metamask.js @@ -12,7 +12,7 @@ function reduceMetamask (state, action) { isUnlocked: false, rpcTarget: 'https://rawtestrpc.metamask.io/', identities: {}, - unconfTxs: {}, + unapprovedTxs: {}, currentFiat: 'USD', conversionRate: 0, conversionDate: 'N/A', @@ -76,17 +76,17 @@ function reduceMetamask (state, action) { case actions.COMPLETED_TX: var stringId = String(action.id) newState = extend(metamaskState, { - unconfTxs: {}, - unconfMsgs: {}, + unapprovedTxs: {}, + unapprovedMsgs: {}, }) - for (const id in metamaskState.unconfTxs) { + for (const id in metamaskState.unapprovedTxs) { if (id !== stringId) { - newState.unconfTxs[id] = metamaskState.unconfTxs[id] + newState.unapprovedTxs[id] = metamaskState.unapprovedTxs[id] } } - for (const id in metamaskState.unconfMsgs) { + for (const id in metamaskState.unapprovedMsgs) { if (id !== stringId) { - newState.unconfMsgs[id] = metamaskState.unconfMsgs[id] + newState.unapprovedMsgs[id] = metamaskState.unapprovedMsgs[id] } } return newState diff --git a/ui/example.js b/ui/example.js index 888748c48..4627c0e9c 100644 --- a/ui/example.js +++ b/ui/example.js @@ -29,7 +29,7 @@ var identities = { }, } -var unconfTxs = {} +var unapprovedTxs = {} addUnconfTx({ from: '0x222462427bcc9133bb46e88bcbe39cd7ef0e7222', to: '0x1113462427bcc9133bb46e88bcbe39cd7ef0e111', @@ -45,7 +45,7 @@ addUnconfTx({ function addUnconfTx (txParams) { var time = (new Date()).getTime() var id = createRandomId() - unconfTxs[id] = { + unapprovedTxs[id] = { id: id, txParams: txParams, time: time, @@ -59,7 +59,7 @@ function getState () { return { isUnlocked: isUnlocked, identities: isUnlocked ? identities : {}, - unconfTxs: isUnlocked ? unconfTxs : {}, + unapprovedTxs: isUnlocked ? unapprovedTxs : {}, selectedAccount: selectedAccount, } } diff --git a/ui/index.js b/ui/index.js index dedfd8c8c..8855064f6 100644 --- a/ui/index.js +++ b/ui/index.js @@ -32,8 +32,8 @@ function startApp (metamaskState, accountManager, opts) { }) // if unconfirmed txs, start on txConf page - var unconfirmedTxsAll = txHelper(metamaskState.unconfTxs, metamaskState.unconfMsgs, metamaskState.network) - if (unconfirmedTxsAll.length > 0) { + var unapprovedTxsAll = txHelper(metamaskState.unapprovedTxs, metamaskState.unapprovedMsgs, metamaskState.network) + if (unapprovedTxsAll.length > 0) { store.dispatch(actions.showConfTxPage()) } diff --git a/ui/lib/tx-helper.js b/ui/lib/tx-helper.js index c984bc9af..fa7a94cdc 100644 --- a/ui/lib/tx-helper.js +++ b/ui/lib/tx-helper.js @@ -1,8 +1,8 @@ const valuesFor = require('../app/util').valuesFor -module.exports = function (unconfTxs, unconfMsgs, network) { - var txValues = network ? valuesFor(unconfTxs).filter(tx => tx.txParams.metamaskNetworkId === network) : valuesFor(unconfTxs) - var msgValues = valuesFor(unconfMsgs) +module.exports = function (unapprovedTxs, unapprovedMsgs, network) { + var txValues = network ? valuesFor(unapprovedTxs).filter(tx => tx.txParams.metamaskNetworkId === network) : valuesFor(unapprovedTxs) + var msgValues = valuesFor(unapprovedMsgs) var allValues = txValues.concat(msgValues) return allValues.sort(tx => tx.time) } From 1b16b4624186265ccbb6f8106e1bf9ff997e2528 Mon Sep 17 00:00:00 2001 From: Frankie Date: Wed, 1 Feb 2017 11:54:01 -0800 Subject: [PATCH 3/6] code clan up and tests --- app/scripts/keyring-controller.js | 4 +- app/scripts/lib/message-manager.js | 44 +++++++------- app/scripts/metamask-controller.js | 34 ++++++----- test/unit/message-manager-test.js | 98 ++++++++++++++++++++++++++++++ ui/app/css/index.css | 6 +- 5 files changed, 141 insertions(+), 45 deletions(-) create mode 100644 test/unit/message-manager-test.js diff --git a/app/scripts/keyring-controller.js b/app/scripts/keyring-controller.js index 5fbd731f3..6b8260a28 100644 --- a/app/scripts/keyring-controller.js +++ b/app/scripts/keyring-controller.js @@ -6,7 +6,6 @@ const ObservableStore = require('obs-store') const filter = require('promise-filter') const encryptor = require('browser-passworder') const normalizeAddress = require('./lib/sig-util').normalize -function noop () {} // Keyrings: const SimpleKeyring = require('./keyrings/simple') const HdKeyring = require('./keyrings/hd') @@ -89,7 +88,6 @@ class KeyringController extends EventEmitter { currentFiat: this.configManager.getCurrentFiat(), conversionRate: this.configManager.getConversionRate(), conversionDate: this.configManager.getConversionDate(), - // messageManager } }) } @@ -319,7 +317,7 @@ class KeyringController extends EventEmitter { // // Attempts to sign the provided @object msgParams. signMessage (msgParams) { - const address = normalize(msgParams.from) + const address = normalizeAddress(msgParams.from) return this.getKeyringForAccount(address) .then((keyring) => { return keyring.signMessage(address, msgParams.data) diff --git a/app/scripts/lib/message-manager.js b/app/scripts/lib/message-manager.js index bc9a9e6c8..490cd4d1c 100644 --- a/app/scripts/lib/message-manager.js +++ b/app/scripts/lib/message-manager.js @@ -11,7 +11,7 @@ module.exports = class MessageManager extends EventEmitter{ getState() { return { - unapprovedMsgs: this.unapprovedMsgs(), + unapprovedMsgs: this.getUnapprovedMsgs(), messages: this.getMsgList(), } } @@ -21,22 +21,15 @@ module.exports = class MessageManager extends EventEmitter{ } get unapprovedMsgCount () { - return Object.keys(this.unapprovedMsgs()).length + return Object.keys(this.getUnapprovedMsgs()).length } - unapprovedMsgs () { + getUnapprovedMsgs () { let messages = this.getMsgList() return messages.filter(msg => msg.status === 'unapproved') .reduce((result, msg) => { result[msg.id] = msg; return result }, {}) } - _saveMsgList (msgList) { - this.emit('updateBadge') - let state = this.memStore.getState() - state.messages = msgList - this.memStore.putState(state) - } - addUnapprovedMessage (msgParams) { // create txData obj with parameters and meta data var time = (new Date()).getTime() @@ -70,34 +63,30 @@ module.exports = class MessageManager extends EventEmitter{ return matching.length > 0 ? matching[0] : null } - brodcastMessage (rawSig, msgId, status) { - this.emit(`${msgId}:finished`, {status, rawSig}) - } - approveMessage (msgParams) { - this.setMessageApproved(msgParams.metamaskId) + this.setMsgStatusApproved(msgParams.metamaskId) return this.prepMsgForSigning(msgParams) } - setMessageApproved (msgId) { + setMsgStatusApproved (msgId) { this._setMsgStatus(msgId, 'approved') } + prepMsgForSigning (msgParams) { delete msgParams.metamaskId return Promise.resolve(msgParams) } - cancelMessage (msgId) { - // reject tx - // clean up - this.brodcastMessage(null, msgId, 'rejected') - this.rejectMsg(msgId) - } - rejectMsg (msgId) { + this.brodcastMessage(null, msgId, 'rejected') this._setMsgStatus(msgId, 'rejected') } + brodcastMessage (rawSig, msgId, status) { + this.emit(`${msgId}:finished`, {status, rawSig}) + } +// PRIVATE METHODS + _setMsgStatus (msgId, status) { let msg = this.getMsg(msgId) if (msg) msg.status = status @@ -112,4 +101,13 @@ module.exports = class MessageManager extends EventEmitter{ } this._saveMsgList(messages) } + + _saveMsgList (msgList) { + this.emit('updateBadge') + let state = this.memStore.getState() + state.messages = msgList + this.memStore.putState(state) + } + + } diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index bab005af2..38358f04b 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -224,7 +224,7 @@ module.exports = class MetamaskController extends EventEmitter { approveTransaction: txManager.approveTransaction.bind(txManager), cancelTransaction: txManager.cancelTransaction.bind(txManager), signMessage: this.signMessage.bind(this), - cancelMessage: messageManager.cancelMessage.bind(messageManager), + cancelMessage: messageManager.rejectMsg.bind(messageManager), // notices checkNotices: noticeController.updateNoticesList.bind(noticeController), @@ -369,31 +369,33 @@ module.exports = class MetamaskController extends EventEmitter { } newUnsignedMessage (msgParams, cb) { - this.keyringController.getState() - .then((state) => { - let msgId = this.messageManager.addUnapprovedMessage(msgParams) - this.sendUpdate() - state.isUnlocked ? this.opts.unlockAccountMessage() : this.opts.showUnconfirmedMessage() - this.messageManager.once(`${msgId}:finished`, (data) => { - switch (data.status) { - case 'approved': - return cb(null, data.rawSig) - case 'rejected': - return cb(new Error('MetaMask Tx Signature: User denied transaction signature.')) - default: - return cb(new Error(`MetaMask Tx Signature: Unknown problem: ${JSON.stringify(msgParams)}`)) - } - }) + let msgId = this.messageManager.addUnapprovedMessage(msgParams) + this.sendUpdate() + this.opts.showUnconfirmedMessage() + this.messageManager.once(`${msgId}:finished`, (data) => { + switch (data.status) { + case 'approved': + return cb(null, data.rawSig) + case 'rejected': + return cb(new Error('MetaMask Message Signature: User denied transaction signature.')) + default: + return cb(new Error(`MetaMask Message Signature: Unknown problem: ${JSON.stringify(msgParams)}`)) + } }) } signMessage (msgParams, cb) { const msgId = msgParams.metamaskId + // sets the status op the message to 'approved' + // and removes the metamaskId for signing return this.messageManager.approveMessage(msgParams) .then((cleanMsgParams) => { + // signs the message return this.keyringController.signMessage(cleanMsgParams) }) .then((rawSig) => { + // tells the listener that the message has been signed + // and can be returned to the dapp this.messageManager.brodcastMessage(rawSig, msgId, 'approved') }).then(() => { cb() diff --git a/test/unit/message-manager-test.js b/test/unit/message-manager-test.js new file mode 100644 index 000000000..68b977058 --- /dev/null +++ b/test/unit/message-manager-test.js @@ -0,0 +1,98 @@ +const assert = require('assert') +const extend = require('xtend') +const EventEmitter = require('events') + +const MessageManger = require('../../app/scripts/lib/message-manager') + +describe('Transaction Manager', function() { + let messageManager + + beforeEach(function() { + messageManager = new MessageManger () + }) + + describe('#getMsgList', function() { + it('when new should return empty array', function() { + var result = messageManager.getMsgList() + assert.ok(Array.isArray(result)) + assert.equal(result.length, 0) + }) + it('should also return transactions from local storage if any', function() { + + }) + }) + + describe('#_saveMsgList', function() { + it('saves the submitted data to the Msg list', function() { + var target = [{ foo: 'bar', metamaskNetworkId: 'unit test' }] + messageManager._saveMsgList(target) + var result = messageManager.getMsgList() + assert.equal(result[0].foo, 'bar') + }) + }) + + describe('#addMsg', function() { + it('adds a Msg returned in getMsgList', function() { + var Msg = { id: 1, status: 'approved', metamaskNetworkId: 'unit test' } + messageManager.addMsg(Msg) + var result = messageManager.getMsgList() + assert.ok(Array.isArray(result)) + assert.equal(result.length, 1) + assert.equal(result[0].id, 1) + }) + }) + + describe('#setMsgStatusApproved', function() { + it('sets the Msg status to approved', function() { + var Msg = { id: 1, status: 'unapproved', metamaskNetworkId: 'unit test' } + messageManager.addMsg(Msg) + messageManager.setMsgStatusApproved(1) + var result = messageManager.getMsgList() + assert.ok(Array.isArray(result)) + assert.equal(result.length, 1) + assert.equal(result[0].status, 'approved') + }) + }) + + describe('#rejectMsg', function() { + it('sets the Msg status to rejected', function() { + var Msg = { id: 1, status: 'unapproved', metamaskNetworkId: 'unit test' } + messageManager.addMsg(Msg) + messageManager.rejectMsg(1) + var result = messageManager.getMsgList() + assert.ok(Array.isArray(result)) + assert.equal(result.length, 1) + assert.equal(result[0].status, 'rejected') + }) + }) + + describe('#_updateMsg', function() { + it('replaces the Msg with the same id', function() { + messageManager.addMsg({ id: '1', status: 'unapproved', metamaskNetworkId: 'unit test' }) + messageManager.addMsg({ id: '2', status: 'approved', metamaskNetworkId: 'unit test' }) + messageManager._updateMsg({ id: '1', status: 'blah', hash: 'foo', metamaskNetworkId: 'unit test' }) + var result = messageManager.getMsg('1') + assert.equal(result.hash, 'foo') + }) + }) + + describe('#getUnapprovedMsgs', function() { + it('returns unapproved Msgs in a hash', function() { + messageManager.addMsg({ id: '1', status: 'unapproved', metamaskNetworkId: 'unit test' }) + messageManager.addMsg({ id: '2', status: 'approved', metamaskNetworkId: 'unit test' }) + let result = messageManager.getUnapprovedMsgs() + assert.equal(typeof result, 'object') + assert.equal(result['1'].status, 'unapproved') + assert.equal(result['2'], undefined) + }) + }) + + describe('#getMsg', function() { + it('returns a Msg with the requested id', function() { + messageManager.addMsg({ id: '1', status: 'unapproved', metamaskNetworkId: 'unit test' }) + messageManager.addMsg({ id: '2', status: 'approved', metamaskNetworkId: 'unit test' }) + assert.equal(messageManager.getMsg('1').status, 'unapproved') + assert.equal(messageManager.getMsg('2').status, 'approved') + }) + }) +}) diff --git a/ui/app/css/index.css b/ui/app/css/index.css index c3beacef2..4b9b5b67d 100644 --- a/ui/app/css/index.css +++ b/ui/app/css/index.css @@ -413,9 +413,9 @@ input.large-input { height: 24px; background: #4dffff; border: solid; - borderColor: #AEAEAE; - borderWidth: 0.5px; - borderRadius: 13px; + border-color: #AEAEAE; + border-width: 0.5px; + border-radius: 13px; } .edit-text { From e2e8a7cca06f7e6f41f417d86710227b0b5428d6 Mon Sep 17 00:00:00 2001 From: Frankie Date: Wed, 1 Feb 2017 11:55:26 -0800 Subject: [PATCH 4/6] Add to CHANGELOG --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ec4e0afe5..65184e02b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ## Current Master +- Fix unapproved messages not being included in extension badge. - Fix rendering bug where the Confirm transaction view would lets you approve transactions when the account has insufficient balance. - Add ability to import accounts in JSON file format (used by Mist, Geth, MyEtherWallet, and more!) From a96f892788d715f7fa8995f2b6bd59f42116385c Mon Sep 17 00:00:00 2001 From: Frankie Date: Wed, 1 Feb 2017 13:25:36 -0800 Subject: [PATCH 5/6] Fix messy merge --- app/scripts/keyring-controller.js | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/app/scripts/keyring-controller.js b/app/scripts/keyring-controller.js index 12e3d2844..e2752831a 100644 --- a/app/scripts/keyring-controller.js +++ b/app/scripts/keyring-controller.js @@ -37,7 +37,6 @@ class KeyringController extends EventEmitter { this.ethStore = opts.ethStore this.encryptor = encryptor this.keyrings = [] - this.identities = {} // Essentially a name hash this.getNetwork = opts.getNetwork } @@ -144,17 +143,6 @@ class KeyringController extends EventEmitter { .then(this.fullUpdate.bind(this)) } - // ClearSeedWordCache - // - // returns Promise( @string currentSelectedAccount ) - // - // Removes the current vault's seed words from the UI's state tree, - // ensuring they are only ever available in the background process. - clearSeedWordCache () { - this.configManager.setSeedWords(null) - return Promise.resolve(this.configManager.getSelectedAccount()) - } - // Set Locked // returns Promise( @object state ) // @@ -206,8 +194,8 @@ class KeyringController extends EventEmitter { this.keyrings.push(keyring) return this.setupAccounts(accounts) }) - .then(() => { return this.password }) - .then(this.persistAllKeyrings.bind(this)) + .then(() => this.persistAllKeyrings()) + .then(() => this.fullUpdate()) .then(() => { return keyring }) From c7b9adbfcbaa51082f193581b80bec4f86996f81 Mon Sep 17 00:00:00 2001 From: Frankie Date: Wed, 1 Feb 2017 15:00:41 -0800 Subject: [PATCH 6/6] swap out set state for updateState --- app/scripts/lib/message-manager.js | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/app/scripts/lib/message-manager.js b/app/scripts/lib/message-manager.js index 490cd4d1c..4a0017342 100644 --- a/app/scripts/lib/message-manager.js +++ b/app/scripts/lib/message-manager.js @@ -103,10 +103,7 @@ module.exports = class MessageManager extends EventEmitter{ } _saveMsgList (msgList) { - this.emit('updateBadge') - let state = this.memStore.getState() - state.messages = msgList - this.memStore.putState(state) + this.memStore.updateState({ messages: msgList }) }