From f41a6252290c7909feb1a1e43fc2277d35b8675b Mon Sep 17 00:00:00 2001 From: martahj Date: Tue, 8 Oct 2024 14:27:51 -0500 Subject: [PATCH] fix: allow getAddTransactionRequest to pass through other params (#27117) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** Updates `getAddTransactionRequest` to pass through additional parameters so that `waitForSubmit` will get passed through to `addTransaction` . Previously, the `waitForSubmit` param, although passed by `addTransaction` and `addTransactionAndWaitForPublish`, was not included in the object returned by `getAddTransactionRequest`. This doesn't seem to have had negative consequences on a standard wallet, but when using a hardware wallet, it meant that `addTransactionAndWaitForPublish` resolved before waiting for the user to accept or reject the transaction. As a consequence, when doing a swap, the user was taken directly to the "Processing..." screen before they had a chance to take action on the transaction. If they rejected the transaction, they remained on that screen indefinitely. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/27117?quickstart=1) ## **Related issues** Fixes: https://consensyssoftware.atlassian.net/browse/MMS-1189 ## **Manual testing steps** 1. Disable smart transactions 2. Confirm that the following flows work and display relevant and expected screens using both a hardware and standard wallet: * Swap that does not need approval step - accept tx * Swap that does not need approval step - reject tx * Swap that needs approval step - reject approval tx * Swap that needs approval step - accept approval tx, reject trade tx * Swap that needs approval step - accept both txs * Send + swap that does not need approval step - accept tx * Send + swap that does not need approval step - reject tx * Send + swap that needs approval step - reject approval tx * Send + swap that needs approval step - accept approval tx, reject trade tx * Send + swap that needs approval step - accept both txs ## **Screenshots/Recordings** ### **Before** Video from bug report (shows rejecting swap flow): https://github.com/user-attachments/assets/bdaa32f1-2c1d-4e23-a97d-9d370baaaf2f ### **After** Accepting swap: https://github.com/user-attachments/assets/bbe99cb3-fa3f-4580-a472-20b3b4a94f31 Rejecting swap: https://github.com/user-attachments/assets/e6ecd2dc-a1a0-4d51-8428-45d19d3a269e ## **Pre-merge author checklist** - [X] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [X] I've completed the PR template to the best of my ability - [X] I’ve included tests if applicable - [X] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md )). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --------- Co-authored-by: Derek Brans --- app/scripts/metamask-controller.js | 2 ++ app/scripts/metamask-controller.test.js | 45 +++++++++++++++++++++++++ 2 files changed, 47 insertions(+) diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index f8f93cbaa5b3..a5b110fadec2 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -4950,6 +4950,7 @@ export default class MetamaskController extends EventEmitter { transactionParams, transactionOptions, dappRequest, + ...otherParams }) { return { internalAccounts: this.accountsController.listAccounts(), @@ -4969,6 +4970,7 @@ export default class MetamaskController extends EventEmitter { securityAlertsEnabled: this.preferencesController.store.getState()?.securityAlertsEnabled, updateSecurityAlertResponse: this.updateSecurityAlertResponse.bind(this), + ...otherParams, }; } diff --git a/app/scripts/metamask-controller.test.js b/app/scripts/metamask-controller.test.js index d1da34c48e0e..bab66d9bc515 100644 --- a/app/scripts/metamask-controller.test.js +++ b/app/scripts/metamask-controller.test.js @@ -517,6 +517,51 @@ describe('MetaMaskController', () => { }); }); + describe('#getAddTransactionRequest', () => { + it('formats the transaction for submission', () => { + const transactionParams = { from: '0xa', to: '0xb' }; + const transactionOptions = { foo: true }; + const result = metamaskController.getAddTransactionRequest({ + transactionParams, + transactionOptions, + }); + expect(result).toStrictEqual({ + internalAccounts: + metamaskController.accountsController.listAccounts(), + dappRequest: undefined, + networkClientId: + metamaskController.networkController.state.selectedNetworkClientId, + selectedAccount: + metamaskController.accountsController.getAccountByAddress( + transactionParams.from, + ), + transactionController: expect.any(Object), + transactionOptions, + transactionParams, + userOperationController: expect.any(Object), + chainId: '0x1', + ppomController: expect.any(Object), + securityAlertsEnabled: expect.any(Boolean), + updateSecurityAlertResponse: expect.any(Function), + }); + }); + it('passes through any additional params to the object', () => { + const transactionParams = { from: '0xa', to: '0xb' }; + const transactionOptions = { foo: true }; + const result = metamaskController.getAddTransactionRequest({ + transactionParams, + transactionOptions, + test: '123', + }); + + expect(result).toMatchObject({ + transactionParams, + transactionOptions, + test: '123', + }); + }); + }); + describe('submitPassword', () => { it('removes any identities that do not correspond to known accounts.', async () => { const fakeAddress = '0xbad0';