Sell crypto balance validation #1308
Conversation
- Cancel previous fetch when switching between Buy/Sell modes - Disable continue button based on input validation, balance, and loading state - Add unit tests for button state validation
…ove-error-message-for-sell-crypto
Summary of ChangesHello @DRadmir, 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 FiatConnect scene by introducing crucial validation for selling cryptocurrency, preventing users from attempting to sell more than their available balance. It also refactors the asynchronous data fetching mechanism to support task cancellation, leading to a more responsive user interface. Furthermore, the action button's state is now more intelligently managed, reflecting the current validation status and data availability. 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 correctly implements balance validation when selling crypto by adding a BalanceValueValidator. The refactoring of the fetch method to use a Task for cancellation is also a good improvement. I have left one comment on FiatSceneViewModel.swift to address an issue in the new fetch logic that could lead to unnecessary network requests and a poor user experience. The change is otherwise solid and well-tested.
Features/FiatConnect/Sources/ViewModels/FiatSceneViewModel.swift
Outdated
Show resolved
Hide resolved
…ove-error-message-for-sell-crypto
|
|
||
| let quotes = try await fiatService.getQuotes(asset: asset, request: request) | ||
|
|
||
| try Task.checkCancellation() |
There was a problem hiding this comment.
try Task.checkCancellation() can we move this to fiatService.getQuotes( ?
There was a problem hiding this comment.
The service layer (GemAPIService) focuses on network operations, while the ViewModel manages the lifecycle of its async tasks. This keeps each layer focused on its own responsibilities.
| var focusField: FiatScene.Field? | ||
| var isPresentingFiatProvider: Bool = false | ||
|
|
||
| private var fetchTask: Task<Void, Never>? |
There was a problem hiding this comment.
I think each fetchTask should be independent, buy and sell should have it's own states - separate from each other
…ove-error-message-for-sell-crypto
Replaces single fetchTask with a dictionary to track fetch tasks per FiatQuoteType. Ensures correct cancellation and management of concurrent fetch operations when quote type changes.
Fix: #1139