Skip to content

Add portfolio chart and statistics types#1079

Merged
gemdev111 merged 4 commits intomainfrom
portfolio-updates
Apr 16, 2026
Merged

Add portfolio chart and statistics types#1079
gemdev111 merged 4 commits intomainfrom
portfolio-updates

Conversation

@gemdev111
Copy link
Copy Markdown
Contributor

Expand portfolio primitives with richer types for charts and stats. Introduces PortfolioType, PortfolioChartType, PortfolioChartData, PortfolioMarginUsage, PortfolioStatistic, and PortfolioData, and imports ChartPeriod for available chart periods. Updates crate exports to expose the new types and adjusts asset_price import accordingly to support portfolio charting and margin/statistics data.

Expand portfolio primitives with richer types for charts and stats. Introduces PortfolioType, PortfolioChartType, PortfolioChartData, PortfolioMarginUsage, PortfolioStatistic, and PortfolioData, and imports ChartPeriod for available chart periods. Updates crate exports to expose the new types and adjusts asset_price import accordingly to support portfolio charting and margin/statistics data.
@gemdev111 gemdev111 self-assigned this Apr 15, 2026
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors portfolio primitives by renaming PerpetualPortfolioChartType to PortfolioType and introducing several new structures, including PortfolioChartData, PortfolioStatistic, and PortfolioData. The review feedback suggests adding missing trait derivations such as Eq, Hash, and PartialEq to these new types to ensure consistency across the crate and improve usability in Swift, particularly for UI state management.

Comment thread crates/primitives/src/portfolio.rs Outdated
Comment on lines 9 to 10
#[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq)]
#[typeshare(swift = "Equatable, Sendable, CaseIterable, Identifiable")]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

For consistency with other primitives in this crate and to improve usability in Swift (e.g., in ForEach or as dictionary keys), PortfolioType should derive Eq and Hash in Rust, and Hashable in the typeshare attribute.

Suggested change
#[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq)]
#[typeshare(swift = "Equatable, Sendable, CaseIterable, Identifiable")]
#[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq, Hash)]
#[typeshare(swift = "Equatable, Sendable, CaseIterable, Identifiable, Hashable")]

Comment thread crates/primitives/src/portfolio.rs Outdated
Comment on lines +17 to +18
#[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq)]
#[typeshare(swift = "Equatable, Sendable, CaseIterable, Identifiable")]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Similar to PortfolioType, PortfolioChartType should derive Eq and Hash in Rust and Hashable in Swift to maintain consistency across the primitive types.

Suggested change
#[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq)]
#[typeshare(swift = "Equatable, Sendable, CaseIterable, Identifiable")]
#[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq, Hash)]
#[typeshare(swift = "Equatable, Sendable, CaseIterable, Identifiable, Hashable")]

Comment thread crates/primitives/src/portfolio.rs Outdated
Comment on lines +25 to +26
#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)]
#[typeshare(swift = "Equatable, Sendable")]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Adding Hashable to the typeshare attribute for PortfolioChartData ensures it can be used in hashed collections in Swift, matching the pattern used for other portfolio-related structs like PortfolioMarginUsage and PerpetualPortfolio.

Suggested change
#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)]
#[typeshare(swift = "Equatable, Sendable")]
#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)]
#[typeshare(swift = "Equatable, Sendable, Hashable")]

Comment thread crates/primitives/src/portfolio.rs Outdated
Comment on lines +125 to +126
#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)]
#[typeshare(swift = "Equatable, Sendable")]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

PortfolioStatistic should also include Hashable in its typeshare definition. Since its members (like PortfolioMarginUsage and ChartValuePercentage) are already marked as Hashable, this maintains a consistent API for the frontend.

Suggested change
#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)]
#[typeshare(swift = "Equatable, Sendable")]
#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)]
#[typeshare(swift = "Equatable, Sendable, Hashable")]

Comment thread crates/primitives/src/portfolio.rs Outdated
Comment on lines +138 to +139
#[derive(Debug, Clone, Serialize, Deserialize)]
#[typeshare(swift = "Sendable")]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

PortfolioData is currently missing PartialEq and Equatable derivations, which are present on almost all other structs in this file. This inconsistency might make it difficult to use in UI state management (e.g., deciding when to re-render). Note that ChartPeriod in asset_price.rs would also need to derive PartialEq for this to compile.

Addresses review feedback on PR #1079. Adds Eq/Hash Rust derives and
Swift Hashable typeshare attributes to PortfolioType, PortfolioChartType,
PortfolioChartData, and PortfolioStatistic. Adds PartialEq + Equatable
to PortfolioData, with matching PartialEq/Eq/Hash + Hashable on
ChartPeriod so the derive compiles.
@gemdev111 gemdev111 merged commit d59a6a5 into main Apr 16, 2026
4 checks passed
@gemdev111 gemdev111 deleted the portfolio-updates branch April 16, 2026 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant