Conversation
Replaces the allowHiddenBalance property with a new BalanceActionType enum to better handle privacy toggling and header actions. Updates WalletHeaderView and related scenes to use isPrivacyEnabled and balanceActionType for improved flexibility. Adds PerpetualPortfolioScene and integrates portfolio presentation in PerpetualsScene. Cleans up unused allowHiddenBalance properties from view models and protocol.
Introduces PerpetualPortfolioChartData and related types for representing portfolio chart data. Adds mapping extensions for GemPerpetualPortfolio types, updates GatewayService and PerpetualService protocols and implementations to support fetching and returning portfolio chart data, and updates test mocks accordingly.
Moved and refactored chart-related types and view models from MarketInsight to Primitives and PrimitivesComponents packages. Unified chart state and price view models, introduced ChartStateView, and updated usages in Perpetuals and MarketInsight features. Improved chart value calculations and formatting, and enhanced modularity and reusability of chart UI components.
Introduces test kit utilities and mock data generators for chart-related models in Primitives and PrimitivesComponents. Adds comprehensive unit tests for ChartValues, ChartPriceViewModel, ChartValuesViewModel, and PerpetualPortfolioSceneViewModel to ensure correct behavior and improve test coverage. Updates Price mock defaults for consistency.
Summary of ChangesHello @gemdev111, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a new 'Perpetual Portfolio' feature, enabling users to visualize their perpetual account performance through dynamic charts. It involved a comprehensive overhaul of the application's charting infrastructure, making components more generic and reusable. Additionally, the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new feature for perpetuals account statistics and includes a significant and well-executed refactoring of chart views and wallet header components. The introduction of ChartStateView and BalanceActionType greatly improves modularity and code clarity. The changes are well-tested.
My review includes a few suggestions to improve localization, adhere to design system constants, and enhance maintainability for future updates. Overall, this is a high-quality contribution.
| public final class PerpetualPortfolioSceneViewModel { | ||
| private let wallet: Wallet | ||
| private let perpetualService: PerpetualServiceable | ||
| private let currencyFormatter = CurrencyFormatter(type: .currency, currencyCode: Currency.usd.rawValue) |
There was a problem hiding this comment.
The currencyFormatter is hardcoded to use USD. This might not align with the user's currency preference. To ensure a consistent experience, please use the currency from Preferences.standard.currency. This will require importing the Preferences package and then updating this line.
| self.perpetualService = perpetualService | ||
| } | ||
|
|
||
| var navigationTitle: String { "Account" } |
There was a problem hiding this comment.
The navigation title is a hardcoded string. For better maintainability and to support localization, please use the Localized strings collection (e.g., Localized.Perpetuals.accountTitle).
| var navigationTitle: String { "Account" } | |
| var navigationTitle: String { Localized.Perpetuals.accountTitle } |
| func chartTypeTitle(_ type: PortfolioChartType) -> String { | ||
| switch type { | ||
| case .value: "Value" | ||
| case .pnl: "PnL" | ||
| } | ||
| } |
There was a problem hiding this comment.
The chart type titles are hardcoded strings. For better maintainability and to support localization, please use the Localized strings collection.
| func chartTypeTitle(_ type: PortfolioChartType) -> String { | |
| switch type { | |
| case .value: "Value" | |
| case .pnl: "PnL" | |
| } | |
| } | |
| func chartTypeTitle(_ type: PortfolioChartType) -> String { | |
| switch type { | |
| case .value: Localized.Perpetuals.Chart.value | |
| case .pnl: Localized.Perpetuals.Chart.pnl | |
| } | |
| } |
| period: selectedPeriod, | ||
| price: price, | ||
| values: values, | ||
| lineColor: .blue, |
There was a problem hiding this comment.
The lineColor for the chart is hardcoded to SwiftUI.Color.blue. It's better to use a color from the app's design system (e.g., Colors.blue) for consistency and easier theming in the future.
| lineColor: .blue, | |
| lineColor: Colors.blue, |
| guard let account = wallet.accounts.first(where: { | ||
| $0.chain == .arbitrum || $0.chain == .hyperCore || $0.chain == .hyperliquid | ||
| }) else { |
There was a problem hiding this comment.
The logic to find a perpetuals-compatible account checks for specific chains (.arbitrum, .hyperCore, .hyperliquid). This approach could be brittle and require changes here every time a new chain with perpetuals support is added.
Consider adding a property to the Chain enum, like supportsPerpetuals, to make this logic more scalable and maintainable.
For example:
// In Chain enum
var supportsPerpetuals: Bool {
switch self {
case .arbitrum, .hyperCore, .hyperliquid:
return true
default:
return false
}
}Then you could simplify the guard statement to:
guard let account = wallet.accounts.first(where: { $0.chain.supportsPerpetuals }) else {
// ...
}
Like Hyperdash, display the change amount (e.g., "+$8.48" or "-$0.75") rather than the current value with percentage. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Replaces PerpetualPortfolioChartData and related types with PerpetualPortfolio, PerpetualPortfolioTimeframeData, and PerpetualPortfolioDataPoint. Updates all usages, extensions, and test kits to use the new structures, and removes the old files and mocks. This unifies and simplifies portfolio data modeling across the codebase.
Introduces ChartValueType to distinguish between price and price change chart types, refactors related view models to use this type instead of a 'signed' boolean, and centralizes price change formatting in a new PriceChangeViewModel. Adds an info section to PerpetualPortfolioScene displaying unrealized PnL, all-time PnL, and volume, with supporting logic in PerpetualPortfolioSceneViewModel. Updates tests and related code to reflect these changes.
Refactored ChartView for improved readability and modularity, including UI extraction and enhanced drag/selection logic. Added PulsingDotView to display a pulsing indicator on the chart, improving visual feedback for the latest data point.
Replaced hardcoded PerpetualPortfolioSceneViewModel strings with localized values. Added new keys for perpetuals-related terms to Localized.swift and updated all Localizable.strings files with translations for these keys.
Introduces the PerpetualAccountSummary struct and adds an accountSummary property to PerpetualPortfolio. Updates mapping and view model logic to display account leverage and margin usage in the portfolio scene, and ensures unrealized PnL is sourced from accountSummary.
Replaces PerpetualPortfolioDataPoint with ChartDateValue in portfolio models, extensions, and tests for consistency and simplification. Removes obsolete mapping and mock helpers, updates related code to use ChartDateValue directly, and adds a GemChartDateValue mapping extension.
|
|
||
| import Foundation | ||
|
|
||
| public enum PortfolioChartType: String, CaseIterable, Identifiable { |
| let priceChangePercentage = (priceChange / base) * 100 | ||
|
|
||
| return ChartPriceModel( | ||
| let priceChangePercentage = base == 0 ? 0 : ((lastCandle.close - base) / base) * 100 |
There was a problem hiding this comment.
priceChangePercentage can you add this as separate var? and add unite test for it
| } | ||
|
|
||
|
|
||
| public func portfolio(wallet: Wallet) async throws -> PerpetualPortfolio { |
There was a problem hiding this comment.
portfolio(wallet: Wallet) should be taking an address only, not a wallet. let's do same for updatePositions
Replaces usage of Wallet in PerpetualService and related view models with explicit address and walletId parameters for portfolio and position updates. Introduces PerpetualPortfolioChartType in Primitives, updates related types and extensions, and moves PortfolioChartType logic. Refactors chart price change calculation into a static method and adds corresponding tests. Adds test kit and unit tests for PriceChangeViewModel.
Replaces repeated logic for retrieving the perpetual address from Wallet with a new Wallet.perpetualAddress computed property. Updates all usages in Perpetuals view models and PerpetualObserverService to use this extension, improving code clarity and maintainability.
2 participants
Perpetuals Portfolio
Chart Refactoring
images:
closes #1621