Conversation
Replaces generic portfolio types with PerpetualPortfolio-specific types across primitives, gem_hypercore, and gemstone. Updates trait and method names to clarify perpetual-specific functionality, adjusts data structures for type safety (e.g., volume as f64), and updates mapping, serialization, and test code accordingly. This improves clarity and correctness for perpetual trading portfolio data handling.
Introduces PerpetualAccountSummary struct and integrates it into PerpetualPortfolio in both primitives and gemstone models. Updates mapping logic and tests to populate and verify the new account_summary field, providing additional portfolio metrics such as account value, leverage, margin usage, and unrealized PnL.
|
Review changes with Changed Files
|
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 significantly enhances the application's ability to track and display perpetual trading portfolio information. It establishes a robust data model for perpetual account summaries and historical performance, integrates this model across the core 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 perpetual portfolio tracking, including account summary metrics. It adds new data structures in primitives and gemstone, and implements the logic to fetch and map this data from the Hypercore provider. The changes are comprehensive, but there are a couple of critical issues that need to be addressed. One is an infinite recursion bug in the provider implementation, and the other is a set of issues in the new gemstone model file that will prevent compilation. My review provides details and suggestions to fix these.
| } | ||
|
|
||
| async fn get_perpetual_portfolio(&self, address: String) -> Result<PerpetualPortfolio, Box<dyn Error + Sync + Send>> { | ||
| let (response, positions) = try_join!(self.get_perpetual_portfolio(&address), self.get_clearinghouse_state(&address))?; |
There was a problem hiding this comment.
This is an infinite recursive call. The get_perpetual_portfolio method is calling itself. You should call the inherent method on HyperCoreClient that makes the RPC request. You can use fully qualified syntax to disambiguate:
HyperCoreClient::get_perpetual_portfolio(self, &address)
| let (response, positions) = try_join!(self.get_perpetual_portfolio(&address), self.get_clearinghouse_state(&address))?; | |
| let (response, positions) = try_join!(HyperCoreClient::get_perpetual_portfolio(self, &address), self.get_clearinghouse_state(&address))?; |
| pub type GemPerpetualPortfolio = PerpetualPortfolio; | ||
| pub type GemPerpetualPortfolioTimeframeData = PerpetualPortfolioTimeframeData; | ||
| pub type GemPerpetualPortfolioDataPoint = PerpetualPortfolioDataPoint; | ||
| pub type GemPerpetualAccountSummary = PerpetualAccountSummary; | ||
|
|
||
| #[uniffi::remote(Record)] | ||
| pub struct GemPerpetualPortfolioDataPoint { | ||
| pub date: DateTime<Utc>, | ||
| pub value: f64, | ||
| } | ||
|
|
||
| #[uniffi::remote(Record)] | ||
| pub struct GemPerpetualPortfolioTimeframeData { | ||
| pub account_value_history: Vec<PerpetualPortfolioDataPoint>, | ||
| pub pnl_history: Vec<PerpetualPortfolioDataPoint>, | ||
| pub volume: f64, | ||
| } | ||
|
|
||
| #[uniffi::remote(Record)] | ||
| pub struct GemPerpetualAccountSummary { | ||
| pub account_value: f64, | ||
| pub account_leverage: f64, | ||
| pub margin_usage: f64, | ||
| pub unrealized_pnl: f64, | ||
| } | ||
|
|
||
| #[uniffi::remote(Record)] | ||
| pub struct GemPerpetualPortfolio { | ||
| pub day: Option<PerpetualPortfolioTimeframeData>, | ||
| pub week: Option<PerpetualPortfolioTimeframeData>, | ||
| pub month: Option<PerpetualPortfolioTimeframeData>, | ||
| pub all_time: Option<PerpetualPortfolioTimeframeData>, | ||
| pub account_summary: Option<PerpetualAccountSummary>, | ||
| } |
There was a problem hiding this comment.
This file has several issues that will prevent it from compiling and working as intended for FFI with uniffi.
- Name Collisions: You've defined type aliases and structs with the same names (e.g.,
GemPerpetualPortfolioDataPoint). This is not allowed in Rust. You should remove the type aliases. - Incorrect Field Types for FFI: The
uniffistructs use types from theprimitivescrate directly in their fields (e.g.,Vec<PerpetualPortfolioDataPoint>). For FFI, you must useuniffi-compatible types. In this case, fields should use theGem...structs you're defining (e.g.,Vec<GemPerpetualPortfolioDataPoint>). - Missing Conversions: After fixing the above, you'll need to implement
Fromtraits to convert from theprimitivesstructs to yourGem...FFI structs. This will also require a change ingemstone/src/gateway/mod.rsto call.into()on the portfolio result before returning. uniffi::remote(Record): This appears to be a typo and should likely be#[uniffi::Record].
Here's an example of how GemPerpetualPortfolioDataPoint could be correctly defined with its conversion. You'll need to apply this pattern to all structs in this file.
use primitives::portfolio::{PerpetualPortfolioDataPoint as PrimitivePerpetualPortfolioDataPoint};
#[uniffi::Record]
pub struct GemPerpetualPortfolioDataPoint {
pub date: DateTime<Utc>,
pub value: f64,
}
impl From<PrimitivePerpetualPortfolioDataPoint> for GemPerpetualPortfolioDataPoint {
fn from(value: PrimitivePerpetualPortfolioDataPoint) -> Self {
Self { date: value.date, value: value.value }
}
}Replaces PerpetualPortfolioDataPoint with ChartDateValue in portfolio-related structs and type definitions across primitives, gem_hypercore, and gemstone. Updates relevant imports and type aliases to improve consistency and code reuse for date-value pairs.
1 participant
Introduces PerpetualAccountSummary struct and integrates it into PerpetualPortfolio in both primitives and gemstone models. Updates mapping logic and tests to populate and verify the new account_summary field, providing additional portfolio metrics such as account value, leverage, margin usage, and unrealized PnL.