-
Notifications
You must be signed in to change notification settings - Fork 5.5k
fix: checksum EVM token addresses used throughout the bridge experience #38971
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
Conversation
2dacf81 to
9d04076
Compare
✨ Files requiring CODEOWNER review ✨🔄 @MetaMask/swaps-engineers (14 files, +99 -183)
|
Builds ready [28b265a]
UI Startup Metrics (1271 ± 116 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
ebcd68e to
9fc76e4
Compare
Builds ready [b834541]
UI Startup Metrics (1283 ± 104 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [27830e5]
UI Startup Metrics (1318 ± 98 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
| const commonFields = { | ||
| symbol: assetMetadata.symbol, | ||
| decimals: assetMetadata.decimals, | ||
| image: getAssetImageUrl(assetId, chainIdInCaip), |
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.
Inconsistent API casing between token metadata functions
The fetchAssetMetadata function now sends checksummed EVM assetIds to the token API (via the updated toAssetId which checksums addresses), while fetchAssetMetadataForAssetIds explicitly lowercases EVM assetIds at line 185 before calling the same API endpoint. This inconsistency means the same token could be looked up with different casing (eip155:1/erc20:0x123ABcDe vs eip155:1/erc20:0x123abcde) depending on which function is used. If the API is case-sensitive or returns differently-cased results, this could cause lookup failures or cache mismatches.
Additional Locations (1)
Builds ready [a98d3a5]
UI Startup Metrics (1267 ± 103 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
| return undefined; | ||
| } | ||
| const normalizedAssetId = ( | ||
| isNonEvmChainId(chainId) ? assetIdInCaip : assetIdInCaip.toLowerCase() |
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.
Wrong chain ID used for non-EVM address case check
In getAssetImageUrl, the isNonEvmChainId(chainId) check uses the original chainId parameter instead of the chain ID extracted from the asset. When toAssetId is called at line 83, it correctly extracts the chain from a CAIP asset type (e.g., a Solana asset). However, the lowercase decision at line 88 still uses the original chainId parameter. If a non-EVM asset (like Solana) is passed with an EVM chainId fallback, the Solana address gets incorrectly lowercased. Since Solana addresses are base58-encoded and case-sensitive, lowercasing them produces an entirely different address, causing token icon URLs to point to the wrong or nonexistent resources.
| ...commonFields, | ||
| address: address.toLowerCase(), | ||
| chainId: decimalToPrefixedHex(reference), | ||
| chainId: hexChainId, |
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 path returns full CAIP type instead of address
In fetchAssetMetadata, the EVM path at line 157 uses address.toLowerCase() directly from the original parameter. However, the function signature accepts address: string | CaipAssetType | Hex, meaning a CAIP asset type like eip155:1/erc20:0x1234... could be passed. The non-EVM path correctly extracts the address reference using parseCaipAssetType(assetId).assetReference (line 143-146), but the EVM path doesn't. When a CAIP asset type is passed for an EVM chain, the returned address field would contain the full lowercased CAIP type string instead of just the extracted hex address.
Builds ready [f802a6f]
UI Startup Metrics (1274 ± 94 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [9e831bf]
UI Startup Metrics (1289 ± 101 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [9e831bf]
UI Startup Metrics (1289 ± 101 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Description
Changes
toAssetIdutil to checksum EVM assetIds and transform Tron assets correctlyChangelog
CHANGELOG entry: fix: toAssetId util should checksum EVM assetIds and support Tron tokens
Related issues
Fixes:
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist
Note
toAssetIdto accept hex/CAIP chain IDs, checksum EVM addresses, and generate Solanatoken:and Trontrc20:asset IDs;getAssetImageUrlnow lowercases EVM paths while preserving non‑EVM case and builds URLs from CAIP asset IDs.fetchAssetMetadata/fetchAssetMetadataForAssetIdsto use newtoAssetIdandgetAssetImageUrl, normalize chain IDs, and return non‑EVM addresses via CAIP references; addsisEvmChainIdandisTronResourcehelpers.toAssetId(fromToken.address, fromChain.chainId), generate images via CDN, and format quote params with CAIP references; updates tests/e2e to expect checksummed ERC20 assetIds/addresses and new image URLs.BRIDGE_CHAINID_COMMON_TOKEN_PAIR; removes unused bridge-status metrics utils and related types.Written by Cursor Bugbot for commit 9e831bf. This will update automatically on new commits. Configure here.