Conversation
New wallets and new accounts use ML-DSA-87 again. Existing accounts keep their scheme and address. Import still discovers accounts of both schemes, and when the indexer is unreachable it falls back to adding the ML-DSA-65 root instead of the ML-DSA-87 one, which is now the default root. The per-wallet scheme rule that kept old wallets on their previous scheme is gone: every new account is ML-DSA-87. Discovery results and the account list sort the current scheme first.
n13
left a comment
There was a problem hiding this comment.
Reviewer model: GPT-6 Sol
Verdict (advisory): Approve
No blocking findings at b206fa709670fe2bc3385c429cd54d0fa1057b42 (base 900fa77912e74b00fe60678b48e6f61b556e2bdd). Existing accounts retain their stored scheme and derivation path for signing. New accounts take the next free ML-DSA-87 index, and import scans both schemes with an ML-DSA-65 root fallback when discovery fails.
Validation: git diff --check and changed-file formatting passed; the Rust release library built; the focused native scheme tests passed (4); the SDK non-native suite passed (531, with 1 skipped); the mobile suite passed (511); and GitHub's Analyze job passed. I stopped the local all-package analyzer at the required 10-second limit; CI completed that check.
Import always discovers accounts of both schemes and adds every account found, whatever its scheme or index. When discovery fails, the error is logged and reported, and no root account is guessed for another scheme.
n13
left a comment
There was a problem hiding this comment.
Reviewer model: GPT-6 Sol
Verdict (advisory): Approve
No blocking findings at bf5cd9f598f56a09085e4e4487dd109f21616f21 (base 900fa77912e74b00fe60678b48e6f61b556e2bdd). I traced wallet creation, added-account index selection, import/discovery, and signing. Stored accounts retain their recorded scheme and path. Import creates the ML-DSA-87 root, discovers both schemes, and no longer adds an unverified other-scheme root if discovery fails.
Validation: git diff --check and changed-file formatting passed. Focused account-model tests passed (4); native discovery and account-creation tests passed (4, using the existing Rust library with identical Rust source); the SDK non-native suite passed (531, with 1 skipped); and the mobile suite passed (511). I stopped the local all-package analyzer at the required 10-second limit. GitHub's Analyze job was still running when I posted this review.
The scan step of wallet import moves into WalletCreationService with an injected AccountDiscoveryService. It adds every account found in either scheme at any index, and hands a failed scan to a callback that decides whether to run it again. The import screen answers that callback with a dialog: "Account scan failed", with Try again and Skip. Try again reruns the scan, Skip finishes the import with the root account only. The failure is still logged and reported to telemetry. Mockito tests cover the scan with a discovery service that fails first: accounts added on the retry, nothing added when the retry is declined, and the root kept active when the scan finds it.
n13
left a comment
There was a problem hiding this comment.
Reviewer model: GPT-6 Sol
Verdict (advisory): Request changes
Reviewed head 62ab11eb5bdd395044ce3b6291f13d8fee374a9f against base 900fa77912e74b00fe60678b48e6f61b556e2bdd.
-
P1 — “Skip” leaves older ML-DSA-65 accounts inaccessible in the app.
mobile-app/lib/v2/screens/import/import_wallet_screen.dart:80-82,129-140andmobile-app/lib/services/wallet_creation_service.dart:85-96: import saves only the new ML-DSA-87 root before scanning, and Skip completes without the discovered accounts. For a returning wallet whose funds are on ML-DSA-65, an indexer outage followed by Skip leaves those accounts absent even after the network returns. There is no later transparent-account rescan; Create Account now derives only ML-DSA-87, and importing the same phrase again fails on the duplicate ML-DSA-87 root before it can scan. Provide a way to retry discovery for the existing wallet after import (and explain the consequence of Skip), with a regression test for Skip → network recovery → ML-DSA-65 account restored. -
P2 — The changed generated mock fails the required formatting gate.
mobile-app/test/unit/wallet_creation_service_test.mocks.dart:747: GitHub's Analyze job stopped at Check Formatting because this file changes underdart format lib test --line-length=120 --set-exit-if-changed. The same file fails a local changed-file format check. Format and commit the mock with the CI formatter.
Validation: git diff --check passed; focused mobile wallet-creation/import tests passed (9); SDK account-model tests passed (4). The other changed Dart files passed formatting. GitHub's Analyze job failed before its analyzer and test steps; native SDK tests were not run locally because this worktree has no built Rust library.
A skipped or interrupted import scan left a returning wallet without its ML-DSA-65 accounts for good, since nothing scans again after import. SettingsService now keeps a per-wallet "account scan pending" flag. The scan sets it when it starts and clears it when it finishes. The home screen runs the pending scans once the accounts have loaded, so a wallet imported while the indexer was down gets its accounts on the next start. The scan-failed dialog says that Skip means the app scans again later. The regenerated mock file is formatted with the CI formatter.
n13
left a comment
There was a problem hiding this comment.
Reviewer model: GPT-6 Sol
Verdict (advisory): Request changes
Reviewed head 3915fe7553b71815c61e391061f0fc9032f53114 against base 900fa77912e74b00fe60678b48e6f61b556e2bdd.
- P2 — A resumed scan leaves the empty root selected after restoring an older funded account.
mobile-app/lib/services/wallet_creation_service.dart:116-123: the initial import scan passesdefaultAccountIdand activates the first discovered account when the new ML-DSA-87 root has no history, butresumePendingAccountScansomits that argument. For an ML-DSA-65 wallet imported during an indexer outage, Skip leaves the new ML-DSA-87 root active; when the next-start scan finds and adds the funded ML-DSA-65 account, the active account remains the empty root because the activation guard at lines 94-98 cannot run. Home therefore continues to show the empty account's balance and activity even after the scan succeeds. Preserve the initial active-root choice for a resumed scan when the user has not since selected another account, and cover Skip → successful resume → active funded account in a regression test.
Validation: git diff --check passed; changed-file formatting passed (18 Dart files); focused native SDK scheme/discovery tests passed (4) using a library built from the identical Rust source tree; the SDK non-native suite passed (532 tests, 1 skipped); and the mobile suite passed (517 tests). The local all-package analyzer was stopped at the required 10-second limit after cold-wallet and miner passed; GitHub's Analyze job was still running at review time.
A scan resumed on a later start left the empty ML-DSA-87 root selected after adding the funded ML-DSA-65 account. The resume now passes the wallet's active transparent account as the default, so the import rule applies: an active account with no history gives way to the first funded account found. An active encrypted account or an account of another wallet is left alone.
n13
left a comment
There was a problem hiding this comment.
Reviewer model: GPT-6 Sol
Verdict (advisory): Request changes
Reviewed head 997a20d10e1a9dc3c60bc16137a7d20abde4132d against base 900fa77912e74b00fe60678b48e6f61b556e2bdd.
-
P2 — A resumed scan can override a newer account selection.
mobile-app/lib/services/wallet_creation_service.dart:94-97,115-123:resumePendingAccountScanscaptures the active transparent account before awaiting network discovery, thendiscoverImportedAccountsselects a funded account based only on that captured ID. Home runs this scan in the background, so a user can switch to an encrypted account or another wallet while discovery is in flight; completion then switches them back. Re-read the active account immediately before activation and switch only if it is still the captured account. Cover a selection change during a delayed scan. -
P2 — Deleting a wallet during its resumed scan can restore accounts without a mnemonic.
mobile-app/lib/services/wallet_creation_service.dart:88-99andquantus_sdk/lib/src/services/settings_service.dart:155-175: the background scan keeps the mnemonic and discovered accounts in memory while the Accounts menu can remove a non-primary wallet, deleting its accounts, mnemonic, and pending flag. When discovery completes,addAccountcan insert those accounts again although the mnemonic is gone, leaving accounts that cannot sign. Coordinate scan completion with wallet removal so a removed wallet cannot be written back, and test removal during a delayed scan.
Validation: git diff --check and changed-Dart-file formatting passed. Focused mobile wallet-creation tests passed (12); SDK account-model and settings tests passed (22). The native SDK discovery test could not load the Rust library in this fresh worktree. I stopped local all-package analysis at the required 10-second limit; GitHub Analyze was still running at review time.
… removal The resumed scan runs while the user can act. Two races are closed: The active account is now re-read right before activation and only switched when it is still the one the scan started with, so a selection made during the scan stays. Removing a wallet clears its pending-scan flag before anything else, and the scan re-checks that flag after discovery and writes nothing when it is gone, so a removed wallet's accounts are not written back without a mnemonic.
n13
left a comment
There was a problem hiding this comment.
Reviewer model: GPT-6 Sol
Verdict (advisory): Request changes
Reviewed head bc68ea46f907095bc7a4ecbf185eaf3f012d62b6 against base 900fa77912e74b00fe60678b48e6f61b556e2bdd.
- P2 — A deleted wallet can still be restored by its background scan.
mobile-app/lib/services/wallet_creation_service.dart:89,94-101,128-135andquantus_sdk/lib/src/services/settings_service.dart:167-175: resume waits for a secure-storage mnemonic read, thendiscoverImportedAccountsunconditionally sets the pending flag to true. If wallet removal clears the flag while that read is in flight but the read returns the old mnemonic, resume sets the flag again, so the later check passes and discovered accounts can be inserted after the wallet's accounts and mnemonic are deleted. A removal after the check at line 94 also races the later awaited account reads and writes. This leaves visible accounts with no mnemonic to sign from. Coordinate removal with the whole scan write phase, including its start, and test removal during the mnemonic read and after discovery returns.
Validation: git diff --check passed; changed Dart files passed formatting (18 files); focused mobile wallet-creation tests passed (14); SDK account-model and settings tests passed (23). Local all-package analysis was stopped at the required 10-second limit; GitHub Analyze was still running. Native SDK tests were not run in this fresh worktree.
Only the import marks a wallet's scan pending. A resumed scan no longer sets the flag again after its awaited mnemonic read, so a wallet removed during that read stays removed. The scan checks the flag right before each account write and before switching the active account, so a removal at any point during the scan stops all further writes.
n13
left a comment
There was a problem hiding this comment.
Reviewer model: GPT-6 Sol
Verdict (advisory): Request changes
Reviewed head 707b9f2240234043524d58210b8a9ea9d6850c28 against base 900fa77912e74b00fe60678b48e6f61b556e2bdd.
- P2 — An old scan can add accounts to a replacement wallet at the same index.
mobile-app/lib/services/wallet_creation_service.dart:103-118,143-160checks only the per-index pending boolean. Wallet removal clears that flag, butnextWalletIndexreuses the freed index and a new import sets the same flag true. If the old background discovery completes after that new import starts, its check passes and it adds accounts derived from the removed wallet's seed to the replacement wallet. Those accounts appear under a wallet whose stored mnemonic cannot sign for them. I reproduced this with a delayed-discovery test spanning removal and index reuse; the old account was written. Bind each scan to a stable wallet identity or generation, and test that a late response cannot write accounts or clear the new import's pending state after index reuse.
Validation: git diff --check and changed-Dart-file formatting passed (18 files). Focused mobile tests passed (16 existing tests plus the temporary reproduction); SDK account-model/settings tests passed (23). I stopped the direct all-package analyzer after 10 seconds; cold-wallet, miner, and SDK analysis had passed before the limit. GitHub's Analyze job was still running when I posted this review.
A boolean flag let an old scan write into a replacement wallet: after the wallet was removed and another seed imported at the same index, the new import set the flag again and the old scan's checks passed. SettingsService now records the root account of the pending scan. The scan compares that record before every write, so a replacement wallet from another seed, which has a different root, stops the old scan and keeps its own pending scan.
n13
left a comment
There was a problem hiding this comment.
Reviewer model: GPT-6 Sol
Verdict (advisory): Request changes
Reviewed head 670a72af76d87828af106b1457820c9b3ac5d437 against base 900fa77912e74b00fe60678b48e6f61b556e2bdd.
-
P2 — A crash before the pending marker is saved can strand an imported wallet without discovery.
mobile-app/lib/v2/screens/import/import_wallet_screen.dart:69-81persists the mnemonic and ML-DSA-87 root beforeWalletCreationService.discoverImportedAccountsrecords the pending scan atmobile-app/lib/services/wallet_creation_service.dart:85. If the app stops, or that marker write fails, between those commits, Home sees a wallet but no pending scan and never discovers its ML-DSA-65 accounts. Reimporting the same seed at the primary index fails on the already saved root. Persist the marker before committing the root with rollback on failure, or reconcile imported roots that have never completed a scan on startup. Cover this interruption boundary. -
P2 — A selection made during the mnemonic read can be overwritten.
mobile-app/lib/services/wallet_creation_service.dart:149-161captures the active local account, then awaitsgetMnemonic. If the user selects an encrypted account or another wallet during that read,_finishPendingScancaptures the new account at line 106; its line 126 equality check then passes and line 128 switches to the discovered account. Compare against the original selection captured before the mnemonic read. A temporary focused regression reproducing this sequence failed becausesetActiveAccountwas called.
Validation: git diff --check and formatting of all 18 changed Dart files passed. The focused mobile wallet-creation suite passed (17 tests), as did the SDK account-model/settings suites (23 tests). The local all-package analyzer was stopped at the required 10-second limit; GitHub's Analyze job passed. Native SDK tests were not run locally in this fresh worktree.
…ade during the mnemonic read WalletCreationService.importWallet now owns the import commit: it saves the mnemonic, records the pending account scan, then inserts the root. An app stopped right after the insert still finishes the scan on its next start. A failed insert leaves no pending scan behind. Dev seeds get no scan. A resumed scan compares the active account with the selection captured before its mnemonic read, so a switch made during that read is kept.
n13
left a comment
There was a problem hiding this comment.
Reviewer model: GPT-6 Sol
Verdict (advisory): Approve
No blocking findings at f89ca8839359e5fbfe0dfa287396c97a1187fc29 (base 900fa77912e74b00fe60678b48e6f61b556e2bdd). New accounts use ML-DSA-87 while stored accounts retain their own scheme and derivation path. Import scans both schemes, and a skipped scan stays recorded for the next app start. I traced the pending-scan checks through wallet removal and replacement at the same index.
Validation: git diff --check and formatting of the 14 changed Dart files passed. Focused mobile wallet-creation tests passed (21), SDK account-model/settings tests passed (23), and native account-discovery/account-creation tests passed (4) using the existing library built from unchanged Rust source. GitHub's Analyze check passed. I stopped local all-package analysis at the required 10-second limit; cold-wallet and miner analysis had passed before the stop.
dewabisma
left a comment
There was a problem hiding this comment.
Have few nits. I believe we should remove unnecessary comment.
|
|
||
| // An import scan skipped or interrupted while the indexer was unreachable | ||
| // is finished here, once the accounts are known. | ||
| ref.listenManual<AsyncValue<List<Account>>>( |
There was a problem hiding this comment.
So now, everytime we mutate accounts, it will rescans? When do we really need this behavior exactly?
Because we have remove account, edit account name, add new account/wallet. Seems, we only need to do account scans when we import wallet only. Just thinking this is not really efficient approach.
But not really that bad though, well can pass this. Just need to make sure we also cleanup this listenManual subscription so it doesn't somehow result in memory leaks even though since it's home screen it should't be but let's be safe here.
There was a problem hiding this comment.
You're right - looking into this?!
There was a problem hiding this comment.
Fixed in 1731d55. The home screen no longer listens to account changes, and adding, removing or renaming accounts never scans the indexer.
The old code didn't actually rescan on every change: a flag limited it to once per home screen, and only for a wallet whose import scan never finished. But the listener made it look that way, and the home screen is rebuilt after every send, so the check ran far more often than it should.
Scanning now happens only for import. The import scan runs as before. If it was skipped or cut off because the indexer was unreachable, WalletInitializer retries it once on the next app start. That retry reads the pending record, so wallets that finished their import scan are never touched. With the listener gone, there's no subscription left to clean up.
| /// Scheme of accounts stored before the scheme was recorded. | ||
| /// Scheme of accounts stored before the scheme was recorded, when ML-DSA-87 | ||
| /// was the only one. | ||
| static const DilithiumScheme legacy = DilithiumScheme.mlDsa87; |
There was a problem hiding this comment.
Do we still need this legacy constant if we only use ml dsa 87 again?
There was a problem hiding this comment.
The wallet always supports both schemes
There was a problem hiding this comment.
This seems like the wrong name here.
There was a problem hiding this comment.
Removed in eb5224c. Both schemes stay supported. legacy was just another name for ML-DSA-87, and that name is wrong now that 87 is the default again. Each place that used it now says DilithiumScheme.mlDsa87 directly: accounts stored before the scheme was recorded (fromStorageName, the single-account migration, old cold wallet vaults), fee sizing for keyless accounts, and the cold wallet tests.
| if (remaining.isEmpty) { | ||
| throw Exception('Cant remove last wallet!'); | ||
| } | ||
| // First, so an account scan finishing later sees the wallet is gone. |
There was a problem hiding this comment.
this is an unclear comment.
There was a problem hiding this comment.
Removed in eb5224c. I also shortened the doc comments this PR added in WalletCreationService.
…anges The home screen listened to the accounts provider and was rebuilt after every send, so the resume check ran far more often than needed. The scan now runs once from WalletInitializer and reads the wallets itself.
ML-DSA-87 is the default again, so calling it legacy is misleading. Uses that mean ML-DSA-87 now name it directly.
n13
left a comment
There was a problem hiding this comment.
Not reviewing again — no new commit or comment since the last review.
What
New wallets and new accounts in the mobile wallet use ML-DSA-87 again instead of ML-DSA-65.
Existing accounts are untouched. Every stored account keeps its recorded scheme, derivation path and address, and the wallet keeps signing with whichever scheme each account has.
Changes
DilithiumSchemeExtension.currentis ML-DSA-87. Thelegacyconstant is removed; code that means ML-DSA-87 (accounts stored before the scheme was recorded, fee sizing for keyless accounts) namesDilithiumScheme.mlDsa87directly.AccountsService.createNewAccountalways derives an ML-DSA-87 account at the next free ML-DSA-87 index. The per-wallet rule that kept a wallet on its previous scheme is removed.Account.compareorder (current scheme first, then by index).WalletCreationService.importWalletowns the import commit: it saves the mnemonic, records the wallet's pending scan inSettingsServiceas its root account id, then inserts the root, so an app stopped right after the insert still scans on its next start.WalletInitializerrunsWalletCreationService.resumePendingAccountScansonce at app start, so a wallet imported while the indexer was down gets its remaining accounts on the next start. Only wallets with an unfinished import scan are scanned. Account changes never trigger a scan. The dialog tells the user this. As on import, an active transparent account of that wallet with no history gives way to the first funded account found; an active encrypted account or an account of another wallet is left alone.WalletCreationService.discoverImportedAccounts, with an injectedAccountDiscoveryService, so it can be tested with a mock.Tests
createNewAccountyields ML-DSA-87 at the next free index, also for a wallet that only holds ML-DSA-65 accounts.flutter testinquantus_sdk(CI set and native) andmobile-apppass.melos run analyzeand the CI format check are clean.