Update wallet sync logic and add onboarding tests#1626
Conversation
Moved initial synchronization completion to CreateWalletModel and removed it from ImportWalletViewModel. Added unit tests for both CreateWalletModel and ImportWalletViewModel to verify address status and asset loading preferences. Updated test dependencies in Package.swift.
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 refactors the wallet synchronization logic by moving the initial synchronization completion to 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
|
There was a problem hiding this comment.
Code Review
This pull request refactors the initial wallet synchronization logic by moving it from the import flow to the creation flow. This is a sensible change, as newly created wallets can be considered synchronized, while imported wallets require a subsequent sync. The addition of unit tests for both CreateWalletModel and ImportWalletViewModel is excellent and ensures the new logic is covered.
My review includes a few suggestions to improve the clarity of the new test names. More importantly, I've highlighted a potential breaking API change in WalletPreferences.swift where a public property has been removed. Please ensure this does not affect other parts of the application.
I am having trouble creating individual review comments. Click here to see my feedback.
Packages/Preferences/Sources/WalletPreferences.swift (47-49)
Removing a public property is a breaking API change for the Preferences package. Please ensure that isCompleteInitialSynchronization is not used by any other modules in the project. If it is, those usages must be updated or removed as part of this pull request to avoid breaking the build.
Features/Onboarding/Tests/CreateWalletModelTests.swift (17)
For better clarity and to more accurately reflect what's being tested, consider renaming this test function. It checks both completeInitialAddressStatus and completeInitialLoadAssets. A name like createWalletCompletesInitialSynchronization would be more descriptive.
func createWalletCompletesInitialSynchronization() async throws {
Features/Onboarding/Tests/ImportWalletViewModelTests.swift (18)
To improve clarity, consider renaming this test to better describe its purpose. The test verifies that both completeInitialAddressStatus and completeInitialLoadAssets are false. A name like importWalletDoesNotCompleteInitialSynchronization would be more encompassing.
func importWalletDoesNotCompleteInitialSynchronization() async throws {
Added PrimitivesTestKit to the OnboardingTest target dependencies in Package.swift. Updated ImportWalletViewModelTests to use PrimitivesTestKit and refactored test setup for importWalletDoesNotSetAddressStatus to clear preferences before running assertions.
c8a269c to
c59e499
Compare
Moved initial synchronization completion to CreateWalletModel and removed it from ImportWalletViewModel. Added unit tests for both CreateWalletModel and ImportWalletViewModel to verify address status and asset loading preferences. Updated test dependencies in Package.swift.