diff --git a/.storybook/main.js b/.storybook/main.js index d14c65f90..635eae9be 100644 --- a/.storybook/main.js +++ b/.storybook/main.js @@ -54,7 +54,7 @@ module.exports = { os: false, path: false, stream: require.resolve('stream-browserify'), - _stream_transform: false, + _stream_transform: require.resolve('readable-stream/lib/_stream_transform.js'), }; config.module.strictExportPresence = true; config.module.rules.push({ diff --git a/app/scripts/controllers/network/network-controller.test.ts b/app/scripts/controllers/network/network-controller.test.ts index 503f23a72..508d57fe7 100644 --- a/app/scripts/controllers/network/network-controller.test.ts +++ b/app/scripts/controllers/network/network-controller.test.ts @@ -1192,7 +1192,7 @@ describe('NetworkController', () => { }); expect(oldChainIdResult).toBe('0x5'); - controller.setActiveNetwork('testNetworkConfigurationId'); + await controller.setActiveNetwork('testNetworkConfigurationId'); const promisifiedSendAsync2 = promisify(provider.sendAsync).bind( provider, ); @@ -2431,15 +2431,9 @@ describe('NetworkController', () => { }, }, beforeCompleting: async () => { - await waitForStateChanges({ - controller, - propertyPath: ['networkStatus'], - operation: () => { - controller.setActiveNetwork( - 'testNetworkConfigurationId', - ); - }, - }); + await controller.setActiveNetwork( + 'testNetworkConfigurationId', + ); }, }, ], @@ -2506,15 +2500,9 @@ describe('NetworkController', () => { network1.mockEssentialRpcCalls({ eth_getBlockByNumber: { beforeCompleting: async () => { - await waitForStateChanges({ - controller, - propertyPath: ['networkStatus'], - operation: () => { - controller.setActiveNetwork( - 'testNetworkConfigurationId', - ); - }, - }); + await controller.setActiveNetwork( + 'testNetworkConfigurationId', + ); }, }, net_version: { @@ -2578,15 +2566,9 @@ describe('NetworkController', () => { latestBlock: POST_1559_BLOCK, eth_getBlockByNumber: { beforeCompleting: async () => { - await waitForStateChanges({ - controller, - propertyPath: ['networkStatus'], - operation: () => { - controller.setActiveNetwork( - 'testNetworkConfigurationId', - ); - }, - }); + await controller.setActiveNetwork( + 'testNetworkConfigurationId', + ); }, }, }); @@ -4032,9 +4014,9 @@ describe('NetworkController', () => { async ({ controller, network }) => { network.mockEssentialRpcCalls(); - expect(() => + await expect(() => controller.setActiveNetwork('invalid-network-configuration-id'), - ).toThrow( + ).rejects.toThrow( new Error( 'networkConfigurationId invalid-network-configuration-id does not match a configured networkConfiguration', ), @@ -4075,7 +4057,7 @@ describe('NetworkController', () => { }); network.mockEssentialRpcCalls(); - controller.setActiveNetwork('testNetworkConfigurationId1'); + await controller.setActiveNetwork('testNetworkConfigurationId1'); expect(controller.store.getState().provider).toStrictEqual({ type: 'rpc', @@ -4144,6 +4126,8 @@ describe('NetworkController', () => { messenger: unrestrictedMessenger, eventType: NetworkControllerEventType.NetworkWillChange, operation: () => { + // Intentionally not awaited because we're checking state + // partway through the operation controller.setActiveNetwork('testNetworkConfigurationId2'); }, beforeResolving: () => { @@ -4208,6 +4192,8 @@ describe('NetworkController', () => { // before networkDidChange count: 1, operation: () => { + // Intentionally not awaited because we're checking state + // partway through the operation. controller.setActiveNetwork('testNetworkConfigurationId1'); }, }); @@ -4267,6 +4253,8 @@ describe('NetworkController', () => { // before networkDidChange count: 1, operation: () => { + // Intentionally not awaited because we're checking state + // partway through the operation controller.setActiveNetwork('testNetworkConfigurationId2'); }, }); @@ -4308,7 +4296,7 @@ describe('NetworkController', () => { }, }); - controller.setActiveNetwork('testNetworkConfigurationId'); + await controller.setActiveNetwork('testNetworkConfigurationId'); const { provider } = controller.getProviderAndBlockTracker(); assert(provider, 'Provider is somehow unset'); @@ -4356,7 +4344,7 @@ describe('NetworkController', () => { const { provider: providerBefore } = controller.getProviderAndBlockTracker(); - controller.setActiveNetwork('testNetworkConfigurationId'); + await controller.setActiveNetwork('testNetworkConfigurationId'); const { provider: providerAfter } = controller.getProviderAndBlockTracker(); @@ -4392,8 +4380,8 @@ describe('NetworkController', () => { const networkDidChange = await waitForPublishedEvents({ messenger: unrestrictedMessenger, eventType: NetworkControllerEventType.NetworkDidChange, - operation: () => { - controller.setActiveNetwork('testNetworkConfigurationId'); + operation: async () => { + await controller.setActiveNetwork('testNetworkConfigurationId'); }, }); @@ -4429,8 +4417,8 @@ describe('NetworkController', () => { const infuraIsUnblocked = await waitForPublishedEvents({ messenger: unrestrictedMessenger, eventType: NetworkControllerEventType.InfuraIsUnblocked, - operation: () => { - controller.setActiveNetwork('testNetworkConfigurationId'); + operation: async () => { + await controller.setActiveNetwork('testNetworkConfigurationId'); }, }); @@ -4462,13 +4450,7 @@ describe('NetworkController', () => { response: SUCCESSFUL_NET_VERSION_RESPONSE, }, }); - await waitForStateChanges({ - controller, - propertyPath: ['networkStatus'], - operation: () => { - controller.setActiveNetwork('testNetworkConfigurationId'); - }, - }); + await controller.setActiveNetwork('testNetworkConfigurationId'); expect(controller.store.getState().networkStatus).toBe('available'); }, @@ -4501,16 +4483,7 @@ describe('NetworkController', () => { latestBlock: POST_1559_BLOCK, }); - await waitForStateChanges({ - controller, - propertyPath: ['networkDetails'], - // setActiveNetwork clears networkDetails first, and then updates it - // to what we expect it to be - count: 2, - operation: () => { - controller.setActiveNetwork('testNetworkConfigurationId'); - }, - }); + await controller.setActiveNetwork('testNetworkConfigurationId'); expect(controller.store.getState().networkDetails).toStrictEqual({ EIPS: { @@ -5657,12 +5630,7 @@ describe('NetworkController', () => { currentNetwork.mockEssentialRpcCalls(); previousNetwork.mockEssentialRpcCalls(); - await waitForLookupNetworkToComplete({ - controller, - operation: () => { - controller.setActiveNetwork('testNetworkConfigurationId2'); - }, - }); + await controller.setActiveNetwork('testNetworkConfigurationId2'); expect(controller.store.getState().provider).toStrictEqual({ type: 'rpc', rpcUrl: 'https://mock-rpc-url-2', @@ -5730,12 +5698,7 @@ describe('NetworkController', () => { }); currentNetwork.mockEssentialRpcCalls(); previousNetwork.mockEssentialRpcCalls(); - await waitForLookupNetworkToComplete({ - controller, - operation: () => { - controller.setActiveNetwork('testNetworkConfigurationId'); - }, - }); + await controller.setActiveNetwork('testNetworkConfigurationId'); await waitForLookupNetworkToComplete({ controller, @@ -5787,12 +5750,7 @@ describe('NetworkController', () => { currentNetwork.mockEssentialRpcCalls(); previousNetwork.mockEssentialRpcCalls(); - await waitForLookupNetworkToComplete({ - controller, - operation: () => { - controller.setActiveNetwork('testNetworkConfigurationId'); - }, - }); + await controller.setActiveNetwork('testNetworkConfigurationId'); expect(controller.store.getState().networkStatus).toBe( 'available', ); @@ -5855,12 +5813,7 @@ describe('NetworkController', () => { }); previousNetwork.mockEssentialRpcCalls(); - await waitForLookupNetworkToComplete({ - controller, - operation: () => { - controller.setActiveNetwork('testNetworkConfigurationId'); - }, - }); + await controller.setActiveNetwork('testNetworkConfigurationId'); expect(controller.store.getState().networkDetails).toStrictEqual({ EIPS: { 1559: true, @@ -5926,12 +5879,7 @@ describe('NetworkController', () => { }); currentNetwork.mockEssentialRpcCalls(); previousNetwork.mockEssentialRpcCalls(); - await waitForLookupNetworkToComplete({ - controller, - operation: () => { - controller.setActiveNetwork('testNetworkConfigurationId'); - }, - }); + await controller.setActiveNetwork('testNetworkConfigurationId'); await waitForLookupNetworkToComplete({ controller, @@ -5986,12 +5934,7 @@ describe('NetworkController', () => { }); currentNetwork.mockEssentialRpcCalls(); previousNetwork.mockEssentialRpcCalls(); - await waitForLookupNetworkToComplete({ - controller, - operation: () => { - controller.setActiveNetwork('testNetworkConfigurationId'); - }, - }); + await controller.setActiveNetwork('testNetworkConfigurationId'); const { provider: providerBefore } = controller.getProviderAndBlockTracker(); @@ -6045,12 +5988,7 @@ describe('NetworkController', () => { currentNetwork.mockEssentialRpcCalls(); previousNetwork.mockEssentialRpcCalls(); - await waitForLookupNetworkToComplete({ - controller, - operation: () => { - controller.setActiveNetwork('testNetworkConfigurationId'); - }, - }); + await controller.setActiveNetwork('testNetworkConfigurationId'); await waitForLookupNetworkToComplete({ controller, @@ -6103,12 +6041,7 @@ describe('NetworkController', () => { response: BLOCKED_INFURA_RESPONSE, }, }); - await waitForLookupNetworkToComplete({ - controller, - operation: () => { - controller.setActiveNetwork('testNetworkConfigurationId'); - }, - }); + await controller.setActiveNetwork('testNetworkConfigurationId'); const promiseForNoInfuraIsUnblockedEvents = waitForPublishedEvents({ messenger: unrestrictedMessenger, @@ -6173,13 +6106,7 @@ describe('NetworkController', () => { }, }); - await waitForStateChanges({ - controller, - propertyPath: ['networkStatus'], - operation: () => { - controller.setActiveNetwork('currentNetworkConfiguration'); - }, - }); + await controller.setActiveNetwork('currentNetworkConfiguration'); expect(controller.store.getState().networkStatus).toBe( 'unavailable', ); @@ -6228,12 +6155,7 @@ describe('NetworkController', () => { latestBlock: POST_1559_BLOCK, }); - await waitForLookupNetworkToComplete({ - controller, - operation: () => { - controller.setActiveNetwork('testNetworkConfigurationId'); - }, - }); + await controller.setActiveNetwork('testNetworkConfigurationId'); expect(controller.store.getState().networkDetails).toStrictEqual({ EIPS: { 1559: false, diff --git a/app/scripts/controllers/network/network-controller.ts b/app/scripts/controllers/network/network-controller.ts index 51653ab4a..62d89b69f 100644 --- a/app/scripts/controllers/network/network-controller.ts +++ b/app/scripts/controllers/network/network-controller.ts @@ -704,7 +704,7 @@ export class NetworkController extends EventEmitter { * @returns The URL of the RPC endpoint representing the newly switched * network. */ - setActiveNetwork(networkConfigurationId: NetworkConfigurationId): string { + async setActiveNetwork(networkConfigurationId: NetworkConfigurationId) { const targetNetwork = this.store.getState().networkConfigurations[networkConfigurationId]; @@ -714,7 +714,7 @@ export class NetworkController extends EventEmitter { ); } - this.#setProviderConfig({ + await this.#setProviderConfig({ type: NETWORK_TYPES.RPC, ...targetNetwork, }); diff --git a/app/scripts/controllers/sign.test.ts b/app/scripts/controllers/sign.test.ts index dac749466..078d99291 100644 --- a/app/scripts/controllers/sign.test.ts +++ b/app/scripts/controllers/sign.test.ts @@ -52,6 +52,7 @@ const messageMock = { const coreMessageMock = { ...messageMock, messageParams: messageParamsMock, + securityProviderResponse: securityProviderResponseMock, }; const stateMessageMock = { diff --git a/app/scripts/controllers/sign.ts b/app/scripts/controllers/sign.ts index d27de2f06..70c65596d 100644 --- a/app/scripts/controllers/sign.ts +++ b/app/scripts/controllers/sign.ts @@ -21,6 +21,7 @@ import { AbstractMessageParams, AbstractMessageParamsMetamask, OriginalRequest, + SecurityProviderRequest, } from '@metamask/message-manager/dist/AbstractMessageManager'; import { BaseControllerV2, @@ -63,9 +64,10 @@ export type CoreMessage = AbstractMessage & { messageParams: AbstractMessageParams; }; -export type StateMessage = Required & { +export type StateMessage = Required< + Omit +> & { msgParams: Required; - securityProviderResponse: any; }; export type SignControllerState = { @@ -107,10 +109,7 @@ export type SignControllerOptions = { preferencesController: PreferencesController; getState: () => any; metricsEvent: (payload: any, options?: any) => void; - securityProviderRequest: ( - requestData: any, - methodName: string, - ) => Promise; + securityProviderRequest: SecurityProviderRequest; }; /** @@ -143,11 +142,6 @@ export default class SignController extends BaseControllerV2< private _metricsEvent: (payload: any, options?: any) => void; - private _securityProviderRequest: ( - requestData: any, - methodName: string, - ) => Promise; - /** * Construct a Sign controller. * @@ -178,12 +172,23 @@ export default class SignController extends BaseControllerV2< this._preferencesController = preferencesController; this._getState = getState; this._metricsEvent = metricsEvent; - this._securityProviderRequest = securityProviderRequest; this.hub = new EventEmitter(); - this._messageManager = new MessageManager(); - this._personalMessageManager = new PersonalMessageManager(); - this._typedMessageManager = new TypedMessageManager(); + this._messageManager = new MessageManager( + undefined, + undefined, + securityProviderRequest, + ); + this._personalMessageManager = new PersonalMessageManager( + undefined, + undefined, + securityProviderRequest, + ); + this._typedMessageManager = new TypedMessageManager( + undefined, + undefined, + securityProviderRequest, + ); this._messageManagers = [ this._messageManager, @@ -412,27 +417,30 @@ export default class SignController extends BaseControllerV2< * Used to cancel a message submitted via eth_sign. * * @param msgId - The id of the message to cancel. + * @returns A full state update. */ cancelMessage(msgId: string) { - this._cancelAbstractMessage(this._messageManager, msgId); + return this._cancelAbstractMessage(this._messageManager, msgId); } /** * Used to cancel a personal_sign type message. * * @param msgId - The ID of the message to cancel. + * @returns A full state update. */ cancelPersonalMessage(msgId: string) { - this._cancelAbstractMessage(this._personalMessageManager, msgId); + return this._cancelAbstractMessage(this._personalMessageManager, msgId); } /** * Used to cancel a eth_signTypedData type message. * * @param msgId - The ID of the message to cancel. + * @returns A full state update. */ cancelTypedMessage(msgId: string) { - this._cancelAbstractMessage(this._typedMessageManager, msgId); + return this._cancelAbstractMessage(this._typedMessageManager, msgId); } /** @@ -586,15 +594,7 @@ export default class SignController extends BaseControllerV2< origin: messageParams.origin as string, }, }; - - const messageId = coreMessage.id; - const existingMessage = this._getMessage(messageId); - - const securityProviderResponse = existingMessage - ? existingMessage.securityProviderResponse - : await this._securityProviderRequest(stateMessage, stateMessage.type); - - return { ...stateMessage, securityProviderResponse }; + return stateMessage; } private _normalizeMsgData(data: string) { diff --git a/app/scripts/controllers/swaps.js b/app/scripts/controllers/swaps.js index ab6105ecf..5030f1cd3 100644 --- a/app/scripts/controllers/swaps.js +++ b/app/scripts/controllers/swaps.js @@ -304,6 +304,7 @@ export default class SwapsController { Object.values(newQuotes).map(async (quote) => { if (quote.trade) { const multiLayerL1TradeFeeTotal = await fetchEstimatedL1Fee( + chainId, { txParams: quote.trade, chainId, diff --git a/app/scripts/controllers/swaps.test.js b/app/scripts/controllers/swaps.test.js index 648f57ccc..f5a43e0dc 100644 --- a/app/scripts/controllers/swaps.test.js +++ b/app/scripts/controllers/swaps.test.js @@ -156,12 +156,12 @@ const getEIP1559GasFeeEstimatesStub = sandbox.stub(() => { describe('SwapsController', function () { let provider; - const getSwapsController = () => { + const getSwapsController = (_provider = provider) => { return new SwapsController({ getBufferedGasLimit: MOCK_GET_BUFFERED_GAS_LIMIT, networkController: getMockNetworkController(), onNetworkDidChange: sinon.stub(), - provider, + provider: _provider, getProviderConfig: MOCK_GET_PROVIDER_CONFIG, getTokenRatesState: MOCK_TOKEN_RATES_STORE, fetchTradesInfo: fetchTradesInfoStub, @@ -722,6 +722,72 @@ describe('SwapsController', function () { ); }); + it('calls returns the correct quotes on the optimism chain', async function () { + fetchTradesInfoStub.resetHistory(); + const OPTIMISM_MOCK_FETCH_METADATA = { + ...MOCK_FETCH_METADATA, + chainId: CHAIN_IDS.OPTIMISM, + }; + const optimismProviderResultStub = { + // 1 gwei + eth_gasPrice: '0x0de0b6b3a7640000', + // by default, all accounts are external accounts (not contracts) + eth_getCode: '0x', + eth_call: + '0x000000000000000000000000000000000000000000000000000103c18816d4e8', + }; + const optimismProvider = createTestProviderTools({ + scaffold: optimismProviderResultStub, + networkId: 10, + chainId: 10, + }).provider; + + swapsController = getSwapsController(optimismProvider); + + fetchTradesInfoStub.resolves(getMockQuotes()); + + // Make it so approval is not required + sandbox + .stub(swapsController, '_getERC20Allowance') + .resolves(BigNumber.from(1)); + + const [newQuotes] = await swapsController.fetchAndSetQuotes( + MOCK_FETCH_PARAMS, + OPTIMISM_MOCK_FETCH_METADATA, + ); + + assert.deepStrictEqual(newQuotes[TEST_AGG_ID_BEST], { + ...getMockQuotes()[TEST_AGG_ID_BEST], + sourceTokenInfo: undefined, + destinationTokenInfo: { + symbol: 'FOO', + decimals: 18, + }, + isBestQuote: true, + // TODO: find a way to calculate these values dynamically + gasEstimate: 2000000, + gasEstimateWithRefund: '0xb8cae', + savings: { + fee: '-0.061067', + metaMaskFee: '0.5050505050505050505', + performance: '6', + total: '5.4338824949494949495', + medianMetaMaskFee: '0.44444444444444444444', + }, + ethFee: '0.113822', + multiLayerL1TradeFeeTotal: '0x0103c18816d4e8', + overallValueOfQuote: '49.886178', + metaMaskFeeInEth: '0.5050505050505050505', + ethValueOfTokens: '50', + }); + assert.strictEqual( + fetchTradesInfoStub.calledOnceWithExactly(MOCK_FETCH_PARAMS, { + ...OPTIMISM_MOCK_FETCH_METADATA, + }), + true, + ); + }); + it('performs the allowance check', async function () { fetchTradesInfoStub.resolves(getMockQuotes()); diff --git a/app/scripts/lib/security-provider-helpers.js b/app/scripts/lib/security-provider-helpers.js deleted file mode 100644 index 986165cd9..000000000 --- a/app/scripts/lib/security-provider-helpers.js +++ /dev/null @@ -1,65 +0,0 @@ -import getFetchWithTimeout from '../../../shared/modules/fetch-with-timeout'; -import { MESSAGE_TYPE } from '../../../shared/constants/app'; - -const fetchWithTimeout = getFetchWithTimeout(); - -export async function securityProviderCheck( - requestData, - methodName, - chainId, - currentLocale, -) { - let dataToValidate; - - if (methodName === MESSAGE_TYPE.ETH_SIGN_TYPED_DATA) { - dataToValidate = { - host_name: requestData.msgParams.origin, - rpc_method_name: methodName, - chain_id: chainId, - data: requestData.msgParams.data, - currentLocale, - }; - } else if ( - methodName === MESSAGE_TYPE.ETH_SIGN || - methodName === MESSAGE_TYPE.PERSONAL_SIGN - ) { - dataToValidate = { - host_name: requestData.msgParams.origin, - rpc_method_name: methodName, - chain_id: chainId, - data: { - signer_address: requestData.msgParams.from, - msg_to_sign: requestData.msgParams.data, - }, - currentLocale, - }; - } else { - dataToValidate = { - host_name: requestData.origin, - rpc_method_name: methodName, - chain_id: chainId, - data: { - from_address: requestData?.txParams?.from, - to_address: requestData?.txParams?.to, - gas: requestData?.txParams?.gas, - gasPrice: requestData?.txParams?.gasPrice, - value: requestData?.txParams?.value, - data: requestData?.txParams?.data, - }, - currentLocale, - }; - } - - const response = await fetchWithTimeout( - 'https://proxy.metafi.codefi.network/opensea/security/v1/validate', - { - method: 'POST', - headers: { - Accept: 'application/json', - 'Content-Type': 'application/json', - }, - body: JSON.stringify(dataToValidate), - }, - ); - return await response.json(); -} diff --git a/app/scripts/lib/security-provider-helpers.test.ts b/app/scripts/lib/security-provider-helpers.test.ts new file mode 100644 index 000000000..6f3d13ef2 --- /dev/null +++ b/app/scripts/lib/security-provider-helpers.test.ts @@ -0,0 +1,117 @@ +import { MESSAGE_TYPE } from '../../../shared/constants/app'; +import { + RequestData, + securityProviderCheck, +} from './security-provider-helpers'; + +describe('securityProviderCheck', () => { + let fetchSpy: jest.SpyInstance; + + beforeEach(() => { + // Spy on the global fetch function + fetchSpy = jest.spyOn(global, 'fetch'); + fetchSpy.mockImplementation(async () => { + return new Response(JSON.stringify('result_mocked'), { status: 200 }); + }); + }); + + const paramsMock = { + origin: 'https://example.com', + data: 'some_data', + from: '0x', + }; + + // Utility function to handle different data properties based on methodName + const getExpectedData = (methodName: string, requestData: RequestData) => { + switch (methodName) { + case MESSAGE_TYPE.ETH_SIGN: + case MESSAGE_TYPE.PERSONAL_SIGN: + return { + signer_address: requestData.msgParams?.from, + msg_to_sign: requestData.msgParams?.data, + }; + case MESSAGE_TYPE.ETH_SIGN_TYPED_DATA: + return requestData.messageParams?.data; + default: + return { + from_address: requestData.txParams?.from, + to_address: requestData.txParams?.to, + gas: requestData.txParams?.gas, + gasPrice: requestData.txParams?.gasPrice, + value: requestData.txParams?.value, + data: requestData.txParams?.data, + }; + } + }; + + test.each([ + [MESSAGE_TYPE.ETH_SIGN_TYPED_DATA], + [MESSAGE_TYPE.ETH_SIGN], + [MESSAGE_TYPE.PERSONAL_SIGN], + ['some_other_method'], + ])( + 'should call fetch with the correct parameters for %s', + async (methodName: string) => { + let requestData: RequestData; + + switch (methodName) { + case MESSAGE_TYPE.ETH_SIGN_TYPED_DATA: + requestData = { + origin: 'https://example.com', + messageParams: paramsMock, + }; + break; + case MESSAGE_TYPE.ETH_SIGN: + case MESSAGE_TYPE.PERSONAL_SIGN: + requestData = { + origin: 'https://example.com', + msgParams: paramsMock, + }; + break; + default: + requestData = { + origin: 'https://example.com', + txParams: { + from: '0x', + to: '0x', + gas: 'some_gas', + gasPrice: 'some_gasPrice', + value: 'some_value', + data: 'some_data', + }, + }; + } + + const result = await securityProviderCheck( + requestData, + methodName, + '1', + 'en', + ); + + expect(fetchSpy).toHaveBeenCalledTimes(1); + expect(fetchSpy).toHaveBeenCalledWith( + 'https://proxy.metafi.codefi.network/opensea/security/v1/validate', + expect.objectContaining({ + method: 'POST', + headers: { + Accept: 'application/json', + 'Content-Type': 'application/json', + }, + body: JSON.stringify({ + host_name: + methodName === 'some_other_method' + ? requestData.origin + : requestData.msgParams?.origin || + requestData.messageParams?.origin, + rpc_method_name: methodName, + chain_id: '1', + data: getExpectedData(methodName, requestData), + currentLocale: 'en', + }), + }), + ); + expect(result).toEqual('result_mocked'); + }, + ); +}); diff --git a/app/scripts/lib/security-provider-helpers.ts b/app/scripts/lib/security-provider-helpers.ts new file mode 100644 index 000000000..4b4bca0d6 --- /dev/null +++ b/app/scripts/lib/security-provider-helpers.ts @@ -0,0 +1,92 @@ +import { Json } from '@metamask/utils'; +import { MessageParams } from '@metamask/message-manager'; +import getFetchWithTimeout from '../../../shared/modules/fetch-with-timeout'; +import { MESSAGE_TYPE } from '../../../shared/constants/app'; + +const fetchWithTimeout = getFetchWithTimeout(); + +export type TransactionRequestData = { + txParams: Record; + messageParams?: never; + msgParams?: never; +}; + +export type MessageRequestData = + | { + msgParams: MessageParams; + txParams?: never; + messageParams?: never; + } + | { + messageParams: MessageParams; + msgParams?: never; + txParams?: never; + } + | TransactionRequestData; + +export type RequestData = { + origin: string; +} & MessageRequestData; + +export async function securityProviderCheck( + requestData: RequestData, + methodName: string, + chainId: string, + currentLocale: string, +): Promise> { + let dataToValidate; + // Core message managers use messageParams but frontend uses msgParams with lots of references + const params = requestData.msgParams || requestData.messageParams; + + if (methodName === MESSAGE_TYPE.ETH_SIGN_TYPED_DATA) { + dataToValidate = { + host_name: params?.origin, + rpc_method_name: methodName, + chain_id: chainId, + data: params?.data, + currentLocale, + }; + } else if ( + methodName === MESSAGE_TYPE.ETH_SIGN || + methodName === MESSAGE_TYPE.PERSONAL_SIGN + ) { + dataToValidate = { + host_name: params?.origin, + rpc_method_name: methodName, + chain_id: chainId, + data: { + signer_address: params?.from, + msg_to_sign: params?.data, + }, + currentLocale, + }; + } else { + dataToValidate = { + host_name: requestData.origin, + rpc_method_name: methodName, + chain_id: chainId, + data: { + from_address: requestData.txParams?.from, + to_address: requestData.txParams?.to, + gas: requestData.txParams?.gas, + gasPrice: requestData.txParams?.gasPrice, + value: requestData.txParams?.value, + data: requestData.txParams?.data, + }, + currentLocale, + }; + } + + const response: Response = await fetchWithTimeout( + 'https://proxy.metafi.codefi.network/opensea/security/v1/validate', + { + method: 'POST', + headers: { + Accept: 'application/json', + 'Content-Type': 'application/json', + }, + body: JSON.stringify(dataToValidate), + }, + ); + return await response.json(); +} diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index e7c7fcc7e..fa619c62f 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -348,18 +348,29 @@ export default class MetamaskController extends EventEmitter { { onPreferencesStateChange: (listener) => this.preferencesController.store.subscribe(listener), - onNetworkStateChange: (cb) => { - this.networkController.store.subscribe((networkState) => { - const modifiedNetworkState = { - ...networkState, - providerConfig: { - ...networkState.provider, - chainId: hexToDecimal(networkState.provider.chainId), - }, - }; - return cb(modifiedNetworkState); - }); - }, + // This handler is misnamed, and is a known issue that will be resolved + // by planned refactors. It should be onNetworkDidChange which happens + // AFTER the provider in the network controller is updated to reflect + // the new state of the network controller. In #18041 we changed this + // handler to be triggered by the change in the network state because + // that is what the handler name implies, but this triggers too soon + // causing the provider of the AssetsContractController to trail the + // network provider by one update. + onNetworkStateChange: (cb) => + networkControllerMessenger.subscribe( + NetworkControllerEventType.NetworkDidChange, + () => { + const networkState = this.networkController.store.getState(); + const modifiedNetworkState = { + ...networkState, + providerConfig: { + ...networkState.provider, + chainId: hexToDecimal(networkState.provider.chainId), + }, + }; + return cb(modifiedNetworkState); + }, + ), }, { provider: this.provider, diff --git a/docs/confirmation-refactoring/README.md b/docs/confirmation-refactoring/README.md new file mode 100644 index 000000000..29abab167 --- /dev/null +++ b/docs/confirmation-refactoring/README.md @@ -0,0 +1,13 @@ +# Confirmation Pages Refactoring + +The following pages document the ongoing refactoring efforts of confirmation pages. They describe the current (2023) code and proposed changes. + +1. [Signature Request Pages](./signature-request/README.md) + +2. [Confirmation Pages Routing](./confirmation-pages-routing/README.md) + +3. [Confirmation Page Structure](./confirmation-page=structure/README.md) + +4. [Confirmation State Management](./confirmation-state-management/README.md) + +5. [Confirmation Backend Architecture](./confirmation-backend-architecture/README.md) diff --git a/docs/confirmation-refactoring/confirmation-backend-architecture/README.md b/docs/confirmation-refactoring/confirmation-backend-architecture/README.md new file mode 100644 index 000000000..453482cbf --- /dev/null +++ b/docs/confirmation-refactoring/confirmation-backend-architecture/README.md @@ -0,0 +1,32 @@ +# Confirmation Background Architecture and Code Cleanup + +## Current Implementation: + +Current confirmation implementation in the background consists of following pieces: + +1. `TransactionController` and utility, helper classes used by it: + `TransactionController` is very important piece in transaction processing. It is described [here](https://github.com/MetaMask/metamask-extension/tree/develop/app/scripts/controllers/transactions#transaction-controller). It consists of 4 important parts: + - `txStateManager`: responsible for the state of a transaction and storing the transaction + - `pendingTxTracker`: watching blocks for transactions to be include and emitting confirmed events + - `txGasUtil`: gas calculations and safety buffering + - `nonceTracker`: calculating nonces +2. `MessageManagers`: + There are 3 different message managers responsible for processing signature requests. These are detailed [here](https://github.com/MetaMask/metamask-extension/tree/develop/docs/refactoring/signature-request#proposed-refactoring). +3. `MetamaskController `: + `MetamaskController ` is responsible for gluing together the different pieces in transaction processing. It is responsible to inject dependencies in `TransactionController`, `MessageManagers`, handling different events, responses to DAPP requests, etc. + +## Areas of Code Cleanup: + +1. Migrating to `@metamask/transaction-controller`. `TransactionController` in extension repo should eventually get replaced by core repo [TransactionController](https://github.com/MetaMask/core/tree/main/packages/transaction-controller). This controller is maintained by core team and also used in Metamask Mobile App. +2. Migrating to `@metamask/message-manager`. Message Managers in extension repo should be deprecated in favour of core repo [MessageManagers](https://github.com/MetaMask/core/tree/main/packages/message-manager). +3. Cleanup Code in `MetamaskController`. [Metamaskcontroller](https://github.com/MetaMask/metamask-extension/blob/develop/app/scripts/metamask-controller.js) is where `TransactionController` and different `MessageManagers` are initialized. It is responsible for injecting required dependencies. Also, it is responsible for handling incoming DAPP requests and invoking appropriate methods in these background classes. Over the period of time lot of code that should have been part of `TransactionController` and `MessageManagers` has ended up in `MetamaskController`. We need to cleanup this code and move to the appropriate classes. + - Code [here](https://github.com/MetaMask/metamask-extension/blob/bc19856d5d9ad1831e1722c84fe6161bed7a0a5a/app/scripts/metamask-controller.js#L3097) to check if `eth_sign` is enabled in preferences and perform other validation on the incoming request should be part of [MessageManager](https://github.com/MetaMask/metamask-extension/blob/develop/app/scripts/lib/message-manager.js) + - Method to sign messages [signMessage](https://github.com/MetaMask/metamask-extension/blob/bc19856d5d9ad1831e1722c84fe6161bed7a0a5a/app/scripts/metamask-controller.js#L3158), [signPersonalMessage](https://github.com/MetaMask/metamask-extension/blob/bc19856d5d9ad1831e1722c84fe6161bed7a0a5a/app/scripts/metamask-controller.js#L3217), [signTypedMessage](https://github.com/MetaMask/metamask-extension/blob/bc19856d5d9ad1831e1722c84fe6161bed7a0a5a/app/scripts/metamask-controller.js#L3470) can be simplified by injecting `KeyringController` into `MessageManagers`. + - There are about 11 different methods to `add`, `approve`, `reject` different types of signature requests. These can probably be moved to a helper class, thus reducing lines of code from `MetamaskController `. + - This [code](https://github.com/MetaMask/metamask-extension/blob/bc19856d5d9ad1831e1722c84fe6161bed7a0a5a/app/scripts/metamask-controller.js#L959) can better be placed in `TransactionController`. + - A lot of other methods in `MetamaskController` which are related to `TransactionController` and the state of `TransactionController` can be moved into `TransactionController` itself like [method1](https://github.com/MetaMask/metamask-extension/blob/bc19856d5d9ad1831e1722c84fe6161bed7a0a5a/app/scripts/metamask-controller.js#L1179), [method2](https://github.com/MetaMask/metamask-extension/blob/bc19856d5d9ad1831e1722c84fe6161bed7a0a5a/app/scripts/metamask-controller.js#L3570), [method3](https://github.com/MetaMask/metamask-extension/blob/bc19856d5d9ad1831e1722c84fe6161bed7a0a5a/app/scripts/metamask-controller.js#L4349), etc. + +### Using ApprovalController for Confirmations + +[ApprovalController](https://github.com/MetaMask/core/tree/main/packages/approval-controller) is written as a helper to `PermissionController`. Its role is to manage requests that require user approval. It can also be used in confirmation code to launch UI. Thus the use of `showUserConfirmation` function in `MetamaskController ` can be removed. +But `ApprovalController` will need some changes to be able to use it for confirmations, for example, it does not support multiple parallel requests from the same origin. diff --git a/docs/confirmation-refactoring/confirmation-page-structure/README.md b/docs/confirmation-refactoring/confirmation-page-structure/README.md new file mode 100644 index 000000000..a0ecc6588 --- /dev/null +++ b/docs/confirmation-refactoring/confirmation-page-structure/README.md @@ -0,0 +1,67 @@ +# Confirmation Pages Structure + +### Current Implementation + +Currently we have following confirmation pages mapping to confirmation routes: + +1. `pages/confirm-deploy-contract` +2. `pages/confirm-send-ether` +3. `pages/confirm-send-token` +4. `pages/confirm-approve` +5. `pages/confirm-token-transaction-base` +6. `pages/confirm-contract-interaction` + +![Confirmation Pages structure](https://raw.githubusercontent.com/MetaMask/metamask-extension/develop/docs/confirmation-refactoring/confirmation-page-structure/current.png) + +`confirm-page-container` component helps to define a structure for confirmation pages it includes: + +1. `header` +2. `content` - transaction details and tabs for hexdata and insights if available +3. `footer` +4. `warnings` + +`confirm-transaction-base` component is responsible for checking transaction details and pass required details like `gas-details`, `hex-data`, etc and passing over to `confirm-page-container`. + +Other confirmation components listed above map to different types of transactions and are responsible for passing over to `confirm-transaction-base` values / components specific to their transaction type. For instance, `confirm-deploy-contract` passes data section to `confirm-transaction-base`. + +## Areas of Refactoring: + +1. ### [confirm-transaction-base](https://github.com/MetaMask/metamask-extension/tree/develop/ui/pages/confirm-transaction-base/confirm-transaction-base.component.js) cleanup: + The `confirm-transaction-base` component is huge 1200 lines component taking care of lot of complexity. We need to break it down into smaller components and move logic to hooks or utility classes. Layout related part can be moved to `confirm-page-container`. + - Extract out code to render data into separate component from [here](https://github.com/MetaMask/metamask-extension/blob/e07ec9dcf3d3f341f83e6b29a29d30edaf7f5b5b/ui/pages/confirm-transaction-base/confirm-transaction-base.component.js#L641). + - Extract out component to render hex data from [here](https://github.com/MetaMask/metamask-extension/blob/e07ec9dcf3d3f341f83e6b29a29d30edaf7f5b5b/ui/pages/confirm-transaction-base/confirm-transaction-base.component.js#L675). + - Extract out code to render title [here](https://github.com/MetaMask/metamask-extension/blob/e07ec9dcf3d3f341f83e6b29a29d30edaf7f5b5b/ui/pages/confirm-transaction-base/confirm-transaction-base.component.js#L894) into separate component. + - Extract out code to render sub-title [here](https://github.com/MetaMask/metamask-extension/blob/e07ec9dcf3d3f341f83e6b29a29d30edaf7f5b5b/ui/pages/confirm-transaction-base/confirm-transaction-base.component.js#L921). It should return null if hideSubtitle is true. + - Extract out code to render gas details [here](https://github.com/MetaMask/metamask-extension/blob/e07ec9dcf3d3f341f83e6b29a29d30edaf7f5b5b/ui/pages/confirm-transaction-base/confirm-transaction-base.component.js#L444), this code can be used [here](https://github.com/MetaMask/metamask-extension/blob/e07ec9dcf3d3f341f83e6b29a29d30edaf7f5b5b/ui/pages/confirm-approve/confirm-approve-content/confirm-approve-content.component.js#L171) and [here](https://github.com/MetaMask/metamask-extension/blob/e07ec9dcf3d3f341f83e6b29a29d30edaf7f5b5b/ui/pages/send/gas-display/gas-display.js#L161) also. + - Extract renderDetails from [here](https://github.com/MetaMask/metamask-extension/blob/e07ec9dcf3d3f341f83e6b29a29d30edaf7f5b5b/ui/pages/confirm-transaction-base/confirm-transaction-base.component.js#L309) into a separate component. Function `setUserAcknowledgedGasMissing` can also be moved to it. + - Code to get error key [getErrorKey](https://github.com/MetaMask/metamask-extension/blob/e07ec9dcf3d3f341f83e6b29a29d30edaf7f5b5b/ui/pages/confirm-transaction-base/confirm-transaction-base.component.js#L230) can be moved to a util function. + - As new component for gas selection popups is created this code [handleEditGas, handleCloseEditGas](https://github.com/MetaMask/metamask-extension/blob/e07ec9dcf3d3f341f83e6b29a29d30edaf7f5b5b/ui/pages/confirm-transaction-base/confirm-transaction-base.component.js#L276) can be moved to it. + - Convert `confirm-transaction-base` into a functional components and extract out all of these functions into a hook - `handleEdit`, `handleCancelAll`, `handleCancel`, `handleSubmit`, `handleSetApprovalForAll`, etc. +2. ### [confirm-transaction-base-container](https://github.com/MetaMask/metamask-extension/tree/develop/ui/pages/confirm-transaction-base/confirm-transaction-base.container.js) cleanup: + This container is doing much work to query and get required transaction related values from state and pass over to `confirm-transaction-base` component. As we refactor state we should get rid of this component. + - remove the use of `state.confirmTransaction` from the component + - create hook to get values derived from metamask state and active transaction. + State cleanup is detailed more in a separate document [here](https://github.com/MetaMask/metamask-extension/tree/develop/docs/confirmation-refactoring/confirmation-state-management). +3. ### [confirm-page-container](https://github.com/MetaMask/metamask-extension/tree/03ccc5366cf31c9fa0fedc2fac533ebc64e6f2b4/ui/components/app/confirm-page-container) cleanup: + As described we should continue to have `confirm-page-container` components taking care of layout. Also wherever possible more re-usable smaller layout components for different part of confirmation page like gas details, gas selection popover, etc should be added. + `confirm-page-container` defines a layout which is used by most comfirmation pages, but some pages like new token allowance implementation for `ERC20` differ from this layout. We will be able to use more and more of these re-usable components for other confirmation pages layouts also. + - Move code specific to transaction to their confirmation component, for instance code related to `ApproveForAll` should be moved to `/pages/confirm-approve`, code related to `hideTitle` can be moved to `/pages/confirm-contract-interaction` etc. + - All header related code [here](https://github.com/MetaMask/metamask-extension/blob/03ccc5366cf31c9fa0fedc2fac533ebc64e6f2b4/ui/components/app/confirm-page-container/confirm-page-container.component.js#L191) should be moved to [confirm-page-container-header](https://github.com/MetaMask/metamask-extension/tree/03ccc5366cf31c9fa0fedc2fac533ebc64e6f2b4/ui/components/app/confirm-page-container/confirm-page-container-header) + - All warnings related code can be moved to a new child component. + - Props passing to `confirm-page-component` should be reduced. A lot of passed props like `origin`, `supportEIP1559` can be obtained directly using selectors. Props passing from `confirm-page-container` down to its child components should also be reduced. +4. ### Edit gas popovers: + + There are 2 different versions popovers for gas editing: + + - Legacy gas popover - [component](https://github.com/MetaMask/metamask-extension/tree/develop/ui/components/app/edit-gas-popover) + - EIP-1559 V2 gas popover - [component1](https://github.com/MetaMask/metamask-extension/tree/develop/ui/components/app/edit-gas-fee-popover), [component2](https://github.com/MetaMask/metamask-extension/tree/develop/ui/components/app/advanced-gas-fee-popover). + Context [transaction-modal-context](https://github.com/MetaMask/metamask-extension/blob/develop/ui/contexts/transaction-modal.js) is used to show hide EIP-1559 gas popovers. + + A parent component can be created for gas editing popover which will wrap both the legacy and EIP-1559 gas popover. Depending on the type of transaction appropriate gas popover can be shown. `transaction-modal-context` can be used to take care to open/close both popovers. + This parent component can be added to `confirm-transaction-base` and `token-allowance` components and thus will be available on all confirmation pages using gas editing. + Code [handleEditGas, handleCloseEditGas](https://github.com/MetaMask/metamask-extension/blob/e07ec9dcf3d3f341f83e6b29a29d30edaf7f5b5b/ui/pages/confirm-transaction-base/confirm-transaction-base.component.js#L276) can be moved to this new component. + +5. ### Gas polling + Gas polling related code in `/pages/confirm-transaction` can be moved into a hook and included in `pages/confirm-transaction-base`, `/app/token-allowance` as only those confirmation pages need gas estimates. + +**Note:** This document **does not cover signature request pages** which are covered separately. diff --git a/docs/confirmation-refactoring/confirmation-page-structure/current.png b/docs/confirmation-refactoring/confirmation-page-structure/current.png new file mode 100644 index 000000000..1a6458e6a Binary files /dev/null and b/docs/confirmation-refactoring/confirmation-page-structure/current.png differ diff --git a/docs/confirmation-refactoring/confirmation-pages-routing/README.md b/docs/confirmation-refactoring/confirmation-pages-routing/README.md new file mode 100644 index 000000000..ed4d7aef0 --- /dev/null +++ b/docs/confirmation-refactoring/confirmation-pages-routing/README.md @@ -0,0 +1,67 @@ +# Refactoring - Confirmation pages routing + +This document details how routing to confirmation pages is currently done and the proposed improvements in routing. + +## Current flow + +The current flow of routing to confirmation pages is un-necessarily complicated and have issues. + +![Confirmation Pages Routing - Current](https://raw.githubusercontent.com/MetaMask/metamask-extension/develop/docs/confirmation-refactoring/confirmation-pages-routing/current.png) + +- There are 2 ways in which confirmation pages can be opened: + 1. User triggers send flow from within Metamask + - If the user triggers the send flow from within MetaMask and selects the recipient and amount on the send screen, an unapproved transaction is created in the background and the user is redirected to the **`/confirm-transaction`** route. + 2. DAPP sends request to Metamask + - If DAPP sends request to Metamask an unapproved transaction or signature request is created in background and UI is triggered open (if it is not already open). + - The router by default renders `pages/home` component. The component looks at the state and if it finds an unapproved transaction or signature request in state it re-routes to **`/confirm-transaction`**. +- For **`/confirm-transaction/`** route, the router renders `pages/confirm-transaction` component. +- For **`/confirm-transaction`** route `pages/confirm-transaction` component renders `pages/confirm-transaction-switch` by default, for transactions with token methods it renders `pages/confirm-transaction/confirm-token-transaction-switch` which also open `pages/confirm-transaction-switch` by default. +- `pages/confirm-token-switch` redirect to specific confirmation page route depending on un-approved transaction or signature request in the state. +- For specific route **`/confirm-transaction/${id}/XXXXX`** routes again `pages/confirm-transaction` is rendered. +- Depending on confirmation route `pages/confirm-transaction` and `pages/confirm-transaction/confirm-token-transaction-switch` renders the specific confirmation page component. + +## Proposed flow + +The proposed routing of confirmation pages looks like. + +![Confirmation Pages Routing - Proposed](https://raw.githubusercontent.com/MetaMask/metamask-extension/develop/docs/confirmation-refactoring/confirmation-pages-routing/proposed.png) + +- There are 2 ways in which confirmation pages can be opened: + 1. User triggers send flow from within Metamask + - If the user triggers the send flow from within MetaMask and selects the recipient and amount on the send screen, an unapproved transaction is created in the background and the user is redirected to a specific transaction route, **`/confirm-transaction/${id}/XXXX`**, depending on the transaction type. + 2. DAPP sends request to Metamask + - If DAPP send request to Metamask an unapproved transaction or signature request is created in background and UI is triggered to open (if it is not already open). + - Instead of rendering `pages/home`, `pages/routes` finds the unapproved transaction in state and reroutes to **`/confirm-transaction`**. +- Router renders `pages/confirm-transaction` component for **`/confirm-transaction`** route. +- `pages/confirm-transaction` component redirect to specific confirmation page route depending on unapproved transaction or signature request in the state. +- Again for specific route **`/confirm-transaction/${id}/XXXXX`** `pages/confirm-transaction` is rendered, it in-turn renders appropriate confirmation page for the specific route. + +## Current Route component mapping + +| Route | Component | +| ------------------------------------------------- | -------------------------------------- | +| `/confirm-transaction/${id}/deploy-contract` | `pages/confirm-deploy-contract` | +| `/confirm-transaction/${id}/send-ether` | `pages/confirm-send-ether` | +| `/confirm-transaction/${id}/send-token` | `pages/confirm-send-token` | +| `/confirm-transaction/${id}/approve` | `pages/confirm-approve` | +| `/confirm-transaction/${id}/set-approval-for-all` | `pages/confirm-approve` | +| `/confirm-transaction/${id}/transfer-from` | `pages/confirm-token-transaction-base` | +| `/confirm-transaction/${id}/safe-transfer-from` | `pages/confirm-token-transaction-base` | +| `/confirm-transaction/${id}/token-method` | `pages/confirm-contract-interaction` | +| `/confirm-transaction/${id}/signature-request` | `pages/confirm-signature-request.js` | + +## Areas of code refactoring + +Current routing code is complicated, it is also currently tied to state change in confirmation pages that makes it more complicated. State refactoring as discussed in this [document](https://github.com/MetaMask/metamask-extension/tree/develop/docs/confirmation-refactoring/confirmation-state-management) will also help simplify it. + +- Any re-usable routing related code should be moved to [useRouting](https://github.com/MetaMask/metamask-extension/blob/develop/ui/hooks/useRouting.js) hook. +- Logic to initially check state and redirect to `/pages/confirm-transaction` can be moved from `/pages/home` to `pages/routes` +- All the route mapping code should be moved to `/pages/confirm-transaction`, this will require getting rid of route mappings in `/pages/confirm-transaction/confirm-token-transaction-switch`, `/pages/confirm-transaction-switch`. +- `/pages/confirm-transaction-switch` has the code that checks the un-approved transaction / message in the state, and based on its type and asset redirect to a specific route, a utility method can be created to do this mapping and can be included in `/pages/confirm-transaction` component. +- During the send flow initiated within metamask user can be redirected to specific confirmations route **`/confirm-transaction/${id}/XXXX`** +- Confirmation components have lot of props passing which needs to be reduced. Values can be obtained from redux state or other contexts directly using hooks. Component [confirm-token-transaction-switch](https://github.com/MetaMask/metamask-extension/blob/develop/ui/pages/confirm-transaction/confirm-token-transaction-switch.js) has a lot of un-necessary props passing which should be removed and will help to further refactor routing. + +- **Routing to mostRecentOverviewPage** + Across confirmation pages there is code to re-direct to `mostRecentOverviewPage`. `mostRecentOverviewPage` is equal to default route `/` or `/asset` whichever was last opened. + Also a lot of components check for state update and as soon as state has `0` pending un-approved transaction or signature request redirect is done to `mostRecentOverviewPage`. This logic can be handled at `/pages/confirm-transaction` which is always rendered for any confirmation page. + Also when the transaction is completed / rejected redirect is done to `mostRecentOverviewPage` explicitly which we should continue to do. diff --git a/docs/refactoring/confirmation-pages-routing/current.png b/docs/confirmation-refactoring/confirmation-pages-routing/current.png similarity index 100% rename from docs/refactoring/confirmation-pages-routing/current.png rename to docs/confirmation-refactoring/confirmation-pages-routing/current.png diff --git a/docs/refactoring/confirmation-pages-routing/proposed.png b/docs/confirmation-refactoring/confirmation-pages-routing/proposed.png similarity index 100% rename from docs/refactoring/confirmation-pages-routing/proposed.png rename to docs/confirmation-refactoring/confirmation-pages-routing/proposed.png diff --git a/docs/confirmation-refactoring/confirmation-state-management/README.md b/docs/confirmation-refactoring/confirmation-state-management/README.md new file mode 100644 index 000000000..a24184154 --- /dev/null +++ b/docs/confirmation-refactoring/confirmation-state-management/README.md @@ -0,0 +1,47 @@ +# Confirmation Pages - Frontend State Management + +State Management is very important piece to keep frontend confirmation code simplified. Currently state management is fragmented over places and is complicated. Following guidelines will be useful for designing State Magagement: + +1. Use state obtained from backend (redux store `state.metamask`) as single source of truth +2. For state derived from the backend state hooks can be written, these will internally use backend state +3. For temporary UI state shared across multiple components React Context can be used, minimise the scope of the context to just the components that need them (this is useful to avoid un-necessary re-rendering cycles in the app) +4. Confirmation React components fall into 2 categories: + - Smart components: state access should go here + - Dumb components: they are used for layout mainly and should ideally have required state data passed to them via props +5. Redux state is a good candidate for implementing state machine on frontend, if require anywhere in confirmation pages. Though currently transient state is mostly confined to single component state machine may not be needed. + +Refactorings: + +- There are confirmations related ducks [here](https://github.com/MetaMask/metamask-extension/tree/develop/ui/ducks): + - [confirm-transaction](https://github.com/MetaMask/metamask-extension/tree/develop/ui/ducks/confirm-transaction): this is redundant and we should be able to get rid of it. + - [gas](https://github.com/MetaMask/metamask-extension/tree/develop/ui/ducks/gas): this is not used anywhere and can be removed. + - [send](https://github.com/MetaMask/metamask-extension/tree/develop/ui/ducks/send): this duck is important state machine for send flow and we should continue to maintain. +- [gasFeeContext](https://github.com/MetaMask/metamask-extension/blob/develop/ui/contexts/gasFee.js) is huge context written on top of [gasFeeInput](https://github.com/MetaMask/metamask-extension/tree/develop/ui/hooks/gasFeeInput) hook. The context / hook provides about 20 different values used in different places in confirmation pages. We need to break this down: + + - Context is required only to provide temporary UI state for confirmation pages which includes: + + - `transaction` - active transaction on confirmation pages + - `editGasMode` - cancel, speedup, swap or default, this is also temporary UI state + + The context can be included in `/pages/confirm-transaction-base` and around `TokenAllowance` in `/pages/confirm-approve`. + + - Hooks can be created for values derived from values derived from above context and metamask state. This include: + - `maxFeePerGas` + - `maxPriorityFeePerGas` + - `supportEIP1559` + - `balanceError` + - `minimumCostInHexWei` + - `maximumCostInHexWei` + - `hasSimulationError` + - `estimateUsed` + - Values which can be obtained from metamask state using selectors should be removed from this context. This includes: + - `gasFeeEstimates` + - `isNetworkBusy` + - `minimumGasLimitDec` is a constant value 21000 should be removed from the context, this can be moved to constants file. + - Create separate hook for transaction functions [here](https://github.com/MetaMask/metamask-extension/blob/develop/ui/hooks/gasFeeInput/useTransactionFunctions.js), this hook can consume GasFeeContext. + - Setters and manual update functions are only used by legacy gas component [edit-gas-fee-popover](https://github.com/MetaMask/metamask-extension/tree/develop/ui/components/app/edit-gas-popover). This component uses useGasFeeInputs hook. We need to create a smaller hook just for this component using the above context and hooks. + +* [confirm-transaction-base.container.js](https://github.com/MetaMask/metamask-extension/blob/develop/ui/pages/confirm-transaction-base/confirm-transaction-base.container.js) and [confirm-transaction-base.component.js](https://github.com/MetaMask/metamask-extension/blob/develop/ui/pages/confirm-transaction-base/confirm-transaction-base.component.js) has a lot of code to derive values from state and selected transactions. This can be simplified by using hooks that will he created. +* We will have a lot of hooks for transaction related fields, these can be grouped into same file / folder. + +As we work on the components we will be able to identify more areas of improvement. diff --git a/docs/refactoring/signature-request/README.md b/docs/confirmation-refactoring/signature-request/README.md similarity index 52% rename from docs/refactoring/signature-request/README.md rename to docs/confirmation-refactoring/signature-request/README.md index 35c1876a1..a80cc046c 100644 --- a/docs/refactoring/signature-request/README.md +++ b/docs/confirmation-refactoring/signature-request/README.md @@ -6,35 +6,35 @@ This document details the plan to refactor and cleanup Signature Request pages i 1. Simple ETH Signature - + 1. Personal Signature - + 1. Typed Data - V1 - + 1. Typed Data - V3 - + 1. Typed Data - V4 - + 1. SIWE Signature - + ## The current flow of control for Signature Request looks like: -![Signature Request Flow - Current](https://raw.githubusercontent.com/MetaMask/metamask-extension/develop/docs/refactoring/signature-request/signature_request_old.png) +![Signature Request Flow - Current](https://raw.githubusercontent.com/MetaMask/metamask-extension/develop/docs/confirmation-refactoring/signature-request/signature_request_old.png) ## The proposed flow of control: -![Signature Request Flow - Proposed](https://raw.githubusercontent.com/MetaMask/metamask-extension/develop/docs/refactoring/signature-request/signature_request_proposed.png) +![Signature Request Flow - Proposed](https://raw.githubusercontent.com/MetaMask/metamask-extension/develop/docs/confirmation-refactoring/signature-request/signature_request_proposed.png) ## Proposed Refactoring: @@ -48,33 +48,9 @@ There are many areas in above flow where the code can be improved upon to cleanu - [PersonalMessageManager](https://github.com/MetaMask/metamask-extension/blob/develop/app/scripts/lib/personal-message-manager.js) - [TypedMessageManager](https://github.com/MetaMask/metamask-extension/blob/develop/app/scripts/lib/typed-message-manager.js) - Above message managers handle different types of message requests sent by DAPP. There is a lot of code duplication between the 3 classes. We can extract out a parent class and move duplicated code to it. Functions that can be moved to parent class: + Above message managers handle different types of message requests sent by DAPP. There is a lot of code duplication between the 3 classes. - 1. `constructor` - variable initialisation: - - ``` - this.messages = []; - this.metricsEvent = metricsEvent; - ``` - - 1. `unapprovedMsgCount` - 1. `getUnapprovedMsgs` - 1. `addUnapprovedMessageAsync` - partially - 1. `addUnapprovedMessage` - partially - 1. `addMsg` - 1. `getMsg` - 1. `approveMessage` - 1. `setMsgStatusApproved` - 1. `setMsgStatusSigned` - 1. `prepMsgForSigning` - 1. `rejectMsg` - 1. `errorMessage` - 1. `clearUnapproved` - 1. `_setMsgStatus` - 1. `_updateMsg` - 1. `_saveMsgList` - - Much de-duplication can be achieved in message menagers. + We should migrate to use `MessageManagers` from `@metamask/core` repo [here](https://github.com/MetaMask/core/tree/main/packages/message-manager). 2. ### Refactoring Routing to Signature Request pages: @@ -86,15 +62,13 @@ There are many areas in above flow where the code can be improved upon to cleanu 3. ### Refactoring in [conf-tx.js](https://github.com/MetaMask/metamask-extension/blob/develop/ui/pages/confirm-transaction/conf-tx.js) - - While fixing routing [conf-tx.js](https://github.com/MetaMask/metamask-extension/blob/develop/ui/pages/confirm-transaction/conf-tx.js) will be renamed to pages/confirm-signature-request component - - We are getting rid of [confirm-transaction](https://github.com/MetaMask/metamask-extension/blob/develop/ui/pages/confirm-transaction/confirm-transaction.component.js) component from the flow. Thus, we need to ensure that any required logic from the component is extracted into a reusable hook and included in pages/confirm-signature-request. - [This](https://github.com/MetaMask/metamask-extension/blob/develop/ui/pages/confirm-transaction/confirm-transaction.component.js#L158) check for valid transaction id should be made re-usable and extracted out. - - The component can be converted to a function react component and use selectors to get state and get rid of `mapStatToProps`. [#17239](https://github.com/MetaMask/metamask-extension/issues/17239) - - Various callbacks to sign message, cancel request, etc for different type of messaged can be moved to respective child components. - - On component mount/update if there are no unapproved messages redirect to `mostRecentlyOverviewedPage` as [here](https://github.com/MetaMask/metamask-extension/blob/76a2a9bb8b6ea04025328d36404ac3b59121dfc8/ui/app/pages/confirm-transaction/conf-tx.js#L187). - - Not pass values like [these](https://github.com/MetaMask/metamask-extension/blob/76a2a9bb8b6ea04025328d36404ac3b59121dfc8/ui/app/pages/confirm-transaction/conf-tx.js#L260) to child components which can be obtained in child components using selectors. + - [conf-tx.js](https://github.com/MetaMask/metamask-extension/blob/develop/ui/pages/confirm-transaction/conf-tx.js) to be renamed to `pages/confirm-signature-request component` + - Get rid of [confirm-transaction](https://github.com/MetaMask/metamask-extension/blob/develop/ui/pages/confirm-transaction/confirm-transaction.component.js) component from signature request routing. Thus, we need to ensure that any required logic from the component is extracted into a reusable hook and included in pages/confirm-signature-request. + - Convert to functional react component and use selectors to get state and get rid of `mapStateToProps`. [#17239](https://github.com/MetaMask/metamask-extension/issues/17239) + - Various callbacks to `sign message`, `cancel request`, etc for different types of messages can be moved to respective child components. + - On component `mount/update` if there are no unapproved messages redirect to `mostRecentlyOverviewedPage` as [here](https://github.com/MetaMask/metamask-extension/blob/76a2a9bb8b6ea04025328d36404ac3b59121dfc8/ui/app/pages/confirm-transaction/conf-tx.js#L187). + - Do not pass values like [these](https://github.com/MetaMask/metamask-extension/blob/76a2a9bb8b6ea04025328d36404ac3b59121dfc8/ui/app/pages/confirm-transaction/conf-tx.js#L260) to child components which can be obtained in child components using selectors. - Extract logic [here](https://github.com/MetaMask/metamask-extension/blob/76a2a9bb8b6ea04025328d36404ac3b59121dfc8/ui/app/pages/confirm-transaction/conf-tx.js#L218) to show success modal for previously confirmed transaction into separate hook. - - **Question** - [this](https://github.com/MetaMask/metamask-extension/blob/76a2a9bb8b6ea04025328d36404ac3b59121dfc8/ui/app/pages/confirm-transaction/conf-tx.js#L241) check for message params look confusing - is it possible for a message request to not have message params or for other transactions to have message params. [Here](https://github.com/MetaMask/metamask-extension/blob/76a2a9bb8b6ea04025328d36404ac3b59121dfc8/ui/app/pages/confirm-transaction/conf-tx.js#L185) we could have just checked for unapproved messages, why are we checking for pending transactions also ? 4. ### Refactoring component rendering Signature Request Pages @@ -110,28 +84,29 @@ There are many areas in above flow where the code can be improved upon to cleanu 5. ### Refactoring in signature-request-original - Rename, this component takes care of ETH sign, personal sign, sign typed data V1 requests. Let's rename it accordingly. - - Get rid of container components and for other components migrate to functional react components. - - Move this [metrics event](https://github.com/MetaMask/metamask-extension/blob/71a0bc8b3ff94478e61294c815770e6bc12a72f5/ui/app/components/app/signature-request-original/signature-request-original.component.js#L47) to pages/confirm-signature-request as it is applicable to all the signature requests types. + - Get rid of container components + - Migrate other classical components to functional react components. + - Move this [metrics event](https://github.com/MetaMask/metamask-extension/blob/71a0bc8b3ff94478e61294c815770e6bc12a72f5/ui/app/components/app/signature-request-original/signature-request-original.component.js#L50) to pages/confirm-signature-request as it is applicable to all the signature requests types. - Header or we can say upper half of the page of all signature request pages (except SIWE) are very similar, this can be extracted into a reusable component used across both signature-request-original and signature-request: - + - - [LedgerInstructions](https://github.com/MetaMask/metamask-extension/blob/develop/ui/components/app/signature-request-original/signature-request-original.component.js#L308) can also be moved to the header. + - [LedgerInstructions](https://github.com/MetaMask/metamask-extension/blob/e07ec9dcf3d3f341f83e6b29a29d30edaf7f5b5b/ui/components/app/signature-request-original/signature-request-original.component.js#L312) can also be moved to the header. - Create a reuable footer component and use it across all confirmation pages. [#17237](https://github.com/MetaMask/metamask-extension/issues/17237) - + - - Create a reuable component for Cancel All requests for use across signature request pages [Code](https://github.com/MetaMask/metamask-extension/blob/develop/ui/components/app/signature-request-original/signature-request-original.component.js#L322). - - Extract [getNetrowkName](https://github.com/MetaMask/metamask-extension/blob/develop/ui/components/app/signature-request-original/signature-request-original.component.js#L56) into a reuable hook / utility method. - - [msgHexToText](https://github.com/MetaMask/metamask-extension/blob/develop/ui/components/app/signature-request-original/signature-request-original.component.js#L75) to be made a utility method. - - Extract [renderBody](https://github.com/MetaMask/metamask-extension/blob/develop/ui/components/app/signature-request-original/signature-request-original.component.js#L110) into a reusable component. + - Create a reusable component for Cancel All requests for use across signature request pages [Code](https://github.com/MetaMask/metamask-extension/blob/e07ec9dcf3d3f341f83e6b29a29d30edaf7f5b5b/ui/components/app/signature-request-original/signature-request-original.component.js#L326). + - Extract [getNetrowkName](https://github.com/MetaMask/metamask-extension/blob/e07ec9dcf3d3f341f83e6b29a29d30edaf7f5b5b/ui/components/app/signature-request-original/signature-request-original.component.js#L60) into a reusable hook / utility method. + - [msgHexToText](https://github.com/MetaMask/metamask-extension/blob/e07ec9dcf3d3f341f83e6b29a29d30edaf7f5b5b/ui/components/app/signature-request-original/signature-request-original.component.js#L79) to be made a utility method. + - Extract [renderBody](https://github.com/MetaMask/metamask-extension/blob/e07ec9dcf3d3f341f83e6b29a29d30edaf7f5b5b/ui/components/app/signature-request-original/signature-request-original.component.js#L114) into a reusable component. 6. ### Refactoring in signature-request - Get rid of container components and for other components migrate to functional react components. - Reuse the Header component created for signature-request pages - Reuse the footer component created for confirmation pages. - - Extract [formatWallet](https://github.com/MetaMask/metamask-extension/blob/develop/ui/components/app/signature-request/signature-request.component.js#L85) into a utility method. + - Extract [formatWallet](https://github.com/MetaMask/metamask-extension/blob/e07ec9dcf3d3f341f83e6b29a29d30edaf7f5b5b/ui/components/app/signature-request/signature-request.component.js#L93) into a utility method. 7. ### Refactoring in signature-request-siwe - - Footer component use `PageContainerFooter` can be converted into a footer component for all confirmation pages. [#17237](https://github.com/MetaMask/metamask-extension/issues/17237) + - Footer component use `PageContainerFooter` can be used as footer component for all confirmation pages. [#17237](https://github.com/MetaMask/metamask-extension/issues/17237) diff --git a/docs/refactoring/signature-request/eth_sign.png b/docs/confirmation-refactoring/signature-request/eth_sign.png similarity index 100% rename from docs/refactoring/signature-request/eth_sign.png rename to docs/confirmation-refactoring/signature-request/eth_sign.png diff --git a/docs/refactoring/signature-request/footer.png b/docs/confirmation-refactoring/signature-request/footer.png similarity index 100% rename from docs/refactoring/signature-request/footer.png rename to docs/confirmation-refactoring/signature-request/footer.png diff --git a/docs/refactoring/signature-request/header.png b/docs/confirmation-refactoring/signature-request/header.png similarity index 100% rename from docs/refactoring/signature-request/header.png rename to docs/confirmation-refactoring/signature-request/header.png diff --git a/docs/refactoring/signature-request/personal_sign.png b/docs/confirmation-refactoring/signature-request/personal_sign.png similarity index 100% rename from docs/refactoring/signature-request/personal_sign.png rename to docs/confirmation-refactoring/signature-request/personal_sign.png diff --git a/docs/refactoring/signature-request/signature_request_old.png b/docs/confirmation-refactoring/signature-request/signature_request_old.png similarity index 100% rename from docs/refactoring/signature-request/signature_request_old.png rename to docs/confirmation-refactoring/signature-request/signature_request_old.png diff --git a/docs/refactoring/signature-request/signature_request_proposed.png b/docs/confirmation-refactoring/signature-request/signature_request_proposed.png similarity index 100% rename from docs/refactoring/signature-request/signature_request_proposed.png rename to docs/confirmation-refactoring/signature-request/signature_request_proposed.png diff --git a/docs/refactoring/signature-request/siwe.png b/docs/confirmation-refactoring/signature-request/siwe.png similarity index 100% rename from docs/refactoring/signature-request/siwe.png rename to docs/confirmation-refactoring/signature-request/siwe.png diff --git a/docs/refactoring/signature-request/v1.png b/docs/confirmation-refactoring/signature-request/v1.png similarity index 100% rename from docs/refactoring/signature-request/v1.png rename to docs/confirmation-refactoring/signature-request/v1.png diff --git a/docs/refactoring/signature-request/v3.png b/docs/confirmation-refactoring/signature-request/v3.png similarity index 100% rename from docs/refactoring/signature-request/v3.png rename to docs/confirmation-refactoring/signature-request/v3.png diff --git a/docs/refactoring/signature-request/v4.png b/docs/confirmation-refactoring/signature-request/v4.png similarity index 100% rename from docs/refactoring/signature-request/v4.png rename to docs/confirmation-refactoring/signature-request/v4.png diff --git a/docs/refactoring/README.md b/docs/refactoring/README.md deleted file mode 100644 index a8b82d577..000000000 --- a/docs/refactoring/README.md +++ /dev/null @@ -1,7 +0,0 @@ -# Confirmation Pages Refactoring - -The document details about refactoring confirmation pages. It describes the current code and improved proposed architecture. - -1. [Signature Request Pages](https://github.com/MetaMask/metamask-extension/tree/develop/docs/refactoring/signature-request) - -2. [Confirmation Pages Routing](https://github.com/MetaMask/metamask-extension/tree/develop/docs/refactoring/confirmation-pages-routing) diff --git a/docs/refactoring/confirmation-pages-routing/README.md b/docs/refactoring/confirmation-pages-routing/README.md deleted file mode 100644 index b47f10055..000000000 --- a/docs/refactoring/confirmation-pages-routing/README.md +++ /dev/null @@ -1,65 +0,0 @@ -# Refactoring - Confirmation pages routing - -This document details how routing to confirmation pages is currently done and the proposed improvements in routing. - -## Current flow - -The current flow of routing to confirmation pages is un-necessarily complicated and have issues. - -![Confirmation Pages Routing - Current](https://raw.githubusercontent.com/MetaMask/metamask-extension/develop/docs/refactoring/confirmation-pages-routing/current.png) - -- There are 2 ways in which confirmation pages can be opened: - 1. User triggers send flow from within Metamask - - If the user triggers the send flow from within MetaMask and selects the recipient and amount on the send screen, an unapproved transaction is created in the background and the user is redirected to the **/confirm-transaction** route. - 2. DAPP sends request to Metamask - - If DAPP sends request to Metamask an un-approved transaction or signature request is created in background and UI is triggered open (if it is not already open). - - The router by default renders `pages/home` component. The component looks at the state and if it finds an un-approved transaction or signature request in state it re-routes to **/confirm-transaction**. -- For **/confirm-transaction/** route, the router renders `pages/confirm-transaction` component. -- For **/confirm-transaction** route `pages/confirm-transaction` component renders `pages/confirm-transaction-switch` by default (for token methods it renders `pages/confirm-transaction/confirm-token-transaction-switch` which also open `pages/confirm-transaction-switch` by default). -- `pages/confirm-token-switch` redirect to specific confirmation page route depending on un-approved transaction or signature request in the state. -- For specific route **/confirm-transaction/${id}/XXXXX** routes also `pages/confirm-transaction` is rendered. -- Depending on confirmation route `pages/confirm-transaction` and `pages/confirm-transaction/confirm-token-transaction-switch` renders specific confirmation page component. - -## Proposed flow - -The proposed routing of confirmation pages looks like. - -![Confirmation Pages Routing - Proposed](https://raw.githubusercontent.com/MetaMask/metamask-extension/develop/docs/refactoring/confirmation-pages-routing/proposed.png) - -- There are 2 ways in which confirmation pages can be opened: - 1. User triggers send flow from within Metamask - - [changed] If the user triggers the send flow from within MetaMask and selects the recipient and amount on the send screen, an unapproved transaction is created in the background and the user is redirected to a specific transaction route, **/confirm-transaction/${id}/XXXX**, depending on the transaction type. - 2. DAPP sends request to Metamask - - If DAPP send request to Metamask an un-approved transaction or signature request is created in background and UI is triggered to open (if it is not already open). - - [changed] Instead of rendering `pages/home`, `pages/routes` finds the unapproved transaction in state and reroutes to **/confirm-transaction**. -- Router renders `pages/confirm-transaction` component for **/confirm-transaction** route. -- `pages/confirm-transaction` component redirect to specific confirmation page route depending on un-approved transaction or signature request in the state. -- Again for specific route **/confirm-transaction/${id}/XXXXX** `pages/confirm-transaction` is rendered, it in-turn renders appropriate confirmation page for the specific route. - -## Current Route component mapping - -| Route | Component | -| ----------------------------------------------- | ------------------------------------ | -| /confirm-transaction/${id}/deploy-contract | pages/confirm-deploy-contract | -| /confirm-transaction/${id}/send-ether | pages/confirm-send-ether | -| /confirm-transaction/${id}/send-token | pages/confirm-send-token | -| /confirm-transaction/${id}/approve | pages/confirm-approve | -| /confirm-transaction/${id}/set-approval-for-all | pages/confirm-approve | -| /confirm-transaction/${id}/transfer-from | pages/confirm-token-transaction-base | -| /confirm-transaction/${id}/safe-transfer-from | pages/confirm-token-transaction-base | -| /confirm-transaction/${id}/token-method | pages/confirm-contract-interaction | -| /confirm-transaction/${id}/signature-request | pages/confirm-signature-request.js | - -## Areas of code refactoring -- **Routing to mostRecentOverviewPage** - Across confirmation pages there is code to re-direct to `mostRecentOverviewPage`. `mostRecentOverviewPage` is equal to default route `/` or `/asset` whichever was last opened. - - Also a lot of components check for state update and as soon as state has `0` pending un-approved transaction or signature request redirect is done to `mostRecentOverviewPage`. This logic can be handled at `/pages/confirm-transaction` which is always rendered for any confirmation page. - - Also when the transaction is completed / rejected redirect is done to `mostRecentOverviewPage` explicitly which we should continue to do. -- Any re-usable routing related code should be moved to [useRouting](https://github.com/MetaMask/metamask-extension/blob/develop/ui/hooks/useRouting.js) hook. -- Logic to initially check state and redirect to `/pages/confirm-transaction` can be moved from `/pages/home` to `pages/routes` -- Confirmation components have lot of props passing which needs to be reduced. Values can be obtained from redux state or other contexts directly using hooks. Component [confirm-token-transaction-switch](https://github.com/MetaMask/metamask-extension/blob/develop/ui/pages/confirm-transaction/confirm-token-transaction-switch.js) has a lot of un-necessary props passing which should be removed and will help to further refactor routing. -- All the route mapping code should be moved to `/pages/confirm-transaction`, this will require getting rid of route mappings in `/pages/confirm-transaction/confirm-token-transaction-switch`, `/pages/confirm-transaction-switch`. -- `/pages/confirm-transaction-switch` has the code that check the un-approved trancation / message in state and reditect to a specific route, a utility method can be create to do this mapping and can be included in `/pages/confirm-transaction` component. -- During the send flow initiated within metamask user should be redirected to specific confirmations route **/confirm-transaction/${id}/XXXX** \ No newline at end of file diff --git a/test/data/mock-state.json b/test/data/mock-state.json index 41d995b9e..b7a08d5c5 100644 --- a/test/data/mock-state.json +++ b/test/data/mock-state.json @@ -16,7 +16,8 @@ "name": null } }, - "warning": null + "warning": null, + "customTokenAmount": "10" }, "history": { "mostRecentOverviewPage": "/mostRecentOverviewPage" diff --git a/ui/components/app/custom-spending-cap/__snapshots__/custom-spending-cap.test.js.snap b/ui/components/app/custom-spending-cap/__snapshots__/custom-spending-cap.test.js.snap new file mode 100644 index 000000000..ba7f51b89 --- /dev/null +++ b/ui/components/app/custom-spending-cap/__snapshots__/custom-spending-cap.test.js.snap @@ -0,0 +1,107 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`CustomSpendingCap should match snapshot 1`] = ` +
+
+
+ +
+
+
+`; diff --git a/ui/components/app/custom-spending-cap/custom-spending-cap.js b/ui/components/app/custom-spending-cap/custom-spending-cap.js index 7f4a46c26..23e8d16ee 100644 --- a/ui/components/app/custom-spending-cap/custom-spending-cap.js +++ b/ui/components/app/custom-spending-cap/custom-spending-cap.js @@ -3,6 +3,8 @@ import { useDispatch, useSelector } from 'react-redux'; import PropTypes from 'prop-types'; import classnames from 'classnames'; import BigNumber from 'bignumber.js'; +import { addHexPrefix } from 'ethereumjs-util'; + import { I18nContext } from '../../../contexts/i18n'; import Box from '../../ui/box'; import FormField from '../../ui/form-field'; @@ -23,24 +25,31 @@ import { import { getCustomTokenAmount } from '../../../selectors'; import { setCustomTokenAmount } from '../../../ducks/app/app'; import { calcTokenAmount } from '../../../../shared/lib/transactions-controller-utils'; +import { hexToDecimal } from '../../../../shared/modules/conversion.utils'; import { MAX_TOKEN_ALLOWANCE_AMOUNT, NUM_W_OPT_DECIMAL_COMMA_OR_DOT_REGEX, DECIMAL_REGEX, } from '../../../../shared/constants/tokens'; import { Numeric } from '../../../../shared/modules/Numeric'; +import { estimateGas } from '../../../store/actions'; +import { getCustomTxParamsData } from '../../../pages/confirm-approve/confirm-approve.util'; +import { useGasFeeContext } from '../../../contexts/gasFee'; import { CustomSpendingCapTooltip } from './custom-spending-cap-tooltip'; export default function CustomSpendingCap({ + txParams, tokenName, currentTokenBalance, dappProposedValue, siteOrigin, passTheErrorText, decimals, + setInputChangeInProgress, }) { const t = useContext(I18nContext); const dispatch = useDispatch(); + const { updateTransaction } = useGasFeeContext(); const inputRef = useRef(null); const value = useSelector(getCustomTokenAmount); @@ -97,12 +106,17 @@ export default function CustomSpendingCap({ getInputTextLogic(value).description, ); - const handleChange = (valueInput) => { + const handleChange = async (valueInput) => { + if (!txParams) { + return; + } + setInputChangeInProgress(true); let spendingCapError = ''; const inputTextLogic = getInputTextLogic(valueInput); const inputTextLogicDescription = inputTextLogic.description; const match = DECIMAL_REGEX.exec(replaceCommaToDot(valueInput)); if (match?.[1]?.length > decimals) { + setInputChangeInProgress(false); return; } @@ -128,6 +142,28 @@ export default function CustomSpendingCap({ } dispatch(setCustomTokenAmount(String(valueInput))); + + try { + const newData = getCustomTxParamsData(txParams.data, { + customPermissionAmount: valueInput, + decimals, + }); + const { from, to, value: txValue } = txParams; + const estimatedGasLimit = await estimateGas({ + from, + to, + value: txValue, + data: newData, + }); + if (estimatedGasLimit) { + await updateTransaction({ + gasLimit: hexToDecimal(addHexPrefix(estimatedGasLimit)), + }); + } + } catch (exp) { + console.error('Error in trying to update gas limit', exp); + } + setInputChangeInProgress(false); }; useEffect(() => { @@ -278,6 +314,10 @@ export default function CustomSpendingCap({ } CustomSpendingCap.propTypes = { + /** + * Transaction params + */ + txParams: PropTypes.object.isRequired, /** * Displayed the token name currently tracked in description related to the input state */ @@ -302,4 +342,8 @@ CustomSpendingCap.propTypes = { * Number of decimals */ decimals: PropTypes.string, + /** + * Updating input state to changing + */ + setInputChangeInProgress: PropTypes.func.isRequired, }; diff --git a/ui/components/app/custom-spending-cap/custom-spending-cap.test.js b/ui/components/app/custom-spending-cap/custom-spending-cap.test.js new file mode 100644 index 000000000..17f475691 --- /dev/null +++ b/ui/components/app/custom-spending-cap/custom-spending-cap.test.js @@ -0,0 +1,61 @@ +import React from 'react'; +import configureMockStore from 'redux-mock-store'; +import thunk from 'redux-thunk'; +import userEvent from '@testing-library/user-event'; + +import mockState from '../../../../test/data/mock-state.json'; +import { renderWithProvider } from '../../../../test/lib/render-helpers'; +import * as Actions from '../../../store/actions'; +import * as GasFeeContext from '../../../contexts/gasFee'; +import CustomSpendingCap from './custom-spending-cap'; + +const props = { + txParams: { + data: '0x095ea7b30000000000000000000000009bc5baf874d2da8d216ae9f137804184ee5afef40000000000000000000000000000000000000000000000000000000000011170', + from: '0x8eeee1781fd885ff5ddef7789486676961873d12', + gas: '0xb41b', + maxFeePerGas: '0x4a817c800', + maxPriorityFeePerGas: '0x4a817c800', + to: '0x665933d73375e385bef40abcccea8b4cccc32d4c', + value: '0x0', + }, + tokenName: 'TST', + currentTokenBalance: '10', + dappProposedValue: '7', + siteOrigin: 'https://metamask.github.io', + decimals: '4', + passTheErrorText: () => undefined, + setInputChangeInProgress: () => undefined, +}; + +describe('CustomSpendingCap', () => { + const store = configureMockStore([thunk])(mockState); + + it('should match snapshot', () => { + const { container } = renderWithProvider( + , + store, + ); + expect(container).toMatchSnapshot(); + }); + + it('should change in token allowance amount should call functions to update gas limit', async () => { + const user = userEvent.setup(); + const spyEstimateGas = jest + .spyOn(Actions, 'estimateGas') + .mockReturnValue(Promise.resolve('1770')); + const updateTransactionMock = jest.fn(); + jest + .spyOn(GasFeeContext, 'useGasFeeContext') + .mockImplementation(() => ({ updateTransaction: updateTransactionMock })); + + const { getByRole } = renderWithProvider( + , + store, + ); + await user.type(getByRole('textbox'), '5'); + + expect(spyEstimateGas).toHaveBeenCalledTimes(1); + expect(updateTransactionMock).toHaveBeenCalledTimes(1); + }); +}); diff --git a/ui/components/app/set-approval-for-all-warning/set-approval-for-all-warning.js b/ui/components/app/set-approval-for-all-warning/set-approval-for-all-warning.js index 83baa2bde..f6f0794f6 100644 --- a/ui/components/app/set-approval-for-all-warning/set-approval-for-all-warning.js +++ b/ui/components/app/set-approval-for-all-warning/set-approval-for-all-warning.js @@ -105,7 +105,7 @@ const SetApproveForAllWarning = ({ key="non_custodial_bold" className="set-approval-for-all-warning__content__bold" > - {t('nftWarningContentBold', [collectionName])} + {t('nftWarningContentBold', [collectionName || ''])} , {t('nftWarningContentGrey')} diff --git a/ui/components/ui/callout/callout.js b/ui/components/ui/callout/callout.js index 8538d69cd..6c28dc117 100644 --- a/ui/components/ui/callout/callout.js +++ b/ui/components/ui/callout/callout.js @@ -5,11 +5,7 @@ import InfoIconInverted from '../icon/info-icon-inverted.component'; import { SEVERITIES, Color } from '../../../helpers/constants/design-system'; import { MILLISECOND } from '../../../../shared/constants/time'; import Typography from '../typography'; -import { ButtonIcon } from '../../component-library/button-icon/deprecated'; -import { - ICON_NAMES, - ICON_SIZES, -} from '../../component-library/icon/deprecated'; +import { ButtonIcon, IconName, IconSize } from '../../component-library'; export default function Callout({ severity, @@ -47,8 +43,8 @@ export default function Callout({ {dismiss && ( { setRemoved(true); diff --git a/ui/components/ui/contract-token-values/contract-token-values.js b/ui/components/ui/contract-token-values/contract-token-values.js index b34b34b6a..616828598 100644 --- a/ui/components/ui/contract-token-values/contract-token-values.js +++ b/ui/components/ui/contract-token-values/contract-token-values.js @@ -16,8 +16,7 @@ import { Color, } from '../../../helpers/constants/design-system'; import { useCopyToClipboard } from '../../../hooks/useCopyToClipboard'; -import { ButtonIcon } from '../../component-library/button-icon/deprecated'; -import { ICON_NAMES } from '../../component-library/icon/deprecated'; +import { ButtonIcon, IconName } from '../../component-library'; export default function ContractTokenValues({ address, @@ -51,7 +50,7 @@ export default function ContractTokenValues({ title={copied ? t('copiedExclamation') : t('copyToClipboard')} > handleCopy(address)} ariaLabel={copied ? t('copiedExclamation') : t('copyToClipboard')} @@ -60,7 +59,7 @@ export default function ContractTokenValues({ { const blockExplorerTokenLink = getAccountLink( diff --git a/ui/components/ui/disclosure/disclosure.js b/ui/components/ui/disclosure/disclosure.js index c45b60153..e20746308 100644 --- a/ui/components/ui/disclosure/disclosure.js +++ b/ui/components/ui/disclosure/disclosure.js @@ -1,8 +1,7 @@ import React, { useState, useRef, useEffect } from 'react'; import PropTypes from 'prop-types'; import classnames from 'classnames'; -import { Icon, ICON_NAMES } from '../../component-library/icon/deprecated'; -import { Size } from '../../../helpers/constants/design-system'; +import { Icon, IconName, IconSize } from '../../component-library'; const Disclosure = ({ children, title, size }) => { const disclosureFooterEl = useRef(null); @@ -27,8 +26,8 @@ const Disclosure = ({ children, title, size }) => { {title} diff --git a/ui/components/ui/dropdown/dropdown.js b/ui/components/ui/dropdown/dropdown.js index a6f556f3c..802fd0a0b 100644 --- a/ui/components/ui/dropdown/dropdown.js +++ b/ui/components/ui/dropdown/dropdown.js @@ -1,11 +1,7 @@ import React, { useCallback } from 'react'; import PropTypes from 'prop-types'; import classnames from 'classnames'; -import { - Icon, - ICON_NAMES, - ICON_SIZES, -} from '../../component-library/icon/deprecated'; +import { Icon, IconName, IconSize } from '../../component-library'; const Dropdown = ({ className, @@ -46,8 +42,8 @@ const Dropdown = ({ })} diff --git a/ui/components/ui/editable-label/editable-label.js b/ui/components/ui/editable-label/editable-label.js index 98385776d..7a0558cd8 100644 --- a/ui/components/ui/editable-label/editable-label.js +++ b/ui/components/ui/editable-label/editable-label.js @@ -3,8 +3,7 @@ import PropTypes from 'prop-types'; import React, { Component } from 'react'; import { Color } from '../../../helpers/constants/design-system'; import { getAccountNameErrorMessage } from '../../../helpers/utils/accounts'; -import { ButtonIcon } from '../../component-library/button-icon/deprecated'; -import { ICON_NAMES } from '../../component-library/icon/deprecated'; +import { ButtonIcon, IconName } from '../../component-library'; export default class EditableLabel extends Component { static propTypes = { @@ -60,7 +59,7 @@ export default class EditableLabel extends Component { autoFocus /> this.handleSubmit(isValidAccountName)} /> @@ -76,7 +75,7 @@ export default class EditableLabel extends Component {
{this.state.value}
this.setState({ isEditing: true })} diff --git a/ui/components/ui/error-message/error-message.component.js b/ui/components/ui/error-message/error-message.component.js index ec7a5e3fc..53f50f90f 100644 --- a/ui/components/ui/error-message/error-message.component.js +++ b/ui/components/ui/error-message/error-message.component.js @@ -1,7 +1,7 @@ import React from 'react'; import PropTypes from 'prop-types'; -import { Icon, ICON_NAMES } from '../../component-library/icon/deprecated'; -import { IconColor, Size } from '../../../helpers/constants/design-system'; +import { Icon, IconName, IconSize } from '../../component-library'; +import { IconColor } from '../../../helpers/constants/design-system'; /** * @deprecated - Please use ActionableMessage type danger @@ -19,8 +19,8 @@ const ErrorMessage = (props, context) => {
diff --git a/ui/components/ui/form-field/form-field.stories.js b/ui/components/ui/form-field/form-field.stories.js index 101e092ab..8a620b9e5 100644 --- a/ui/components/ui/form-field/form-field.stories.js +++ b/ui/components/ui/form-field/form-field.stories.js @@ -4,7 +4,7 @@ import React, { useState } from 'react'; import Typography from '../typography'; import Tooltip from '../tooltip'; -import { Icon, ICON_NAMES } from '../../component-library/icon/deprecated'; +import { Icon, IconName } from '../../component-library'; import { AlignItems } from '../../../helpers/constants/design-system'; import README from './README.mdx'; import FormField from '.'; @@ -70,7 +70,7 @@ export const FormFieldWithTitleDetail = (args) => { Click Me ), - checkmark: , + checkmark: , }; return ; @@ -108,7 +108,7 @@ export const CustomComponents = (args) => { position="top" html={Custom tooltip} > - + } titleDetail={TitleDetail} diff --git a/ui/components/ui/menu/menu.stories.js b/ui/components/ui/menu/menu.stories.js index 9ff6c660f..1eed57ddb 100644 --- a/ui/components/ui/menu/menu.stories.js +++ b/ui/components/ui/menu/menu.stories.js @@ -1,6 +1,6 @@ import React, { useState } from 'react'; import { action } from '@storybook/addon-actions'; -import { ICON_NAMES } from '../../component-library/icon/deprecated'; +import { IconName } from '../../component-library'; import { Menu, MenuItem } from '.'; export default { @@ -10,11 +10,11 @@ export default { export const DefaultStory = () => { return ( - + Menu Item 1 Menu Item 2 - + Menu Item 3 @@ -29,14 +29,11 @@ export const Anchored = () => { <> - + Menu Item 1 Menu Item 2 - + Menu Item 3 diff --git a/ui/components/ui/new-network-info/new-network-info.js b/ui/components/ui/new-network-info/new-network-info.js index 7c1278ec4..e8637b285 100644 --- a/ui/components/ui/new-network-info/new-network-info.js +++ b/ui/components/ui/new-network-info/new-network-info.js @@ -26,7 +26,7 @@ import { IMPORT_TOKEN_ROUTE } from '../../../helpers/constants/routes'; import Chip from '../chip/chip'; import { setFirstTimeUsedNetwork } from '../../../store/actions'; import { NETWORK_TYPES } from '../../../../shared/constants/network'; -import { Icon, ICON_NAMES } from '../../component-library/icon/deprecated'; +import { Icon, IconName } from '../../component-library'; const NewNetworkInfo = () => { const t = useContext(I18nContext); @@ -106,7 +106,7 @@ const NewNetworkInfo = () => { ) : ( ) diff --git a/ui/components/ui/nickname-popover/nickname-popover.component.js b/ui/components/ui/nickname-popover/nickname-popover.component.js index 60785b18a..51a135fab 100644 --- a/ui/components/ui/nickname-popover/nickname-popover.component.js +++ b/ui/components/ui/nickname-popover/nickname-popover.component.js @@ -11,11 +11,7 @@ import { shortenAddress } from '../../../helpers/utils/util'; import { useCopyToClipboard } from '../../../hooks/useCopyToClipboard'; import { getTokenList, getBlockExplorerLinkText } from '../../../selectors'; import { NETWORKS_ROUTE } from '../../../helpers/constants/routes'; -import { - ICON_NAMES, - ICON_SIZES, -} from '../../component-library/icon/deprecated'; -import { ButtonIcon } from '../../component-library/button-icon/deprecated'; +import { ButtonIcon, IconName, IconSize } from '../../component-library'; const NicknamePopover = ({ address, @@ -67,8 +63,8 @@ const NicknamePopover = ({ title={copied ? t('copiedExclamation') : t('copyToClipboard')} > handleCopy(address)} /> diff --git a/ui/components/ui/popover/popover.component.js b/ui/components/ui/popover/popover.component.js index 2653d64c9..c5b073d86 100644 --- a/ui/components/ui/popover/popover.component.js +++ b/ui/components/ui/popover/popover.component.js @@ -18,13 +18,14 @@ import { TEXT_ALIGN, BLOCK_SIZES, } from '../../../helpers/constants/design-system'; + import { + ButtonIcon, Icon, - ICON_NAMES, - ICON_SIZES, -} from '../../component-library/icon/deprecated'; -import { ButtonIcon } from '../../component-library/button-icon/deprecated'; -import { Text } from '../../component-library'; + IconName, + IconSize, + Text, +} from '../../component-library'; const defaultHeaderProps = { padding: [6, 4, 4], @@ -86,7 +87,7 @@ const Popover = ({ > {onBack ? ( {onClose ? ( diff --git a/ui/components/ui/qr-code/qr-code.js b/ui/components/ui/qr-code/qr-code.js index 4907cee50..d06d4c9f2 100644 --- a/ui/components/ui/qr-code/qr-code.js +++ b/ui/components/ui/qr-code/qr-code.js @@ -9,11 +9,7 @@ import Tooltip from '../tooltip'; import { useI18nContext } from '../../../hooks/useI18nContext'; import { AddressCopyButton } from '../../multichain/address-copy-button'; import Box from '../box/box'; -import { - Icon, - ICON_NAMES, - ICON_SIZES, -} from '../../component-library/icon/deprecated'; +import { Icon, IconName, IconSize } from '../../component-library'; export default connect(mapStateToProps)(QrCodeView); @@ -80,8 +76,8 @@ function QrCodeView(props) { >
{toChecksumHexAddress(data)}
diff --git a/ui/components/ui/review-spending-cap/review-spending-cap.js b/ui/components/ui/review-spending-cap/review-spending-cap.js index 9cbdd728b..1fe08023e 100644 --- a/ui/components/ui/review-spending-cap/review-spending-cap.js +++ b/ui/components/ui/review-spending-cap/review-spending-cap.js @@ -4,12 +4,7 @@ import { I18nContext } from '../../../contexts/i18n'; import Box from '../box'; import Tooltip from '../tooltip'; import Typography from '../typography'; -import { ButtonLink } from '../../component-library'; -import { - Icon, - ICON_NAMES, - ICON_SIZES, -} from '../../component-library/icon/deprecated'; +import { ButtonLink, Icon, IconName, IconSize } from '../../component-library'; import { AlignItems, DISPLAY, @@ -86,7 +81,7 @@ export default function ReviewSpendingCap({ color={TextColor.errorDefault} > {t('beCareful')} @@ -100,16 +95,16 @@ export default function ReviewSpendingCap({ {valueIsGreaterThanBalance && ( )} {Number(tokenValue) === 0 && ( )} diff --git a/ui/components/ui/sender-to-recipient/sender-to-recipient.component.js b/ui/components/ui/sender-to-recipient/sender-to-recipient.component.js index af6abb630..49cdcc8e0 100644 --- a/ui/components/ui/sender-to-recipient/sender-to-recipient.component.js +++ b/ui/components/ui/sender-to-recipient/sender-to-recipient.component.js @@ -9,7 +9,7 @@ import AccountMismatchWarning from '../account-mismatch-warning/account-mismatch import { useI18nContext } from '../../../hooks/useI18nContext'; import { toChecksumHexAddress } from '../../../../shared/modules/hexstring-utils'; import NicknamePopovers from '../../app/modals/nickname-popovers'; -import { Icon, ICON_NAMES } from '../../component-library/icon/deprecated'; +import { Icon, IconName } from '../../component-library'; import { DEFAULT_VARIANT, CARDS_VARIANT, @@ -199,7 +199,7 @@ function Arrow({ variant }) {
) : (
- +
); } diff --git a/ui/components/ui/tabs/dropdown-tab/dropdown-tab.js b/ui/components/ui/tabs/dropdown-tab/dropdown-tab.js deleted file mode 100644 index 6da690afc..000000000 --- a/ui/components/ui/tabs/dropdown-tab/dropdown-tab.js +++ /dev/null @@ -1,65 +0,0 @@ -import React from 'react'; -import PropTypes from 'prop-types'; -import classnames from 'classnames'; -import Dropdown from '../../dropdown'; -import Box from '../../box'; - -export const DropdownTab = (props) => { - const { - activeClassName, - className, - 'data-testid': dataTestId, - isActive, - onClick, - onChange, - tabIndex, - options, - selectedOption, - } = props; - - return ( - { - event.preventDefault(); - onClick(tabIndex); - }} - > - - - ); -}; - -DropdownTab.propTypes = { - activeClassName: PropTypes.string, - className: PropTypes.string, - 'data-testid': PropTypes.string, - isActive: PropTypes.bool, // required, but added using React.cloneElement - options: PropTypes.arrayOf( - PropTypes.exact({ - name: PropTypes.string, - value: PropTypes.string.isRequired, - }), - ).isRequired, - selectedOption: PropTypes.string, - onChange: PropTypes.func, - onClick: PropTypes.func, - tabIndex: PropTypes.number, // required, but added using React.cloneElement -}; - -DropdownTab.defaultProps = { - activeClassName: undefined, - className: undefined, - onChange: undefined, - onClick: undefined, - selectedOption: undefined, -}; diff --git a/ui/components/ui/tabs/dropdown-tab/index.scss b/ui/components/ui/tabs/dropdown-tab/index.scss deleted file mode 100644 index 01ee7f077..000000000 --- a/ui/components/ui/tabs/dropdown-tab/index.scss +++ /dev/null @@ -1,23 +0,0 @@ -.tab { - .dropdown__select { - border: none; - font-size: unset; - width: 100%; - background-color: unset; - padding-left: 8px; - padding-right: 20px; - line-height: unset; - - option { - background-color: var(--color-background-default); - } - - &:focus-visible { - outline: none; - } - } - - .dropdown__icon-caret-down { - right: 0; - } -} diff --git a/ui/components/ui/tabs/flask/dropdown-tab/dropdown-tab.js b/ui/components/ui/tabs/flask/dropdown-tab/dropdown-tab.js new file mode 100644 index 000000000..bbb48602c --- /dev/null +++ b/ui/components/ui/tabs/flask/dropdown-tab/dropdown-tab.js @@ -0,0 +1,160 @@ +import React, { useCallback, useEffect, useRef, useState } from 'react'; +import PropTypes from 'prop-types'; +import classnames from 'classnames'; +import Box from '../../../box'; +import { + AlignItems, + BLOCK_SIZES, + BackgroundColor, + BorderColor, + BorderRadius, + BorderStyle, + DISPLAY, + FLEX_DIRECTION, + FLEX_WRAP, + TextVariant, +} from '../../../../../helpers/constants/design-system'; +import { Icon, IconName, IconSize, Text } from '../../../../component-library'; + +export const DropdownTab = ({ + activeClassName, + className, + 'data-testid': dataTestId, + isActive, + onClick, + onChange, + tabIndex, + options, + selectedOption, +}) => { + const [isOpen, setIsOpen] = useState(false); + + const dropdownRef = useRef(null); + + const selectOption = useCallback( + (event, option) => { + event.stopPropagation(); + onChange(option.value); + setIsOpen(false); + }, + [onChange], + ); + + const openDropdown = (event) => { + event.preventDefault(); + setIsOpen(true); + onClick(tabIndex); + }; + + const selectedOptionName = options.find( + (option) => option.value === selectedOption, + )?.name; + + useEffect(() => { + function handleClickOutside(event) { + if ( + dropdownRef.current && + !dropdownRef.current.contains(event.target) && + isOpen + ) { + setIsOpen(false); + } + } + + document.addEventListener('mousedown', handleClickOutside); + return () => { + document.removeEventListener('mousedown', handleClickOutside); + }; + }, [dropdownRef, isOpen]); + + return ( + + + + {selectedOptionName} + + + + {isOpen && ( + + {options.map((option, i) => ( + selectOption(event, option)} + style={{ + cursor: 'pointer', + textTransform: 'none', + whiteSpace: 'nowrap', + overflow: 'hidden', + textOverflow: 'ellipsis', + }} + > + {option.name} + + ))} + + )} + + ); +}; + +DropdownTab.propTypes = { + activeClassName: PropTypes.string, + className: PropTypes.string, + 'data-testid': PropTypes.string, + isActive: PropTypes.bool, // required, but added using React.cloneElement + options: PropTypes.arrayOf( + PropTypes.exact({ + name: PropTypes.string, + value: PropTypes.string.isRequired, + }), + ).isRequired, + selectedOption: PropTypes.string, + onChange: PropTypes.func, + onClick: PropTypes.func, + tabIndex: PropTypes.number, // required, but added using React.cloneElement +}; + +DropdownTab.defaultProps = { + activeClassName: undefined, + className: undefined, + onChange: undefined, + onClick: undefined, + selectedOption: undefined, +}; diff --git a/ui/components/ui/tabs/flask/dropdown-tab/dropdown-tab.test.js b/ui/components/ui/tabs/flask/dropdown-tab/dropdown-tab.test.js new file mode 100644 index 000000000..61b19a847 --- /dev/null +++ b/ui/components/ui/tabs/flask/dropdown-tab/dropdown-tab.test.js @@ -0,0 +1,48 @@ +import * as React from 'react'; +import { render, fireEvent } from '@testing-library/react'; +import DropdownTab from '.'; + +describe('DropdownTab', () => { + const onChange = jest.fn(); + const onClick = jest.fn(); + let args; + beforeEach(() => { + args = { + activeClassName: 'active', + tabIndex: 1, + options: [ + { name: 'foo', value: 'foo' }, + { name: 'bar', value: 'bar' }, + ], + selectedOption: 'foo', + onChange, + onClick, + }; + }); + it('should render the DropdownTab component without crashing', () => { + const { getByText } = render(); + + expect(getByText(args.options[0].name)).toBeDefined(); + }); + + it('registers click', () => { + const { container } = render(); + + fireEvent.click(container.firstChild); + + expect(onClick).toHaveBeenCalledWith(args.tabIndex); + }); + + it('registers selection', () => { + const { container, getByText } = render(); + + fireEvent.click(container.firstChild); + + const element = getByText(args.options[1].name); + + fireEvent.click(element); + + expect(onClick).toHaveBeenCalledWith(args.tabIndex); + expect(onChange).toHaveBeenCalledWith(args.options[1].value); + }); +}); diff --git a/ui/components/ui/tabs/dropdown-tab/index.js b/ui/components/ui/tabs/flask/dropdown-tab/index.js similarity index 100% rename from ui/components/ui/tabs/dropdown-tab/index.js rename to ui/components/ui/tabs/flask/dropdown-tab/index.js diff --git a/ui/components/ui/tabs/index.js b/ui/components/ui/tabs/index.js index c20ebbfc1..43366ec6f 100644 --- a/ui/components/ui/tabs/index.js +++ b/ui/components/ui/tabs/index.js @@ -1,5 +1,4 @@ import Tabs from './tabs.component'; import Tab from './tab'; -import DropdownTab from './dropdown-tab'; -export { Tabs, Tab, DropdownTab }; +export { Tabs, Tab }; diff --git a/ui/components/ui/tabs/index.scss b/ui/components/ui/tabs/index.scss index 4398ddf50..20ee131b7 100644 --- a/ui/components/ui/tabs/index.scss +++ b/ui/components/ui/tabs/index.scss @@ -1,5 +1,4 @@ @import 'tab/index'; -@import 'dropdown-tab/index'; .tabs { flex-grow: 1; @@ -11,6 +10,5 @@ position: sticky; top: 0; z-index: 2; - overflow: hidden; } } diff --git a/ui/components/ui/tabs/tabs.stories.js b/ui/components/ui/tabs/tabs.stories.js index d8d4c4b39..09f19f0bc 100644 --- a/ui/components/ui/tabs/tabs.stories.js +++ b/ui/components/ui/tabs/tabs.stories.js @@ -1,5 +1,5 @@ import React from 'react'; -import DropdownTab from './dropdown-tab'; +import DropdownTab from './flask/dropdown-tab'; import Tab from './tab/tab.component'; import Tabs from './tabs.component'; diff --git a/ui/hooks/useTransactionInsights.js b/ui/hooks/useTransactionInsights.js index ed7916c4f..d20dbfcc7 100644 --- a/ui/hooks/useTransactionInsights.js +++ b/ui/hooks/useTransactionInsights.js @@ -5,7 +5,8 @@ import { CHAIN_ID_TO_NETWORK_ID_MAP } from '../../shared/constants/network'; import { stripHexPrefix } from '../../shared/modules/hexstring-utils'; import { TransactionType } from '../../shared/constants/transaction'; import { getInsightSnaps } from '../selectors'; -import { DropdownTab, Tab } from '../components/ui/tabs'; +import { Tab } from '../components/ui/tabs'; +import DropdownTab from '../components/ui/tabs/flask/dropdown-tab'; import { SnapInsight } from '../components/app/confirm-page-container/flask/snap-insight'; const isAllowedTransactionTypes = (transactionType) => diff --git a/ui/pages/confirm-approve/confirm-approve.js b/ui/pages/confirm-approve/confirm-approve.js index 6980d4862..76d6db4c8 100644 --- a/ui/pages/confirm-approve/confirm-approve.js +++ b/ui/pages/confirm-approve/confirm-approve.js @@ -144,6 +144,7 @@ export default function ConfirmApprove({ const { iconUrl: siteImage = '' } = subjectMetadata[origin] || {}; + // Code below may need a additional look as ERC1155 tokens do not have a name let tokensText; if ( assetStandard === TokenStandard.ERC721 || @@ -199,6 +200,7 @@ export default function ConfirmApprove({ toAddress={toAddress} tokenSymbol={tokenSymbol} decimals={decimals} + fromAddressIsLedger={fromAddressIsLedger} /> {showCustomizeGasPopover && !supportsEIP1559 && ( +
+ +
+
+ 0 + + of + + 2 +
+
+ requests waiting to be acknowledged +
+
+
+ + +
+
+
+
+
+ + + Edit + +
+
+
+
+
+
+
+
+ + + + + +
+
+
+
+
+
+
+ Test Account +
+
+
+
+
+
+ +
+
+
+
+
+
+
+ + + + + +
+
+
+
+
+
+
+ 0x0c5...AaFb +
+
+
+
+
+
+
+ +
+
+ + https://metamask.github.io + +
+
+
+ + Sending ETH + +
+
+
+

+
+ + + + + 0 + +
+

+
+
+
+
+
+ +
+

+ Network is busy. Gas prices are high and estimates are less accurate. +

+
+
+
+
+
+
+ +
+
+
+ + + +
+
+
+
+
+
+
+
+
+
+
+ Gas +
+ + ( + estimated + ) + +
+
+
+ + + +
+
+
+
+
+
+
+
+
+ + + 0.00021 + +
+
+
+
+
+
+ + + 0.00021 + + + ETH + +
+
+
+
+
+
+
+
+ Unknown processing time +
+
+
+
+
+ + Max fee: + +
+
+
+ + + 0.00021 + + + ETH + +
+
+
+
+
+
+
+
+
+ Total +
+
+
+
+
+ + + 0.00021 + +
+
+
+
+
+
+ + + 0.00021 + + + ETH + +
+
+
+
+
+
+
+ Amount + gas fee +
+
+
+ + Max amount: + + +
+ + + 0.00021 + + + ETH + +
+
+
+
+
+
+
+
+
+
+ + +
+
+ + Reject 2 transactions + +
+
+
+ , +] +`; diff --git a/ui/pages/confirm-send-ether/confirm-send-ether.component.js b/ui/pages/confirm-send-ether/confirm-send-ether.component.js deleted file mode 100644 index 02b113010..000000000 --- a/ui/pages/confirm-send-ether/confirm-send-ether.component.js +++ /dev/null @@ -1,33 +0,0 @@ -import React, { Component } from 'react'; -import PropTypes from 'prop-types'; -import ConfirmTransactionBase from '../confirm-transaction-base'; -import { SEND_ROUTE } from '../../helpers/constants/routes'; - -export default class ConfirmSendEther extends Component { - static contextTypes = { - t: PropTypes.func, - }; - - static propTypes = { - editTransaction: PropTypes.func, - history: PropTypes.object, - }; - - handleEdit({ txData }) { - const { editTransaction, history } = this.props; - editTransaction(txData).then(() => { - history.push(SEND_ROUTE); - }); - } - - render() { - return ( - - this.handleEdit(confirmTransactionData) - } - /> - ); - } -} diff --git a/ui/pages/confirm-send-ether/confirm-send-ether.container.js b/ui/pages/confirm-send-ether/confirm-send-ether.container.js deleted file mode 100644 index 9bca08091..000000000 --- a/ui/pages/confirm-send-ether/confirm-send-ether.container.js +++ /dev/null @@ -1,22 +0,0 @@ -import { connect } from 'react-redux'; -import { compose } from 'redux'; -import { withRouter } from 'react-router-dom'; -import { editExistingTransaction } from '../../ducks/send'; -import { clearConfirmTransaction } from '../../ducks/confirm-transaction/confirm-transaction.duck'; -import { AssetType } from '../../../shared/constants/transaction'; -import ConfirmSendEther from './confirm-send-ether.component'; - -const mapDispatchToProps = (dispatch) => { - return { - editTransaction: async (txData) => { - const { id } = txData; - await dispatch(editExistingTransaction(AssetType.native, id.toString())); - dispatch(clearConfirmTransaction()); - }, - }; -}; - -export default compose( - withRouter, - connect(undefined, mapDispatchToProps), -)(ConfirmSendEther); diff --git a/ui/pages/confirm-send-ether/confirm-send-ether.js b/ui/pages/confirm-send-ether/confirm-send-ether.js new file mode 100644 index 000000000..04a207f9e --- /dev/null +++ b/ui/pages/confirm-send-ether/confirm-send-ether.js @@ -0,0 +1,35 @@ +import React from 'react'; +import { useDispatch } from 'react-redux'; +import { useHistory } from 'react-router-dom'; + +import { AssetType } from '../../../shared/constants/transaction'; +import { clearConfirmTransaction } from '../../ducks/confirm-transaction/confirm-transaction.duck'; +import { editExistingTransaction } from '../../ducks/send'; +import { SEND_ROUTE } from '../../helpers/constants/routes'; +import ConfirmTransactionBase from '../confirm-transaction-base'; + +const ConfirmSendEther = () => { + const dispatch = useDispatch(); + const history = useHistory(); + + const editTransaction = async (txData) => { + const { id } = txData; + await dispatch(editExistingTransaction(AssetType.native, id.toString())); + dispatch(clearConfirmTransaction()); + }; + + const handleEdit = ({ txData }) => { + editTransaction(txData).then(() => { + history.push(SEND_ROUTE); + }); + }; + + return ( + handleEdit(confirmTransactionData)} + /> + ); +}; + +export default ConfirmSendEther; diff --git a/ui/pages/confirm-send-ether/confirm-send-ether.stories-to-do.js b/ui/pages/confirm-send-ether/confirm-send-ether.stories-to-do.js deleted file mode 100644 index fe3d13146..000000000 --- a/ui/pages/confirm-send-ether/confirm-send-ether.stories-to-do.js +++ /dev/null @@ -1,26 +0,0 @@ -import React from 'react'; -import ConfirmSendEther from '.'; - -// eslint-disable-next-line import/no-anonymous-default-export -export default { - title: 'Pages/ConfirmSendEther', - - component: ConfirmSendEther, - argTypes: { - editTransaction: { - action: 'editTransaction', - }, - history: { - control: 'object', - }, - txParams: { - control: 'object', - }, - }, -}; - -export const DefaultStory = (args) => { - return ; -}; - -DefaultStory.storyName = 'Default'; diff --git a/ui/pages/confirm-send-ether/confirm-send-ether.stories.js b/ui/pages/confirm-send-ether/confirm-send-ether.stories.js new file mode 100644 index 000000000..3f29014c2 --- /dev/null +++ b/ui/pages/confirm-send-ether/confirm-send-ether.stories.js @@ -0,0 +1,58 @@ +import React from 'react'; +import { Provider } from 'react-redux'; + +import mockState from '../../../test/data/mock-state.json'; +import configureStore from '../../store/store'; +import ConfirmSendEther from './confirm-send-ether'; + +const sendEther = { + id: 9597986287241458, + time: 1681203297082, + status: 'unapproved', + metamaskNetworkId: '5', + originalGasEstimate: '0x5208', + userEditedGasLimit: false, + chainId: '0x5', + loadingDefaults: false, + dappSuggestedGasFees: { + maxPriorityFeePerGas: '0x3b9aca00', + maxFeePerGas: '0x2540be400', + }, + sendFlowHistory: [], + txParams: { + from: '0x0dcd5d886577d5081b0c52e242ef29e70be3e7bc', + to: '0x0c54fccd2e384b4bb6f2e405bf5cbc15a017aafb', + value: '0x0', + gas: '0x5208', + maxFeePerGas: '0x2540be400', + maxPriorityFeePerGas: '0x3b9aca00', + }, + origin: 'https://metamask.github.io', + actionId: 1830698773, + type: 'simpleSend', + securityProviderResponse: null, + userFeeLevel: 'dappSuggested', + defaultGasEstimates: { + estimateType: 'dappSuggested', + gas: '0x5208', + maxFeePerGas: '0x2540be400', + maxPriorityFeePerGas: '0x3b9aca00', + }, +}; + +mockState.metamask.unapprovedTxs[sendEther.id] = sendEther; +mockState.confirmTransaction = { + txData: sendEther, +}; +const store = configureStore(mockState); + +export default { + title: 'Pages/ConfirmSendEther', + decorators: [(story) => {story()}], +}; + +export const DefaultStory = () => { + return ; +}; + +DefaultStory.storyName = 'Default'; diff --git a/ui/pages/confirm-send-ether/confirm-send-ether.test.js b/ui/pages/confirm-send-ether/confirm-send-ether.test.js new file mode 100644 index 000000000..2ecab5826 --- /dev/null +++ b/ui/pages/confirm-send-ether/confirm-send-ether.test.js @@ -0,0 +1,64 @@ +import React from 'react'; + +import { renderWithProvider } from '../../../test/lib/render-helpers'; +import { setBackgroundConnection } from '../../../test/jest'; +import mockState from '../../../test/data/mock-state.json'; +import configureStore from '../../store/store'; +import ConfirmSendEther from './confirm-send-ether'; + +setBackgroundConnection({ + getGasFeeTimeEstimate: jest.fn(), + getGasFeeEstimatesAndStartPolling: jest.fn(), + promisifiedBackground: jest.fn(), + tryReverseResolveAddress: jest.fn(), + getNextNonce: jest.fn(), + addKnownMethodData: jest.fn(), +}); + +const sendEther = { + id: 9597986287241458, + time: 1681203297082, + status: 'unapproved', + metamaskNetworkId: '5', + originalGasEstimate: '0x5208', + userEditedGasLimit: false, + chainId: '0x5', + loadingDefaults: false, + dappSuggestedGasFees: { + maxPriorityFeePerGas: '0x3b9aca00', + maxFeePerGas: '0x2540be400', + }, + sendFlowHistory: [], + txParams: { + from: '0x0dcd5d886577d5081b0c52e242ef29e70be3e7bc', + to: '0x0c54fccd2e384b4bb6f2e405bf5cbc15a017aafb', + value: '0x0', + gas: '0x5208', + maxFeePerGas: '0x2540be400', + maxPriorityFeePerGas: '0x3b9aca00', + }, + origin: 'https://metamask.github.io', + actionId: 1830698773, + type: 'simpleSend', + securityProviderResponse: null, + userFeeLevel: 'dappSuggested', + defaultGasEstimates: { + estimateType: 'dappSuggested', + gas: '0x5208', + maxFeePerGas: '0x2540be400', + maxPriorityFeePerGas: '0x3b9aca00', + }, +}; + +mockState.metamask.unapprovedTxs[sendEther.id] = sendEther; +mockState.confirmTransaction = { + txData: sendEther, +}; +const store = configureStore(mockState); + +describe('ConfirmSendEther', () => { + it('should render correct information for for confirm send ether', () => { + const { getAllByTestId } = renderWithProvider(, store); + expect(getAllByTestId('page-container')).toMatchSnapshot(); + }); +}); diff --git a/ui/pages/confirm-send-ether/index.js b/ui/pages/confirm-send-ether/index.js index eba4b48b1..f57cbc84e 100644 --- a/ui/pages/confirm-send-ether/index.js +++ b/ui/pages/confirm-send-ether/index.js @@ -1 +1 @@ -export { default } from './confirm-send-ether.container'; +export { default } from './confirm-send-ether'; diff --git a/ui/pages/confirm-token-transaction-base/confirm-token-transaction-base.js b/ui/pages/confirm-token-transaction-base/confirm-token-transaction-base.js index e7f53a4db..ee0b5c869 100644 --- a/ui/pages/confirm-token-transaction-base/confirm-token-transaction-base.js +++ b/ui/pages/confirm-token-transaction-base/confirm-token-transaction-base.js @@ -126,6 +126,7 @@ export default function ConfirmTokenTransactionBase({ assetName || `${getTitleTokenDescription('text')} #${tokenId}`; } else if (assetStandard === TokenStandard.ERC20) { title = `${tokenAmount} ${tokenSymbol}`; + subtotalDisplay = `${tokenAmount} ${tokenSymbol}`; } const hexWeiValue = useMemo(() => { diff --git a/ui/pages/confirm-transaction-base/confirm-transaction-base.component.js b/ui/pages/confirm-transaction-base/confirm-transaction-base.component.js index d4103634c..2e9bfaa60 100644 --- a/ui/pages/confirm-transaction-base/confirm-transaction-base.component.js +++ b/ui/pages/confirm-transaction-base/confirm-transaction-base.component.js @@ -813,6 +813,7 @@ export default class ConfirmTransactionBase extends Component { image, isApprovalOrRejection, assetStandard, + title, } = this.props; const { submitting, @@ -873,6 +874,7 @@ export default class ConfirmTransactionBase extends Component { showEdit={!isContractInteractionFromDapp && Boolean(onEdit)} action={functionType} image={image} + title={title} titleComponent={this.renderTitleComponent()} subtitleComponent={this.renderSubtitleComponent()} detailsComponent={this.renderDetails()} diff --git a/ui/pages/send/send-content/send-content.component.js b/ui/pages/send/send-content/send-content.component.js index b5f1c26e8..b57b59a51 100644 --- a/ui/pages/send/send-content/send-content.component.js +++ b/ui/pages/send/send-content/send-content.component.js @@ -7,7 +7,6 @@ import { ETH_GAS_PRICE_FETCH_WARNING_KEY, GAS_PRICE_FETCH_FAILURE_ERROR_KEY, GAS_PRICE_EXCESSIVE_ERROR_KEY, - INSUFFICIENT_FUNDS_FOR_GAS_ERROR_KEY, } from '../../../helpers/constants/error-keys'; import { AssetType } from '../../../../shared/constants/transaction'; import { CONTRACT_ADDRESS_LINK } from '../../../helpers/constants/common'; @@ -30,7 +29,6 @@ export default class SendContent extends Component { isEthGasPrice: PropTypes.bool, noGasPrice: PropTypes.bool, networkOrAccountNotSupports1559: PropTypes.bool, - getIsBalanceInsufficient: PropTypes.bool, asset: PropTypes.object, assetError: PropTypes.string, recipient: PropTypes.object, @@ -46,7 +44,6 @@ export default class SendContent extends Component { isEthGasPrice, noGasPrice, networkOrAccountNotSupports1559, - getIsBalanceInsufficient, asset, assetError, recipient, @@ -58,8 +55,6 @@ export default class SendContent extends Component { gasError = GAS_PRICE_EXCESSIVE_ERROR_KEY; } else if (noGasPrice) { gasError = GAS_PRICE_FETCH_FAILURE_ERROR_KEY; - } else if (getIsBalanceInsufficient) { - gasError = INSUFFICIENT_FUNDS_FOR_GAS_ERROR_KEY; } const showHexData = this.props.showHexData && diff --git a/ui/pages/send/send-content/send-content.component.test.js b/ui/pages/send/send-content/send-content.component.test.js index 3868da35e..660ecf9bb 100644 --- a/ui/pages/send/send-content/send-content.component.test.js +++ b/ui/pages/send/send-content/send-content.component.test.js @@ -4,7 +4,6 @@ import configureMockStore from 'redux-mock-store'; import { renderWithProvider } from '../../../../test/lib/render-helpers'; import mockSendState from '../../../../test/data/mock-send-state.json'; -import { INSUFFICIENT_FUNDS_ERROR } from '../send.constants'; import SendContent from '.'; jest.mock('../../../store/actions', () => ({ @@ -148,41 +147,6 @@ describe('SendContent Component', () => { expect(gasWarning).toBeInTheDocument(); }); }); - - it('should show gas warning for gas error state in draft transaction', async () => { - const props = { - gasIsExcessive: false, - showHexData: false, - }; - - const gasErrorState = { - ...mockSendState, - send: { - ...mockSendState.send, - draftTransactions: { - '1-tx': { - ...mockSendState.send.draftTransactions['1-tx'], - gas: { - error: INSUFFICIENT_FUNDS_ERROR, - }, - }, - }, - }, - }; - - const mockStore = configureMockStore()(gasErrorState); - - const { queryByTestId } = renderWithProvider( - , - mockStore, - ); - - const gasWarning = queryByTestId('gas-warning-message'); - - await waitFor(() => { - expect(gasWarning).toBeInTheDocument(); - }); - }); }); describe('Recipient Warning', () => { diff --git a/ui/pages/send/send-footer/send-footer.component.js b/ui/pages/send/send-footer/send-footer.component.js index f16ad7e09..82afae257 100644 --- a/ui/pages/send/send-footer/send-footer.component.js +++ b/ui/pages/send/send-footer/send-footer.component.js @@ -8,6 +8,7 @@ import { } from '../../../helpers/constants/routes'; import { MetaMetricsEventCategory } from '../../../../shared/constants/metametrics'; import { SEND_STAGES } from '../../../ducks/send'; +import { INSUFFICIENT_FUNDS_ERROR } from '../send.constants'; export default class SendFooter extends Component { static propTypes = { @@ -92,12 +93,14 @@ export default class SendFooter extends Component { render() { const { t } = this.context; - const { sendStage } = this.props; + const { sendStage, sendErrors } = this.props; return ( this.onCancel()} onSubmit={(e) => this.onSubmit(e)} - disabled={this.props.disabled} + disabled={ + this.props.disabled && sendErrors.gasFee !== INSUFFICIENT_FUNDS_ERROR + } cancelText={sendStage === SEND_STAGES.EDIT ? t('reject') : t('cancel')} /> ); diff --git a/ui/pages/token-allowance/token-allowance.js b/ui/pages/token-allowance/token-allowance.js index 7ee7fd887..10cc6ced0 100644 --- a/ui/pages/token-allowance/token-allowance.js +++ b/ui/pages/token-allowance/token-allowance.js @@ -35,7 +35,6 @@ import { getUnapprovedTxCount, getUnapprovedTransactions, getUseCurrencyRateCheck, - isHardwareWallet, } from '../../selectors'; import { NETWORK_TO_NAME_MAP } from '../../../shared/constants/network'; import { @@ -95,6 +94,7 @@ export default function TokenAllowance({ currentTokenBalance, toAddress, tokenSymbol, + fromAddressIsLedger, }) { const t = useContext(I18nContext); const dispatch = useDispatch(); @@ -105,6 +105,7 @@ export default function TokenAllowance({ const thisOriginIsAllowedToSkipFirstPage = ALLOWED_HOSTS.includes(hostname); const [showContractDetails, setShowContractDetails] = useState(false); + const [inputChangeInProgress, setInputChangeInProgress] = useState(false); const [showFullTxDetails, setShowFullTxDetails] = useState(false); const [isFirstPage, setIsFirstPage] = useState( dappProposedTokenAmount !== '0' && !thisOriginIsAllowedToSkipFirstPage, @@ -122,7 +123,6 @@ export default function TokenAllowance({ const unapprovedTxCount = useSelector(getUnapprovedTxCount); const unapprovedTxs = useSelector(getUnapprovedTransactions); const useCurrencyRateCheck = useSelector(getUseCurrencyRateCheck); - const isHardwareWalletConnected = useSelector(isHardwareWallet); let customTokenAmount = useSelector(getCustomTokenAmount); if (thisOriginIsAllowedToSkipFirstPage && dappProposedTokenAmount) { customTokenAmount = dappProposedTokenAmount; @@ -403,12 +403,14 @@ export default function TokenAllowance({ {isFirstPage ? ( setErrorText(value)} decimals={decimals} + setInputChangeInProgress={setInputChangeInProgress} /> ) : ( ) : null} - {!isFirstPage && isHardwareWalletConnected && ( + {!isFirstPage && fromAddressIsLedger && ( @@ -526,7 +528,9 @@ export default function TokenAllowance({ submitText={isFirstPage ? t('next') : t('approveButtonText')} onCancel={() => handleReject()} onSubmit={() => (isFirstPage ? handleNextClick() : handleApprove())} - disabled={disableNextButton || disableApproveButton} + disabled={ + inputChangeInProgress || disableNextButton || disableApproveButton + } > {unapprovedTxCount > 1 && (