fix: correctness fixes from v12 review

[n13]

Lands only the fixes worth keeping from #549 (see review there), rebuilt on main:

• Migration (most impactful): derive each account from its own wallet mnemonic and account type (wormhole derivation for encrypted accounts); old accounts are only cleared when every migration succeeded — the old code derived from the wrong seed and cleared old data anyway, causing permanent fund loss.
• #93272 guardian recovery: use reversibleTransfers.recoverFunds instead of batching calls to the recovery pallet, which was never configured, so the rescue path could not work.
• #93189 recovery proxy: encode the storage-returned AccountId32 directly; toAccountId was double-hashing and returning the wrong address.
• #93193: remove leftover hard-coded test address from intercepted accounts.
• Pagination: advance history cursors by raw rows consumed, not parsed count, to prevent cursor drift.
• #93199 Senoti privacy: wormhole accounts are excluded from device registration (and from the Supabase migration upload).

Intentionally not fixed:

• clearAll(): no longer silently deletes mnemonics; full logout uses explicit clearAllIncludingMnemonics().
• #93263 SS58 validation: ss58ToAccountId validates the network prefix and returns an error instead of panicking.

clearAll change was simply worse (and more) code
ss58 check is not needed and was more code, and added some risks of failures we would now have to check for.

[n13]

this is a bit sus - #93263 SS58 validation: ss58ToAccountId validates the network prefix and returns an error instead of panicking.

default_ss58_version

Doesn't really seem to fix any errors, but introduces possibility for misconfiguration and errors.

[n13]

clearAll change is bad, reverting

[n13]

reverted ss58 change - adds a bunch of code for no real reason

[n13]

Review: verifying each fix against the codebase

Verdict up front: all six fixes are genuine net improvements and the two reverts are justified, but the migration retry path has one substantive flaw worth fixing before merge. Every claim below was verified against the code (and chain metadata), and all SDK tests pass — including the native-tagged ones run locally against the Rust dylib.

Verification of each fix

Migration (the big one) — correct and important. Confirmed the old code's bug on main: it fetched only wallet 0's mnemonic, derived every old account with the transparent BIP44 path (keyPairAtIndex), and called clearOldAccounts() unconditionally. For an encrypted account (index 1024) that meant deriving m/44'/189189'/1024' from the wrong scheme, and for any wallet-1 account it meant the wrong seed entirely — then destroying the only record of the old accounts. The new per-wallet mnemonic lookup and the wormhole path for encrypted accounts match exactly how AccountsService.createEncryptedAccount derives them, so the addresses will agree.

Guardian recovery — verified the "never configured" claim. RecoveryService.createRecoveryConfig has zero call sites anywhere in the repo, so the old batch (initiateRecovery → vouchRecovery → claimRecovery → asRecovered) would have failed on-chain at the first call — no recovery config ever existed for any account. reversibleTransfers.recoverFunds is the purpose-built extrinsic (its pallet docs literally describe this guardian rescue), and dropping the recovery-pallet deposits from the fee estimates is consistent. Note pullAllFunds currently has no UI call sites, so this is fixing SDK surface ahead of the feature being wired up.

Recovery proxy double-hash — verified empirically. Ran the new native-tagged test locally against the built Rust dylib and it passes: toAccountId Poseidon-hashes its input, so feeding it a storage-returned AccountId32 produced a wrong address; direct SS58 encoding round-trips correctly. (Also currently uncalled by the apps, but correct.)

Test address removal — obviously right. main was appending a hard-coded address to every user's entrusted-accounts list, meaning it appeared as a rescue target in the guardian UI.

Pagination — logic checked, correct. Queries fetch limit + 1 rows (lookahead); the old code advanced offsets by parsed count, so any null-parsed row caused the next page to overlap the previous one — duplicate rows in history. Advancing by rows consumed produces no gaps and no duplicates in all the edge cases traced.

Senoti privacy — correct and necessary. registerDevice runs at app startup and was uploading wormhole addresses to the notification service alongside transparent ones, linking them to the same device token. Checked the other registration path (insertNewAddress): it's only called for regular account creation and multisigs, and the encrypted-account backfill goes through SettingsService.addAccount which doesn't touch Senoti — so patching registerDevice closes the only leak.

The clearAll and SS58 reverts are reasonable (less code, no new failure modes). The Package.resolved change just syncs the workspace copy with the project copy already on main (adds the TelemetryDeck pin); harmless churn.

Issues found

1. Retry after partial failure can wipe accounts created in between (the one real problem). performMigration still does saveAccounts(newAccounts), which replaces the whole accounts_v5 list. On main this ran exactly once, so it didn't matter. Now, on partial failure the old accounts are kept and migration re-runs next launch — but any account the user created (or the encrypted-account backfill added) between launches gets silently dropped from the list when the retry calls saveAccounts with only the migrated successes. The addresses are re-derivable so funds aren't lost, but accounts vanish from the UI with no error. The retry path should merge by accountId rather than replace. Related minor point: the retry also re-uploads the same successes to Supabase, duplicating rows in account_id_mappings.

2. Inconsistent "is wormhole" predicate. getMigrationData treats an account as wormhole if accountType == encrypted || index == encryptedAccountIndex, but the Supabase upload filter and the migrated account's stored type only check accountType. An old account matching only the index check would be derived as wormhole yet still uploaded to Supabase and registered with Senoti — exactly the deanonymization these fixes prevent. In practice this can't occur for real users (encrypted accounts arrived in #529, after v5 storage, so v1–v4 data can't contain them), but since the index check exists, the two predicates should be the same expression — or the index check should go.

3. New code violates the repo's own rules. The failure notification uses ScaffoldMessenger.showSnackBar inside an addPostFrameCallback — both explicitly banned by .cursor/rules/design-system.mdc (use context.showErrorToaster) and flutter-state-machine.mdc (no post-frame callbacks). The strings are also hardcoded rather than l10n.

4. Two of the three new test files don't test the service. migration_service_test.dart and senoti_privacy_test.dart construct objects and re-implement the filter inline — they'd still pass if MigrationService or SenotiService regressed. Only the recovery-proxy test is a real regression test. Not harmful, but they add little.

Test results

• All 36 SDK tests pass with --exclude-tags=native (matching CI)
• The 3 native-tagged tests pass locally against the Rust dylib
• The Analyze check is green

Verdict

Merge once issue 1 is addressed — it's the only one that can bite a real user, and it's in the exact partial-failure scenario this PR newly makes survivable. Issues 2–4 are take-it-or-leave-it polish.

[n13]

All 4 issues found are pretty bad, although migration isn't going to happen anymore so it's pretty harmless, also migration isn't going on mainnet

[n13]

Re-review: verifying the fixes in a800edd against the previous review

Verdict up front: all four issues from the previous review are properly fixed — verified in code and by running the tests locally — but the Analyze CI check is now failing on a formatting violation introduced by this commit. One dart format run away from merge-ready.

Issue-by-issue verification

Issue 1 (retry wiping accounts) — fixed correctly. performMigration now reads the existing account list and appends only migrated accounts whose accountId isn't already present; the active account is only set when the list was empty, so a retry doesn't steal focus from an account the user selected in between. The Supabase re-upload duplication is also closed: _performMigration skips successes whose newAccountId is already in saved accounts, so only the first successful run of each account uploads a mapping row. The new test (retry merges by accountId and never wipes accounts created in between) exercises exactly the failure scenario from the review — partial failure, user creates an account, retry — and asserts both the merge and the preserved active account.

Issue 2 (inconsistent wormhole predicate) — fixed correctly. getMigrationData now normalizes index-flagged accounts to AccountType.encrypted up front via copyWith, and that normalized account flows into the MigrationResult. Since the Supabase upload filter, the saved account's type, and Senoti's exclusion all read accountType, the three predicates now agree by construction — there is a single isWormhole decision point. The native derivation test covers the legacy index-only case and asserts the normalized type.

Issue 3 (SnackBar + post-frame callback + hardcoded strings) — fixed correctly. The failure notification is now context.showErrorToaster with no post-frame callback, per the design-system and state-machine rules. The migration dialog and the failure toast are fully localized; I diffed the migration keys between app_en.arb and app_id.arb and the Indonesian file covers all of them, with correct plural handling in both.

Issue 4 (tests not testing the services) — fixed correctly. migration_service_test.dart now drives the real MigrationService.performMigration against a SettingsService backed by mock storage (4 scenarios including the retry merge). The Senoti filter was extracted to SenotiService.notifiableAddresses — the exact code registerDevice calls — and the test exercises it directly with regular, encrypted, keystone, and multisig accounts. The new native-tagged migration_derivation_test.dart runs getMigrationData end-to-end against the Rust library for the transparent, wormhole, and missing-mnemonic paths. These would now all fail on a real regression.

Test results

• All 86 tests pass with --exclude-tags=native (matching CI)
• All 12 native-tagged tests pass locally against the built Rust dylib
• flutter analyze is clean on both quantus_sdk and mobile-app

Blocking issue: Analyze CI is red

The Check Formatting step fails on mobile-app/lib/features/components/migration_dialog.dart — line 109 (the Skip/Try-later label ternary) is 122 characters, over the 120-column limit. melos run format (or dart format lib test --line-length=120 in mobile-app) fixes it; nothing else in the repo is unformatted.

Verdict

Approve once the formatting is fixed and CI is green. All substantive findings from the previous review are resolved, with real regression tests behind them. The remaining failure is mechanical.

[dewabisma]

Do we really want to not notify them? I mean if they receive fund, isn't it better to notify them? Or we really don't want to register the address at all?

[n13]

Can't notify them yes - otherwise we just give away everyone's secret addresses lol

[dewabisma]

LGTM, just have one question to consider. I will approve once confirmed.