From bb0dff94434444a6c1043b5d2c835f29c9865d09 Mon Sep 17 00:00:00 2001 From: Matthew Walsh Date: Tue, 11 Apr 2023 14:18:43 +0100 Subject: [PATCH] Trigger transaction popup using ApprovalController (#18400) --- app/scripts/background.js | 2 - app/scripts/controllers/transactions/index.js | 60 +++++++- .../controllers/transactions/index.test.js | 135 ++++++++++++++++-- app/scripts/metamask-controller.js | 9 +- shared/constants/app.ts | 1 + test/data/mock-state.json | 13 +- ui/selectors/selectors.js | 7 - 7 files changed, 200 insertions(+), 27 deletions(-) diff --git a/app/scripts/background.js b/app/scripts/background.js index d92fddb81..1f73492cc 100644 --- a/app/scripts/background.js +++ b/app/scripts/background.js @@ -727,7 +727,6 @@ export function setupController( } function getUnapprovedTransactionCount() { - const unapprovedTxCount = controller.txController.getUnapprovedTxCount(); const { unapprovedDecryptMsgCount } = controller.decryptMessageManager; const { unapprovedEncryptionPublicKeyMsgCount } = controller.encryptionPublicKeyManager; @@ -736,7 +735,6 @@ export function setupController( const waitingForUnlockCount = controller.appStateController.waitingForUnlock.length; return ( - unapprovedTxCount + unapprovedDecryptMsgCount + unapprovedEncryptionPublicKeyMsgCount + pendingApprovalCount + diff --git a/app/scripts/controllers/transactions/index.js b/app/scripts/controllers/transactions/index.js index c3026e043..ce5e85d0d 100644 --- a/app/scripts/controllers/transactions/index.js +++ b/app/scripts/controllers/transactions/index.js @@ -52,7 +52,10 @@ import { determineTransactionType, isEIP1559Transaction, } from '../../../../shared/modules/transaction.utils'; -import { ORIGIN_METAMASK } from '../../../../shared/constants/app'; +import { + ORIGIN_METAMASK, + MESSAGE_TYPE, +} from '../../../../shared/constants/app'; import { calcGasTotal, getSwapsTokensReceivedFromTxMeta, @@ -156,6 +159,7 @@ export default class TransactionController extends EventEmitter { this.getAccountType = opts.getAccountType; this.getTokenStandardAndDetails = opts.getTokenStandardAndDetails; this.securityProviderRequest = opts.securityProviderRequest; + this.messagingSystem = opts.messenger; this.memStore = new ObservableStore({}); @@ -798,6 +802,7 @@ export default class TransactionController extends EventEmitter { this.txStateManager.getTransactionWithActionId(actionId); if (existingTxMeta) { this.emit('newUnapprovedTx', existingTxMeta); + this._requestApproval(existingTxMeta); existingTxMeta = await this.addTransactionGasDefaults(existingTxMeta); return existingTxMeta; } @@ -870,6 +875,7 @@ export default class TransactionController extends EventEmitter { this.addTransaction(txMeta); this.emit('newUnapprovedTx', txMeta); + this._requestApproval(txMeta); txMeta = await this.addTransactionGasDefaults(txMeta); @@ -1355,6 +1361,7 @@ export default class TransactionController extends EventEmitter { try { // approve this.txStateManager.setTxStatusApproved(txId); + this._acceptApproval(txMeta); // get next nonce const fromAddress = txMeta.txParams.from; // wait for a nonce @@ -1734,6 +1741,7 @@ export default class TransactionController extends EventEmitter { async cancelTransaction(txId, actionId) { const txMeta = this.txStateManager.getTransaction(txId); this.txStateManager.setTxStatusRejected(txId); + this._rejectApproval(txMeta); this._trackTransactionMetricsEvent( txMeta, TransactionMetaMetricsEvent.rejected, @@ -2596,4 +2604,54 @@ export default class TransactionController extends EventEmitter { }, ); } + + _requestApproval(txMeta) { + const id = this._getApprovalId(txMeta); + const { origin } = txMeta; + const type = MESSAGE_TYPE.TRANSACTION; + const requestData = { txId: txMeta.id }; + + this.messagingSystem + .call( + 'ApprovalController:addRequest', + { + id, + origin, + type, + requestData, + }, + true, + ) + .catch(() => { + // Intentionally ignored as promise not currently used + }); + } + + _acceptApproval(txMeta) { + const id = this._getApprovalId(txMeta); + + try { + this.messagingSystem.call('ApprovalController:acceptRequest', id); + } catch (error) { + log.error('Failed to accept transaction approval request', error); + } + } + + _rejectApproval(txMeta) { + const id = this._getApprovalId(txMeta); + + try { + this.messagingSystem.call( + 'ApprovalController:rejectRequest', + id, + new Error('Rejected'), + ); + } catch (error) { + log.error('Failed to reject transaction approval request', error); + } + } + + _getApprovalId(txMeta) { + return String(txMeta.id); + } } diff --git a/app/scripts/controllers/transactions/index.test.js b/app/scripts/controllers/transactions/index.test.js index c16f1fa2c..601b1fb88 100644 --- a/app/scripts/controllers/transactions/index.test.js +++ b/app/scripts/controllers/transactions/index.test.js @@ -29,7 +29,10 @@ import { GasRecommendations, } from '../../../../shared/constants/gas'; import { METAMASK_CONTROLLER_EVENTS } from '../../metamask-controller'; -import { ORIGIN_METAMASK } from '../../../../shared/constants/app'; +import { + MESSAGE_TYPE, + ORIGIN_METAMASK, +} from '../../../../shared/constants/app'; import { NetworkStatus } from '../../../../shared/constants/network'; import { TRANSACTION_ENVELOPE_TYPE_NAMES } from '../../../../shared/lib/transactions-controller-utils'; import TransactionController from '.'; @@ -52,7 +55,8 @@ describe('Transaction Controller', function () { fromAccount, fragmentExists, networkStatusStore, - getCurrentChainId; + getCurrentChainId, + messengerMock; beforeEach(function () { fragmentExists = false; @@ -76,6 +80,7 @@ describe('Transaction Controller', function () { blockTrackerStub.getLatestBlock = noop; getCurrentChainId = sinon.stub().callsFake(() => currentChainId); + messengerMock = { call: sinon.stub().returns(Promise.resolve()) }; txController = new TransactionController({ provider, @@ -108,6 +113,7 @@ describe('Transaction Controller', function () { getAccountType: () => 'MetaMask', getDeviceModel: () => 'N/A', securityProviderRequest: () => undefined, + messenger: messengerMock, }); txController.nonceTracker.getNonceLock = () => Promise.resolve({ nextNonce: 0, releaseLock: noop }); @@ -489,6 +495,67 @@ describe('Transaction Controller', function () { { message: 'MetaMask is having trouble connecting to the network' }, ); }); + + it('should create an approval request', async function () { + const txMeta = await txController.addUnapprovedTransaction( + undefined, + { + from: selectedAddress, + to: recipientAddress, + }, + ORIGIN_METAMASK, + ); + + assert.equal(messengerMock.call.callCount, 1); + assert.deepEqual(messengerMock.call.getCall(0).args, [ + 'ApprovalController:addRequest', + { + id: String(txMeta.id), + origin: ORIGIN_METAMASK, + requestData: { txId: txMeta.id }, + type: MESSAGE_TYPE.TRANSACTION, + }, + true, // Show popup + ]); + }); + + it('should still create an approval request when called twice with same actionId', async function () { + await txController.addUnapprovedTransaction( + undefined, + { + from: selectedAddress, + to: recipientAddress, + }, + ORIGIN_METAMASK, + undefined, + undefined, + '12345', + ); + + const secondTxMeta = await txController.addUnapprovedTransaction( + undefined, + { + from: selectedAddress, + to: recipientAddress, + }, + undefined, + undefined, + undefined, + '12345', + ); + + assert.equal(messengerMock.call.callCount, 2); + assert.deepEqual(messengerMock.call.getCall(1).args, [ + 'ApprovalController:addRequest', + { + id: String(secondTxMeta.id), + origin: ORIGIN_METAMASK, + requestData: { txId: secondTxMeta.id }, + type: MESSAGE_TYPE.TRANSACTION, + }, + true, // Show popup + ]); + }); }); describe('#createCancelTransaction', function () { @@ -997,9 +1064,11 @@ describe('Transaction Controller', function () { }); describe('#approveTransaction', function () { - it('does not overwrite set values', async function () { - const originalValue = '0x01'; - const txMeta = { + let originalValue, txMeta, signStub, pubStub; + + beforeEach(function () { + originalValue = '0x01'; + txMeta = { id: '1', status: TransactionStatus.unapproved, metamaskNetworkId: currentNetworkId, @@ -1019,17 +1088,22 @@ describe('Transaction Controller', function () { providerResultStub.eth_gasPrice = wrongValue; providerResultStub.eth_estimateGas = '0x5209'; - const signStub = sinon + signStub = sinon .stub(txController, 'signTransaction') .callsFake(() => Promise.resolve()); - const pubStub = sinon - .stub(txController, 'publishTransaction') - .callsFake(() => { - txController.setTxHash('1', originalValue); - txController.txStateManager.setTxStatusSubmitted('1'); - }); + pubStub = sinon.stub(txController, 'publishTransaction').callsFake(() => { + txController.setTxHash('1', originalValue); + txController.txStateManager.setTxStatusSubmitted('1'); + }); + }); + afterEach(function () { + signStub.restore(); + pubStub.restore(); + }); + + it('does not overwrite set values', async function () { await txController.approveTransaction(txMeta.id); const result = txController.txStateManager.getTransaction(txMeta.id); const params = result.txParams; @@ -1042,8 +1116,21 @@ describe('Transaction Controller', function () { TransactionStatus.submitted, 'should have reached the submitted status.', ); - signStub.restore(); - pubStub.restore(); + }); + + it('should accept the approval request', async function () { + await txController.approveTransaction(txMeta.id); + + assert.equal(messengerMock.call.callCount, 1); + assert.deepEqual(messengerMock.call.getCall(0).args, [ + 'ApprovalController:acceptRequest', + txMeta.id, + ]); + }); + + it('should not throw if accepting approval request throws', async function () { + messengerMock.call.throws(); + await txController.approveTransaction(txMeta.id); }); }); @@ -1108,7 +1195,7 @@ describe('Transaction Controller', function () { }); describe('#cancelTransaction', function () { - it('should emit a status change to rejected', function (done) { + beforeEach(function () { txController.txStateManager._addTransactionsToState([ { id: 0, @@ -1181,7 +1268,9 @@ describe('Transaction Controller', function () { history: [{}], }, ]); + }); + it('should emit a status change to rejected', function (done) { txController.once('tx:status-update', (txId, status) => { try { assert.equal( @@ -1198,6 +1287,22 @@ describe('Transaction Controller', function () { txController.cancelTransaction(0); }); + + it('should reject the approval request', function () { + txController.cancelTransaction(0); + + assert.equal(messengerMock.call.callCount, 1); + assert.deepEqual(messengerMock.call.getCall(0).args, [ + 'ApprovalController:rejectRequest', + '0', + new Error('Rejected'), + ]); + }); + + it('should not throw if rejecting approval request throws', async function () { + messengerMock.call.throws(); + txController.cancelTransaction(0); + }); }); describe('#createSpeedUpTransaction', function () { diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 28a7311c0..c1d8216a0 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -1007,8 +1007,15 @@ export default class MetamaskController extends EventEmitter { getDeviceModel: this.getDeviceModel.bind(this), getTokenStandardAndDetails: this.getTokenStandardAndDetails.bind(this), securityProviderRequest: this.securityProviderRequest.bind(this), + messenger: this.controllerMessenger.getRestricted({ + name: 'TransactionController', + allowedActions: [ + `${this.approvalController.name}:addRequest`, + `${this.approvalController.name}:acceptRequest`, + `${this.approvalController.name}:rejectRequest`, + ], + }), }); - this.txController.on('newUnapprovedTx', () => opts.showUserConfirmation()); this.txController.on(`tx:status-update`, async (txId, status) => { if ( diff --git a/shared/constants/app.ts b/shared/constants/app.ts index 54a39496a..c8be0573e 100644 --- a/shared/constants/app.ts +++ b/shared/constants/app.ts @@ -53,6 +53,7 @@ export const MESSAGE_TYPE = { PERSONAL_SIGN: 'personal_sign', SEND_METADATA: 'metamask_sendDomainMetadata', SWITCH_ETHEREUM_CHAIN: 'wallet_switchEthereumChain', + TRANSACTION: 'transaction', WALLET_REQUEST_PERMISSIONS: 'wallet_requestPermissions', WATCH_ASSET: 'wallet_watchAsset', WATCH_ASSET_LEGACY: 'metamask_watchAsset', diff --git a/test/data/mock-state.json b/test/data/mock-state.json index 93d4515f4..41d995b9e 100644 --- a/test/data/mock-state.json +++ b/test/data/mock-state.json @@ -1535,7 +1535,18 @@ "origin": "tmashuang.github.io" } ], - "desktopEnabled": false + "desktopEnabled": false, + "pendingApprovals": { + "testApprovalId": { + "id": "testApprovalId", + "time": 1528133319641, + "origin": "metamask", + "type": "transaction", + "requestData": { "txId": "testTransactionId" }, + "requestState": { "test": "value" } + } + }, + "pendingApprovalCount": 1 }, "send": { "amountMode": "INPUT", diff --git a/ui/selectors/selectors.js b/ui/selectors/selectors.js index bbe5e53e8..b4e04c7f6 100644 --- a/ui/selectors/selectors.js +++ b/ui/selectors/selectors.js @@ -484,21 +484,14 @@ export function getCurrentCurrency(state) { export function getTotalUnapprovedCount(state) { const { - unapprovedMsgCount = 0, - unapprovedPersonalMsgCount = 0, unapprovedDecryptMsgCount = 0, unapprovedEncryptionPublicKeyMsgCount = 0, - unapprovedTypedMessagesCount = 0, pendingApprovalCount = 0, } = state.metamask; return ( - unapprovedMsgCount + - unapprovedPersonalMsgCount + unapprovedDecryptMsgCount + unapprovedEncryptionPublicKeyMsgCount + - unapprovedTypedMessagesCount + - getUnapprovedTxCount(state) + pendingApprovalCount + getSuggestedAssetCount(state) );