Skip to content

Commit

Permalink
chore: remove bridge src token list from controller state (#29492)
Browse files Browse the repository at this point in the history
<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**
This reduces data stored in the BridgeController by removing stored
bridge-specific token lists, and reusing token data from the assets
controllers instead.

Changes
- **AssetList**: compare symbols in order to determine which token to
highlight as "selected"
- **AssetPickerModal**: fetch topAssets list on network change, yield
tokens with balances first
- **pages/bridge**: remove custom src token list (falls back to
AssetPicker's default list)
- **bridge-controller, ducks/bridge**: remove src token methods and
states
- **hooks/bridge**: remove allowlist check for tokens since these have
moved to the bridge-api

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/29492?quickstart=1)

## **Related issues**

Fixes: https://consensyssoftware.atlassian.net/browse/MMS-1825
Depends on
consensys-vertical-apps/va-mmcx-bridge-api#136

## **Manual testing steps**

1. Verify that Swap+Send token allowlists have not changed
2. Verify that Send token allowlists have not changed
3. Bridging 137:BNB -> 10:USDC should result in no quotes
4. Bridging 10:USDC  -> 137:USDC should result in no quotes
5. Bridge src token should be pre-filled when navigating from asset page
6. Bridge inputs should be restored after fetching quotes and reopening
extension

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **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/main/.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/main/.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.
  • Loading branch information
micaelae authored Jan 16, 2025
1 parent cdd603c commit f503a48
Show file tree
Hide file tree
Showing 28 changed files with 109 additions and 492 deletions.
3 changes: 0 additions & 3 deletions app/scripts/constants/sentry-state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,9 +110,6 @@ export const SENTRY_BACKGROUND_STATE = {
destTokens: {},
destTopAssets: [],
destTokensLoadingStatus: true,
srcTokens: {},
srcTopAssets: [],
srcTokensLoadingStatus: true,
quoteRequest: {
walletAddress: false,
srcTokenAddress: true,
Expand Down
35 changes: 1 addition & 34 deletions app/scripts/controllers/bridge/bridge-controller.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ import mockBridgeQuotesErc20Native from '../../../../test/data/bridge/mock-quote
import mockBridgeQuotesNativeErc20 from '../../../../test/data/bridge/mock-quotes-native-erc20.json';
import mockBridgeQuotesNativeErc20Eth from '../../../../test/data/bridge/mock-quotes-native-erc20-eth.json';
import {
type QuoteResponse,
RequestStatus,
type QuoteResponse,
} from '../../../../shared/types/bridge';
import { decimalToHex } from '../../../../shared/modules/conversion.utils';
import BridgeController from './bridge-controller';
Expand Down Expand Up @@ -198,39 +198,6 @@ describe('BridgeController', function () {
});
});

it('selectSrcNetwork should set the bridge src tokens and top assets', async function () {
await bridgeController.selectSrcNetwork('0xa');
expect(bridgeController.state.bridgeState.srcTokens).toStrictEqual({
'0x0000000000000000000000000000000000000000': {
address: '0x0000000000000000000000000000000000000000',
decimals: 18,
iconUrl: './images/eth_logo.svg',
name: 'Ether',
symbol: 'ETH',
},
'0x1f9840a85d5af5bf1d1762f925bdaddc4201f984': {
address: '0x1f9840a85d5af5bf1d1762f925bdaddc4201f984',
symbol: 'ABC',
decimals: 16,
aggregators: ['lifl', 'socket'],
},
});
expect(
bridgeController.state.bridgeState.srcTokensLoadingStatus,
).toStrictEqual(RequestStatus.FETCHED);
expect(bridgeController.state.bridgeState.srcTopAssets).toStrictEqual([
{
address: '0x1f9840a85d5af5bf1d1762f925bdaddc4201f984',
symbol: 'ABC',
},
]);
expect(bridgeController.state.bridgeState.quoteRequest).toStrictEqual({
slippage: 0.5,
srcTokenAddress: '0x0000000000000000000000000000000000000000',
walletAddress: undefined,
});
});

it('updateBridgeQuoteRequestParams should update the quoteRequest state', function () {
bridgeController.updateBridgeQuoteRequestParams({ srcChainId: 1 });
expect(bridgeController.state.bridgeState.quoteRequest).toStrictEqual({
Expand Down
20 changes: 0 additions & 20 deletions app/scripts/controllers/bridge/bridge-controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -96,10 +96,6 @@ export default class BridgeController extends StaticIntervalPollingController<Br
`${BRIDGE_CONTROLLER_NAME}:setBridgeFeatureFlags`,
this.setBridgeFeatureFlags.bind(this),
);
this.messagingSystem.registerActionHandler(
`${BRIDGE_CONTROLLER_NAME}:selectSrcNetwork`,
this.selectSrcNetwork.bind(this),
);
this.messagingSystem.registerActionHandler(
`${BRIDGE_CONTROLLER_NAME}:selectDestNetwork`,
this.selectDestNetwork.bind(this),
Expand Down Expand Up @@ -215,22 +211,6 @@ export default class BridgeController extends StaticIntervalPollingController<Br
);
};

selectSrcNetwork = async (chainId: Hex) => {
this.update((state) => {
state.bridgeState.srcTokensLoadingStatus = RequestStatus.LOADING;
return state;
});
try {
await this.#setTopAssets(chainId, 'srcTopAssets');
await this.#setTokens(chainId, 'srcTokens');
} finally {
this.update((state) => {
state.bridgeState.srcTokensLoadingStatus = RequestStatus.FETCHED;
return state;
});
}
};

selectDestNetwork = async (chainId: Hex) => {
this.update((state) => {
state.bridgeState.destTokensLoadingStatus = RequestStatus.LOADING;
Expand Down
3 changes: 0 additions & 3 deletions app/scripts/controllers/bridge/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,7 @@ export const DEFAULT_BRIDGE_CONTROLLER_STATE: BridgeControllerState = {
chains: {},
},
},
srcTokens: {},
srcTokensLoadingStatus: undefined,
destTokensLoadingStatus: undefined,
srcTopAssets: [],
destTokens: {},
destTopAssets: [],
quoteRequest: {
Expand Down
1 change: 0 additions & 1 deletion app/scripts/controllers/bridge/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ type BridgeControllerActions =
| BridgeControllerAction<BridgeBackgroundAction.SET_FEATURE_FLAGS>
| BridgeControllerAction<BridgeBackgroundAction.RESET_STATE>
| BridgeControllerAction<BridgeBackgroundAction.GET_BRIDGE_ERC20_ALLOWANCE>
| BridgeControllerAction<BridgeUserAction.SELECT_SRC_NETWORK>
| BridgeControllerAction<BridgeUserAction.SELECT_DEST_NETWORK>
| BridgeControllerAction<BridgeUserAction.UPDATE_QUOTE_PARAMS>;

Expand Down
4 changes: 0 additions & 4 deletions app/scripts/metamask-controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -4179,10 +4179,6 @@ export default class MetamaskController extends EventEmitter {
this.controllerMessenger,
`${BRIDGE_CONTROLLER_NAME}:${BridgeBackgroundAction.GET_BRIDGE_ERC20_ALLOWANCE}`,
),
[BridgeUserAction.SELECT_SRC_NETWORK]: this.controllerMessenger.call.bind(
this.controllerMessenger,
`${BRIDGE_CONTROLLER_NAME}:${BridgeUserAction.SELECT_SRC_NETWORK}`,
),
[BridgeUserAction.SELECT_DEST_NETWORK]:
this.controllerMessenger.call.bind(
this.controllerMessenger,
Expand Down
4 changes: 0 additions & 4 deletions shared/types/bridge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,6 @@ export enum RequestStatus {
ERROR,
}
export enum BridgeUserAction {
SELECT_SRC_NETWORK = 'selectSrcNetwork',
SELECT_DEST_NETWORK = 'selectDestNetwork',
UPDATE_QUOTE_PARAMS = 'updateBridgeQuoteRequestParams',
}
Expand All @@ -186,9 +185,6 @@ export enum BridgeBackgroundAction {
}
export type BridgeControllerState = {
bridgeFeatureFlags: BridgeFeatureFlags;
srcTokens: Record<string, SwapsTokenObject>;
srcTopAssets: { address: string }[];
srcTokensLoadingStatus?: RequestStatus;
destTokensLoadingStatus?: RequestStatus;
destTokens: Record<string, SwapsTokenObject>;
destTopAssets: { address: string }[];
Expand Down
2 changes: 0 additions & 2 deletions test/e2e/default-fixture.js
Original file line number Diff line number Diff line change
Expand Up @@ -140,8 +140,6 @@ function defaultFixture(inputChainId = CHAIN_IDS.LOCALHOST) {
},
destTokens: {},
destTopAssets: [],
srcTokens: {},
srcTopAssets: [],
},
},
CurrencyController: {
Expand Down
2 changes: 0 additions & 2 deletions test/e2e/fixture-builder.js
Original file line number Diff line number Diff line change
Expand Up @@ -457,8 +457,6 @@ class FixtureBuilder {
},
destTokens: {},
destTopAssets: [],
srcTokens: {},
srcTopAssets: [],
},
};
return this;
Expand Down
1 change: 0 additions & 1 deletion test/e2e/tests/metrics/errors.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -875,7 +875,6 @@ describe('Sentry errors', function () {
walletAddress: false,
},
destTokensLoadingStatus: false,
srcTokensLoadingStatus: false,
quotesLastFetched: true,
quotesLoadingStatus: true,
quotesRefreshCount: true,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,6 @@
"chains": { "0x1": "object", "0xa4b1": "object", "0xe708": "object" }
}
},
"srcTokens": {},
"srcTopAssets": {},
"destTokens": {},
"destTopAssets": {},
"quoteRequest": {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -340,8 +340,6 @@
"chains": { "0x1": "object", "0xa4b1": "object", "0xe708": "object" }
}
},
"srcTokens": {},
"srcTopAssets": {},
"destTokens": {},
"destTopAssets": {},
"quoteRequest": {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,9 +152,7 @@
}
},
"destTokens": {},
"destTopAssets": {},
"srcTokens": {},
"srcTopAssets": {}
"destTopAssets": {}
}
},
"SubjectMetadataController": { "subjectMetadata": "object" },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -161,9 +161,7 @@
}
},
"destTokens": {},
"destTopAssets": {},
"srcTokens": {},
"srcTopAssets": {}
"destTopAssets": {}
}
},
"TransactionController": { "transactions": "object" },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,6 @@ export default function AssetList({
assetItemProps = {},
}: AssetListProps) {
const t = useI18nContext();
const selectedTokenAddress = asset?.address;

const currentNetwork = useSelector(getCurrentNetwork);
// If a network is provided, display tokens in that network
Expand Down Expand Up @@ -101,11 +100,9 @@ export default function AssetList({

const isMatchingChainId = token.chainId === networkToUse?.chainId;
const isMatchingAddress =
// the native asset can have an undefined, null, '', or zero address
(token.type === AssetType.native &&
!token.address &&
!selectedTokenAddress) ||
tokenAddress === selectedTokenAddress?.toLowerCase();
// the native asset can have an undefined, null, '', or zero address so compare symbols
(token.type === AssetType.native && token.symbol === asset?.symbol) ||
tokenAddress === asset?.address?.toLowerCase();
const isSelected = isMatchingChainId && isMatchingAddress;

const isDisabled = isTokenDisabled?.(token) ?? false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,14 +42,14 @@ import {
getShouldHideZeroBalanceTokens,
getTokenExchangeRates,
getTokenList,
getUseExternalServices,
} from '../../../../selectors';
import {
getConversionRate,
getCurrentCurrency,
getNativeCurrency,
} from '../../../../ducks/metamask/metamask';
import { useTokenTracker } from '../../../../hooks/useTokenTracker';
import { getTopAssets } from '../../../../ducks/swaps/swaps';
import { getRenderableTokenData } from '../../../../hooks/useTokensToSearch';
import { getSwapsBlockedTokens } from '../../../../ducks/send';
import { isEqualCaseInsensitive } from '../../../../../shared/modules/string-utils';
Expand All @@ -60,6 +60,8 @@ import {
import { useMultichainBalances } from '../../../../hooks/useMultichainBalances';
import { AvatarType } from '../../avatar-group/avatar-group.types';
import { NETWORK_TO_SHORT_NETWORK_NAME_MAP } from '../../../../../shared/constants/bridge';
import { useAsyncResult } from '../../../../hooks/useAsyncResult';
import { fetchTopAssetsList } from '../../../../pages/swaps/swaps.util';
import {
ERC20Asset,
NativeAsset,
Expand Down Expand Up @@ -183,9 +185,19 @@ export function AssetPickerModal({
const { assetsWithBalance: multichainTokensWithBalance } =
useMultichainBalances();

// Swaps token list
const tokenList = useSelector(getTokenList) as TokenListMap;
const topTokens = useSelector(getTopAssets, isEqual);

const allowExternalServices = useSelector(getUseExternalServices);
// Swaps top tokens
const { value: topTokensResponse } = useAsyncResult<
{ address: string }[] | undefined
>(async () => {
if (allowExternalServices && selectedNetwork?.chainId) {
return await fetchTopAssetsList(selectedNetwork.chainId);
}
return undefined;
}, [selectedNetwork?.chainId, allowExternalServices]);
const topTokens = topTokensResponse ?? [];

const getIsDisabled = useCallback(
({
Expand All @@ -212,6 +224,18 @@ export function AssetPickerModal({
);
}, [tokensWithBalances, tokens]);

/**
* Generates a list of tokens sorted in this order
* - native tokens with balance
* - tokens with highest to lowest balance in selected currency
* - selected network's native token
* - matches URL token parameter
* - matches search query
* - detected tokens (without balance)
* - popularity
* - all other tokens
* - blocked tokens
*/
const tokenListGenerator = useCallback(
function* (
shouldAddToken: (
Expand All @@ -227,6 +251,18 @@ export function AssetPickerModal({
string?: string;
})
> {
const blockedTokens = [];

// Yield multichain tokens with balances
if (isMultiselectEnabled) {
for (const token of multichainTokensWithBalance) {
if (shouldAddToken(token.symbol, token.address, token.chainId)) {
yield token;
}
}
}

// Yield the native token for the selected chain
const nativeToken: AssetWithDisplayData<NativeAsset> = {
address: null,
symbol: nativeCurrency,
Expand All @@ -248,26 +284,15 @@ export function AssetPickerModal({
yield nativeToken;
}

const blockedTokens = [];

// Yield multichain tokens with balances
if (isMultiselectEnabled) {
for (const token of multichainTokensWithBalance) {
if (shouldAddToken(token.symbol, token.address, token.chainId)) {
yield token;
}
}
}

for (const token of memoizedUsersTokens) {
if (shouldAddToken(token.symbol, token.address, currentChainId)) {
yield { ...token, chainId: currentChainId };
}
}

// topTokens should already be sorted by popularity
for (const address of Object.keys(topTokens)) {
const token = tokenList?.[address];
// topTokens are sorted by popularity
for (const topToken of topTokens) {
const token = tokenList?.[topToken.address];
if (
token &&
shouldAddToken(token.symbol, token.address, currentChainId)
Expand Down Expand Up @@ -384,7 +409,6 @@ export function AssetPickerModal({
customTokenListGenerator,
isMultiselectEnabled,
selectedChainIds,
isMultiselectEnabled,
selectedNetwork,
]);

Expand Down
11 changes: 0 additions & 11 deletions ui/ducks/bridge/actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,17 +68,6 @@ export const resetBridgeState = () => {
};

// User actions
export const setFromChain = (chainId: Hex) => {
return async (dispatch: MetaMaskReduxDispatch) => {
dispatch(
callBridgeControllerMethod<Hex>(
BridgeUserAction.SELECT_SRC_NETWORK,
chainId,
),
);
};
};

export const setToChain = (chainId: Hex) => {
return async (dispatch: MetaMaskReduxDispatch) => {
dispatch(
Expand Down
Loading

0 comments on commit f503a48

Please sign in to comment.