From f5a34375e124c06501549dce1a649419b066ae90 Mon Sep 17 00:00:00 2001 From: Norbert Elter <72046715+itsyoboieltr@users.noreply.github.com> Date: Wed, 19 Jun 2024 17:01:48 +0200 Subject: [PATCH 1/3] feat: Convert Swaps test from mocha to jest (#25297) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/25297?quickstart=1) This PR converts `app/scripts/controllers/swaps.test.js` from mocha to jest. In particular, replaces instances of `assert` with `expect`, and removes `sinon` and uses `jest`'s mocking capabilities in its place. The `jest.config.js` was updated to include `app/scripts/controllers/swaps.test.js` in the `testMatch` array. The `.mocharc.js` was updated to ignore the test. ## **Related issues** Fixes: #19355 ## **Manual testing steps** 1. Running `yarn jest -- app/scripts/controllers/swaps.test.js` and seeing all tests pass 2. Running `yarn test:unit:mocha` and not seeing the metametrics tests run. ## **Screenshots/Recordings** Not applicable ## **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. --- .eslintrc.js | 2 + .mocharc.js | 1 + app/scripts/controllers/swaps.test.js | 441 +++++++++++++------------- jest.config.js | 1 + 4 files changed, 217 insertions(+), 228 deletions(-) diff --git a/.eslintrc.js b/.eslintrc.js index 3edfd2469d3f..03460a7ccaf0 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -269,6 +269,7 @@ module.exports = { excludedFiles: [ 'app/scripts/controllers/app-state.test.js', 'app/scripts/controllers/mmi-controller.test.js', + 'app/scripts/controllers/swaps.test.js', 'app/scripts/controllers/metametrics.test.js', 'app/scripts/controllers/permissions/**/*.test.js', 'app/scripts/controllers/preferences.test.js', @@ -301,6 +302,7 @@ module.exports = { '**/__snapshots__/*.snap', 'app/scripts/controllers/app-state.test.js', 'app/scripts/controllers/mmi-controller.test.ts', + 'app/scripts/controllers/swaps.test.js', 'app/scripts/controllers/metametrics.test.js', 'app/scripts/controllers/permissions/**/*.test.js', 'app/scripts/controllers/preferences.test.js', diff --git a/.mocharc.js b/.mocharc.js index fc5b45ce387c..06a42220564c 100644 --- a/.mocharc.js +++ b/.mocharc.js @@ -8,6 +8,7 @@ module.exports = { './app/scripts/controllers/app-state.test.js', './app/scripts/controllers/permissions/**/*.test.js', './app/scripts/controllers/mmi-controller.test.ts', + './app/scripts/controllers/swaps.test.js', './app/scripts/controllers/metametrics.test.js', './app/scripts/controllers/preferences.test.js', './app/scripts/constants/error-utils.test.js', diff --git a/app/scripts/controllers/swaps.test.js b/app/scripts/controllers/swaps.test.js index 1389a4b59a55..670c9ed0cf4a 100644 --- a/app/scripts/controllers/swaps.test.js +++ b/app/scripts/controllers/swaps.test.js @@ -1,6 +1,3 @@ -import { strict as assert } from 'assert'; -import sinon from 'sinon'; - import { BigNumber } from '@ethersproject/bignumber'; import { mapValues } from 'lodash'; import BigNumberjs from 'bignumber.js'; @@ -127,21 +124,15 @@ const EMPTY_INIT_STATE = { }, }; -const sandbox = sinon.createSandbox(); -let fetchTradesInfoStub = sandbox.stub(); -const getCurrentChainIdStub = sandbox.stub(); -const getLayer1GasFeeStub = sandbox.stub(); -const getNetworkClientIdStub = sandbox.stub(); -getCurrentChainIdStub.returns(CHAIN_IDS.MAINNET); -getNetworkClientIdStub.returns('1'); -getLayer1GasFeeStub.resolves('0x1'); -const getEIP1559GasFeeEstimatesStub = sandbox.stub(() => { - return { - gasFeeEstimates: { - high: '150', - }, - gasEstimateType: GasEstimateTypes.legacy, - }; +const fetchTradesInfoStub = jest.fn(); +const getCurrentChainIdStub = jest.fn().mockReturnValue(CHAIN_IDS.MAINNET); +const getLayer1GasFeeStub = jest.fn().mockReturnValue('0x1'); +const getNetworkClientIdStub = jest.fn().mockReturnValue('1'); +const getEIP1559GasFeeEstimatesStub = jest.fn().mockReturnValue({ + gasFeeEstimates: { + high: '150', + }, + gasEstimateType: GasEstimateTypes.legacy, }); describe('SwapsController', function () { @@ -161,7 +152,7 @@ describe('SwapsController', function () { }); }; - before(function () { + beforeEach(function () { const providerResultStub = { // 1 gwei eth_gasPrice: '0x0de0b6b3a7640000', @@ -173,26 +164,23 @@ describe('SwapsController', function () { networkId: 1, chainId: 1, }).provider; + jest.useFakeTimers(); }); afterEach(function () { - sandbox.restore(); + jest.useRealTimers(); + jest.restoreAllMocks(); }); describe('constructor', function () { it('should setup correctly', function () { const swapsController = getSwapsController(); - assert.deepStrictEqual( - swapsController.store.getState(), - EMPTY_INIT_STATE, - ); - assert.deepStrictEqual( - swapsController.getBufferedGasLimit, + expect(swapsController.store.getState()).toStrictEqual(EMPTY_INIT_STATE); + expect(swapsController.getBufferedGasLimit).toStrictEqual( MOCK_GET_BUFFERED_GAS_LIMIT, ); - assert.strictEqual(swapsController.pollCount, 0); - assert.deepStrictEqual( - swapsController.getProviderConfig, + expect(swapsController.pollCount).toStrictEqual(0); + expect(swapsController.getProviderConfig).toStrictEqual( MOCK_GET_PROVIDER_CONFIG, ); }); @@ -208,64 +196,57 @@ describe('SwapsController', function () { it('should set selected quote agg id', function () { const selectedAggId = 'test'; swapsController.setSelectedQuoteAggId(selectedAggId); - assert.deepStrictEqual( + expect( swapsController.store.getState().swapsState.selectedAggId, - selectedAggId, - ); + ).toStrictEqual(selectedAggId); }); it('should set swaps tokens', function () { const tokens = []; swapsController.setSwapsTokens(tokens); - assert.deepStrictEqual( + expect( swapsController.store.getState().swapsState.tokens, - tokens, - ); + ).toStrictEqual(tokens); }); it('should set trade tx id', function () { const tradeTxId = 'test'; swapsController.setTradeTxId(tradeTxId); - assert.strictEqual( + expect( swapsController.store.getState().swapsState.tradeTxId, - tradeTxId, - ); + ).toStrictEqual(tradeTxId); }); it('should set swaps tx gas price', function () { const gasPrice = 1; swapsController.setSwapsTxGasPrice(gasPrice); - assert.deepStrictEqual( + expect( swapsController.store.getState().swapsState.customGasPrice, - gasPrice, - ); + ).toStrictEqual(gasPrice); }); it('should set swaps tx gas limit', function () { const gasLimit = '1'; swapsController.setSwapsTxGasLimit(gasLimit); - assert.deepStrictEqual( + expect( swapsController.store.getState().swapsState.customMaxGas, - gasLimit, - ); + ).toStrictEqual(gasLimit); }); it('should set background swap route state', function () { const routeState = 'test'; swapsController.setBackgroundSwapRouteState(routeState); - assert.deepStrictEqual( + expect( swapsController.store.getState().swapsState.routeState, - routeState, - ); + ).toStrictEqual(routeState); }); it('should set swaps error key', function () { const errorKey = 'test'; swapsController.setSwapsErrorKey(errorKey); - assert.deepStrictEqual( + expect( swapsController.store.getState().swapsState.errorKey, - errorKey, - ); + ).toStrictEqual(errorKey); }); it('should set initial gas estimate', async function () { @@ -288,9 +269,9 @@ describe('SwapsController', function () { await swapsController.getBufferedGasLimit(); const { gasEstimate, gasEstimateWithRefund } = swapsController.store.getState().swapsState.quotes[initialAggId]; - assert.strictEqual(gasEstimate, bufferedGasLimit); - assert.strictEqual( - gasEstimateWithRefund, + + expect(gasEstimate).toStrictEqual(bufferedGasLimit); + expect(gasEstimateWithRefund).toStrictEqual( `0x${new BigNumberjs(maxGas, 10) .minus(estimatedRefund, 10) .toString(16)}`, @@ -300,10 +281,9 @@ describe('SwapsController', function () { it('should set custom approve tx data', function () { const data = 'test'; swapsController.setCustomApproveTxData(data); - assert.deepStrictEqual( + expect( swapsController.store.getState().swapsState.customApproveTxData, - data, - ); + ).toStrictEqual(data); }); }); @@ -316,14 +296,13 @@ describe('SwapsController', function () { }); it('returns empty object if passed undefined or empty object', async function () { - assert.deepStrictEqual( + expect( await swapsController._findTopQuoteAndCalculateSavings(), - {}, - ); - assert.deepStrictEqual( + ).toStrictEqual({}); + + expect( await swapsController._findTopQuoteAndCalculateSavings({}), - {}, - ); + ).toStrictEqual({}); }); it('returns the top aggId and quotes with savings and fee values if passed necessary data and an even number of quotes', async function () { @@ -331,9 +310,8 @@ describe('SwapsController', function () { await swapsController._findTopQuoteAndCalculateSavings( getTopQuoteAndSavingsMockQuotes(), ); - assert.equal(topAggId, TEST_AGG_ID_1); - assert.deepStrictEqual( - resultQuotes, + expect(topAggId).toStrictEqual(TEST_AGG_ID_1); + expect(resultQuotes).toStrictEqual( getTopQuoteAndSavingsBaseExpectedResults(), ); }); @@ -353,8 +331,8 @@ describe('SwapsController', function () { const [topAggId, resultQuotes] = await swapsController._findTopQuoteAndCalculateSavings(testInput); - assert.equal(topAggId, TEST_AGG_ID_1); - assert.deepStrictEqual(resultQuotes, expectedResultQuotes); + expect(topAggId).toStrictEqual(TEST_AGG_ID_1); + expect(resultQuotes).toStrictEqual(expectedResultQuotes); }); it('returns the top aggId, without best quote flagged, and quotes with fee values if passed necessary data but no custom convert rate exists', async function () { @@ -394,8 +372,8 @@ describe('SwapsController', function () { const [topAggId, resultQuotes] = await swapsController._findTopQuoteAndCalculateSavings(testInput); - assert.equal(topAggId, TEST_AGG_ID_1); - assert.deepStrictEqual(resultQuotes, expectedResultQuotes); + expect(topAggId).toStrictEqual(TEST_AGG_ID_1); + expect(resultQuotes).toStrictEqual(expectedResultQuotes); }); it('returns the top aggId and quotes with savings and fee values if passed necessary data and the source token is ETH', async function () { @@ -457,8 +435,8 @@ describe('SwapsController', function () { const [topAggId, resultQuotes] = await swapsController._findTopQuoteAndCalculateSavings(testInput); - assert.equal(topAggId, TEST_AGG_ID_1); - assert.deepStrictEqual(resultQuotes, expectedResultQuotes); + expect(topAggId).toStrictEqual(TEST_AGG_ID_1); + expect(resultQuotes).toStrictEqual(expectedResultQuotes); }); it('returns the top aggId and quotes with savings and fee values if passed necessary data and the source token is ETH and an ETH fee is included in the trade value of what would be the best quote', async function () { @@ -533,8 +511,8 @@ describe('SwapsController', function () { const [topAggId, resultQuotes] = await swapsController._findTopQuoteAndCalculateSavings(testInput); - assert.equal(topAggId, TEST_AGG_ID_2); - assert.deepStrictEqual(resultQuotes, expectedResultQuotes); + expect(topAggId).toStrictEqual(TEST_AGG_ID_2); + expect(resultQuotes).toStrictEqual(expectedResultQuotes); }); it('returns the top aggId and quotes with savings and fee values if passed necessary data and the source token is not ETH and an ETH fee is included in the trade value of what would be the best quote', async function () { @@ -568,31 +546,36 @@ describe('SwapsController', function () { const [topAggId, resultQuotes] = await swapsController._findTopQuoteAndCalculateSavings(testInput); - assert.equal(topAggId, TEST_AGG_ID_2); - assert.deepStrictEqual(resultQuotes, expectedResultQuotes); + expect(topAggId).toStrictEqual(TEST_AGG_ID_2); + expect(resultQuotes).toStrictEqual(expectedResultQuotes); }); }); describe('fetchAndSetQuotes', function () { it('returns null if fetchParams is not provided', async function () { const quotes = await swapsController.fetchAndSetQuotes(undefined); - assert.strictEqual(quotes, null); + expect(quotes).toStrictEqual(null); }); it('calls fetchTradesInfo with the given fetchParams and returns the correct quotes', async function () { - fetchTradesInfoStub.resolves(getMockQuotes()); + const fetchTradesInfoSpy = jest + .spyOn(swapsController, '_fetchTradesInfo') + .mockReturnValue(getMockQuotes()); // Make it so approval is not required - sandbox - .stub(swapsController, '_getERC20Allowance') - .resolves(BigNumber.from(1)); + jest + .spyOn(swapsController, '_getERC20Allowance') + .mockReturnValue(BigNumber.from(1)); + + // Make the network fetch error message disappear + jest.spyOn(swapsController, '_setSwapsNetworkConfig').mockReturnValue(); const [newQuotes] = await swapsController.fetchAndSetQuotes( MOCK_FETCH_PARAMS, MOCK_FETCH_METADATA, ); - assert.deepStrictEqual(newQuotes[TEST_AGG_ID_BEST], { + expect(newQuotes[TEST_AGG_ID_BEST]).toStrictEqual({ ...getMockQuotes()[TEST_AGG_ID_BEST], sourceTokenInfo: undefined, destinationTokenInfo: { @@ -615,16 +598,15 @@ describe('SwapsController', function () { metaMaskFeeInEth: '0.50505050505050505050505050505050505', ethValueOfTokens: '50', }); - assert.strictEqual( - fetchTradesInfoStub.calledOnceWithExactly(MOCK_FETCH_PARAMS, { - ...MOCK_FETCH_METADATA, - }), - true, - ); + + expect(fetchTradesInfoSpy).toHaveBeenCalledTimes(1); + expect(fetchTradesInfoSpy).toHaveBeenCalledWith(MOCK_FETCH_PARAMS, { + ...MOCK_FETCH_METADATA, + }); }); it('calls returns the correct quotes on the optimism chain', async function () { - fetchTradesInfoStub.resetHistory(); + fetchTradesInfoStub.mockReset(); const OPTIMISM_MOCK_FETCH_METADATA = { ...MOCK_FETCH_METADATA, chainId: CHAIN_IDS.OPTIMISM, @@ -645,19 +627,24 @@ describe('SwapsController', function () { swapsController = getSwapsController(optimismProvider); - fetchTradesInfoStub.resolves(getMockQuotes()); + const fetchTradesInfoSpy = jest + .spyOn(swapsController, '_fetchTradesInfo') + .mockReturnValue(getMockQuotes()); // Make it so approval is not required - sandbox - .stub(swapsController, '_getERC20Allowance') - .resolves(BigNumber.from(1)); + jest + .spyOn(swapsController, '_getERC20Allowance') + .mockReturnValue(BigNumber.from(1)); + + // Make the network fetch error message disappear + jest.spyOn(swapsController, '_setSwapsNetworkConfig').mockReturnValue(); const [newQuotes] = await swapsController.fetchAndSetQuotes( MOCK_FETCH_PARAMS, OPTIMISM_MOCK_FETCH_METADATA, ); - assert.deepStrictEqual(newQuotes[TEST_AGG_ID_BEST], { + expect(newQuotes[TEST_AGG_ID_BEST]).toStrictEqual({ ...getMockQuotes()[TEST_AGG_ID_BEST], sourceTokenInfo: undefined, destinationTokenInfo: { @@ -681,49 +668,56 @@ describe('SwapsController', function () { metaMaskFeeInEth: '0.50505050505050505050505050505050505', ethValueOfTokens: '50', }); - assert.strictEqual( - fetchTradesInfoStub.calledOnceWithExactly(MOCK_FETCH_PARAMS, { - ...OPTIMISM_MOCK_FETCH_METADATA, - }), - true, - ); + + expect(fetchTradesInfoSpy).toHaveBeenCalledTimes(1); + expect(fetchTradesInfoSpy).toHaveBeenCalledWith(MOCK_FETCH_PARAMS, { + ...OPTIMISM_MOCK_FETCH_METADATA, + }); }); it('performs the allowance check', async function () { - fetchTradesInfoStub.resolves(getMockQuotes()); + jest + .spyOn(swapsController, '_fetchTradesInfo') + .mockReturnValue(getMockQuotes()); // Make it so approval is not required - const allowanceStub = sandbox - .stub(swapsController, '_getERC20Allowance') - .resolves(BigNumber.from(1)); + const getERC20AllowanceSpy = jest + .spyOn(swapsController, '_getERC20Allowance') + .mockReturnValue(BigNumber.from(1)); + + // Make the network fetch error message disappear + jest.spyOn(swapsController, '_setSwapsNetworkConfig').mockReturnValue(); await swapsController.fetchAndSetQuotes( MOCK_FETCH_PARAMS, MOCK_FETCH_METADATA, ); - assert.strictEqual( - allowanceStub.calledOnceWithExactly( - MOCK_FETCH_PARAMS.sourceToken, - MOCK_FETCH_PARAMS.fromAddress, - CHAIN_IDS.MAINNET, - ), - true, + expect(getERC20AllowanceSpy).toHaveBeenCalledTimes(1); + expect(getERC20AllowanceSpy).toHaveBeenCalledWith( + MOCK_FETCH_PARAMS.sourceToken, + MOCK_FETCH_PARAMS.fromAddress, + CHAIN_IDS.MAINNET, ); }); it('gets the gas limit if approval is required', async function () { - fetchTradesInfoStub.resolves(MOCK_QUOTES_APPROVAL_REQUIRED); + jest + .spyOn(swapsController, '_fetchTradesInfo') + .mockReturnValue(MOCK_QUOTES_APPROVAL_REQUIRED); // Ensure approval is required - sandbox - .stub(swapsController, '_getERC20Allowance') - .resolves(BigNumber.from(0)); + jest + .spyOn(swapsController, '_getERC20Allowance') + .mockReturnValue(BigNumber.from(0)); + + // Make the network fetch error message disappear + jest.spyOn(swapsController, '_setSwapsNetworkConfig').mockReturnValue(); const timedoutGasReturnResult = { gasLimit: 1000000 }; - const timedoutGasReturnStub = sandbox - .stub(swapsController, 'timedoutGasReturn') - .resolves(timedoutGasReturnResult); + const timedoutGasReturnSpy = jest + .spyOn(swapsController, 'timedoutGasReturn') + .mockReturnValue(timedoutGasReturnResult); await swapsController.fetchAndSetQuotes( MOCK_FETCH_PARAMS, @@ -731,30 +725,33 @@ describe('SwapsController', function () { ); // Mocked quotes approvalNeeded is null, so it will only be called with the gas - assert.strictEqual( - timedoutGasReturnStub.calledOnceWithExactly( - MOCK_APPROVAL_NEEDED, - TEST_AGG_ID_APPROVAL, - ), - true, + expect(timedoutGasReturnSpy).toHaveBeenCalledTimes(1); + expect(timedoutGasReturnSpy).toHaveBeenCalledWith( + MOCK_APPROVAL_NEEDED, + TEST_AGG_ID_APPROVAL, ); }); it('marks the best quote', async function () { - fetchTradesInfoStub.resolves(getMockQuotes()); + jest + .spyOn(swapsController, '_fetchTradesInfo') + .mockReturnValue(getMockQuotes()); // Make it so approval is not required - sandbox - .stub(swapsController, '_getERC20Allowance') - .resolves(BigNumber.from(1)); + jest + .spyOn(swapsController, '_getERC20Allowance') + .mockReturnValue(BigNumber.from(1)); + + // Make the network fetch error message disappear + jest.spyOn(swapsController, '_setSwapsNetworkConfig').mockReturnValue(); const [newQuotes, topAggId] = await swapsController.fetchAndSetQuotes( MOCK_FETCH_PARAMS, MOCK_FETCH_METADATA, ); - assert.strictEqual(topAggId, TEST_AGG_ID_BEST); - assert.strictEqual(newQuotes[topAggId].isBestQuote, true); + expect(topAggId).toStrictEqual(TEST_AGG_ID_BEST); + expect(newQuotes[topAggId].isBestQuote).toStrictEqual(true); }); it('selects the best quote', async function () { @@ -771,29 +768,38 @@ describe('SwapsController', function () { .toString(), }; const quotes = { ...getMockQuotes(), [bestAggId]: bestQuote }; - fetchTradesInfoStub.resolves(quotes); + + jest.spyOn(swapsController, '_fetchTradesInfo').mockReturnValue(quotes); // Make it so approval is not required - sandbox - .stub(swapsController, '_getERC20Allowance') - .resolves(BigNumber.from(1)); + jest + .spyOn(swapsController, '_getERC20Allowance') + .mockReturnValue(BigNumber.from(1)); + + // Make the network fetch error message disappear + jest.spyOn(swapsController, '_setSwapsNetworkConfig').mockReturnValue(); const [newQuotes, topAggId] = await swapsController.fetchAndSetQuotes( MOCK_FETCH_PARAMS, MOCK_FETCH_METADATA, ); - assert.strictEqual(topAggId, bestAggId); - assert.strictEqual(newQuotes[topAggId].isBestQuote, true); + expect(topAggId).toStrictEqual(bestAggId); + expect(newQuotes[topAggId].isBestQuote).toStrictEqual(true); }); it('does not mark as best quote if no conversion rate exists for destination token', async function () { - fetchTradesInfoStub.resolves(getMockQuotes()); + jest + .spyOn(swapsController, '_fetchTradesInfo') + .mockReturnValue(getMockQuotes()); // Make it so approval is not required - sandbox - .stub(swapsController, '_getERC20Allowance') - .resolves(BigNumber.from(1)); + jest + .spyOn(swapsController, '_getERC20Allowance') + .mockReturnValue(BigNumber.from(1)); + + // Make the network fetch error message disappear + jest.spyOn(swapsController, '_setSwapsNetworkConfig').mockReturnValue(); swapsController.getTokenRatesState = () => ({ marketData: { @@ -806,27 +812,28 @@ describe('SwapsController', function () { MOCK_FETCH_METADATA, ); - assert.strictEqual(newQuotes[topAggId].isBestQuote, undefined); + expect(newQuotes[topAggId].isBestQuote).toStrictEqual(undefined); }); it('should replace ethers instance when called with a different chainId than was current when the controller was instantiated', async function () { - fetchTradesInfoStub = sandbox.stub(); + fetchTradesInfoStub.mockReset(); const _swapsController = getSwapsController(); const currentEthersInstance = _swapsController.ethersProvider; + // Make the network fetch error message disappear + jest + .spyOn(_swapsController, '_setSwapsNetworkConfig') + .mockReturnValue(); + await _swapsController.fetchAndSetQuotes(MOCK_FETCH_PARAMS, { ...MOCK_FETCH_METADATA, chainId: CHAIN_IDS.GOERLI, }); const newEthersInstance = _swapsController.ethersProvider; - assert.notStrictEqual( - currentEthersInstance, - newEthersInstance, - 'Ethers provider should be replaced', - ); + expect(currentEthersInstance).not.toStrictEqual(newEthersInstance); }); it('should not replace ethers instance when called with the same chainId that was current when the controller was instantiated', async function () { @@ -840,17 +847,16 @@ describe('SwapsController', function () { }); const currentEthersInstance = _swapsController.ethersProvider; + // Make the network fetch error message disappear + jest.spyOn(swapsController, '_setSwapsNetworkConfig').mockReturnValue(); + await swapsController.fetchAndSetQuotes(MOCK_FETCH_PARAMS, { ...MOCK_FETCH_METADATA, chainId: CHAIN_IDS.MAINNET, }); const newEthersInstance = _swapsController.ethersProvider; - assert.strictEqual( - currentEthersInstance, - newEthersInstance, - 'Ethers provider should not be replaced', - ); + expect(currentEthersInstance).toStrictEqual(newEthersInstance); }); it('should replace ethers instance, and _ethersProviderChainId, twice when called twice with two different chainIds, and successfully set the _ethersProviderChainId when returning to the original chain', async function () { @@ -868,6 +874,11 @@ describe('SwapsController', function () { const firstEthersProviderChainId = _swapsController._ethersProviderChainId; + // Make the network fetch error message disappear + jest + .spyOn(_swapsController, '_setSwapsNetworkConfig') + .mockReturnValue(); + await _swapsController.fetchAndSetQuotes(MOCK_FETCH_PARAMS, { ...MOCK_FETCH_METADATA, chainId: CHAIN_IDS.GOERLI, @@ -877,15 +888,9 @@ describe('SwapsController', function () { const secondEthersProviderChainId = _swapsController._ethersProviderChainId; - assert.notStrictEqual( - firstEthersInstance, - secondEthersInstance, - 'Ethers provider should be replaced', - ); - assert.notStrictEqual( - firstEthersInstance, + expect(firstEthersInstance).not.toStrictEqual(secondEthersInstance); + expect(firstEthersInstance).not.toStrictEqual( secondEthersProviderChainId, - 'Ethers provider chainId should be replaced', ); await _swapsController.fetchAndSetQuotes(MOCK_FETCH_PARAMS, { @@ -897,25 +902,15 @@ describe('SwapsController', function () { const thirdEthersProviderChainId = _swapsController._ethersProviderChainId; - assert.notStrictEqual( - firstEthersProviderChainId, + expect(firstEthersProviderChainId).not.toStrictEqual( thirdEthersInstance, - 'Ethers provider should be replaced', ); - assert.notStrictEqual( - secondEthersInstance, - thirdEthersInstance, - 'Ethers provider should be replaced', - ); - assert.notStrictEqual( - firstEthersInstance, + expect(secondEthersInstance).not.toStrictEqual(thirdEthersInstance); + expect(firstEthersInstance).not.toStrictEqual( thirdEthersProviderChainId, - 'Ethers provider chainId should be replaced', ); - assert.notStrictEqual( - secondEthersProviderChainId, + expect(secondEthersProviderChainId).not.toStrictEqual( thirdEthersProviderChainId, - 'Ethers provider chainId should be replaced', ); await _swapsController.fetchAndSetQuotes(MOCK_FETCH_PARAMS, { @@ -926,10 +921,8 @@ describe('SwapsController', function () { const lastEthersProviderChainId = _swapsController._ethersProviderChainId; - assert.strictEqual( - firstEthersProviderChainId, + expect(firstEthersProviderChainId).toStrictEqual( lastEthersProviderChainId, - 'Ethers provider chainId should match what it was originally', ); }); }); @@ -939,7 +932,8 @@ describe('SwapsController', function () { const { swapsState: old } = swapsController.store.getState(); swapsController.resetSwapsState(); const { swapsState } = swapsController.store.getState(); - assert.deepStrictEqual(swapsState, { + + expect(swapsState).toStrictEqual({ ...EMPTY_INIT_STATE.swapsState, tokens: old.tokens, swapsQuoteRefreshTime: old.swapsQuoteRefreshTime, @@ -952,41 +946,50 @@ describe('SwapsController', function () { }); it('clears polling timeout', function () { - swapsController.pollingTimeout = setTimeout( - () => assert.fail(), - POLLING_TIMEOUT, - ); + swapsController.pollingTimeout = setTimeout(() => { + throw new Error('Polling timeout not cleared'); + }, POLLING_TIMEOUT); + + // Reseting swaps state should clear the polling timeout swapsController.resetSwapsState(); - assert.strictEqual(swapsController.pollingTimeout._idleTimeout, -1); + + // Verify by ensuring the error is not thrown, indicating that the timer was cleared + expect(jest.runOnlyPendingTimers).not.toThrow(); }); }); describe('stopPollingForQuotes', function () { it('clears polling timeout', function () { - swapsController.pollingTimeout = setTimeout( - () => assert.fail(), - POLLING_TIMEOUT, - ); + swapsController.pollingTimeout = setTimeout(() => { + throw new Error('Polling timeout not cleared'); + }, POLLING_TIMEOUT); + + // Stop polling for quotes should clear the polling timeout swapsController.stopPollingForQuotes(); - assert.strictEqual(swapsController.pollingTimeout._idleTimeout, -1); + + // Verify by ensuring the error is not thrown, indicating that the timer was cleared + expect(jest.runOnlyPendingTimers).not.toThrow(); }); it('resets quotes state correctly', function () { swapsController.stopPollingForQuotes(); const { swapsState } = swapsController.store.getState(); - assert.deepStrictEqual(swapsState.quotes, {}); - assert.strictEqual(swapsState.quotesLastFetched, null); + expect(swapsState.quotes).toStrictEqual({}); + expect(swapsState.quotesLastFetched).toStrictEqual(null); }); }); describe('resetPostFetchState', function () { it('clears polling timeout', function () { - swapsController.pollingTimeout = setTimeout( - () => assert.fail(), - POLLING_TIMEOUT, - ); + swapsController.pollingTimeout = setTimeout(() => { + throw new Error('Polling timeout not cleared'); + }, POLLING_TIMEOUT); + + // Reset post fetch state should clear the polling timeout swapsController.resetPostFetchState(); - assert.strictEqual(swapsController.pollingTimeout._idleTimeout, -1); + + // Verify by ensuring the error is not thrown, indicating that the timer was cleared + expect(jest.runOnlyPendingTimers).not.toThrow(); }); it('updates state correctly', function () { @@ -1014,7 +1017,7 @@ describe('SwapsController', function () { swapsController.resetPostFetchState(); const { swapsState } = swapsController.store.getState(); - assert.deepStrictEqual(swapsState, { + expect(swapsState).toStrictEqual({ ...EMPTY_INIT_STATE.swapsState, tokens, fetchParams, @@ -1036,6 +1039,7 @@ describe('SwapsController', function () { metaMaskFeeInEth: '5', ethValueOfTokens: '0.3', }; + const values = [ { overallValueOfQuote: '3', @@ -1058,12 +1062,7 @@ describe('SwapsController', function () { ]; const median = getMedianEthValueQuote(values); - - assert.deepEqual( - median, - expectedResult, - 'should have returned correct median quote object', - ); + expect(median).toStrictEqual(expectedResult); }); it('calculates median correctly with even sample', function () { @@ -1072,6 +1071,7 @@ describe('SwapsController', function () { metaMaskFeeInEth: '6.5', ethValueOfTokens: '0.25', }; + const values = [ { overallValueOfQuote: '3', @@ -1098,13 +1098,9 @@ describe('SwapsController', function () { ethValueOfTokens: '0.6', }, ]; - const median = getMedianEthValueQuote(values); - assert.deepEqual( - median, - expectedResult, - 'should have returned correct median quote object', - ); + const median = getMedianEthValueQuote(values); + expect(median).toStrictEqual(expectedResult); }); it('calculates median correctly with an uneven sample where multiple quotes have the median overall value', function () { @@ -1158,13 +1154,9 @@ describe('SwapsController', function () { metaMaskFeeInEth: '0.8', }, ]; - const median = getMedianEthValueQuote(values); - assert.deepEqual( - median, - expectedResult, - 'should have returned correct median quote object', - ); + const median = getMedianEthValueQuote(values); + expect(median).toStrictEqual(expectedResult); }); it('calculates median correctly with an even sample where multiple quotes have the same overall value as either of the two middle values', function () { @@ -1212,29 +1204,22 @@ describe('SwapsController', function () { metaMaskFeeInEth: '0.8', }, ]; - const median = getMedianEthValueQuote(values); - assert.deepEqual( - median, - expectedResult, - 'should have returned correct median quote object', - ); + const median = getMedianEthValueQuote(values); + expect(median).toStrictEqual(expectedResult); }); it('throws on empty or non-array sample', function () { - assert.throws( - () => getMedianEthValueQuote([]), - 'should throw on empty array', + expect(() => getMedianEthValueQuote([])).toThrow( + 'Expected non-empty array param.', ); - assert.throws( - () => getMedianEthValueQuote(), - 'should throw on non-array param', + expect(() => getMedianEthValueQuote()).toThrow( + 'Expected non-empty array param.', ); - assert.throws( - () => getMedianEthValueQuote({}), - 'should throw on non-array param', + expect(() => getMedianEthValueQuote({})).toThrow( + 'Expected non-empty array param.', ); }); }); diff --git a/jest.config.js b/jest.config.js index 1b51294301ae..3640d1457480 100644 --- a/jest.config.js +++ b/jest.config.js @@ -58,6 +58,7 @@ module.exports = { '/app/scripts/controllers/push-platform-notifications/**/*.test.ts', '/app/scripts/controllers/user-storage/**/*.test.ts', '/app/scripts/controllers/metamask-notifications/**/*.test.ts', + '/app/scripts/controllers/swaps.test.js', '/app/scripts/controllers/metametrics.test.js', '/app/scripts/flask/**/*.test.js', '/app/scripts/lib/**/*.test.(js|ts)', From 97b730374330551edcde85f6a518cce43164147b Mon Sep 17 00:00:00 2001 From: Daniel <80175477+dan437@users.noreply.github.com> Date: Wed, 19 Jun 2024 18:30:25 +0200 Subject: [PATCH 2/3] fix: Disable Smart Transactions for the new Send&Swap feature (#25422) --- app/scripts/lib/transaction/smart-transactions.test.ts | 7 +++++++ app/scripts/lib/transaction/smart-transactions.ts | 9 ++++++++- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/app/scripts/lib/transaction/smart-transactions.test.ts b/app/scripts/lib/transaction/smart-transactions.test.ts index 507ab83ecdf2..04c029c3ef8b 100644 --- a/app/scripts/lib/transaction/smart-transactions.test.ts +++ b/app/scripts/lib/transaction/smart-transactions.test.ts @@ -134,6 +134,13 @@ describe('submitSmartTransactionHook', () => { expect(result).toEqual({ transactionHash: undefined }); }); + it('falls back to regular transaction submit if the transaction type is "swapAndSend"', async () => { + const request: SubmitSmartTransactionRequestMocked = createRequest(); + request.transactionMeta.type = TransactionType.swapAndSend; + const result = await submitSmartTransactionHook(request); + expect(result).toEqual({ transactionHash: undefined }); + }); + it('falls back to regular transaction submit if /getFees throws an error', async () => { const request: SubmitSmartTransactionRequestMocked = createRequest(); jest diff --git a/app/scripts/lib/transaction/smart-transactions.ts b/app/scripts/lib/transaction/smart-transactions.ts index 38f57f597947..97e37965d6ea 100644 --- a/app/scripts/lib/transaction/smart-transactions.ts +++ b/app/scripts/lib/transaction/smart-transactions.ts @@ -10,6 +10,7 @@ import { TransactionController, TransactionMeta, TransactionParams, + TransactionType, } from '@metamask/transaction-controller'; import log from 'loglevel'; import { @@ -120,9 +121,15 @@ class SmartTransactionHook { } async submit() { + const isUnsupportedTransactionTypeForSmartTransaction = + this.#transactionMeta?.type === TransactionType.swapAndSend; + // Will cause TransactionController to publish to the RPC provider as normal. const useRegularTransactionSubmit = { transactionHash: undefined }; - if (!this.#isSmartTransaction) { + if ( + !this.#isSmartTransaction || + isUnsupportedTransactionTypeForSmartTransaction + ) { return useRegularTransactionSubmit; } const { id: approvalFlowId } = await this.#controllerMessenger.call( From 038f3d672770b4f5c9db7a2fe2fb5b5293cd04c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Albert=20Oliv=C3=A9?= Date: Wed, 19 Jun 2024 20:00:38 +0200 Subject: [PATCH 3/3] feat(mmi): start implementing mmi ofa feature (#24749) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** As Order Flow Auction (OFA) has launched on MetaMask retail side, MMI is exploring ways to add support of OFA on the MMI extension and for their user base. The architecture will be slightly different as MMI has the custodian in between the transaction flow where the signing is happening. In general custodians do the signing and broadcasting themselves but we have introduced an updated API on our side (namely ECA3) which allows custodians to send back signed raw transactions which then is broadcasted by MMI through a backend service (unlike how MetaMask works). ## **Related issues** Fixes: https://consensyssoftware.atlassian.net/browse/MMI-5059 ## **Manual testing steps** 1. Toggle On Smart Transactions in the settings 2. Perform a transaction and sign it on the custody website 3. Check in Servo that it has been broadcasted successfully ## **Screenshots/Recordings** ### **Before** ### **After** ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask 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** - [x] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [x] 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. --- .../confirm-transaction-base.component.js | 6 + .../confirm-transaction-base.container.js | 4 + .../confirm-transaction-base.test.js | 124 ++++++++++++++++-- 3 files changed, 123 insertions(+), 11 deletions(-) diff --git a/ui/pages/confirmations/confirm-transaction-base/confirm-transaction-base.component.js b/ui/pages/confirmations/confirm-transaction-base/confirm-transaction-base.component.js index e7f99f6f3b36..18cefa0e724b 100644 --- a/ui/pages/confirmations/confirm-transaction-base/confirm-transaction-base.component.js +++ b/ui/pages/confirmations/confirm-transaction-base/confirm-transaction-base.component.js @@ -178,6 +178,7 @@ export default class ConfirmTransactionBase extends Component { smartTransactionsOptInStatus: PropTypes.bool, currentChainSupportsSmartTransactions: PropTypes.bool, selectedNetworkClientId: PropTypes.string, + isSmartTransactionsEnabled: PropTypes.bool, }; state = { @@ -825,6 +826,7 @@ export default class ConfirmTransactionBase extends Component { toAddress, showCustodianDeepLink, clearConfirmTransaction, + isSmartTransactionsEnabled, } = this.props; const { noteText } = this.state; @@ -836,6 +838,10 @@ export default class ConfirmTransactionBase extends Component { txData.metadata.note = noteText; } + if (isSmartTransactionsEnabled) { + txData.origin += '#smartTransaction'; + } + txData.metadata.custodianPublishesTransaction = custodianPublishesTransaction; txData.metadata.rpcUrl = rpcUrl; diff --git a/ui/pages/confirmations/confirm-transaction-base/confirm-transaction-base.container.js b/ui/pages/confirmations/confirm-transaction-base/confirm-transaction-base.container.js index 3734b231dad5..00cbdeefa01d 100644 --- a/ui/pages/confirmations/confirm-transaction-base/confirm-transaction-base.container.js +++ b/ui/pages/confirmations/confirm-transaction-base/confirm-transaction-base.container.js @@ -58,6 +58,9 @@ import { import { getCurrentChainSupportsSmartTransactions, getSmartTransactionsOptInStatus, + ///: BEGIN:ONLY_INCLUDE_IF(build-mmi) + getSmartTransactionsEnabled, + ///: END:ONLY_INCLUDE_IF } from '../../../../shared/modules/selectors'; import { getMostRecentOverviewPage } from '../../../ducks/history/history'; import { @@ -345,6 +348,7 @@ const mapStateToProps = (state, ownProps) => { isNotification, custodianPublishesTransaction, rpcUrl, + isSmartTransactionsEnabled: getSmartTransactionsEnabled(state), ///: END:ONLY_INCLUDE_IF }; }; diff --git a/ui/pages/confirmations/confirm-transaction-base/confirm-transaction-base.test.js b/ui/pages/confirmations/confirm-transaction-base/confirm-transaction-base.test.js index 5cf90c0da6cb..bc36b2667576 100644 --- a/ui/pages/confirmations/confirm-transaction-base/confirm-transaction-base.test.js +++ b/ui/pages/confirmations/confirm-transaction-base/confirm-transaction-base.test.js @@ -45,9 +45,9 @@ setBackgroundConnection({ }), ), getGasFeeTimeEstimate: jest.fn(), - promisifiedBackground: jest.fn(), tryReverseResolveAddress: jest.fn(), getNextNonce: jest.fn(), + updateTransaction: jest.fn(), }); const mockTxParamsFromAddress = '0x123456789'; @@ -470,14 +470,12 @@ describe('Confirm Transaction Base', () => { const sendTransaction = jest .fn() .mockResolvedValue(state.confirmTransaction.txData); - const updateTransaction = jest.fn().mockResolvedValue(); const updateTransactionValue = jest.fn().mockResolvedValue(); const showCustodianDeepLink = jest.fn(); const setWaitForConfirmDeepLinkDialog = jest.fn(); const props = { sendTransaction, - updateTransaction, updateTransactionValue, showCustodianDeepLink, setWaitForConfirmDeepLinkDialog, @@ -497,12 +495,19 @@ describe('Confirm Transaction Base', () => { expect(sendTransaction).toHaveBeenCalled(); }); - it('handleMainSubmit calls sendTransaction correctly', async () => { + it('handleMMISubmit calls sendTransaction correctly and then showCustodianDeepLink', async () => { const state = { appState: { ...baseStore.appState, gasLoadingAnimationIsShowing: false, }, + confirmTransaction: { + ...baseStore.confirmTransaction, + txData: { + ...baseStore.confirmTransaction.txData, + custodyStatus: true, + }, + }, metamask: { ...baseStore.metamask, accounts: { @@ -516,7 +521,9 @@ describe('Confirm Transaction Base', () => { networksMetadata: { ...baseStore.metamask.networksMetadata, [NetworkType.mainnet]: { - EIPS: { 1559: true }, + EIPS: { + 1559: true, + }, status: NetworkStatus.Available, }, }, @@ -525,6 +532,47 @@ describe('Confirm Transaction Base', () => { gasPrice: '0x59682f00', }, noGasPrice: false, + keyrings: [ + { + type: 'Custody', + accounts: [mockTxParamsFromAddress], + }, + ], + custodyAccountDetails: { + [mockTxParamsFromAddress]: { + address: mockTxParamsFromAddress, + details: 'details', + custodyType: 'testCustody - Saturn', + custodianName: 'saturn-dev', + }, + }, + mmiConfiguration: { + custodians: [ + { + envName: 'saturn-dev', + displayName: 'Saturn Custody', + isNoteToTraderSupported: false, + }, + ], + }, + internalAccounts: { + accounts: { + 'cf8dace4-9439-4bd4-b3a8-88c821c8fcb3': { + address: mockTxParamsFromAddress, + id: 'cf8dace4-9439-4bd4-b3a8-88c821c8fcb3', + metadata: { + name: 'Custody Account A', + keyring: { + type: 'Custody', + }, + }, + options: {}, + methods: ETH_EOA_METHODS, + type: EthAccountType.Eoa, + }, + }, + selectedAccount: 'cf8dace4-9439-4bd4-b3a8-88c821c8fcb3', + }, }, send: { ...baseStore.send, @@ -546,12 +594,19 @@ describe('Confirm Transaction Base', () => { }, }; - const sendTransaction = jest.fn().mockResolvedValue(); + const sendTransaction = jest + .fn() + .mockResolvedValue(state.confirmTransaction.txData); + const showCustodianDeepLink = jest.fn(); + const setWaitForConfirmDeepLinkDialog = jest.fn(); const props = { sendTransaction, + showCustodianDeepLink, + setWaitForConfirmDeepLinkDialog, toAddress: mockPropsToAddress, toAccounts: [{ address: mockPropsToAddress }], + isMainBetaFlask: false, }; const { getByTestId } = await render({ props, state }); @@ -560,10 +615,12 @@ describe('Confirm Transaction Base', () => { await act(async () => { fireEvent.click(confirmButton); }); - expect(sendTransaction).toHaveBeenCalled(); + expect(setWaitForConfirmDeepLinkDialog).toHaveBeenCalled(); + await expect(sendTransaction).toHaveBeenCalled(); + expect(showCustodianDeepLink).toHaveBeenCalled(); }); - it('handleMMISubmit calls sendTransaction correctly and then showCustodianDeepLink', async () => { + it('should append #smartTransaction to txData.origin when smartTransactionsOptInStatus and currentChainSupportsSmartTransactions are true', async () => { const state = { appState: { ...baseStore.appState, @@ -574,6 +631,7 @@ describe('Confirm Transaction Base', () => { txData: { ...baseStore.confirmTransaction.txData, custodyStatus: true, + origin: 'metamask#smartTransaction', }, }, metamask: { @@ -600,6 +658,47 @@ describe('Confirm Transaction Base', () => { gasPrice: '0x59682f00', }, noGasPrice: false, + keyrings: [ + { + type: 'Custody', + accounts: [mockTxParamsFromAddress], + }, + ], + custodyAccountDetails: { + [mockTxParamsFromAddress]: { + address: mockTxParamsFromAddress, + details: 'details', + custodyType: 'testCustody - Saturn', + custodianName: 'saturn-dev', + }, + }, + mmiConfiguration: { + custodians: [ + { + envName: 'saturn-dev', + displayName: 'Saturn Custody', + isNoteToTraderSupported: false, + }, + ], + }, + internalAccounts: { + accounts: { + 'cf8dace4-9439-4bd4-b3a8-88c821c8fcb3': { + address: mockTxParamsFromAddress, + id: 'cf8dace4-9439-4bd4-b3a8-88c821c8fcb3', + metadata: { + name: 'Custody Account A', + keyring: { + type: 'Custody', + }, + }, + options: {}, + methods: ETH_EOA_METHODS, + type: EthAccountType.Eoa, + }, + }, + selectedAccount: 'cf8dace4-9439-4bd4-b3a8-88c821c8fcb3', + }, }, send: { ...baseStore.send, @@ -642,9 +741,12 @@ describe('Confirm Transaction Base', () => { await act(async () => { fireEvent.click(confirmButton); }); - expect(setWaitForConfirmDeepLinkDialog).toHaveBeenCalled(); - await expect(sendTransaction).toHaveBeenCalled(); - expect(showCustodianDeepLink).toHaveBeenCalled(); + + expect(sendTransaction).toHaveBeenCalledWith( + expect.objectContaining({ + origin: 'metamask#smartTransaction', + }), + ); }); describe('when rendering the recipient value', () => {