From ff635d238ce177c124e70b5f9e33a03d606e031a Mon Sep 17 00:00:00 2001 From: Nick Gambino <35090461+gambinish@users.noreply.github.com> Date: Mon, 25 Nov 2024 13:23:03 -0800 Subject: [PATCH] fix: PortfolioView swap native token bug (#28639) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** When `PORTFOLIO_VIEW` feature flag is enabled, when swapping a native token from a different chain than the globally selected chain, the incorrect native token would be prepoulated in the `fromToken` in the swap UI. For instance, if user is on Ethereum mainnet, navigated to POL, then attempted to swap, the globally selected network would change from Ethereum mainnet to Polygon mainnet (expected), but the swaps `fromToken` would still be POL (unexpected) Changes in this PR fixes this, and prepoulates `fromToken` with the native token from the correct chain. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/28639?quickstart=1) ## **Related issues** Fixes: https://github.com/MetaMask/metamask-extension/issues/28534 ## **Manual testing steps** 1. `PORTFOLIO_VIEW=1 yarn webpack --watch` 2. Import wallet with at least two networks added 3. When "All Networks" is toggled, attempt to swap a native token from another network. Ensure that the token prepopulated in the swap UI is the native token from the correct chain 4. Ensure swap completes successfully. ## **Screenshots/Recordings** https://github.com/user-attachments/assets/016ffa54-9ed1-450c-9aa0-da27f0fd6caa ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --------- Co-authored-by: David Walsh --- ui/pages/asset/components/asset-page.tsx | 17 +- ui/selectors/selectors.js | 17 +- ui/selectors/selectors.test.js | 199 +++++++++++++++++++++++ 3 files changed, 223 insertions(+), 10 deletions(-) diff --git a/ui/pages/asset/components/asset-page.tsx b/ui/pages/asset/components/asset-page.tsx index 0f4529861dbc..68d7f9d18e65 100644 --- a/ui/pages/asset/components/asset-page.tsx +++ b/ui/pages/asset/components/asset-page.tsx @@ -101,11 +101,21 @@ const AssetPage = ({ const selectedAccount = useSelector(getSelectedAccount); const currency = useSelector(getCurrentCurrency); const conversionRate = useSelector(getConversionRate); - const isBridgeChain = useSelector(getIsBridgeChain); const isBuyableChain = useSelector(getIsNativeTokenBuyable); - const defaultSwapsToken = useSelector(getSwapsDefaultToken, isEqual); + + const { chainId, type, symbol, name, image, decimals } = asset; + + // These need to be specific to the asset and not the current chain + const defaultSwapsToken = useSelector( + (state) => getSwapsDefaultToken(state, chainId), + isEqual, + ); + const isSwapsChain = useSelector((state) => getIsSwapsChain(state, chainId)); + const isBridgeChain = useSelector((state) => + getIsBridgeChain(state, chainId), + ); + const account = useSelector(getSelectedInternalAccount, isEqual); - const isSwapsChain = useSelector(getIsSwapsChain); const isSigningEnabled = account.methods.includes(EthMethod.SignTransaction) || account.methods.includes(EthMethod.SignUserOperation); @@ -132,7 +142,6 @@ const AssetPage = ({ const selectedAccountTokenBalancesAcrossChains = tokenBalances[selectedAccount.address]; - const { chainId, type, symbol, name, image, decimals } = asset; const isMetaMetricsEnabled = useSelector(getParticipateInMetaMetrics); const isMarketingEnabled = useSelector(getDataCollectionForMarketing); const metaMetricsId = useSelector(getMetaMetricsId); diff --git a/ui/selectors/selectors.js b/ui/selectors/selectors.js index b6df07bd2bd0..eea467ec0f16 100644 --- a/ui/selectors/selectors.js +++ b/ui/selectors/selectors.js @@ -1495,14 +1495,17 @@ export function getWeb3ShimUsageStateForOrigin(state, origin) { * objects, per the above description. * * @param {object} state - the redux state object + * @param {string} overrideChainId - the chainId to override the current chainId * @returns {SwapsEthToken} The token object representation of the currently * selected account's ETH balance, as expected by the Swaps API. */ -export function getSwapsDefaultToken(state) { +export function getSwapsDefaultToken(state, overrideChainId = null) { const selectedAccount = getSelectedAccount(state); const balance = selectedAccount?.balance; - const chainId = getCurrentChainId(state); + const currentChainId = getCurrentChainId(state); + + const chainId = overrideChainId ?? currentChainId; const defaultTokenObject = SWAPS_CHAINID_DEFAULT_TOKEN_MAP[chainId]; return { @@ -1516,8 +1519,9 @@ export function getSwapsDefaultToken(state) { }; } -export function getIsSwapsChain(state) { - const chainId = getCurrentChainId(state); +export function getIsSwapsChain(state, overrideChainId) { + const currentChainId = getCurrentChainId(state); + const chainId = overrideChainId ?? currentChainId; const isNotDevelopment = process.env.METAMASK_ENVIRONMENT !== 'development' && process.env.METAMASK_ENVIRONMENT !== 'testing'; @@ -1526,8 +1530,9 @@ export function getIsSwapsChain(state) { : ALLOWED_DEV_SWAPS_CHAIN_IDS.includes(chainId); } -export function getIsBridgeChain(state) { - const chainId = getCurrentChainId(state); +export function getIsBridgeChain(state, overrideChainId) { + const currentChainId = getCurrentChainId(state); + const chainId = overrideChainId ?? currentChainId; return ALLOWED_BRIDGE_CHAIN_IDS.includes(chainId); } diff --git a/ui/selectors/selectors.test.js b/ui/selectors/selectors.test.js index b2c3cd894e44..c749d8ff3fe7 100644 --- a/ui/selectors/selectors.test.js +++ b/ui/selectors/selectors.test.js @@ -1978,4 +1978,203 @@ describe('#getConnectedSitesList', () => { expect(selectors.getSelectedEvmInternalAccount(state)).toBe(undefined); }); }); + + describe('getSwapsDefaultToken', () => { + it('returns the token object for the current chainId when no overrideChainId is provided', () => { + const expectedToken = { + symbol: 'ETH', + name: 'Ether', + address: '0x0000000000000000000000000000000000000000', + decimals: 18, + balance: '966987986469506564059', + string: '966.988', + iconUrl: './images/black-eth-logo.svg', + }; + + const result = selectors.getSwapsDefaultToken(mockState); + + expect(result).toStrictEqual(expectedToken); + }); + + it('returns the token object for the overridden chainId when overrideChainId is provided', () => { + const getCurrentChainIdSpy = jest.spyOn(selectors, 'getCurrentChainId'); + const expectedToken = { + symbol: 'POL', + name: 'Polygon', + address: '0x0000000000000000000000000000000000000000', + decimals: 18, + balance: '966987986469506564059', + string: '966.988', + iconUrl: './images/pol-token.svg', + }; + + const result = selectors.getSwapsDefaultToken( + mockState, + CHAIN_IDS.POLYGON, + ); + + expect(result).toStrictEqual(expectedToken); + expect(getCurrentChainIdSpy).not.toHaveBeenCalled(); // Ensure overrideChainId is used + }); + }); + + describe('getIsSwapsChain', () => { + it('returns true for an allowed chainId in production environment', () => { + process.env.METAMASK_ENVIRONMENT = 'production'; + + const state = { + ...mockState, + metamask: { + ...mockState.metamask, + selectedNetworkClientId: 'testNetworkConfigurationId', // corresponds to mainnet RPC in mockState + }, + }; + + const result = selectors.getIsSwapsChain(state); + + expect(result).toBe(true); + }); + + it('returns true for an allowed chainId in development environment', () => { + process.env.METAMASK_ENVIRONMENT = 'development'; + + const state = { + ...mockState, + metamask: { + ...mockState.metamask, + selectedNetworkClientId: 'goerli', + }, + }; + + const result = selectors.getIsSwapsChain(state); + + expect(result).toBe(true); + }); + + it('returns false for a disallowed chainId in production environment', () => { + process.env.METAMASK_ENVIRONMENT = 'production'; + + const state = { + ...mockState, + metamask: { + ...mockState.metamask, + selectedNetworkClientId: 'fooChain', // corresponds to mainnet RPC in mockState + networkConfigurationsByChainId: { + '0x8080': { + chainId: '0x8080', + name: 'Custom Mainnet RPC', + nativeCurrency: 'ETH', + defaultRpcEndpointIndex: 0, + rpcEndpoints: [ + { + type: 'custom', + url: 'https://testrpc.com', + networkClientId: 'fooChain', + }, + ], + }, + }, + }, + }; + + const result = selectors.getIsSwapsChain(state); + + expect(result).toBe(false); + }); + + it('returns false for a disallowed chainId in development environment', () => { + process.env.METAMASK_ENVIRONMENT = 'development'; + + const state = { + ...mockState, + metamask: { + ...mockState.metamask, + selectedNetworkClientId: 'fooChain', // corresponds to mainnet RPC in mockState + networkConfigurationsByChainId: { + '0x8080': { + chainId: '0x8080', + name: 'Custom Mainnet RPC', + nativeCurrency: 'ETH', + defaultRpcEndpointIndex: 0, + rpcEndpoints: [ + { + type: 'custom', + url: 'https://testrpc.com', + networkClientId: 'fooChain', + }, + ], + }, + }, + }, + }; + + const result = selectors.getIsSwapsChain(state); + + expect(result).toBe(false); + }); + + it('respects the overrideChainId parameter', () => { + process.env.METAMASK_ENVIRONMENT = 'production'; + + const getCurrentChainIdSpy = jest.spyOn(selectors, 'getCurrentChainId'); + + const result = selectors.getIsSwapsChain(mockState, '0x89'); + expect(result).toBe(true); + expect(getCurrentChainIdSpy).not.toHaveBeenCalled(); // Ensure overrideChainId is used + }); + }); + + describe('getIsBridgeChain', () => { + it('returns true for an allowed bridge chainId', () => { + const state = { + ...mockState, + metamask: { + ...mockState.metamask, + selectedNetworkClientId: 'testNetworkConfigurationId', // corresponds to mainnet RPC in mockState + }, + }; + + const result = selectors.getIsBridgeChain(state); + + expect(result).toBe(true); + }); + + it('returns false for a disallowed bridge chainId', () => { + const state = { + ...mockState, + metamask: { + ...mockState.metamask, + selectedNetworkClientId: 'fooChain', // corresponds to mainnet RPC in mockState + networkConfigurationsByChainId: { + '0x8080': { + chainId: '0x8080', + name: 'Custom Mainnet RPC', + nativeCurrency: 'ETH', + defaultRpcEndpointIndex: 0, + rpcEndpoints: [ + { + type: 'custom', + url: 'https://testrpc.com', + networkClientId: 'fooChain', + }, + ], + }, + }, + }, + }; + + const result = selectors.getIsBridgeChain(state); + + expect(result).toBe(false); + }); + + it('respects the overrideChainId parameter', () => { + const getCurrentChainIdSpy = jest.spyOn(selectors, 'getCurrentChainId'); + + const result = selectors.getIsBridgeChain(mockState, '0x89'); + + expect(result).toBe(true); + expect(getCurrentChainIdSpy).not.toHaveBeenCalled(); // Ensure overrideChainId is used + }); + }); });