fix/PRO-3299/add-params-portfolio-endpoint#303
Conversation
WalkthroughThe internal logic for constructing the POST request body in the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant getWalletPortfolio
participant API
Caller->>getWalletPortfolio: Call with parameters
getWalletPortfolio->>getWalletPortfolio: Build request body with new params (blockchains, unlistedAssets, filterSpam, pnl)
getWalletPortfolio->>API: POST /getWalletPortfolio (with extended body)
API-->>getWalletPortfolio: Respond with portfolio data
getWalletPortfolio-->>Caller: Return result
Suggested reviewers
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ 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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: |
2bc9016
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f44281ee.x-e62.pages.dev |
| Branch Preview URL: | https://fix-pro-3299-add-params-port.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: 0
🧹 Nitpick comments (1)
src/services/pillarXApiWalletPortfolio.ts (1)
71-73: Consider adding validation for empty chain names.When mapping through the chain IDs, there's no handling for cases where
chainIdToChainNameTokensDatamight return an empty string. This could lead to invalid comma-separated values like ", , Ethereum".Consider filtering out empty strings before joining:
- blockchains: CompatibleChains.map((chain) => - chainIdToChainNameTokensData(chain.chainId) - ).join(', '), + blockchains: CompatibleChains.map((chain) => + chainIdToChainNameTokensData(chain.chainId) + ).filter(Boolean).join(', '),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
src/apps/pillarx-app/components/MediaGridCollection/tests/__snapshots__/DisplayCollectionImage.test.tsx.snapis excluded by!**/*.snapsrc/apps/pillarx-app/components/TokenMarketDataRow/tests/__snapshots__/LeftColumnTokenMarketDataRow.test.tsx.snapis excluded by!**/*.snapsrc/apps/pillarx-app/components/TokensWithMarketDataTile/test/__snapshots__/TokensWithMarketDataTile.test.tsx.snapis excluded by!**/*.snapsrc/containers/__snapshots__/Main.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (1)
src/services/pillarXApiWalletPortfolio.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/services/pillarXApiWalletPortfolio.ts (2)
src/utils/blockchain.ts (1)
CompatibleChains(260-289)src/services/tokensData.ts (1)
chainIdToChainNameTokensData(213-234)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: build
🔇 Additional comments (2)
src/services/pillarXApiWalletPortfolio.ts (2)
74-76: New feature flags enabled by default.The code adds three new boolean parameters all set to 'true':
unlistedAssets,filterSpam, andpnl. Since these are being enabled by default for all calls, verify that this behavior is intentional and won't cause unexpected side effects for existing functionality.Is there a need for these parameters to be configurable rather than hardcoded to 'true'? Consider whether future use cases might require toggling these features.
71-77:Details
❓ Verification inconclusive
Potential chain name inconsistency between declaration and usage.
The implementation adds new parameters to the wallet portfolio endpoint, which enhances the API's functionality. However, there's a potential inconsistency between the chain names defined in
CompatibleChainsand those returned bychainIdToChainNameTokensData:
- ChainId 100: "Gnosis" vs "XDAI"
- ChainId 10: "Optimism" vs "Optimistic"
- ChainId 56: "BNB Smart Chain" vs "BNB Smart Chain (BEP20)"
Please verify that these chain names match what the backend API expects:
🏁 Script executed:
#!/bin/bash # Compare chain names in CompatibleChains and chainIdToChainNameTokensData echo "Comparing chain names in CompatibleChains and chainIdToChainNameTokensData:" # Extract and compare for each chain ID for chain_id in 1 137 8453 100 56 10 42161; do echo -n "Chain ID $chain_id: " name_in_compatible=$(rg "chainId: $chain_id" -A 1 | rg "chainName:" | head -1 | sed -E 's/.*chainName: .([^"]+).,.*/\1/') # Find the matching case in chainIdToChainNameTokensData name_in_function=$(rg "case $chain_id:" -A 1 src/services/tokensData.ts | rg "return" | head -1 | sed -E "s/.*return '([^']+)'.*/\1/") echo "CompatibleChains: \"$name_in_compatible\", chainIdToChainNameTokensData: \"$name_in_function\"" doneLength of output: 3783
I’d like to confirm the exact declarations for
CompatibleChainsand the 3 mappings inchainIdToChainNameTokensData. Running:
🏁 Script executed:
#!/bin/bash set -eux # 1) Find where CompatibleChains is defined and show its entries echo "🔍 CompatibleChains declaration:" rg -n "CompatibleChains" -n . while IFS=: read -r file line _; do echo "---- $file (around line $line) ----" sed -n "$((line-3)),$((line+15))p" "$file" done < <(rg -n "CompatibleChains" -n .) # 2) Show mapping cases for chain IDs 10, 56, and 100 echo for id in 10 56 100; do echo "🔍 chainIdToChainNameTokensData – case $id:" rg -n "case $id" -n src/services/tokensData.ts -C 2 doneLength of output: 4620
To accurately extract the declared
CompatibleChainsnames and the exact mappings, let’s pull the full array block and the specificcasestatements:
🏁 Script executed:
#!/bin/bash set -eux # 1) Extract the CompatibleChains array from blockchain.ts file="src/utils/blockchain.ts" declaration_line=$(rg -n "export const CompatibleChains" -n "$file" | head -1 | cut -d: -f2) echo "🔍 CompatibleChains declared at $file:$declaration_line" sed -n "$((declaration_line-3)),$((declaration_line+60))p" "$file" # 2) Show the chainIdToChainNameTokensData mappings for IDs 100, 56, and 10 for id in 100 56 10; do echo echo "🔍 Mapping in src/services/tokensData.ts – case $id" rg -n "case $id:" -C2 src/services/tokensData.ts doneLength of output: 475
Confirm backend chain‐name expectations
The names returned by
chainIdToChainNameTokensDatadon’t match the labels inCompatibleChains. Please verify which naming convention the wallet-portfolio endpoint expects and align both definitions accordingly:• In src/utils/blockchain.ts –
CompatibleChainsdeclares:
– Chain ID 100 → “Gnosis”
– Chain ID 56 → “BNB Smart Chain”
– Chain ID 10 → “Optimism”• In src/services/tokensData.ts –
chainIdToChainNameTokensDatamaps:
– Chain ID 100 → “XDAI”
– Chain ID 56 → “BNB Smart Chain (BEP20)”
– Chain ID 10 → “Optimistic”Action items:
- Confirm with the backend team which exact strings the API expects for these chains.
- Update either
CompatibleChainsorchainIdToChainNameTokensData(or both) so that they’re fully consistent.
github-actions
bot
temporarily deployed
to
Preview (fix/PRO-3299/add-params-portfolio-endpoint)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/apps/the-exchange/components/SwapReceiveCard/SwapReceiveCard.tsx (2)
62-62: Consider consistent handling of empty chain filtersWhile this change correctly uses the chain ID string directly, the handling of empty/undefined values differs from other components. Here you're using string interpolation with an empty fallback (
${chain || ''}), while other components useundefinedwhen no filter should be applied.- filterBlockchains: `${chain || ''}`, + filterBlockchains: chain || undefined,
1-154: Remaining chainIdToChainNameTokensData usagesThis file still has usages of both
chainIdToChainNameTokensDataandchainNameToChainIdTokensDatafunctions (lines 76 and 125-127) while the current PR is removing similar usages across the codebase. Consider updating these instances for consistency with the rest of the changes in this PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/apps/the-exchange/components/SwapReceiveCard/SwapReceiveCard.tsx(1 hunks)src/apps/the-exchange/components/TokenSearchInput/TokenSearchInput.tsx(1 hunks)src/apps/token-atlas/components/TokensSearchInput/TokensSearchInput.tsx(1 hunks)src/apps/token-atlas/index.tsx(3 hunks)src/services/pillarXApiSearchTokens.ts(1 hunks)src/services/pillarXApiWalletPortfolio.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/services/pillarXApiWalletPortfolio.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/services/pillarXApiSearchTokens.ts (1)
src/utils/blockchain.ts (1)
CompatibleChains(260-289)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: build
- GitHub Check: Cloudflare Pages
🔇 Additional comments (6)
src/services/pillarXApiSearchTokens.ts (1)
52-54: Good standardization of blockchain identifiersThis change correctly updates the code to use chain IDs directly rather than chain names, aligning with the broader standardization across the application.
src/apps/the-exchange/components/TokenSearchInput/TokenSearchInput.tsx (1)
92-93: Improved blockchain filtering approachGood conversion to using direct chain IDs instead of mapped chain names for the filtering parameter. The conditional logic handles the case where no blockchain filter should be applied (when chainId is 0) by setting it to undefined.
src/apps/token-atlas/components/TokensSearchInput/TokensSearchInput.tsx (1)
71-72: Consistent chain ID filtering approachThis change correctly implements the same pattern for blockchain filtering as in the TokenSearchInput component, using the raw chain ID string instead of a mapped chain name when a filter should be applied.
src/apps/token-atlas/index.tsx (3)
141-141: Standardized blockchain parameter formatThe change from using chain name mapping to direct string conversion of chain IDs aligns with the PR objective of standardizing blockchain parameters across the codebase.
155-155: Consistent use of string chain IDsThis change maintains consistency with other API calls by using string template literals for the chain ID.
171-171: Chain parameter formatting aligns with standardThe change to use template literals for chain IDs correctly handles the null/undefined case with fallback to empty string. This works well with the skip condition on line 173 that prevents API calls when both asset and chain are missing.
2 participants
Description
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Summary by CodeRabbit