Add dashboard portfolio charts #912
Conversation
|
@joeperpetua 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 this pull request introduce a new portfolio management feature in the application. This includes the addition of a 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Outside diff range and nitpick comments (14)
components/skeletons/portfolioSummarySkeleton.tsx (1)
6-21: Remove unnecessary Fragment wrapper.Since the component only has a single child element, the Fragment wrapper is not needed.
- <> <div className="w-[90%] md:w-[47%]"> <Skeleton variant="rounded" height={'27vh'} sx={{ bgcolor: "grey.900", borderRadius: "8px", margin: "20px auto", padding: "40px", }} /> </div> - </>utils/feltService.ts (2)
39-39: Add a newline before the function declaration.For consistency with the rest of the file, maintain a single blank line between functions.
44-44: Remove unnecessary blank lines within the function body.For consistency with other functions in this file, remove the extra blank lines.
Also applies to: 45-45
services/argentPortfolioService.ts (1)
1-66: Consider implementing rate limiting.Given that this service makes multiple API calls, it would be beneficial to implement rate limiting to prevent hitting API limits.
Consider using a rate limiting library like
bottleneck:import Bottleneck from 'bottleneck'; const limiter = new Bottleneck({ minTime: 100, // Minimum time between requests maxConcurrent: 3 // Maximum concurrent requests }); // Then wrap your API calls: const fetchDataWithRateLimit = limiter.wrap(fetchData);Would you like me to help implement this rate limiting solution?
styles/dashboard.module.css (3)
Line range hint
411-415: Fix duplicate margin-top declaration in media query.There are two
margin-topdeclarations for.questContainer:margin-top: 1rem; margin-top: 3rem;The first declaration is redundant and will be overridden.
Remove the redundant declaration:
.questContainer { flex-direction: row; flex-wrap: wrap; gap: 2rem; max-width: var(--dashboard-max-width); margin-bottom: 3rem; - margin-top: 1rem; margin-top: 3rem; justify-content: center; }
342-342: Remove unnecessary transparent border.The
border: solid 1px transparentdeclaration doesn't serve any visible purpose unless it's for a hover effect (which isn't defined).If this isn't needed for a hover state or other interaction, consider removing it:
- border: solid 1px transparent;
335-335: Consider using min-height instead of fixed height.A fixed height of 200px might cause content overflow if the portfolio summary content grows beyond this size.
Consider using
min-heightinstead to accommodate varying content sizes:- height: 200px; + min-height: 200px;types/backTypes.d.ts (3)
532-560: LGTM! Consider adding JSDoc comments for better documentation.The DApp-related types are well-structured and provide a solid foundation for the portfolio feature. Consider adding JSDoc comments to document the purpose of each type and any important field constraints.
Example documentation:
/** Represents a link to external resources for a DApp */ export type ArgentDappLink = { /** Display name of the link */ name: string; /** URL of the resource */ url: string; /** Display order position */ position: number; };
581-609: LGTM! Consider implementing caching for position data.The position and product types are well-structured to support portfolio visualization. Given the complex nested structure and multiple balance calculations required, consider implementing caching to optimize performance.
Consider these caching strategies:
- Cache position totals at the product level
- Implement TTL-based caching for APY and health ratio data
- Use memoization for balance calculations in the UI components
611-628: LGTM! Consider adding branded types for addresses and amounts.The user-related types are concise and well-focused. To improve type safety, consider using branded types for addresses and amounts.
/** Branded type for Ethereum addresses */ export type Address = string & { readonly __brand: unique symbol }; /** Branded type for token amounts */ export type TokenAmount = string & { readonly __brand: unique symbol }; export type ArgentUserToken = { tokenAddress: Address; tokenBalance: TokenAmount; }app/[addressOrDomain]/page.tsx (4)
192-204: ImplementfetchPortfolioAssetsto fetch real dataThe
fetchPortfolioAssetsfunction currently contains placeholder data and a TODO comment. Implement the actual logic to fetch portfolio assets from the Argent API to display real user data.Would you like assistance in implementing the API call for fetching portfolio assets or opening a GitHub issue to track this task?
291-300: EnsuresortedProtocolsremains unmodified when handling extra protocolsUsing
splice()modifies the originalsortedProtocolsarray. If the intention is to keep the original array intact, consider making a copy before splicing.Apply this diff to make a copy of
sortedProtocolsbefore modifying:let protocolsCopy = [...sortedProtocols]; let otherProtocols = protocolsCopy.length > 5 ? protocolsCopy.splice(4) : []; if (otherProtocols.length === 0) { return; } sortedProtocols.length = 4; // Keep only the first 4 protocols sortedProtocols.push({ itemLabel: "Others", itemValue: otherProtocols.reduce((valueSum, protocol) => valueSum + Number(protocol.itemValue), 0).toFixed(2), itemValueSymbol: "$", color: "" });
302-313: Handle indexing when assigning protocol colorsEnsure that the
portfolioProtocolColorsarray has enough colors to cover all protocols insortedProtocols. If there are more protocols than colors, this could lead to undefined colors.Consider dynamically assigning colors or adding a check to handle cases where there are more protocols than predefined colors.
223-238: Ensure proper handling of debts inhandleDebtIn the
handleDebtfunction, when calculating the newitemValue, ensure that the value remains accurate after adjusting for debts, and handle any potential rounding errors due to floating-point arithmetic.Consider using a library like
big.jsfor precise decimal arithmetic if high accuracy is required.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- app/[addressOrDomain]/page.tsx (5 hunks)
- components/dashboard/PortfolioSummary.tsx (1 hunks)
- components/skeletons/portfolioSummarySkeleton.tsx (1 hunks)
- services/argentPortfolioService.ts (1 hunks)
- styles/dashboard.module.css (8 hunks)
- styles/globals.css (1 hunks)
- types/backTypes.d.ts (1 hunks)
- types/frontTypes.d.ts (1 hunks)
- utils/feltService.ts (1 hunks)
🔇 Additional comments (7)
components/skeletons/portfolioSummarySkeleton.tsx (3)
1-4: LGTM! Clean imports and proper TypeScript typing.The component is properly typed with FunctionComponent and imports are well-organized.
24-24: LGTM! Proper export statement.The default export is appropriate for a single component file.
1-24: Verify skeleton dimensions match the design.Since this skeleton is part of the dashboard portfolio charts feature, please ensure that:
- The skeleton's dimensions match the actual PortfolioSummary component
- The loading state appearance aligns with the Figma design referenced in issue Add "Portfolio by protocol used" section with a donut chart and a "Claim your reward" button #880
utils/feltService.ts (1)
40-47: Verify the decimal places requirement.The function rounds to 5 decimal places, which matches
gweiToEth, but please verify if this precision is sufficient for all token types in the portfolio dashboard.✅ Verification successful
Let me gather more information about the token decimals specified in the constants and common files.
5 decimal places is sufficient for token precision requirements
Based on the evidence found in the codebase:
- USDC uses 6 decimals
- ETH, LORDS, STRK, and SITH tokens use 18 decimals
- Rounding to 5 decimal places in
tokenToDecimalis adequate since:
- For USDC (6 decimals), it preserves all significant digits
- For 18 decimal tokens, 5 decimal places after conversion provides more than enough precision for display and calculations in the portfolio dashboard
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for token decimal specifications in the codebase rg -g '!*.{lock,json}' -i 'decimal.*token|token.*decimal'Length of output: 2600
Script:
#!/bin/bash # Check token decimal specifications in constants cat constants/common.ts | grep -A 10 "TOKEN_DECIMAL_MAP" # Search for any other decimal specifications in token-related code rg "decimals.*=.*\d+" -g "!*.{lock,json,test.*}"Length of output: 267
types/frontTypes.d.ts (1)
351-356: Verify ChartItem usage in the codebase.Let's ensure the suggested type changes won't break existing implementations.
✅ Verification successful
Based on the search results, I can now generate the final response:
ChartItem type definition aligns with its usage across the codebase
The type is correctly used in:
components/dashboard/PortfolioSummary.tsxfor rendering chart items and handling data normalizationapp/[addressOrDomain]/page.tsxfor managing portfolio assets and protocol dataAll implementations properly handle:
itemValueas string (with numeric conversions when needed)itemValueSymbolfor currency/percentage displayitemLabelfor display namescolorfor chart styling🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for ChartItem type usage and value assignments rg -A 5 "ChartItem" # Look for specific value assignments to understand current usage rg "itemValue.*=.*"Length of output: 6972
styles/dashboard.module.css (1)
315-344: LGTM! Well-structured CSS for the portfolio summary section.The new portfolio summary classes are well-organized and follow a logical structure. The responsive design considerations are appropriate, making good use of flexbox for layout adjustments on mobile devices.
Also applies to: 385-400
types/backTypes.d.ts (1)
562-579: LGTM! Verify token decimal handling in the implementation.The token-related types are comprehensive and well-designed. The inclusion of decimals and both ETH/currency values will support accurate portfolio calculations.
Let's verify that token decimals are properly handled in the implementation:
Also applies to: 616-623
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Token decimal handling is properly implemented across the codebase
The implementation shows robust decimal handling through:
- Dedicated utility functions in
feltService.tsfor token decimal conversions using the Big.js library- Proper usage of token decimals in portfolio calculations in
argentPortfolioService.ts- Consistent decimal precision tracking in token-related operations
Key implementations:
tokenToDecimal()function properly scales token values using the correct decimal places- Portfolio value calculations in
[addressOrDomain]/page.tsxcorrectly apply token decimals- Token decimal constants are properly defined in
TOKEN_DECIMAL_MAP🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for proper decimal handling in token calculations # Look for decimal precision handling in mathematical operations # Search for token decimal usage rg -l "decimals" --type ts # Search for potential decimal-related calculations rg "(?i)(multiply|divide|format|calculate|math|precision|decimal)" --type tsLength of output: 17976
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (4)
utils/feltService.ts (1)
40-54: Add JSDoc documentation for the new function.While input validation has been added as suggested in the past review, the function still lacks documentation.
Apply this diff:
+/** + * Converts a token value from its raw representation to decimal format + * @param tokenValue - The raw token value as a string + * @param decimals - Number of decimal places for the token + * @returns The decimal representation of the token value + * @throws {Error} When decimals is negative + */ export function tokenToDecimal(tokenValue: string, decimals: number) {🧰 Tools
🪛 Biome
[error] 41-41: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
components/dashboard/PortfolioSummary.tsx (1)
1-126: Consider enhancing component reusability.Since this component will be reused for different types of charts (portfolio by protocol and assets), consider these architectural improvements:
- Extract chart options into a separate configuration file
- Create a higher-order component to handle data fetching and normalization
- Add prop types for different visualization modes (protocol vs assets)
app/[addressOrDomain]/page.tsx (1)
192-204: TODO: Replace mock data with actual API implementation.The current implementation uses hardcoded mock data. This needs to be replaced with actual data from the Argent API service.
Would you like me to help implement the actual API integration using the
fetchUserTokensendpoint mentioned in the PR objectives?services/argentPortfolioService.ts (1)
29-29: Useconsole.errorinstead ofconsole.logfor error messagesCurrently, errors are being logged using
console.log. For better error tracking and to follow best practices, consider usingconsole.errorwhen logging errors. This will ensure that error messages are appropriately categorized and easier to debug.Apply this diff to update the error logging:
- console.log("Error fetching data from Argent API", err); + console.error("Error fetching data from Argent API", err);- console.log("Error while calculating token price", err); + console.error("Error while calculating token price", err);Also applies to: 63-63
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- app/[addressOrDomain]/page.tsx (5 hunks)
- components/dashboard/PortfolioSummary.tsx (1 hunks)
- services/argentPortfolioService.ts (1 hunks)
- styles/dashboard.module.css (8 hunks)
- styles/globals.css (1 hunks)
- utils/feltService.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- styles/dashboard.module.css
- styles/globals.css
🧰 Additional context used
🪛 Biome
utils/feltService.ts
[error] 41-41: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (11)
components/dashboard/PortfolioSummary.tsx (6)
14-19: Define the ChartItem interface.The
ChartItemtype is used inPortfolioSummaryPropsandChartEntrybut is not defined.
21-39: LGTM! Well-structured presentational component.The ChartEntry component is clean, focused, and handles value formatting appropriately.
41-41: Remove unused isLoading prop.The
isLoadingprop is defined but not used in the component.
43-49: Ensure type consistency in normalizeMinValue function.The function currently returns a mix of strings and numbers due to
toFixed(2).
76-79: Add null check for onHover handler.The current implementation may cause runtime errors when no elements are hovered.
89-91: Implement onClick handler for claim reward button.The empty onClick handler provides no feedback to users.
app/[addressOrDomain]/page.tsx (1)
45-56: LGTM: Well-structured types and state management.The types are properly defined and the state variables follow React conventions.
Also applies to: 83-85
services/argentPortfolioService.ts (4)
11-17: Previous suggestion on improving API configuration management is still applicableThe earlier recommendation to move API constants like
API_BASE,API_VERSION, andAPI_HEADERSto a configuration file or use environment variables remains relevant. This change would enhance flexibility and maintainability across different environments.
19-32: Previous suggestion on enhancing error handling and implementing a timeoutThe prior feedback about adding a request timeout, implementing a retry mechanism for transient failures, and structuring error logging in the
fetchDatafunction is still applicable. Incorporating these improvements will make your API interactions more robust and reliable.
44-47: Previous suggestion on input validation for wallet address is still validThe earlier recommendation to add input validation for the
walletAddressparameter remains important. Validating the wallet address format before making the API call can prevent unnecessary requests and potential errors.
54-66: Previous suggestion on improving error handling incalculateTokenPriceis still applicableThe prior feedback regarding the need to:
- Validate the
tokenAddressformat.- Handle potential overflow/underflow in
Big.jsoperations.- Validate the
currencyparameter.Implementing these checks will enhance the robustness and reliability of the
calculateTokenPricefunction.
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Outside diff range and nitpick comments (2)
components/dashboard/PortfolioSummary.tsx (1)
22-40: Enhance accessibility for chart legend entries.The colored circles are purely decorative and should be marked as such for screen readers. Also, consider adding ARIA labels for better screen reader support.
- <svg width={16} height={16}> + <svg width={16} height={16} aria-hidden="true" role="presentation"> <circle cx="50%" cy="50%" r="8" fill={color} /> </svg> - <Typography type={TEXT_TYPE.BODY_MIDDLE}>{itemLabel}</Typography> + <Typography type={TEXT_TYPE.BODY_MIDDLE} aria-label={`${itemLabel}: ${value}`}>{itemLabel}</Typography>app/[addressOrDomain]/page.tsx (1)
192-204: TODO: Implementation needed for portfolio assets.The current implementation uses mock data. As per the PR objectives, this should be implemented using the Argent API service.
Would you like me to help implement the portfolio assets fetching logic using the Argent API? I can generate a solution that:
- Fetches user tokens using
fetchUserTokens- Calculates token prices and percentages
- Formats the data for the chart
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- app/[addressOrDomain]/page.tsx (5 hunks)
- components/dashboard/PortfolioSummary.tsx (1 hunks)
- services/argentPortfolioService.ts (1 hunks)
- tests/utils/feltService.test.js (2 hunks)
- utils/feltService.ts (1 hunks)
🧰 Additional context used
🪛 Biome
utils/feltService.ts
[error] 41-41: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
[error] 41-41: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (4)
utils/feltService.ts (1)
40-51: Add unit tests for the token conversion function.This function is critical for portfolio calculations but lacks test coverage. Tests should verify:
- Correct decimal conversion for various token decimals
- Handling of zero values
- Error cases for invalid inputs
- Precision/rounding behavior
Let's verify if any tests exist:
Would you like me to help generate a comprehensive test suite for this function?
🧰 Tools
🪛 Biome
[error] 41-41: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
[error] 41-41: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
tests/utils/feltService.test.js (2)
Line range hint
1-7: LGTM: Import statement is properly updated.The new import for
tokenToDecimalfollows the existing pattern and is correctly formatted.
80-97: Verify the tokenToDecimal implementation exists.Let's ensure the tested function is properly implemented in the utils.
✅ Verification successful
Implementation of tokenToDecimal is correctly implemented and matches test cases
The implementation in
utils/feltService.tsproperly handles all test cases:
- Converts token values with correct decimal places using BigNumber arithmetic
- Returns "0" for invalid inputs (empty string, NaN decimals, negative decimals)
- Rounds to 5 decimal places as shown in test examples
- Uses safe big number operations to handle large token values
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the tokenToDecimal function implementation exists # and matches the test cases' expectations # Search for the tokenToDecimal function implementation ast-grep --pattern 'export function tokenToDecimal($_) { $$$ }' # Alternatively, search for the function definition rg -A 5 'export.*function.*tokenToDecimal'Length of output: 471
Script:
#!/bin/bash # Let's get the complete implementation to verify all test cases rg -A 15 'export.*function.*tokenToDecimal' utils/feltService.tsLength of output: 451
app/[addressOrDomain]/page.tsx (1)
15-15: LGTM! Well-structured imports and type definitions.The new imports are properly organized, and the type definitions for
ChartItemMapandDebtStatusare well-structured with clear interfaces.Also applies to: 21-21, 34-37, 45-56
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Thanks @Marchand-Nicolas for the feedback :) I think the behavior is due to the "Ekubo Protocol" being an asset not managed directly through the Ekubo platform. The API will list only assets in managed in a DeFi app to be part of a protocol, I think this is an understandable behavior, and the "Ekubo Protocol" asset should be managed by the "Portfolio by assets type" chart instead. What do you think? Example of Argent Portfolio differentiating between Ekubo protocol balance and Ekubo token balance: |
Marchand-Nicolas
added
🚧 In progress do not merge
Pull Request type
Resolves: #880
Other information
The component and API should be very easy to reuse for the assets type chart ( #879 ). All the needed endpoints and types should be already created if I didn't miss anything when checking the API behavior. All error cases should be checked, both for fetching the data and processing it.
The logic at the end came up to be a little complex, as the protocols in the Argent Portfolio are being generated from two main endpoints,
tokens/dappsandtokens/info.Also the information is not centralized in one endpoint alone, so it is necessary to hit different ones to get data like protocols names, token parsing data and token current price. This should not be an issue for the assets type chart, as it technically would only need the data from the user balance endpoint implemented in
fetchUserTokens(addr).Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Documentation