fix(auth): restore wallet coins for legacy wallet migrations and seed file imports #3126
fix(auth): restore wallet coins for legacy wallet migrations and seed file imports #3126
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughBumps Flutter to 3.35.3 across Docker, CI, workflows, docs, and pubspecs. Adds SparklineRepository to initialization (main.dart and AppBootstrapper signature/flow). Enhances wallet model with copyWith. Updates WalletsRepository to merge/mark legacy wallets and handle deletion. Adjusts AuthBloc logging and restore/sign-in flow, including coin activation after restore. Updates SDK submodule pointer. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as UI
participant AuthBloc as AuthBloc
participant WalletRepo as WalletsRepository
participant SDK as KomodoDefiSdk
participant Legacy as LegacyStorage
participant Coins as CoinsService
User->>UI: Restore wallet (seed, name, config)
UI->>AuthBloc: AuthRestoreRequested(event)
AuthBloc->>WalletRepo: Check wallet exists
WalletRepo-->>AuthBloc: exists? (true/false)
alt Wallet exists
AuthBloc->>AuthBloc: Dispatch AuthSignInRequested(...)
note right of AuthBloc: Log: "Wallet <name> already exist, attempting sign-in"
else New or legacy restore
AuthBloc->>SDK: Restore from seed
SDK-->>AuthBloc: Restored wallet
opt Legacy migration
AuthBloc->>Legacy: Migrate/delete legacy wallet
note right of AuthBloc: Log deletion after migration
end
opt Activate coins if any
AuthBloc->>Coins: Activate event.wallet.config.activatedCoins
Coins-->>AuthBloc: Activated
end
AuthBloc->>SDK: Get current user
SDK-->>AuthBloc: currentUser or null
alt currentUser != null
AuthBloc->>AuthBloc: Emit loggedIn(currentUser)
AuthBloc->>SDK: Listen to auth state changes
else null
AuthBloc->>AuthBloc: Throw "user is not signed in"
end
end
sequenceDiagram
autonumber
participant Main as main.dart
participant Bootstrap as AppBootstrapper
participant Hive as HiveInit
participant Spark as SparklineRepository
participant SDK as KomodoDefiSdk
participant MM2 as Mm2Api
Main->>Bootstrap: ensureInitialized(SDK, MM2, Spark)
Bootstrap->>Hive: init()
Hive-->>Bootstrap: done
Bootstrap->>Spark: init()
Spark-->>Bootstrap: done
Bootstrap-->>Main: initialization complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (2 passed, 1 inconclusive)❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
Poem
Pre-merge checks and finishing touches (2 passed, 1 inconclusive)❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Visit the preview URL for this PR (updated for commit 14979de): https://walletrc--pull-3126-merge-g1fdzu6q.web.app (expires Sat, 20 Sep 2025 12:08:36 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: f66a4ff03faa546f12f0ae5a841bd9eff2714dcc |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes critical issues with legacy wallet migrations and seed file imports where wallet coins weren't being properly restored and HD wallet toggle options were missing. The fix ensures activated coins from legacy wallets are properly migrated to the new wallet system, while also updating Flutter version dependencies across the project.
- Restore activated coins for legacy wallets during migration and seed imports
- Fix HD wallet toggle not appearing for legacy wallet migrations
- Update Flutter version from 3.35.2 to 3.35.3 across all configuration files
Reviewed Changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk | Update SDK submodule commit hash |
| pubspec.yaml | Update Flutter version constraint to 3.35.3 |
| packages/komodo_ui_kit/pubspec.yaml | Update Flutter version constraint to 3.35.3 |
| lib/services/initializer/app_bootstrapper.dart | Add sparkline repository parameter and refactor initialization |
| lib/model/wallet.dart | Add copyWith methods for Wallet and WalletConfig classes |
| lib/main.dart | Initialize sparkline repository earlier in bootstrap process |
| lib/blocs/wallets_repository.dart | Fix legacy wallet handling to set proper wallet type and improve code formatting |
| lib/bloc/auth_bloc/auth_bloc.dart | Restore activated coins during wallet restoration and improve migration flow |
| docs/MULTIPLE_FLUTTER_VERSIONS.md | Update Flutter version references to 3.35.3 |
| docs/FLUTTER_VERSION.md | Update Flutter version references to 3.35.3 |
| app_theme/pubspec.yaml | Update Flutter version constraint to 3.35.3 |
| .github/workflows/roll-sdk-packages.yml | Update Flutter version in CI workflow |
| .github/actions/flutter-deps/action.yml | Update Flutter version in GitHub action |
| .docker/komodo-wallet-android.dockerfile | Update Flutter version in Docker environment |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
.docker/komodo-wallet-android.dockerfile (1)
12-17: Harden the download step to fail fast on HTTP errors and follow redirects.Use curl -fSL and explicit -o to avoid partial/corrupt downloads going unnoticed.
-RUN curl -O https://storage.googleapis.com/flutter_infra_release/releases/stable/linux/flutter_linux_${FLUTTER_VERSION}-stable.tar.xz && \ - tar -xvf flutter_linux_${FLUTTER_VERSION}-stable.tar.xz -C ${HOME} && \ +RUN curl -fSL -o flutter_linux_${FLUTTER_VERSION}-stable.tar.xz \ + https://storage.googleapis.com/flutter_infra_release/releases/stable/linux/flutter_linux_${FLUTTER_VERSION}-stable.tar.xz && \ + tar -xvf flutter_linux_${FLUTTER_VERSION}-stable.tar.xz -C ${HOME} && \ rm flutter_linux_${FLUTTER_VERSION}-stable.tar.xz && \ flutter config --no-analytics && \ yes "y" | flutter doctor --android-licenses && \ flutter doctorlib/model/wallet.dart (2)
25-32: Docstring default wallet type is incorrect.The comment says WalletType.iguana but the code defaults to WalletType.hdwallet. Align the docs.
- /// [walletType] - The [WalletType] of the wallet. Defaults to [WalletType.iguana]. + /// [walletType] - The [WalletType] of the wallet. Defaults to [WalletType.hdwallet].
137-145: WalletConfig.copy drops isLegacyWallet. This can break legacy-flow logic.copy() should preserve the flag; otherwise subsequent copies lose legacy status.
WalletConfig copy() { return WalletConfig( activatedCoins: [...activatedCoins], hasBackup: hasBackup, type: type, seedPhrase: seedPhrase, pubKey: pubKey, + isLegacyWallet: isLegacyWallet, ); }lib/blocs/wallets_repository.dart (1)
70-81: Bug: writing List into storage likely breaks serialization._storage.write expects JSON-serializable data; persist maps via toJson().
if (wallet.isLegacyWallet) { - final wallets = await _getLegacyWallets(); - wallets.removeWhere((w) => w.id == wallet.id); - await _legacyWalletStorage.write(allWalletsStorageKey, wallets); + final wallets = await _getLegacyWallets(); + wallets.removeWhere((w) => w.id == wallet.id); + await _legacyWalletStorage.write( + allWalletsStorageKey, + wallets.map((w) => w.toJson()).toList(), + ); return; }lib/bloc/auth_bloc/auth_bloc.dart (1)
160-166: Bug: registration path returns early and leaves UI stuck in loading.
emit(AuthBlocState.loading());is called, then on existing wallet it justreturn;— no sign-in is triggered, so the UI can hang.Apply:
emit(AuthBlocState.loading()); - if (await _didSignInExistingWallet(event.wallet, event.password)) { - return; - } + if (await _didSignInExistingWallet(event.wallet, event.password)) { + _log.info('Wallet ${event.wallet.name} already exists, attempting sign-in'); + add(AuthSignInRequested(wallet: event.wallet, password: event.password)); + return; + }
🧹 Nitpick comments (7)
.github/actions/flutter-deps/action.yml (1)
6-11: Expose flutter version as a composite action input to avoid future drifts.Hardcoding the version here forces PR-wide edits next time. Consider an input with a default, then use it in the step.
Apply this diff in the composite action to add an input and use it:
name: "Flutter Dependencies" description: "Installs Flutter and any other dependencies required for the build" runs: using: "composite" +inputs: + flutter-version: + description: "Flutter version to install" + required: false + default: "3.35.3" steps: - name: Get stable flutter uses: subosito/flutter-action@v2 with: # NB! Keep up-to-date with the flutter version used for development - flutter-version: "3.35.3" + flutter-version: "${{ inputs.flutter-version }}" channel: "stable"docs/FLUTTER_VERSION.md (1)
5-5: Version bump looks consistent across this doc. Add reference for the “Known issues” note.Consider linking to a concrete issue/PR so the statement is actionable.
Also applies to: 18-19, 25-25
.github/workflows/roll-sdk-packages.yml (1)
49-49: Good update; consider centralizing Flutter version and enabling cache.
- Define FLUTTER_VERSION as an env at the top (or reuse from a single source) to avoid drift across workflows.
- subosito/flutter-action supports caching; enabling it will speed runs.
name: Roll SDK Packages +env: + FLUTTER_VERSION: "3.35.3" @@ - flutter-version: "3.35.3" + flutter-version: "${{ env.FLUTTER_VERSION }}" + cache: trueAlso applies to: 92-92
lib/model/wallet.dart (1)
82-93: Minor: avoid unnecessary deep copy in Wallet.copyWith when config is unchanged.Returning the existing instance is cheaper and preserves referential equality when desired.
Wallet copyWith({ String? id, String? name, WalletConfig? config, }) { return Wallet( id: id ?? this.id, name: name ?? this.name, - config: config ?? this.config.copy(), + config: config ?? this.config, ); }lib/blocs/wallets_repository.dart (2)
40-47: Filtering Trezor wallets here: OK short-term; push into SDK when feasible.You already left a TODO; keeping the logic in one place will prevent drift across clients.
100-129: Name validation: consider trimming for duplicate check (optional).You intentionally check duplicates on raw name for back-compat; if that is required, ignore this. Otherwise, use trimmedName for duplicates to avoid invisible-space collisions.
- if (_cachedWallets!.firstWhereOrNull((w) => w.name == name) != null) { + if (_cachedWallets!.firstWhereOrNull((w) => w.name == trimmedName) != null) { return LocaleKeys.walletCreationExistNameError.tr(); }lib/services/initializer/app_bootstrapper.dart (1)
12-16: Add guard for concurrent calls to ensureInitialized
- Only one call site found at lib/main.dart:65, signature is updated accordingly.
- Introduce a Completer-based guard in AppBootstrapper.ensureInitialized to prevent double initialization.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
.docker/komodo-wallet-android.dockerfile(2 hunks).github/actions/flutter-deps/action.yml(1 hunks).github/workflows/roll-sdk-packages.yml(2 hunks)app_theme/pubspec.yaml(1 hunks)docs/FLUTTER_VERSION.md(2 hunks)docs/MULTIPLE_FLUTTER_VERSIONS.md(4 hunks)lib/bloc/auth_bloc/auth_bloc.dart(7 hunks)lib/blocs/wallets_repository.dart(7 hunks)lib/main.dart(1 hunks)lib/model/wallet.dart(2 hunks)lib/services/initializer/app_bootstrapper.dart(3 hunks)packages/komodo_ui_kit/pubspec.yaml(1 hunks)pubspec.yaml(1 hunks)sdk(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-31T19:24:53.317Z
Learnt from: CR
PR: KomodoPlatform/komodo-wallet#0
File: AGENTS.md:0-0
Timestamp: 2025-08-31T19:24:53.317Z
Learning: Run `flutter analyze` before committing code
Applied to files:
.docker/komodo-wallet-android.dockerfile
🔇 Additional comments (24)
.github/actions/flutter-deps/action.yml (1)
10-11: Bump to Flutter 3.35.3 looks good.Matches the repo-wide version update and keeps CI aligned with dev tooling.
pubspec.yaml (1)
22-22: Approve Flutter version bump across repository
Scan for “3.35.2” returned no matches; all pubspec.yaml files (root, packages/komodo_ui_kit, app_theme) specifyflutter: ">=3.35.3 <4.0.0".packages/komodo_ui_kit/pubspec.yaml (1)
7-7: UI kit min Flutter version aligned to 3.35.3.Consistent with the workspace upgrade.
app_theme/pubspec.yaml (1)
10-10: App theme min Flutter version aligned to 3.35.3.All good.
sdk (1)
1-1: Submodule bump: audit SDK commit and CI configuration
- SDK updated from f22ca69 to 508d63f (includes: fix(auth): store BIP39 compatibility regardless of wallet type; feat(cex-market-data): add CoinPaprika API fallback; feat(types): parent display name suffix via subclass). Confirm this SHA matches the intended SDK release, includes all legacy wallet migration fixes, and introduces no breaking API changes.
- CI workflows under .github/workflows don’t reference submodules: on
actions/checkout@—ensure your checkout steps are set to fetch submodules recursively so the updated SDK is included in CI builds.- (Optional) If a stable annotated tag exists for 508d63f, pin the submodule to that tag and document the bump in CHANGELOG for traceability.
.docker/komodo-wallet-android.dockerfile (1)
3-3: Version bump to 3.35.3: LGTM.docs/MULTIPLE_FLUTTER_VERSIONS.md (1)
60-60: Docs sync to 3.35.3: LGTM.Everything reads consistently across Sidekick, FVM, Windows, and project usage.
Also applies to: 95-100, 134-139, 161-161
lib/blocs/wallets_repository.dart (5)
23-25: Ctor defaults: LGTM.Nice use of initializer list for optional dependencies.
51-68: Legacy wallet normalization: LGTM.Explicitly forcing iguana type and marking isLegacyWallet improves downstream behavior.
133-136: Coins reset: LGTM.Selective deactivation avoids toggling defaults.
152-155: Encryption call formatting change: LGTM.No functional change; readability improved.
76-81: Verify legacy wallet IDs exist in storage.If legacy entries lack an id, removeWhere by id may wipe multiple items (e.g., empty ids). If uncertain, fall back to matching by name.
Would you like a follow-up patch that falls back to name when id is missing?
lib/services/initializer/app_bootstrapper.dart (2)
29-33: LGTM: clearer warm-up completion log.Message is concise and useful for startup telemetry.
47-61: Sparkline init is idempotent and failure-safe; no changes needed.
SparklineRepository.init()returns early if already initialized (isInitialized), so repeated calls are safe.- On first call, any exceptions bubble up; if you need non-blocking behavior, wrap this one call in try/catch at the bootstrapper (optional).
lib/main.dart (1)
62-66: Ignore missing import suggestion: SparklineRepository is already publicly exported by package:komodo_cex_market_data/komodo_cex_market_data.dart, which is imported at the top of lib/main.dart — no additional import required.Likely an incorrect or invalid review comment.
lib/bloc/auth_bloc/auth_bloc.dart (9)
97-97: LGTM: clearer login logs.Improved, user-focused log messages during sign-in.
Also applies to: 114-115
165-176: LGTM: registration flow logs and options.Good clarity and correct derivation/weak-password propagation.
Also applies to: 178-181
209-215: Good: existing-wallet restore now dispatches sign-in.Prevents silent no-op on restore when the wallet already exists.
218-220: LGTM: emit loading before restore.Sets expectations for the UI during a potentially long operation.
233-237: LGTM: clearer post-restore log.Concise and helpful for telemetry.
257-263: LGTM: validate user presence then emit logged-in and start watcher.Solid post-restore sequencing.
45-46: No action needed:@overrideis correct
The mixinTrezorAuthMixininlib/bloc/auth_bloc/trezor_auth_mixin.dartdeclares afinal _log, which provides an implicit getter. Retaining@overrideon the subclass’s_logfield is valid.
247-251: Legacy wallet deletion targets correct record
deleteWallet checkswallet.isLegacyWalletand removes the legacy entry by matchingwallet.id, so it won’t delete the restored SDK wallet.
240-242: addActivatedCoins ignores already-activated coins (idempotent); batching is purely an optional performance optimization.
|
unable to build the old one here. |
I made a typo/mistake in the description. The version should be EDIT: I also updated the My ┌─────────────────────────────────────────────────────────┐
│ A new version of Flutter is available! │
│ │
│ To update to the latest version, run "flutter upgrade". │
└─────────────────────────────────────────────────────────┘
Flutter 3.22.3 • channel stable • https://github.com/flutter/flutter.git
Framework • revision b0850beeb2 (1 year, 2 months ago) • 2024-07-16 21:43:41 -0700
Engine • revision 235db911ba
Tools • Dart 3.4.4 • DevTools 2.34.3 |
Fixes:
Issues possibly fixed by other PRs between last release and this PR:
Before
Screen.Recording.2025-09-09.at.09.40.04.mov
After
Screen.Recording.2025-09-09.at.10.48.06.mov
Creating legacy wallets from v0.8.3 for testing
Summary by CodeRabbit
Improvements
Bug Fixes
Documentation
Chores