-
Notifications
You must be signed in to change notification settings - Fork 5.5k
fix: exclude non evm tokens and chains if BFT is off #38914
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
✨ Files requiring CODEOWNER review ✨✅ @MetaMask/confirmations (2 files, +101 -12)
👨🔧 @MetaMask/core-extension-ux (1 files, +53 -7)
💎 @MetaMask/metamask-assets (1 files, +27 -3)
|
Builds ready [4634e5c]
UI Startup Metrics (1262 ± 93 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚀 Bundle size reduced!]
|
| : filteredAssets.filter( | ||
| (asset) => | ||
| isEvmChainId(asset.chainId) || | ||
| (!isEvm && asset.chainId === currentNetwork.chainId), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Missing undefined check before calling isEvmChainId
The isEvmChainId(asset.chainId) function is called without checking if asset.chainId is defined. The Asset type allows chainId to be undefined (chainId?: string | number). If chainId is undefined, isEvmChainId will call toEvmCaipChainId(undefined) which could throw or produce unexpected behavior. In contrast, useSendAssets.ts correctly guards with if (!token.chainId) { return false; } before calling isEvmChainId. The same defensive check is missing here at both filter locations in the useMemo callback.
Additional Locations (1)
Builds ready [0a1afcd]
UI Startup Metrics (1306 ± 104 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚀 Bundle size reduced!]
|
Builds ready [3e58170]
UI Startup Metrics (1272 ± 103 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [3e58170]
UI Startup Metrics (1272 ± 103 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
OGPoyraz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally - confirmation changes LGTM.
When BFT is off - nonEVM tokens are filtered as well as the nonEVM asset page send button is also disabled as expected.
Builds ready [3e58170]
UI Startup Metrics (1272 ± 103 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [3e58170]
UI Startup Metrics (1272 ± 103 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
| // Exception: Keep the currently selected non-EVM chain visible | ||
| if (!useExternalServices) { | ||
| return network.chainId === selectedNonEvmChainId; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EVM networks filtered out when account group undefined and BFT off
Low Severity
The BFT filtering check at lines 268-270 returns network.chainId === selectedNonEvmChainId when external services are disabled, which filters out EVM networks since they won't match a non-EVM chain ID. EVM networks are expected to pass via the earlier condition evmAccountGroup && network.isEvm, but if evmAccountGroup is null (possible per the selector implementation), EVM networks get incorrectly filtered out. The comment states "only show EVM networks" but the code doesn't guarantee that when evmAccountGroup is undefined. The same pattern exists in the non-State2 path at lines 289-291.
Additional Locations (1)
Builds ready [d155f5b]
UI Startup Metrics (1304 ± 110 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [d000e29]
UI Startup Metrics (1303 ± 108 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
15ced27
| const filteredNfts = nfts.filter((nft) => { | ||
| // Filter out non-EVM NFTs to prevent their chain IDs from appearing | ||
| // in the network filter dropdown | ||
| return isEvmChainId(nft.chainId as CaipChainId | Hex); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing undefined guard for chainId in filter
Low Severity
The Asset type defines chainId as optional (chainId?: string | number), but the filtering logic casts token.chainId and nft.chainId directly to CaipChainId | Hex without checking for undefined. If an asset has an undefined chainId, isEvmChainId() will call toEvmCaipChainId(undefined) and then parseCaipChainId(), which would throw a runtime error. Adding a guard like if (!token.chainId) return false before the isEvmChainId call would prevent potential crashes.
Builds ready [15ced27]
UI Startup Metrics (1293 ± 92 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Description
This PR ensures that non-EVM tokens and chains are excluded from the UI when the Basic Functionality Toggle (BFT) is turned OFF.
Changelog
CHANGELOG entry: Makes sure UI filters out non-evm chains and tokens when BFT is OFF.
Related issues
Fixes: #38111
Manual testing steps
Scenario 1:
Scenario 2:
Screenshots/Recordings
Before
After
Screen.Recording.2025-12-19.at.00.45.23.mov
Screen.Recording.2025-12-19.at.00.47.09.mov
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Enforces EVM-only experience when
getUseExternalServicesis false, with an exception to keep the currently selected non‑EVM chain visible.token-list.tsx): Filters non‑EVM assets when BFT is OFF; still shows assets for the selected non‑EVM chain; includes zero-balance and TRX resource checks; preserves sortingdefault-networks.tsx): Hides non‑EVM networks in default and featured lists when BFT is OFF, except the selected non‑EVM chain; updates network item filtering logic accordinglyuseSendAssets.ts): Filters out non‑EVM tokens and NFTs when BFT is OFF so non‑EVM chains don’t appear in the send asset/network selectors; adds testsdisableSendForNonEvmtoCoinButtons/TokenButtonsand disables Send on asset pages for non‑EVM when BFT is OFF; keeps Swap disabled when external services are off; updatesnon-evm-overview.test.tsxexpectationsdisableSendForNonEvmto native/token buttonsWritten by Cursor Bugbot for commit 15ced27. This will update automatically on new commits. Configure here.