Skip to content

Commit

Permalink
fix: allow getAddTransactionRequest to pass through other params (#27117
Browse files Browse the repository at this point in the history
)

## **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 <[email protected]>
  • Loading branch information
martahj and dbrans authored Oct 8, 2024
1 parent 83d5331 commit f41a625
Show file tree
Hide file tree
Showing 2 changed files with 47 additions and 0 deletions.
2 changes: 2 additions & 0 deletions app/scripts/metamask-controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -4950,6 +4950,7 @@ export default class MetamaskController extends EventEmitter {
transactionParams,
transactionOptions,
dappRequest,
...otherParams
}) {
return {
internalAccounts: this.accountsController.listAccounts(),
Expand All @@ -4969,6 +4970,7 @@ export default class MetamaskController extends EventEmitter {
securityAlertsEnabled:
this.preferencesController.store.getState()?.securityAlertsEnabled,
updateSecurityAlertResponse: this.updateSecurityAlertResponse.bind(this),
...otherParams,
};
}

Expand Down
45 changes: 45 additions & 0 deletions app/scripts/metamask-controller.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down

0 comments on commit f41a625

Please sign in to comment.