feat: fetch data for "Portfolio by asset type" donut chart #920
Conversation
|
@kfastov is attempting to deploy a commit to the LFG Labs Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe changes in the pull request significantly modify the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
|
I'll do optimizations and some tests (on different accounts) today and then mark as ready |
|
Hey @kfastov , thanks for the mention! If I may say, looking at the API behavior, all the protocol pair tokens for a given protocol share the same symbol. This are all the count of tokens that use the same symbol filtering by name containing "pair" (ci):
One approach would be displaying a sum of all the tokens for a given protocol under the same symbol (as they are listed in the API). This would make the chart more aligned to the actual portfolio data, as ignoring them altogether would kinda defeat the purpose of the chart. This should be achievable using a map with the symbol as a key and incrementing the balance on each encounter of a userToken with the same symbol. What do you think? @Marchand-Nicolas PD: you can also use my account to test the chart (0x06F17c9e19D91900ac039f60344cECC6494297CBd37BbB9B3618B048a2Ed3633) |
|
What I aim now is just matching the Argent logic. It seems that Argent portfolio just shows the "free" undeposited tokens, but I agree that it may be counter-intuitive. |
|
@kfastov Hey, if that is doable, I think that indeed it would be better to take in account tokens that are locked in protocols such as zklend or ekubo |
Thanks for the help, it seems perfectly appropriate to me |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
I am working on asset exclusion logic now. Hope to finish it tomorrow. If I realize I can't, I'll just pick a simpler strategy.
If I understood correctly (please correct me if I'm wrong), it would make asset chart partially resemble the protocol chart, which also kinda defeats its purpose. But I would instead display a sum of all positions across all protocols for a given token, as it's already implemented in this draft PR.
Yes, that's how it's currently implemented. |
|
I like the idea of the of displaying the base asset instead of the protocol one, but I think it could not be applicable to all cases. The last comment was regarding Pair Tokens, and those tokens are a mix of other two one (ETH/USDC) for example. The issue arises mainly on how the protocol manage pair tokens, as some create a new token for that pair, and others display them individually, some examples are:
I guess as the protocols don't have a standardized way to manage them, we could either chose one of the approaches or tey to handle both. For the pair tokens, we could either display them by protocol pair symbol (last comment) or, what I think may be better, is to display the pair symbols as indovidual entries in the chart, e.g. STRK/ETH, USDT/LORDS, etc You can check the response of |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
app/[addressOrDomain]/page.tsx (4)
58-64: Avoid Hardcoding Protocol IDs; Implement Dynamic Exclusion LogicHardcoding protocol IDs makes future maintenance challenging and can lead to errors if IDs change or new protocols need to be excluded. Consider fetching these IDs from a configuration file, environment variables, or implementing a dynamic exclusion mechanism based on protocol properties.
Apply this diff to modify the exclusion logic:
-const EXCLUDED_PROTOCOL_IDS = [ - '8e4c3af5-f9bc-4646-9b66-3cd7bf7c8aec', // zkLend - '19a6474c-17a9-4d61-95ab-5f768cd01485', // Nimbora - '6203cd68-991a-4bae-a18a-ff42207ce813', // Vesu - // TODO: Add more protocols or add better exclusion logic (ban all protocols?) -]; +// Fetch excluded protocol IDs from a configuration or implement dynamic logic +const EXCLUDED_PROTOCOL_IDS = getExcludedProtocolIds();
216-241: Remove Console Statements from Production CodeConsole statements are useful during development but should be removed or replaced with a proper logging mechanism in production to avoid cluttering the console and potential performance impacts.
Apply this diff to remove the debug statements:
- console.log(tokenInfo); - - if (tokenInfo.dappId) { - console.log('Dapp info for token', tokenInfo.name, { - dappName: dapps[tokenInfo.dappId]?.name, - dappId: tokenInfo.dappId, - dappDetails: dapps[tokenInfo.dappId] - }); - } - - // ... - - console.log('Added token value:', value, 'from token:', tokenInfo.symbol);
Line range hint
451-476: Optimize Asynchronous Operations and Ensure Consistent Error HandlingIn
fetchPortfolioProtocols, asynchronous operations inside loops can be optimized usingPromise.allto enhance performance. Additionally, ensure that all potential errors are properly caught and handled to prevent unhandled exceptions.Example refactoring:
- await getProtocolsFromTokens(protocolsMap, userTokens, tokens, dapps); - await handleDebt(protocolsMap, userDapps, tokens); - await getProtocolsFromDapps(protocolsMap, userDapps, tokens, dapps); + await Promise.all([ + getProtocolsFromTokens(protocolsMap, userTokens, tokens, dapps), + handleDebt(protocolsMap, userDapps, tokens), + getProtocolsFromDapps(protocolsMap, userDapps, tokens, dapps), + ]);Consider reviewing the dependency array in your
useCallbackhook to include all dependencies used within the function.
478-506: Improve Error Handling and Code Readability infetchPortfolioDataEnsure that all possible errors during data fetching are properly caught and handled to provide better resilience. Additionally, consider splitting
fetchPortfolioDatainto smaller, more focused functions for improved readability and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
app/[addressOrDomain]/page.tsx(4 hunks)
🔇 Additional comments (2)
app/[addressOrDomain]/page.tsx (2)
299-311: Ensure Accurate Calculation for 'Others' Category to Prevent Rounding Errors
When aggregating the "Others" category, rounding can cause the total percentage to exceed 100%. Verify the calculations and consider implementing a correction mechanism to adjust for rounding discrepancies.
Example adjustment:
// After calculating percentages
const totalPercentage = sortedAssets.reduce((sum, asset) => sum + parseFloat(asset.itemValue), 0);
const roundingError = 100 - totalPercentage;
if (roundingError !== 0) {
// Adjust the "Others" category
const othersIndex = sortedAssets.findIndex(asset => asset.itemLabel === "Others");
if (othersIndex !== -1) {
sortedAssets[othersIndex].itemValue = (parseFloat(sortedAssets[othersIndex].itemValue) + roundingError).toFixed(2);
}
}510-513:
Include Missing Dependencies in useEffect Hook
The useEffect hook depends on fetchQuestData, fetchPageData, and fetchPortfolioData functions. To prevent potential issues with stale closures or missing updates, include these functions in the dependency array.
Apply this diff to update the dependency array:
- }, [identity]);
+ }, [identity, fetchQuestData, fetchPageData, fetchPortfolioData]);Likely invalid or redundant comment.
app/[addressOrDomain]/page.tsx
Outdated
| const colors = ["#1E2097", "#637DEB", "#2775CA", "#5CE3FE", "#F4FAFF"]; | ||
| sortedAssets.forEach((asset, index) => { | ||
| asset.color = colors[index]; | ||
| }); | ||
|
|
There was a problem hiding this comment.
Handle Potential Index Out-of-Bounds When Assigning Colors
The colors array has five colors, but if sortedAssets contains more than five items (including "Others"), it may result in undefined colors. Ensure that the number of colors matches the number of items or implement logic to assign default colors when the index exceeds the array length.
Apply this diff to prevent index errors:
- const colors = ["#1E2097", "#637DEB", "#2775CA", "#5CE3FE", "#F4FAFF"];
- sortedAssets.forEach((asset, index) => {
- asset.color = colors[index];
- });
+ const colors = ["#1E2097", "#637DEB", "#2775CA", "#5CE3FE", "#F4FAFF"];
+ sortedAssets.forEach((asset, index) => {
+ asset.color = colors[index % colors.length]; // Use modulo to recycle colors if needed
+ });📝 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.
| const colors = ["#1E2097", "#637DEB", "#2775CA", "#5CE3FE", "#F4FAFF"]; | |
| sortedAssets.forEach((asset, index) => { | |
| asset.color = colors[index]; | |
| }); | |
| const colors = ["#1E2097", "#637DEB", "#2775CA", "#5CE3FE", "#F4FAFF"]; | |
| sortedAssets.forEach((asset, index) => { | |
| asset.color = colors[index % colors.length]; // Use modulo to recycle colors if needed | |
| }); |
There was a problem hiding this comment.
We have only 5 sections in chart, but you are right, better safe than sorry
There was a problem hiding this comment.
@kfastov, understood. Implementing the modulo operation will make the code more robust in case the number of chart sections changes in the future.
app/[addressOrDomain]/page.tsx
Outdated
| for await (const token of userTokens) { | ||
| const tokenInfo = tokens[token.tokenAddress]; | ||
| if (!tokenInfo || token.tokenBalance === "0") continue; | ||
|
|
||
| // TODO: Implement fetch from Argent API | ||
| const assets = [ | ||
| { color: "#1E2097", itemLabel: "USDC", itemValue: "46.68", itemValueSymbol: "%" }, | ||
| { color: "#637DEB", itemLabel: "USDT", itemValue: "27.94", itemValueSymbol: "%" }, | ||
| { color: "#2775CA", itemLabel: "STRK", itemValue: "22.78", itemValueSymbol: "%" }, | ||
| { color: "#5CE3FE", itemLabel: "ETH", itemValue: "0.36", itemValueSymbol: "%" }, | ||
| { color: "#F4FAFF", itemLabel: "Others", itemValue: "2.36", itemValueSymbol: "%" }, | ||
| ]; | ||
| setPortfolioAssets(assets); | ||
| console.log(tokenInfo); | ||
|
|
||
| if (tokenInfo.dappId) { | ||
| console.log('Dapp info for token', tokenInfo.name, { | ||
| dappName: dapps[tokenInfo.dappId]?.name, | ||
| dappId: tokenInfo.dappId, | ||
| dappDetails: dapps[tokenInfo.dappId] | ||
| }); | ||
| } | ||
|
|
||
| // Skip tokens from excluded protocols | ||
| if (tokenInfo.dappId && EXCLUDED_PROTOCOL_IDS.includes(tokenInfo.dappId)) { | ||
| continue; | ||
| } | ||
|
|
||
| const value = await calculateTokenPrice( | ||
| token.tokenAddress, | ||
| tokenToDecimal(token.tokenBalance, tokenInfo.decimals), | ||
| "USD" | ||
| ); | ||
|
|
||
| // Add to total value regardless of protocol | ||
| totalValue += value; | ||
| totalTokenValue += value; | ||
| console.log('Added token value:', value, 'from token:', tokenInfo.symbol); | ||
|
|
||
| // Only add to asset breakdown if not from excluded protocol | ||
| const symbol = tokenInfo.symbol || "Unknown"; | ||
| assetValues[symbol] = (assetValues[symbol] || 0) + value; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize Asynchronous Operations in Loops to Improve Performance
Using await inside a for loop results in sequential execution, which can significantly slow down the process when dealing with many items. Consider using Promise.all with map to execute asynchronous operations in parallel.
Apply this diff to refactor the loop:
- for await (const token of userTokens) {
- // Existing code...
- const value = await calculateTokenPrice(
- token.tokenAddress,
- tokenToDecimal(token.tokenBalance, tokenInfo.decimals),
- "USD"
- );
- // Existing code...
- }
+ const assetValuePromises = userTokens.map(async (token) => {
+ // Existing code...
+ const value = await calculateTokenPrice(
+ token.tokenAddress,
+ tokenToDecimal(token.tokenBalance, tokenInfo.decimals),
+ "USD"
+ );
+ // Existing code...
+ });
+ await Promise.all(assetValuePromises);Similarly, apply this refactoring to other loops within the function to enhance overall performance.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Refactored to parallel processing
app/[addressOrDomain]/page.tsx
Outdated
| for await (const userDapp of userDapps) { | ||
| const positions = userDapp.products[0]?.positions; | ||
| if (!positions) continue; | ||
|
|
||
| for (const position of positions) { | ||
| const totalBalances = position.totalBalances; | ||
| for (const tokenAddress of Object.keys(totalBalances)) { | ||
| const tokenBalance = totalBalances[tokenAddress]; | ||
| const tokenInfo = tokens[tokenAddress]; | ||
| if (!tokenInfo) continue; | ||
| if (tokenBalance === "0") continue; | ||
|
|
||
| const value = await calculateTokenPrice( | ||
| tokenAddress, | ||
| tokenToDecimal(tokenBalance, tokenInfo.decimals), | ||
| "USD" | ||
| ); | ||
|
|
||
| totalProtocolValue += value; | ||
|
|
||
| // Don't add negative balances to total value (or may have percentages > 100%) | ||
| if (value < 0) continue; | ||
| // Add to total value regardless of protocol | ||
| totalValue += value; | ||
| console.log('Added protocol value:', value, 'from token:', tokenInfo.symbol, 'protocol:', dapps[userDapp.dappId].name); | ||
| // Only add to asset breakdown if not from excluded protocol. Restaking anyone? | ||
| if (tokenInfo.dappId && EXCLUDED_PROTOCOL_IDS.includes(tokenInfo.dappId)) { | ||
| continue; | ||
| } | ||
| const symbol = tokenInfo.symbol || "Unknown"; | ||
| assetValues[symbol] = (assetValues[symbol] || 0) + value; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor Nested Loops for Efficient Asynchronous Processing
The nested loops with asynchronous calls inside for loops cause sequential execution. Refactor these loops using Promise.all to execute asynchronous operations concurrently and improve performance.
Example refactoring:
- for await (const userDapp of userDapps) {
- // Existing code...
- for (const position of positions) {
- // Existing code...
- const value = await calculateTokenPrice(...);
- // Existing code...
- }
- }
+ const dappPromises = userDapps.map(async (userDapp) => {
+ // Existing code...
+ const positionPromises = positions.map(async (position) => {
+ // Existing code...
+ const value = await calculateTokenPrice(...);
+ // Existing code...
+ });
+ await Promise.all(positionPromises);
+ });
+ await Promise.all(dappPromises);Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Refactored to flatten + parallel processing
There was a problem hiding this comment.
app/[addressOrDomain]/page.tsx
Outdated
| }) => { | ||
| const { dapps, tokens, userTokens, userDapps } = data; | ||
| try { | ||
| // TODO: correct this condition |
There was a problem hiding this comment.
What do you mean ?
There was a problem hiding this comment.
Nothing, just a misplaced comment, was intended for my code, already deleted
There was a problem hiding this comment.
Also, console logs are deleted too
There was a problem hiding this comment.
Removed useless comments and logs
|
Also there are some conflicts now, but it should be quick to fix |
|
Omg I messed up with branches and didn't pushed last 2 commits, sorry for that |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
services/argentPortfolioService.ts (2)
59-67: Enhance error handling implementation.The new error handling for missing token prices is a good addition, but consider these improvements:
- The error message check is fragile and could break if the API changes its error format
- The return value of 0 should be documented as it represents a significant business logic decision
Consider this enhanced implementation:
let data: ArgentTokenValue; try { data = await fetchData<ArgentTokenValue>(`${API_BASE}/${API_VERSION}/tokens/prices/${tokenAddress}?chain=starknet¤cy=${currency}`); } catch (err) { - if (err instanceof Error && err.message.includes('No price for token')) { + // Return 0 for tokens without price data (e.g., new or unlisted tokens) + if (err instanceof Error && ( + err.message.includes('No price for token') || + err.message.includes('404') // Add other relevant error patterns + )) { return 0; } throw err; }
59-67: Consider caching token prices for performance optimization.Since this service is used for portfolio visualization, multiple calls for the same token prices might occur. Consider implementing a caching mechanism to improve performance and reduce API calls.
app/[addressOrDomain]/page.tsx (1)
Line range hint
485-511: Fix dependency array and improve validationTwo issues need attention:
- The dependency array includes 'address' which isn't used in the function
- The validation condition could be more explicit
Apply these changes:
const fetchPortfolioProtocols = useCallback(async (data: { dapps: ArgentDappMap, tokens: ArgentTokenMap, userTokens: ArgentUserToken[], userDapps: ArgentUserDapp[] }) => { - if (!dapps || !tokens || (!userTokens && !userDapps)) return; + if (!dapps || !tokens || !userTokens || !userDapps) { + console.warn('Missing required data for protocol calculation'); + return; + } // ... rest of the function -}, [address]); +}, []);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
app/[addressOrDomain]/page.tsx(3 hunks)services/argentPortfolioService.ts(1 hunks)
🔇 Additional comments (3)
services/argentPortfolioService.ts (1)
59-67: Verify error handling behavior across different token types.
Given the PR's focus on portfolio visualization and the discussions about protocol tokens, we should verify how this error handling behaves with different token types.
✅ Verification successful
Let me gather more context about the token price handling implementation.
Let me check the token type definitions and any specific handling for different token types.
Based on the gathered information, I can now provide the final response:
The error handling for token prices is appropriate and consistent with the token types.
The error handling specifically catches the "No price for token" error and returns 0, which is the correct behavior because:
- The
ArgentTokenValuetype shows this is a generic price lookup endpoint that works for all token types - The error handling is the only implementation in the codebase for token price fetching
- The function is used uniformly across different token categories (base tokens, protocol tokens) as seen in the type definitions
- Returning 0 for tokens without prices is a safe fallback that won't break portfolio calculations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for token price handling patterns across the codebase
# to ensure consistent behavior with protocol and base tokens
# Look for other instances of token price calculations
rg -l "tokens/prices" --type ts
# Check for related error handling patterns
rg "No price for token" --type ts
# Look for token type checks or filtering
ast-grep --pattern 'if ($COND.includes("token")) { $BODY }'
Length of output: 278
Script:
#!/bin/bash
# Get more context around the token price fetching implementation
rg -B 5 -A 10 "tokens/prices" services/argentPortfolioService.ts
# Look for token-related interfaces and types
rg "interface.*Token" --type ts
rg "type.*Token" --type ts
# Search for other price-related error handling
rg "catch.*err.*price" -A 3 --type ts
Length of output: 1084
Script:
#!/bin/bash
# Get the token type definitions
cat types/backTypes.d.ts
# Look for any token type-specific handling
rg "ArgentToken|ArgentTokenValue|ArgentUserToken" -B 2 -A 2 --type ts
# Check for any token filtering or validation
rg "token.*filter|validate.*token" --type ts
Length of output: 16136
app/[addressOrDomain]/page.tsx (2)
513-543: LGTM! Well-structured data fetching implementation
The function demonstrates good practices:
- Parallel data fetching with Promise.all
- Proper error handling
- Loading state management
- Case normalization for addresses
548-549: LGTM! Appropriate effect dependency
The portfolio data fetch is correctly triggered when the identity changes.
app/[addressOrDomain]/page.tsx
Outdated
| // TODO: correct this condition | ||
| if (!tokens || !userTokens) return; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve data validation condition
The TODO comment indicates that the condition needs improvement. The current check is incomplete as it doesn't validate all required parameters.
Replace with a more comprehensive validation:
- // TODO: correct this condition
- if (!tokens || !userTokens) return;
+ if (!dapps || !tokens || !userTokens || !userDapps) {
+ console.warn('Missing required data for portfolio calculation');
+ return;
+ }Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Removed todo and improved validation
app/[addressOrDomain]/page.tsx
Outdated
| // First pass: calculate total value and individual token values | ||
| for await (const token of userTokens) { | ||
| const tokenInfo = tokens[token.tokenAddress]; | ||
| if (!tokenInfo || token.tokenBalance === "0") continue; | ||
|
|
||
| // Skip protocol tokens (like LPT pair tokens, staking, etc.) | ||
| if (tokenInfo.dappId) { | ||
| continue; | ||
| } | ||
|
|
||
| const value = await calculateTokenPrice( | ||
| token.tokenAddress, | ||
| tokenToDecimal(token.tokenBalance, tokenInfo.decimals), | ||
| "USD" | ||
| ); | ||
|
|
||
| // Add to total value regardless of protocol | ||
| totalValue += value; | ||
| totalTokenValue += value; | ||
|
|
||
| // Only add to asset breakdown if not from excluded protocol | ||
| const symbol = tokenInfo.symbol || "Unknown"; | ||
| assetValues[symbol] = (assetValues[symbol] || 0) + value; | ||
| } | ||
|
|
||
| // Process tokens from userDapps | ||
| for await (const userDapp of userDapps) { | ||
| const positions = userDapp.products[0]?.positions; | ||
| if (!positions) continue; | ||
|
|
||
| for (const position of positions) { | ||
| const totalBalances = position.totalBalances; | ||
| for (const tokenAddress of Object.keys(totalBalances)) { | ||
| const tokenBalance = totalBalances[tokenAddress]; | ||
| const tokenInfo = tokens[tokenAddress]; | ||
| if (!tokenInfo) continue; | ||
| if (tokenBalance === "0") continue; | ||
|
|
||
| const value = await calculateTokenPrice( | ||
| tokenAddress, | ||
| tokenToDecimal(tokenBalance, tokenInfo.decimals), | ||
| "USD" | ||
| ); | ||
|
|
||
| totalProtocolValue += value; | ||
|
|
||
| // Don't add negative balances to total value (or may have percentages > 100%) | ||
| if (value < 0) continue; | ||
| // Add to total value regardless of protocol | ||
| totalValue += value; | ||
| // Only add to asset breakdown if the token is not from a protocol itself | ||
| if (tokenInfo.dappId) { | ||
| continue; | ||
| } | ||
| const symbol = tokenInfo.symbol || "Unknown"; | ||
| assetValues[symbol] = (assetValues[symbol] || 0) + value; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Convert to percentages and format | ||
| const sortedAssets = Object.entries(assetValues) | ||
| .sort(([, a], [, b]) => b - a) | ||
| .map(([symbol, value]) => ({ | ||
| itemLabel: symbol, | ||
| itemValue: ((value / totalValue) * 100).toFixed(2), | ||
| itemValueSymbol: "%", | ||
| }, | ||
| { | ||
| color: "#F4FAFF", | ||
| color: "" // Colors will be assigned later | ||
| })); | ||
|
|
||
| // Handle "Others" category if needed | ||
| if (sortedAssets.length > 4) { | ||
| const others = sortedAssets.slice(4).reduce( | ||
| (sum, asset) => sum + parseFloat(asset.itemValue), | ||
| 0 | ||
| ); | ||
| sortedAssets.splice(4); | ||
| sortedAssets.push({ | ||
| itemLabel: "Others", | ||
| itemValue: "2.36", | ||
| itemValue: others.toFixed(2), | ||
| itemValueSymbol: "%", | ||
| }, | ||
| ]; | ||
| setPortfolioAssets(assets); | ||
| color: "" | ||
| }); | ||
| } | ||
|
|
||
| // Assign colors | ||
| const colors = ["#1E2097", "#637DEB", "#2775CA", "#5CE3FE", "#F4FAFF"]; | ||
| sortedAssets.forEach((asset, index) => { | ||
| asset.color = colors[index]; | ||
| }); | ||
|
|
||
| return sortedAssets; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize async operations and add error handling
The function has several areas that could be improved:
- Sequential async operations in loops can be optimized
- Missing error handling for price calculations
- Potential division by zero when totalValue is 0
Consider this refactoring:
const calculateAssetPercentages = async (
userTokens: ArgentUserToken[],
tokens: ArgentTokenMap,
dapps: ArgentDappMap,
userDapps: ArgentUserDapp[],
) => {
let totalValue = 0;
let totalTokenValue = 0;
let totalProtocolValue = 0;
const assetValues: { [symbol: string]: number } = {};
- // First pass: calculate total value and individual token values
- for await (const token of userTokens) {
+ // Process user tokens in parallel
+ const userTokenPromises = userTokens.map(async (token) => {
const tokenInfo = tokens[token.tokenAddress];
if (!tokenInfo || token.tokenBalance === "0") continue;
if (tokenInfo.dappId) {
continue;
}
+ try {
const value = await calculateTokenPrice(
token.tokenAddress,
tokenToDecimal(token.tokenBalance, tokenInfo.decimals),
"USD"
);
+ return { value, symbol: tokenInfo.symbol || "Unknown" };
+ } catch (error) {
+ console.error(`Error calculating price for token ${token.tokenAddress}:`, error);
+ return { value: 0, symbol: tokenInfo.symbol || "Unknown" };
+ }
- totalValue += value;
- totalTokenValue += value;
- const symbol = tokenInfo.symbol || "Unknown";
- assetValues[symbol] = (assetValues[symbol] || 0) + value;
});
+ const userTokenResults = await Promise.all(userTokenPromises);
+ userTokenResults.forEach(({ value, symbol }) => {
+ if (value > 0) {
+ totalValue += value;
+ totalTokenValue += value;
+ assetValues[symbol] = (assetValues[symbol] || 0) + value;
+ }
+ });
// Similar refactoring for userDapps processing...
// Prevent division by zero
+ if (totalValue === 0) {
+ return [];
+ }
const sortedAssets = Object.entries(assetValues)
.sort(([, a], [, b]) => b - a)
.map(([symbol, value]) => ({
itemLabel: symbol,
itemValue: ((value / totalValue) * 100).toFixed(2),
itemValueSymbol: "%",
color: ""
}));Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Refactored to parallel processing + added zero division protection and some error handling
There was a problem hiding this comment.
@kfastov Great job! I'm glad you refactored to parallel processing, added zero division protection, and improved error handling. This should enhance the performance and reliability of the function. 😊
(_/)
(^‿^)
(")_(")
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
|
Sorry, haven't pushed last commit, just a few minutes, local merge problems |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
app/[addressOrDomain]/page.tsx (2)
318-321: Consider moving colors to constants.The color array should be moved to a constants file for better maintainability and reusability.
-const colors = ["#1E2097", "#637DEB", "#2775CA", "#5CE3FE", "#F4FAFF"]; +import { PORTFOLIO_ASSET_COLORS } from '@constants/colors'; + sortedAssets.forEach((asset, index) => { - asset.color = colors[index % colors.length]; + asset.color = PORTFOLIO_ASSET_COLORS[index % PORTFOLIO_ASSET_COLORS.length]; });
Line range hint
503-531: Improve error handling to prevent memory leaks.The error handling should clean up state in case of failure to prevent stale data.
} catch (error) { showNotification( "Error while calculating address portfolio stats", "error" ); console.log("Error while calculating address portfolio stats", error); + setPortfolioProtocols([]); // Reset state on error }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
app/[addressOrDomain]/page.tsx(3 hunks)
🔇 Additional comments (2)
app/[addressOrDomain]/page.tsx (2)
325-342: LGTM! Well-structured with proper validation and error handling.
The function properly validates input data and handles errors appropriately.
569-569: LGTM! Proper integration of portfolio data fetching.
The portfolio data fetching is correctly integrated into the component lifecycle.
app/[addressOrDomain]/page.tsx
Outdated
| const fetchPortfolioData = useCallback(async (addr: string) => { | ||
| setLoadingProtocols(true); | ||
| try { | ||
| // Argent API requires lowercase address | ||
| const normalizedAddr = addr.toLowerCase(); | ||
| const [dappsData, tokensData, userTokensData, userDappsData] = | ||
| await Promise.all([ | ||
| fetchDapps(), | ||
| fetchTokens(), | ||
| fetchUserTokens(normalizedAddr), | ||
| fetchUserDapps(normalizedAddr), | ||
| ]); | ||
|
|
||
| const data = { | ||
| dapps: dappsData, | ||
| tokens: tokensData, | ||
| userTokens: userTokensData, | ||
| userDapps: userDappsData, | ||
| }; | ||
|
|
||
| await Promise.all([ | ||
| fetchPortfolioProtocols(data), | ||
| fetchPortfolioAssets(data), | ||
| ]); | ||
| } catch (error) { | ||
| showNotification("Error while fetching address portfolio", "error"); | ||
| console.log("Error while fetching address portfolio", error); | ||
| } finally { | ||
| setLoadingProtocols(false); | ||
| } | ||
| }, [fetchPortfolioProtocols, fetchPortfolioAssets]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding request cancellation.
Long-running requests should be cancellable when the component unmounts to prevent memory leaks and state updates on unmounted components.
+const abortController = new AbortController();
try {
const [dappsData, tokensData, userTokensData, userDappsData] =
await Promise.all([
- fetchDapps(),
- fetchTokens(),
- fetchUserTokens(normalizedAddr),
- fetchUserDapps(normalizedAddr),
+ fetchDapps({ signal: abortController.signal }),
+ fetchTokens({ signal: abortController.signal }),
+ fetchUserTokens(normalizedAddr, { signal: abortController.signal }),
+ fetchUserDapps(normalizedAddr, { signal: abortController.signal }),
]);Add cleanup in useEffect:
useEffect(() => {
const controller = new AbortController();
if (!identity) return;
fetchQuestData(identity.owner);
fetchPageData(identity.owner);
fetchPortfolioData(identity.owner);
return () => controller.abort();
}, [identity]);There was a problem hiding this comment.
https://github.com/lfglabs-dev/starknet.quest/pull/920/files#r1827017886 Might be helpful in addition to the suggestion I made
There was a problem hiding this comment.
@Marchand-Nicolas, thank you for pointing this out. Incorporating request cancellation, as suggested in the linked comment, would indeed improve the component by preventing potential memory leaks when unmounting.
🔗 Analysis chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Fetch the comment with ID 1827017886 using GitHub CLI
gh api "repos/lfglabs-dev/starknet.quest/pulls/comments/1827017886" --jq '.body'
Length of output: 1205
app/[addressOrDomain]/page.tsx
Outdated
| fetchPortfolioAssets(data), | ||
| ]); | ||
| } catch (error) { | ||
| showNotification("Error while fetching address portfolio", "error"); |
There was a problem hiding this comment.
Could you implement some retry mechanism for all requests to portfolio ? It looks like the first time we fetch any address, the request fails (and after like 10 seconds of retrying it finally works). I think this is because it takes some time for them to gather and cache all the data
There was a problem hiding this comment.
Yep I'll do an API wrapper with retry feature now, sorry for the delay
app/[addressOrDomain]/page.tsx
Outdated
| const fetchPortfolioData = useCallback(async (addr: string) => { | ||
| setLoadingProtocols(true); | ||
| try { | ||
| // Argent API requires lowercase address | ||
| const normalizedAddr = addr.toLowerCase(); | ||
| const [dappsData, tokensData, userTokensData, userDappsData] = | ||
| await Promise.all([ | ||
| fetchDapps(), | ||
| fetchTokens(), | ||
| fetchUserTokens(normalizedAddr), | ||
| fetchUserDapps(normalizedAddr), | ||
| ]); | ||
|
|
||
| const data = { | ||
| dapps: dappsData, | ||
| tokens: tokensData, | ||
| userTokens: userTokensData, | ||
| userDapps: userDappsData, | ||
| }; | ||
|
|
||
| await Promise.all([ | ||
| fetchPortfolioProtocols(data), | ||
| fetchPortfolioAssets(data), | ||
| ]); | ||
| } catch (error) { | ||
| showNotification("Error while fetching address portfolio", "error"); | ||
| console.log("Error while fetching address portfolio", error); | ||
| } finally { | ||
| setLoadingProtocols(false); | ||
| } | ||
| }, [fetchPortfolioProtocols, fetchPortfolioAssets]); |
There was a problem hiding this comment.
https://github.com/lfglabs-dev/starknet.quest/pull/920/files#r1827017886 Might be helpful in addition to the suggestion I made
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
services/argentPortfolioService.ts (1)
78-80: Clarify error message to accurately reflect the number of attemptsThe error message thrown after retries may be misleading if the number of attempts differs from
maxRetries. If the function attemptsmaxRetriestimes and then fails, the message "Failed after ${maxRetries} retries" is accurate. However, if it attempts an extra time due to the loop logic, this message could be confusing. Consider adjusting the error message to reflect the actual number of attempts.Apply this diff to adjust the error message:
- `Failed after ${maxRetries} retries. Endpoint: ${endpoint}. Error: ${lastError.message}` + `Failed after ${retries} attempts. Endpoint: ${endpoint}. Error: ${lastError.message}`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
services/argentPortfolioService.ts(3 hunks)
🔇 Additional comments (4)
services/argentPortfolioService.ts (4)
19-22: Definition of default retry parameters is appropriate
The constants for default retry parameters are well-defined, providing clear and adjustable settings for the retry logic.
39-81: fetchDataWithRetry function enhances data fetching robustness
The introduction of the fetchDataWithRetry function with exponential backoff significantly improves the resilience of data fetching operations. The use of configurable options allows for flexibility and fine-tuning.
Line range hint 84-104: Refactored data fetching functions correctly utilize retry mechanism
The functions fetchDapps, fetchTokens, fetchUserTokens, and fetchUserDapps have been updated to use fetchDataWithRetry, enhancing error handling and reliability in data fetching.
108-121: Improved error handling in calculateTokenPrice function
The retry mechanism integrated into calculateTokenPrice, along with specific handling for the 'No price for token' error, is properly implemented. It effectively differentiates between retryable and non-retryable errors.
services/argentPortfolioService.ts
Outdated
| while (retries <= maxRetries) { | ||
| try { | ||
| return await fetchData<T>(endpoint); | ||
| } catch (err) { | ||
| lastError = err instanceof Error ? err : new Error(String(err)); | ||
|
|
||
| if (!filterErrorRetryable(lastError)) { | ||
| throw lastError; | ||
| } | ||
|
|
||
| if (retries === maxRetries) { | ||
| break; | ||
| } | ||
|
|
||
| await new Promise((resolve) => setTimeout(resolve, delay)); | ||
| delay = Math.min(delay * backoffFactor, maxDelay); // Cap the delay | ||
| retries++; | ||
| } |
There was a problem hiding this comment.
Adjust retry logic to prevent exceeding maximum retries
The fetchDataWithRetry function may attempt the request one more time than intended, potentially exceeding maxRetries. With the current loop condition while (retries <= maxRetries), and retries being incremented after the delay, this could result in up to maxRetries + 1 attempts. To ensure the number of attempts does not exceed maxRetries, consider modifying the loop condition or when retries is incremented.
Apply this diff to correct the logic:
- while (retries <= maxRetries) {
+ while (retries < maxRetries) {📝 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.
| while (retries <= maxRetries) { | |
| try { | |
| return await fetchData<T>(endpoint); | |
| } catch (err) { | |
| lastError = err instanceof Error ? err : new Error(String(err)); | |
| if (!filterErrorRetryable(lastError)) { | |
| throw lastError; | |
| } | |
| if (retries === maxRetries) { | |
| break; | |
| } | |
| await new Promise((resolve) => setTimeout(resolve, delay)); | |
| delay = Math.min(delay * backoffFactor, maxDelay); // Cap the delay | |
| retries++; | |
| } | |
| while (retries < maxRetries) { | |
| try { | |
| return await fetchData<T>(endpoint); | |
| } catch (err) { | |
| lastError = err instanceof Error ? err : new Error(String(err)); | |
| if (!filterErrorRetryable(lastError)) { | |
| throw lastError; | |
| } | |
| if (retries === maxRetries) { | |
| break; | |
| } | |
| await new Promise((resolve) => setTimeout(resolve, delay)); | |
| delay = Math.min(delay * backoffFactor, maxDelay); // Cap the delay | |
| retries++; | |
| } |
There was a problem hiding this comment.
Nope. Imagine maxRetries == 1. Then function will try this request once, than REtry it exactly 1 time. If one want to disable retries completely, they would just set maxRetries to 0. If it would lead to not trying requests at all, I would call it flaw in the logic
There was a problem hiding this comment.
@kfastov, understood. Thank you for the clarification.
✏️ Learnings added
Learnt from: kfastov
PR: lfglabs-dev/starknet.quest#920
File: services/argentPortfolioService.ts:58-75
Timestamp: 2024-11-04T15:16:02.171Z
Learning: In the `fetchDataWithRetry` function in `services/argentPortfolioService.ts`, the retry logic intentionally uses `while (retries <= maxRetries)` to allow for the initial attempt plus the specified number of retries. Setting `maxRetries` to `0` results in only the initial request without any retries.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
It's max retries because you may not need so many retries: any subsequent request may become successful |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
services/argentPortfolioService.ts (3)
Line range hint
24-37: Consider adding request timeout.The fetch implementation looks good with proper error handling and abort signal support. However, consider adding a timeout to prevent hanging requests.
const fetchData = async <T>(endpoint: string, { signal }: { signal?: AbortSignal }): Promise<T> => { try { - const response = await fetch(endpoint, { headers: API_HEADERS, signal }); + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 30000); // 30s timeout + const response = await fetch(endpoint, { + headers: API_HEADERS, + signal: signal ?? controller.signal, + }); + clearTimeout(timeoutId); if (!response.ok) { throw new Error( `Error ${response.status}: ${await response.text()}` ); } return await response.json(); } catch (err) { console.log("Error fetching data from Argent API", err); throw err; } };
39-88: LGTM! Robust retry implementation with good error handling.The retry mechanism is well-implemented with:
- Configurable retry parameters
- Proper abort handling
- Custom error filtering
- Exponential backoff with maximum delay
Consider adding request timeout as suggested in the fetchData function to ensure consistent behavior across retries.
90-107: Consider documenting retry behavior.The API functions are well-implemented with proper error handling and type safety. Consider adding JSDoc comments to document the retry behavior and signal usage for better developer experience.
+/** + * Fetches dapps from Argent API with automatic retries. + * @param {Object} options - The options object + * @param {AbortSignal} [options.signal] - Optional abort signal to cancel the request + * @returns {Promise<ArgentDappMap>} A map of dapp IDs to dapp objects + * @throws {Error} If the request fails after all retries + */ export const fetchDapps = async ({ signal }: { signal?: AbortSignal }) => {app/[addressOrDomain]/page.tsx (3)
204-323: Consider extracting magic numbers into named constants.The function uses magic numbers that could be made more maintainable by extracting them into named constants.
+const MAX_VISIBLE_ASSETS = 4; +const ASSET_COLORS = ["#1E2097", "#637DEB", "#2775CA", "#5CE3FE", "#F4FAFF"]; const calculateAssetPercentages = async (...) => { // ... - if (sortedAssets.length > 4) { + if (sortedAssets.length > MAX_VISIBLE_ASSETS) { const others = sortedAssets - .slice(4) + .slice(MAX_VISIBLE_ASSETS) .reduce((sum, asset) => sum + parseFloat(asset.itemValue), 0); - sortedAssets.splice(4); + sortedAssets.splice(MAX_VISIBLE_ASSETS); // ... } - const colors = ["#1E2097", "#637DEB", "#2775CA", "#5CE3FE", "#F4FAFF"]; sortedAssets.forEach((asset, index) => { - asset.color = colors[index % colors.length]; + asset.color = ASSET_COLORS[index % ASSET_COLORS.length]; });
Line range hint
503-531: Remove unused dependency from useCallback.The dependency array includes
addresswhich isn't used within the function. This could cause unnecessary re-renders.const fetchPortfolioProtocols = useCallback(async (data: { dapps: ArgentDappMap, tokens: ArgentTokenMap, userTokens: ArgentUserToken[], userDapps: ArgentUserDapp[] - }) => { + }) => { // ... function implementation - }, [address]); + }, []);
565-574: Consider using AbortController for all fetch operations.While the abort controller is properly implemented for portfolio data fetching, consider extending it to
fetchQuestDataandfetchPageDatafor consistent cancellation behavior across all data fetching operations.useEffect(() => { const abortController = new AbortController(); if (!identity) return; - fetchQuestData(identity.owner); - fetchPageData(identity.owner); + fetchQuestData(identity.owner, abortController); + fetchPageData(identity.owner, abortController); fetchPortfolioData(identity.owner, abortController); return () => abortController.abort(); }, [identity]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
app/[addressOrDomain]/page.tsx(6 hunks)services/argentPortfolioService.ts(2 hunks)
🧰 Additional context used
📓 Learnings (1)
services/argentPortfolioService.ts (1)
Learnt from: kfastov
PR: lfglabs-dev/starknet.quest#920
File: services/argentPortfolioService.ts:58-75
Timestamp: 2024-11-04T15:16:02.344Z
Learning: In the `fetchDataWithRetry` function in `services/argentPortfolioService.ts`, the retry logic intentionally uses `while (retries <= maxRetries)` to allow for the initial attempt plus the specified number of retries. Setting `maxRetries` to `0` results in only the initial request without any retries.
🔇 Additional comments (4)
services/argentPortfolioService.ts (2)
19-22: LGTM! Well-chosen retry configuration.
The retry mechanism constants are well-configured with industry-standard values:
- 3 retries is a good balance
- 2-second initial delay with exponential backoff
- 10-second maximum delay prevents excessive waiting
112-130: Verify business logic for no-price tokens.
The function returns 0 when no price is available for a token. Please verify if this is the intended behavior as it might affect portfolio calculations.
Also, consider improving error logging to help with debugging:
} catch (err) {
if (err instanceof Error && err.message.includes('No price for token')) {
+ console.log(`No price available for token ${tokenAddress}`);
return 0;
}
+ console.error(`Failed to fetch price for token ${tokenAddress}:`, err);
throw err;
}Let's verify the impact of returning 0 for no-price tokens:
✅ Verification successful
Let me analyze the usage of calculateTokenPrice to understand the impact of returning 0 for no-price tokens. I can see it's used in portfolio calculations, but let me verify how these values are aggregated.
Returning 0 for no-price tokens is appropriate for portfolio calculations
The current implementation of returning 0 for tokens without prices is correct because:
- It's used in portfolio balance calculations where values are aggregated using
+= - A zero value ensures these tokens don't affect total balance calculations
- It handles gracefully the case where price feeds might be temporarily unavailable
- It's consistent with how portfolio valuations typically handle unlisted/unsupported tokens
The suggested error logging improvements would still be valuable for monitoring and debugging purposes, but the core business logic is sound.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of calculateTokenPrice to understand the impact
rg -l "calculateTokenPrice" | xargs rg -A 5 "calculateTokenPrice"
Length of output: 2671
Script:
#!/bin/bash
# Check how the calculated values are used in portfolio calculations
ast-grep --pattern 'value += await calculateTokenPrice($$$)'
ast-grep --pattern 'protocolBalance += await calculateTokenPrice($$$)'
ast-grep --pattern 'const value = await calculateTokenPrice($$$)'
Length of output: 1904
app/[addressOrDomain]/page.tsx (2)
325-342: LGTM! Well-structured function with proper error handling.
The function has good data validation, clear error handling with user notifications, and proper separation of concerns.
533-563: LGTM! Well-implemented data fetching with proper cancellation.
The function demonstrates good practices:
- Uses AbortController for request cancellation
- Implements parallel data fetching
- Has proper error handling and loading state management
There was a problem hiding this comment.
The code is clean and it works well, GG!
Just two requests on the failed requests:
- Dont show the error notification when these requests fails as they will always fail the first time, and then will work after a few seconds (i.e. Error while fetching address portfolio)
- Keep showing the skeletons if the request failed (I got the skeleton, then the request failed, I got the error notification, and at the same time the skeleton disappear. Finally, when the next requests succeeded, the charts appeared. So in between the failed request and the one which succeeded, there was no skeleton)
|
@Marchand-Nicolas the problem is that the page is being rendered twice and the first time it aborts the requests. I tried to debug this issue but failed so far. It was there even before my commits. I'll try to investigate it later, but for now I can just remove the error notification if it was caused by requests being aborted by re-render. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
app/[addressOrDomain]/page.tsx (4)
214-238: Improve error handling for price calculations.The current error handling only logs errors. Consider aggregating errors to determine if too many price calculations failed, which might indicate a systemic issue.
+ let errorCount = 0; const userTokenPromises = userTokens.map(async (token) => { // ... existing code ... } catch (err) { + errorCount++; console.log(`Error calculating price for token ${token.tokenAddress}:`, err); + if (errorCount > userTokens.length / 2) { + throw new Error('Too many price calculation failures'); + } return null; } });
325-342: Enhance error messages for better debugging.The error messages could be more specific to help with troubleshooting.
} catch (error) { - showNotification("Error while fetching portfolio assets", "error"); + const errorMessage = error instanceof Error + ? `Failed to fetch portfolio assets: ${error.message}` + : "Error while fetching portfolio assets"; + showNotification(errorMessage, "error"); console.log("Error while fetching portfolio assets", error); }
Line range hint
503-531: Remove unnecessary dependency from useCallback.The
addressdependency is not used within the function and should be removed from the dependency array.}, [address]); + }, []);
557-565: Improve error handling for specific API errors.Consider handling specific API error cases separately to provide more meaningful feedback to users.
} catch (error) { console.log("Error while fetching address portfolio", error); if (error instanceof Error && error.name === 'AbortError') { return; } + if (error instanceof Response) { + const message = await error.text(); + showNotification(`API Error: ${message}`, "error"); + return; + } showNotification("Error while fetching address portfolio", "error"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
app/[addressOrDomain]/page.tsx(6 hunks)
🔇 Additional comments (1)
app/[addressOrDomain]/page.tsx (1)
572-579: LGTM! Good implementation of cleanup pattern.
The abort controller implementation correctly handles cleanup and prevents memory leaks from pending requests.
Yes that's not a big deal, just make sur the skeleton keeps being displayed during the whole loading process |
|
My bad it seems to be working properly @kfastov |
Pull Request type
Please add the labels corresponding to the type of changes your PR introduces:
Resolves: #879
Other information
I picked the approach to hide protocol assets and instead show the base ones. Motivation behind this:
Example for Nico.strk (outdated, will update it soon):
Huge thanks to @joeperpetua who already did all the heavy work in #912 creating the charts and the Argent API service
Summary by CodeRabbit
New Features
Bug Fixes
Documentation