Amount scene refactor #1532
Amount scene refactor #1532
Conversation
Introduces a strategy pattern for amount handling by adding AmountStrategy and related classes for transfer, stake, and perpetual actions. Refactors AmountScene, AmountNavigationView, and AmountSceneViewModel to use the new strategy abstraction, improving modularity and maintainability.
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 architectural improvement by refactoring the 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 significant and well-executed refactoring of the AmountSceneViewModel by applying the Strategy design pattern. The logic for different transaction types (transfer, stake, perpetual) has been cleanly separated into dedicated strategy classes, which dramatically simplifies the view model and improves modularity and maintainability. The introduction of AmountStrategy, AmountStrategyFactory, and supporting state-holder classes like ValidatorSelection is a great improvement to the architecture. The changes in the views and tests are consistent with this new design. My feedback includes a couple of minor suggestions to improve code clarity and adopt more idiomatic Swift constructs in the new strategy classes.
| var isAutocloseEnabled: Bool { | ||
| if case .open = data.positionAction { true } else { false } | ||
| } |
There was a problem hiding this comment.
This if case expression to return a boolean can be made more explicit and arguably more readable by using a switch statement. This ensures all cases of PerpetualPositionAction are explicitly handled, improving clarity and maintainability.
var isAutocloseEnabled: Bool {
switch data.positionAction {
case .open:
true
case .increase, .reduce:
false
}
}| var freezeData: FreezeData? { | ||
| if case .freeze(let data) = self { | ||
| data | ||
| } else { | ||
| nil | ||
| } | ||
| } |
There was a problem hiding this comment.
This if case block can be simplified using a guard case statement, which is more idiomatic in Swift for extracting an associated value from an enum case and returning early if it doesn't match. This improves conciseness and readability.
var freezeData: FreezeData? {
guard case .freeze(let data) = self else {
return nil
}
return data
}
DRadmir
left a comment
DRadmir
left a comment
There was a problem hiding this comment.
No need to use *Strategy in the naming
| case unstake(Delegation) | ||
| case redelegate(Delegation, validators: [DelegationValidator], recommended: DelegationValidator?) | ||
| case withdraw(Delegation) | ||
| case freeze(FreezeData) |
There was a problem hiding this comment.
Should we treat freeze as an edge case for staking?
| LeveragePickerSheet( | ||
| title: perpetual.leverageTitle, | ||
| leverageOptions: leverageSelection.options, | ||
| selectedLeverage: Binding( |
There was a problem hiding this comment.
Can we incapsulate this binding to model?
struct LeverageSheetModel {
let title: String
let options: [LeverageOption]
let binding: Binding<LeverageOption>
}
then create this model in AmountPerpetualStrategy
| var minimumValue: BigInt { | ||
| switch action { | ||
| case .send: .zero | ||
| case .deposit: asset.symbol == "USDC" ? BigInt(5_000_000) : .zero |
There was a problem hiding this comment.
enum PerpetualLimits {
static let minDepositUSDC = BigInt(5_000_000)
static let minWithdrawUSDC = BigInt(2_000_000)
}
Moved freeze and unfreeze logic from AmountStakeViewModel to a new AmountFreezeViewModel. Updated AmountDataProvider and AmountScene to use the new model for freeze actions. Adjusted related tests and removed resource selection handling from AmountStakeViewModel to improve separation of concerns.
Refactored protocol and related conformances from AmountViewModeling to AmountDataProvidable for improved naming consistency. Introduced AmountPerpetualLimits struct for minimum deposit and withdraw values, and updated usages in AmountStakeViewModel and AmountTransferViewModel. Minor logic simplifications in AmountFreezeViewModel and AmountPerpetualViewModel.
Refactored AmountSheetType to pass required data for leverage selector and updated related view logic to use new bindings and onChange handlers. Improved resource selection in AmountScene to use @bindable and onChange, and updated ViewModel methods for resource and leverage changes to match new signature.
Updated multiple files to consistently use the 'case let' pattern matching syntax for improved readability and Swift best practices. Also made minor test improvements and refactored variable extraction in view models.
Updated LeverageSelection, ResourceSelection, and ValidatorSelection to accept explicit options and selected values in their initializers, improving flexibility and consistency. Adjusted related view models and scene code to use the new initializers and removed static options usage.
Replaces LeverageSelection, ResourceSelection, and ValidatorSelection with a generic SelectionState<T> class to unify selection logic across the app. Updates all related view models and views to use the new SelectionState, simplifying code and improving maintainability.
Summary
AmountSceneViewModelusing Strategy pattern (~700 → ~260 lines)AmountTransferViewModel,AmountStakeViewModel,AmountFreezeViewModel,AmountPerpetualViewModelAmountDataProvidableprotocol andAmountDataProviderenumValidatorSelection,LeverageSelection,ResourceSelectionwith genericSelectionState<T>Test plan
🤖 Generated with Claude Code