-
Notifications
You must be signed in to change notification settings - Fork 57
Add wallet setup scene for name and avatar selection #1531
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add wallet setup scene for name and avatar selection #1531
Conversation
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 overhauls the wallet onboarding experience by introducing a dedicated setup scene for new and imported wallets. This change centralizes the process of naming a wallet and selecting its avatar, which was previously scattered across different flows. The underlying navigation logic for both creating and importing wallets has been consolidated into new, purpose-built view models, improving modularity and maintainability. These changes aim to provide a more consistent and user-friendly initial setup for all wallets. 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new wallet setup scene for setting the name and avatar, which is a great addition to the onboarding flow. The refactoring to introduce CreateWalletModel and ImportWalletViewModel to handle navigation logic is a significant improvement for code structure and testability. I've provided a few suggestions to improve performance by debouncing database writes and to reduce code duplication in the new models. Overall, this is a well-structured PR with significant positive changes.
31ede2c to
f8563d6
Compare
Introduce SetupWalletScene shown after wallet creation and import flows, allowing users to set wallet name and select avatar (emoji/NFT image). - Add SetupWalletScene and SetupWalletViewModel for unified wallet setup - Move WalletAvatar components from ManageWallets to Onboarding package - Create CreateWalletModel and ImportWalletViewModel for navigation logic - Rename CreteWalletViewModel to NewSecretPhraseViewModel - Rename ImportWalletViewModel to ImportWalletSceneViewModel - Remove name field from ImportWalletScene (now in SetupWalletScene) - Pass avatarService through RootSceneViewModel instead of Environment - Update WalletService.importWallet to accept Wallet parameter
f8563d6 to
2bcc138
Compare
…ability-to-name-and-select-image-for-new-wallet
Features/Onboarding/Sources/ViewModels/ImportWalletViewModel.swift
Outdated
Show resolved
Hide resolved
Replaces usage of Localized.Wallet.walletName with Localized.Wallet.name in SetupWalletScene and removes the unused walletName key from all localization files and the Localized.swift enum. Also updates SetupWalletViewModel to use the correct import wallet title localization.
- Present WalletImageScene as modal sheet instead of push navigation - Move navigationPath from ViewModels to NavigationStack scenes - Split async wallet creation methods for better separation - Move hasExistingWallets check to scene layer
| } | ||
|
|
||
| func createWallet(words: [String]) async throws -> Wallet { | ||
| let wallet = try await walletService.loadOrCreateWallet( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we added a new step, should we drop user to the wallet instead of showing wallet configuration screen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We decided to keep the wallet configuration screen, since we can load assets in the background while the user is setting the wallet name and icon
| return RecipientImport(name: result.name, address: result.address) | ||
| } | ||
| return RecipientImport(name: name, address: input) | ||
| let walletName = name.isEmpty ? WalletNameGenerator(type: type, walletService: walletService).name : name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add unit test for this name.isEmpty ? WalletNameGenerator(type: type, walletService: walletService).name : name
| .package(name: "FeatureServices", path: "../../Packages/FeatureServices"), | ||
| .package(name: "ChainServices", path: "../../Packages/ChainServices") | ||
| .package(name: "ChainServices", path: "../../Packages/ChainServices"), | ||
| .package(name: "Store", path: "../../Packages/Store") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we avoid importing Store ? and use services instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Store is required for WalletRequest
Refactored CreateWalletUITests and ImportWalletReceiveBitcoinUITests to use reusable flow functions for wallet creation and import, improving readability and maintainability. Added utility methods to XCUIApplication extension for tapping wallet actions. Updated ImportWalletViewModel to store hasExistingWallets as a property. Added a second test mnemonic to UITestKitConstants for multi-wallet scenarios.
Updated several localized strings in Localized.swift and multiple language resource files to remove Markdown-style bold formatting (e.g., **%@**) from user-facing messages. This change ensures consistency and avoids rendering issues in UI components that do not support Markdown.
…ability-to-name-and-select-image-for-new-wallet # Conflicts: # Gem/App.swift # Gem/ViewModels/RootSceneViewModel.swift
Introduce SetupWalletScene shown after wallet creation and import flows,
allowing users to set wallet name and select avatar (emoji/NFT image).
Screencast
https://github.com/user-attachments/assets/a4203dae-dae2-4efa-9701-9655229c2464
https://github.com/user-attachments/assets/353d696a-4d55-4206-82fb-ef36b20e07cf
TODO
Close: #1517