Conversation
Add PriceChangeCalculator and TotalFiatValue types and centralize price-change logic. Replace ad-hoc percentage/amount calculations across charts, PnL, price impact and perpetual estimations with PriceChangeCalculator. Introduce TotalValueViewModel to present TotalFiatValue and refactor WalletHeaderViewModel, WalletHeaderView, and related header view models to support subtitle badge and color. Update store TotalValueRequest to compute aggregated total value and PnL, remove WalletFiatValue, and add corresponding tests and test helpers. Misc: adjust usages in Perpetuals, Chart views, PnLViewModel, and update unit tests to match the new types.
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 wallet's user interface by introducing a daily profit/loss (P&L) display directly in the header. It achieves this by centralizing all price change calculations into a new, dedicated utility and refactoring the underlying data models and requests to support this new P&L data. The changes improve consistency in financial calculations and provide users with immediate insights into their wallet's performance. 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 24-hour profit and loss (P&L) display in the wallet header, which is a great enhancement for users. The implementation includes a new centralized PriceChangeCalculator and a TotalFiatValue model, which are positive steps towards better code structure and reusability. The refactoring across the codebase to use these new components is well-executed.
However, I've found a critical issue in the P&L calculation logic within PriceChangeCalculator for the specific edge case where an asset's value drops by 100%. This leads to incorrect P&L reporting. The corresponding test case also seems to be flawed, reinforcing this incorrect behavior. Addressing this is crucial for the financial correctness of the feature.
The rest of the changes look good and align with the goals of the pull request.
| return from == 0 ? 0 : (to - from) / from * 100 | ||
| case .amount(let percentage, let value): | ||
| let denominator = 100 + percentage | ||
| return denominator == 0 ? 0 : value * percentage / denominator |
There was a problem hiding this comment.
The calculation for the .amount case is incorrect when percentage is exactly -100. In this scenario, the denominator becomes zero, and the function currently returns 0, which is incorrect.
- A -100% change implies the final value is 0. The profit and loss (P&L) amount should be the negative of the initial value (e.g., if value goes from $100 to $0, P&L is -$100, not $0).
- The formula
P&L = V_final * percentage / (100 + percentage)has a singularity atpercentage = -100, whereV_finalmust be 0, and the initial value cannot be determined.
The current implementation silently returns an incorrect P&L of 0, leading to flawed portfolio P&L calculations. This might require a data model change to provide the initial value or price change amount directly. The calculator should not return a misleading 0 for this case.
| #expect(PriceChangeCalculator.calculate(.amount(percentage: 10, value: 110)) == 10) | ||
| #expect(PriceChangeCalculator.calculate(.amount(percentage: -10, value: 90)) == -10) | ||
| #expect(PriceChangeCalculator.calculate(.amount(percentage: 0, value: 100)) == 0) | ||
| #expect(PriceChangeCalculator.calculate(.amount(percentage: -100, value: 100)) == 0) |
There was a problem hiding this comment.
This test case for a -100% change is flawed and validates incorrect behavior:
- Logically impossible scenario: A -100% change in value means the final value must be 0. The test provides a final
valueof 100, which is a contradiction. - Incorrect expectation: The expected P&L of 0 is wrong. For a -100% change, the P&L is the negative of the initial value.
This test should be corrected to reflect a valid scenario and a correct expectation. The current test is misleading and hides a critical bug in the P&L calculation.
Update test fixtures to use an Ethereum asset instead of Bitcoin and adjust prices accordingly (50000 -> 3000) in TotalValueRequestTests. Remove a redundant/invalid expectation for a -100% price change in PriceChangeCalculatorTests. These changes keep tests consistent with expected asset/pricing assumptions.
The tests were failing because priceStore.updatePrice() requires a fiat rate to be present in the database for the given currency. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
| subtitle, | ||
| isEnabled: $isPrivacyEnabled | ||
| ) | ||
| .font(.system(size: 16, weight: .medium)) |
There was a problem hiding this comment.
Maybe something from our fonts fits?
| var assetImage: AssetImage? { get } | ||
| var title: String { get } | ||
| var subtitle: String? { get } | ||
| var subtitleBadge: String? { get } |
| import PrimitivesTestKit | ||
| import Primitives | ||
|
|
||
| struct TotalValueRequestTests { |
There was a problem hiding this comment.
Can we assert the exact expected values here?
Use price=1100 to get clean divisible numbers: - value = 3300 (3 * 1100) - pnlAmount = 300 (3300 * 10 / 110) - pnlPercentage = 10 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Replace separate subtitle badge with an inline percentage and optional arrow image. HeaderViewModel protocol now exposes subtitleImage instead of subtitleBadge. WalletHeaderView concatenates amount and percentage into a single subtitle and shows a small up/down arrow Image (styled with subtitle color) instead of a capsule badge; spacing adjusted. WalletHeaderViewModel builds the combined subtitle and returns an arrow image based on pnl sign. Tests updated to expect the new subtitle format, and system image names for the arrows were added.
3 participants
percentage badge
PriceChangeCalculatorfor percentage and amountcalculations
TotalFiatValuemodel to track wallet value with P&L dataTotalValueRequestto calculate and return P&L from 24h price changesPriceChangeCalculatorPriceChangeCalculatordirectlyFoundation/BigInt where unused)
TotalValueRequestwith functional reduce patternimages:
closes #1652