Conversation
WalkthroughThis pull request simplifies native token handling across the project by removing the dependency on the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant TA as Token Atlas Component
participant G as getNativeAssetForChainId
participant Q as Query Hooks
U->>TA: Select a token
TA->>G: Retrieve native token by chain ID
G-->>TA: Return native token info
TA->>TA: Compare selected token with native token
alt Token is native
TA->>Q: Query API with id & asset as undefined
else Token is not native
TA->>Q: Query API with complete token details
end
Possibly related PRs
Suggested reviewers
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Deploying x with
|
| Latest commit: |
ffd0bdb
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://8c85ed34.x-e62.pages.dev |
| Branch Preview URL: | https://fix-sending-tokens-issue.x-e62.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/apps/pillarx-app/components/PointsTile/test/__snapshots__/PointsTile.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (7)
src/apps/the-exchange/components/EnterAmount/EnterAmount.tsx(1 hunks)src/apps/token-atlas/api/token.ts(2 hunks)src/apps/token-atlas/index.tsx(3 hunks)src/components/BottomMenuModal/AccountModal.tsx(2 hunks)src/components/Form/AssetSelect/index.tsx(2 hunks)src/services/tokensData.ts(2 hunks)src/utils/blockchain.ts(0 hunks)
💤 Files with no reviewable changes (1)
- src/utils/blockchain.ts
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: build
- GitHub Check: Cloudflare Pages
🔇 Additional comments (19)
src/components/BottomMenuModal/AccountModal.tsx (2)
88-92: Simplification of native token detection logicThe code has been simplified to use a more direct approach for detecting native tokens, checking if the asset's contract address equals the balance token or if the balance token is null and the asset's contract is a zero address, removing the dependency on the
nativeTokensByChainmapping.
107-107: Reordered useMemo dependenciesThe dependency array in the
useMemohook has been reordered to placeaddressesEqualbeforeisZeroAddress. While this change won't affect functionality, maintaining consistent ordering of dependencies is good practice.src/apps/the-exchange/components/EnterAmount/EnterAmount.tsx (2)
175-175: Simplified native balance detectionThe
isNativeBalancecondition has been simplified to check only if the balance token is null or a zero address, removing the dependency on thenativeTokensByChainmapping.
178-178: Updated native token validation in return statementThe return statement has been adjusted to check if the balance is a native balance and the asset address is a zero address, providing a more direct approach to determining valid asset addresses.
src/components/Form/AssetSelect/index.tsx (4)
25-25: Removed nativeTokensByChain importThe
nativeTokensByChainimport has been removed from the import statement, consistent with the approach in other files to reduce dependency on predefined native token lists.
147-149: Simplified native balance detectionThe
isNativeBalancecondition has been simplified to check only if the balance token is null or a zero address, removing the dependency on thenativeTokensByChainmapping.
150-153: Simplified native asset detectionThe
isNativeAssetcondition has been simplified to check if the asset contract is falsy or a zero address, making the code more direct and easier to understand.
154-159: Combined asset-balance matching logicThe return statement has been restructured to combine all checks into a single, more readable condition:
- Check if the balance is native AND the asset is native, OR
- Check if both the balance token and asset contract exist AND they are equal
This approach is clearer and more maintainable than the previous implementation.
src/services/tokensData.ts (3)
4-7: Good additions for native token handlingThe added imports for
CompatibleChainsandgetNativeAssetForChainIdare necessary for the new functionality to add native tokens to the token list.
62-75: Native token integration looks goodThis is a well-structured implementation to add native tokens to the token list. The code iterates through the compatible chains, gets the native asset for each chain ID, and correctly formats it to match the Token type structure.
A couple notes:
- It's used within the existing check for empty tokensData, which prevents duplicate additions on subsequent calls
- The chainIdToChainNameTokensData function appropriately maps chain IDs to the expected naming format
70-70:Details
✅ Verification successful
Verify chain name mapping for all supported native tokens
Make sure that all chain IDs in
CompatibleChainsare properly handled by thechainIdToChainNameTokensDatafunction.
🏁 Script executed:
#!/bin/bash # Check if all chain IDs in CompatibleChains are handled in the chainIdToChainNameTokensData function # First, find all chain IDs in CompatibleChains echo "Getting all chain IDs from CompatibleChains..." CHAIN_IDS=$(rg -A 30 "export const CompatibleChains" src/utils/blockchain.ts | rg "chainId:" | sed -E 's/.*chainId: ([0-9]+).*/\1/') echo "Found chain IDs: $CHAIN_IDS" # Then, check if they're all handled in the chainIdToChainNameTokensData function echo "Checking if all chain IDs are handled in chainIdToChainNameTokensData function..." for id in $CHAIN_IDS; do if ! rg "case $id:" src/services/tokensData.ts > /dev/null; then echo "WARNING: Chain ID $id from CompatibleChains is not explicitly handled in chainIdToChainNameTokensData" else echo "Chain ID $id is handled in chainIdToChainNameTokensData" fi doneLength of output: 3991
Mapping Verification Confirmed: Numeric Chain IDs Are Correctly Handled
The verification shows that the expected numeric chain IDs (1, 137, 8453, and 100) from
CompatibleChainsare all explicitly mapped in thechainIdToChainNameTokensDatafunction. Note that the additional non-numeric warnings were artifacts from the extraction process and do not indicate actual missing mappings.
- Verified chain IDs: 1, 137, 8453, 100.
- Ensure that as new numeric chain IDs are added to
CompatibleChains, the function is updated accordingly.src/apps/token-atlas/index.tsx (5)
23-24: Good addition of the native asset utilityAdding the import for
getNativeAssetForChainIdis appropriate for the native token detection implementation.
65-72: Well-structured token formattingThis formatting of the selected token into a consistent structure that can be compared with the native token is clean and straightforward.
74-79: Effective native token detection logicThe approach to determine if the selected token is a native token by comparing name, symbol, and address is thorough. This will correctly identify native tokens like ETH, MATIC, etc.
87-90: Good conditional query parameter handlingSetting the
idandassetparameters to undefined for native tokens ensures that the API won't attempt to fetch data that doesn't exist for native tokens.
99-104: Consistent parameter handling across queriesThe same conditional approach for
idandassetparameters is correctly applied to the token graph query as well, maintaining consistency.src/apps/token-atlas/api/token.ts (3)
32-32: Good update to parameter typesMaking the
idparameter optional is necessary to support the changes inindex.tsxwhere these parameters are conditionally set to undefined for native tokens.
35-35: Improved conditional query string constructionThe updated query string construction correctly handles optional parameters, only including them when they are provided. This implementation is more flexible and robust.
52-55: Consistent parameter type updatesMaking the
idparameter optional and thesymbolparameter required aligns with how these parameters are being used in the application.
| query: ({ id, asset, symbol, from, to }) => { | ||
| const toParam = to !== undefined ? `&to=${from * 1000}` : ''; | ||
| return `?id=${id}&asset=${asset}&from=${from * 1000}${toParam}&${chainIdsQuery}&testnets=${String(isTestnet)}`; | ||
| return `${id ? `id=${id}` : ''}${asset ? `&asset=${asset}` : ''}&symbol=${symbol}&from=${from * 1000}${toParam}&${chainIdsQuery}&testnets=${String(isTestnet)}`; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the query string construction for all cases
Let's verify that the query string construction works correctly for all combinations of parameters:
🏁 Script executed:
#!/bin/bash
# Test the query string construction for the getTokenGraph endpoint
echo "Testing query string construction for various parameter combinations"
# Define a simple function to simulate the getTokenGraph query construction
cat << 'EOF' > /tmp/test_query.js
function buildQuery(id, asset, symbol, from, to) {
const toParam = to !== undefined ? `&to=${from * 1000}` : '';
return `${id ? `id=${id}` : ''}${asset ? `&asset=${asset}` : ''}&symbol=${symbol}&from=${from * 1000}${toParam}&chainIds=123&testnets=false`;
}
// Test cases
console.log("Case 1 - All parameters defined:");
console.log(buildQuery(123, "ETH", "ETH", 1000, 2000));
console.log("\nCase 2 - id undefined:");
console.log(buildQuery(undefined, "ETH", "ETH", 1000, 2000));
console.log("\nCase 3 - asset undefined:");
console.log(buildQuery(123, undefined, "ETH", 1000, 2000));
console.log("\nCase 4 - both id and asset undefined:");
console.log(buildQuery(undefined, undefined, "ETH", 1000, 2000));
console.log("\nCase 5 - to undefined:");
console.log(buildQuery(123, "ETH", "ETH", 1000, undefined));
EOF
# Run the test script
node /tmp/test_query.jsLength of output: 682
Fix the "to" parameter calculation in the query string construction
The test output confirms that when the to parameter is provided, the query string incorrectly computes its value using from * 1000 instead of to * 1000. For example, in Case 1 the output is:
id=123&asset=ETH&symbol=ETH&from=1000000&to=1000000&chainIds=123&testnets=false
but the expectation is for the to value to be converted using the to parameter (i.e., 2000 * 1000 = 2000000). Please adjust the logic accordingly.
- File:
src/apps/token-atlas/api/token.ts(around line 61) - Suggested change:
Replace the current implementation:with:const toParam = to !== undefined ? `&to=${from * 1000}` : '';
const toParam = to !== undefined ? `&to=${to * 1000}` : '';
| query: ({ id, asset, symbol, from, to }) => { | ||
| const toParam = to !== undefined ? `&to=${from * 1000}` : ''; | ||
| return `?id=${id}&asset=${asset}&from=${from * 1000}${toParam}&${chainIdsQuery}&testnets=${String(isTestnet)}`; | ||
| return `${id ? `id=${id}` : ''}${asset ? `&asset=${asset}` : ''}&symbol=${symbol}&from=${from * 1000}${toParam}&${chainIdsQuery}&testnets=${String(isTestnet)}`; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhanced query string formatting
The updated query string construction for the token graph endpoint follows the same pattern as the token info endpoint, ensuring consistency across the API.
However, there's a potential issue with the query string when both id and asset are undefined - the string would start with an & character.
- return `${id ? `id=${id}` : ''}${asset ? `&asset=${asset}` : ''}&symbol=${symbol}&from=${from * 1000}${toParam}&${chainIdsQuery}&testnets=${String(isTestnet)}`;
+ // Build parameters array and filter out empty ones
+ const params = [
+ id ? `id=${id}` : '',
+ asset ? `asset=${asset}` : '',
+ `symbol=${symbol}`,
+ `from=${from * 1000}`,
+ toParam.startsWith('&') ? toParam.substring(1) : toParam,
+ chainIdsQuery,
+ `testnets=${String(isTestnet)}`
+ ].filter(param => param !== '');
+
+ // Join with & and return
+ return params.join('&');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| query: ({ id, asset, symbol, from, to }) => { | |
| const toParam = to !== undefined ? `&to=${from * 1000}` : ''; | |
| return `?id=${id}&asset=${asset}&from=${from * 1000}${toParam}&${chainIdsQuery}&testnets=${String(isTestnet)}`; | |
| return `${id ? `id=${id}` : ''}${asset ? `&asset=${asset}` : ''}&symbol=${symbol}&from=${from * 1000}${toParam}&${chainIdsQuery}&testnets=${String(isTestnet)}`; | |
| query: ({ id, asset, symbol, from, to }) => { | |
| const toParam = to !== undefined ? `&to=${from * 1000}` : ''; | |
| // Build parameters array and filter out empty ones | |
| const params = [ | |
| id ? `id=${id}` : '', | |
| asset ? `asset=${asset}` : '', | |
| `symbol=${symbol}`, | |
| `from=${from * 1000}`, | |
| toParam.startsWith('&') ? toParam.substring(1) : toParam, | |
| chainIdsQuery, | |
| `testnets=${String(isTestnet)}` | |
| ].filter(param => param !== ''); | |
| // Join with & and return | |
| return params.join('&'); | |
| } |
2 participants
Description
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Summary by CodeRabbit
New Features
Refactor