Autoclose limit/market #1510
Autoclose limit/market #1510
Conversation
Added assetId and symbol to AutocloseOpenData and updated its initializer and usage throughout the codebase. Introduced OpenPositionItemViewModel for displaying open position data in the UI. Refactored AutocloseScene and AutocloseSceneViewModel to use the new view model and improved handling of entry price display. Adjusted test kit and transfer logic to support the new data structure.
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 introduces a significant upgrade to the autoclose feature, empowering users with the choice between market and limit order execution for their autoclose strategies. The changes encompass the integration of this new preference into user settings, a refined user interface for displaying position information, and more resilient backend logic for profit/loss calculations. These updates provide a more flexible and informative autoclose experience, ensuring better control and transparency for users. Highlights
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 feature allowing users to select between 'limit' and 'market' for autoclose orders. This is achieved by adding a new user preference, which is then integrated throughout the relevant view models, services, and data structures. The settings screen has been updated with UI for this new preference, and the autoclose scene has been refactored for better support of both opening and modifying positions. The changes are well-structured and demonstrate a good separation of concerns. I have identified a couple of minor issues that should be addressed.
| public var pnlColor: Color { | ||
| guard let price else { return Colors.secondaryText } | ||
| let pnl = estimator.calculatePnL(price: price) | ||
| return PriceChangeColor.color(for: pnl) | ||
| let roe = estimator.calculateROE(price: price) | ||
| return PriceChangeColor.color(for: roe) | ||
| } |
There was a problem hiding this comment.
The logic for determining pnlColor has been changed to use calculateROE instead of calculatePnL. While the sign of ROE and PnL should be the same, making this functionally equivalent for color determination, it's worth noting that expectedPnL also calculates ROE. This change is good for consistency, but you could consider calculating roe once and using it for both expectedPnL and pnlColor to avoid redundant calculations, especially if these properties are accessed frequently.
| private let title: String | ||
| private let options: [PerpetualOrderTypeViewModel] | ||
| @Binding var selection: PerpetualOrderType | ||
|
|
||
| public init( | ||
| title: String, | ||
| options: [PerpetualOrderTypeViewModel], | ||
| selection: Binding<PerpetualOrderType> | ||
| ) { | ||
| self.title = title | ||
| self.options = options | ||
| _selection = selection | ||
| } |
There was a problem hiding this comment.
The title property and initializer parameter are unused within this view, as the navigation title is hardcoded to Localized.Perpetual.autoClose. To avoid confusion and remove dead code, you can remove the title property and the corresponding parameter from the initializer. The call site in PreferencesScene will also need to be updated accordingly.
private let options: [PerpetualOrderTypeViewModel]
@Binding var selection: PerpetualOrderType
public init(
options: [PerpetualOrderTypeViewModel],
selection: Binding<PerpetualOrderType>
) {
self.options = options
_selection = selection
}| // TODO: - Localize? | ||
| public var title: String { | ||
| switch type { | ||
| case .market: "Market" | ||
| case .limit: "Limit" | ||
| } | ||
| } |
There was a problem hiding this comment.
As pointed out by the TODO comment, the strings for order types are currently hardcoded. It's important to move these to your localization files to ensure the app can be fully translated. I've suggested using a Localized enum, assuming a structure similar to other parts of the app.
public var title: String {
switch type {
case .market: Localized.Perpetual.OrderType.market
case .limit: Localized.Perpetual.OrderType.limit
}
}Eliminated the ability to set a default autoclose order type in user preferences and removed related UI components and view models. All autoclose operations now default to the 'market' order type. Cleaned up associated code in Perpetuals, Settings, Preferences, and PrimitivesComponents modules.
OpenPositionItemViewModel now displays an empty string for zero size values instead of formatting them. In AmountInputConfig, the .perpetual scene type no longer uses the swap action style, aligning its behavior with other staking-related types.
Refines the actionStyle property to only return a CurrencyInputActionStyle for the .transfer sceneType. All other scene types, including .deposit and .withdraw, now return nil.
Simplified the logic for entryPriceTitle by making it always return the localized entry price string, removing the conditional based on type. Updated AutocloseScene to match the new non-optional entryPriceTitle usage.
Eliminated the autocloseOrderType property and related parameters from PerpetualConfirmData, TPSLOrderData, and associated builders, factories, and extension methods. This simplifies the data structures and function signatures, reflecting that autocloseOrderType is no longer required for these operations.
OpenPositionItemViewModelto display position info during order setupcloses #1493