Repository navigation
Perps accounts stats - #889
Conversation
|
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 capability to handle and display perpetual portfolio data. It establishes a robust foundation by defining new data models, extending existing traits to support portfolio retrieval, and implementing the necessary logic within the Hypercore client to interact with its portfolio API. The changes also ensure that this new functionality is properly exposed through the gateway and made available for foreign function interface consumption, providing a comprehensive solution for portfolio tracking. 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
The pull request introduces portfolio functionality across chain_traits, primitives, gem_hypercore, and gemstone crates. It defines new data structures for portfolio data, implements a get_portfolio method in the ChainPerpetual trait, and provides an implementation for the Hypercore client. Overall, the changes are well-structured, but there are a few areas that could be improved for robustness and correctness, particularly regarding data type handling and a critical recursive call.
| } | ||
|
|
||
| async fn get_portfolio(&self, address: String) -> Result<Portfolio, Box<dyn Error + Sync + Send>> { | ||
| self.get_portfolio(&address).await |
There was a problem hiding this comment.
This line appears to be a recursive call to self.get_portfolio(&address).await. Since self here refers to HyperCoreClient<C>, and this method is implementing the ChainPerpetual trait for HyperCoreClient<C>, calling self.get_portfolio again will call this very method, leading to infinite recursion and a stack overflow. You should be calling the get_portfolio method on the underlying RPC client, likely self.client.get_portfolio(&address).await or similar, depending on how the RPC client is exposed.
| self.get_portfolio(&address).await | |
| self.client.get_portfolio(&address).await |
| pub account_value_history: Vec<(i64, String)>, | ||
| pub pnl_history: Vec<(i64, String)>, | ||
| pub vlm: String, |
There was a problem hiding this comment.
Storing numerical values like account_value_history, pnl_history, and vlm as String can lead to runtime parsing errors and performance overhead if these values are frequently converted to numeric types for calculations. It's generally more robust to store them as appropriate numeric types (e.g., f64, Decimal if precision is critical) directly.
| } | ||
|
|
||
| fn timestamp_to_datetime(timestamp_ms: i64) -> DateTime<Utc> { | ||
| Utc.timestamp_millis_opt(timestamp_ms).single().unwrap_or_else(Utc::now) |
There was a problem hiding this comment.
The unwrap_or_else(Utc::now) call for timestamp_millis_opt can mask issues with invalid timestamps by silently replacing them with the current time. It would be more robust to propagate this potential error (e.g., by returning Option<DateTime<Utc>> or a Result) or explicitly handle it, rather than silently substituting potentially incorrect data.
| "perpWeek" => portfolio.perp_week = Some(data.into()), | ||
| "perpMonth" => portfolio.perp_month = Some(data.into()), | ||
| "perpAllTime" => portfolio.perp_all_time = Some(data.into()), | ||
| _ => {} |
There was a problem hiding this comment.
The _ => {} arm in the match statement silently ignores any unknown timeframe values. While this might be the intended behavior for future-proofing, it could also hide issues if an unexpected timeframe is received due to an API change or data corruption. Consider logging a warning for unknown timeframes to aid debugging or returning an error if strict parsing is required.
| #[serde(rename_all = "camelCase")] | ||
| pub struct PortfolioDataPoint { | ||
| pub date: DateTime<Utc>, | ||
| pub value: String, |
There was a problem hiding this comment.
Similar to the gem_hypercore crate, storing value as a String in PortfolioDataPoint can be problematic. If value represents a numerical quantity that will be used in calculations, it should ideally be stored as a numeric type (e.g., f64 or a fixed-point decimal type) to avoid repeated parsing and potential precision issues.
| pub struct PortfolioTimeframeData { | ||
| pub account_value_history: Vec<PortfolioDataPoint>, | ||
| pub pnl_history: Vec<PortfolioDataPoint>, | ||
| pub volume: String, |
There was a problem hiding this comment.
The volume field in PortfolioTimeframeData is also stored as a String. If this represents a numerical volume, consider changing its type to a numeric one for consistency and to prevent parsing errors or inefficiencies during calculations.
| #[uniffi::remote(Record)] | ||
| pub struct GemPortfolioDataPoint { | ||
| pub date: DateTime<Utc>, | ||
| pub value: String, |
There was a problem hiding this comment.
The value field in GemPortfolioDataPoint is defined as a String. If this field is intended to hold a numerical value, it would be more appropriate and efficient to use a numeric type (e.g., f64 or GemBigInt/GemBigUint if arbitrary precision is needed) to avoid repeated string parsing and potential runtime errors.
| pub struct GemPortfolioTimeframeData { | ||
| pub account_value_history: Vec<PortfolioDataPoint>, | ||
| pub pnl_history: Vec<PortfolioDataPoint>, | ||
| pub volume: String, |
There was a problem hiding this comment.
The volume field in GemPortfolioTimeframeData is also a String. For consistency and better data handling, if volume represents a numerical quantity, it should be changed to a numeric type.
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.
1 participant
No description provided.