feat(services): Recover legacy Android Keystore account mappings - #3182
feat(services): Recover legacy Android Keystore account mappings#3182justjoolz wants to merge 3 commits into
Conversation
Kay-Zee
left a comment
There was a problem hiding this comment.
The removeOldKey fix is a nice catch — the promise hanging on a mismatched key was a real bug.
A few things on the new code, inline: the savePendingRotation seedphrase requirement conflicts with what the service actually sends (and stores a mnemonic in plain prefs), and the recovery scanner has a cold-start issue and a cache-integrity edge.
One broader note: the rotation flow in onflow/FRW-monorepo#1427 creates seedphrase-derived keys, not Keystore alias keys, so this scanner wouldn't recover an interrupted rotation from that flow. Still useful for the legacy #2831 situation, but worth adjusting the description so it doesn't read as the recovery mechanism for the new WAL.
| val publicKey = state.getString("publicKey") | ||
| ?: throw IllegalArgumentException("Pending rotation missing publicKey") | ||
| val seedphrase = state.getString("seedphrase") | ||
| ?: throw IllegalArgumentException("Pending rotation missing seedphrase") |
There was a problem hiding this comment.
The service writes its first two markers (pre-tx and api-registered) without a seedphrase — the field is optional on the TS side — so this throw kills the rotation at the very first write. Either tolerate the absence, or better, take the seedphrase out of the contract entirely (see below).
There was a problem hiding this comment.
Removed by scope reduction.
NativeFRWBridge.kt now has zero diff.
| prefs.edit { | ||
| putBoolean("$address.$FIELD_EXISTS", true) | ||
| putString("$address.$FIELD_PUBLIC_KEY", publicKey) | ||
| putString("$address.$FIELD_SEEDPHRASE", seedphrase) |
There was a problem hiding this comment.
This puts the mnemonic in plain SharedPreferences. We have allowBackup=false, so it won't hit cloud backups, but it's still plaintext at rest — and since nothing ever resumes a stuck rotation, it can sit there indefinitely. EncryptedSharedPreferences at minimum, though I'd rather not persist the seedphrase at all — pubkey/phase/txId covers the recovery cases.
There was a problem hiding this comment.
Removed by scope reduction. This PR no longer persists or handles mnemonic material.
| // will be written to KeyStorageManager so that | ||
| // buildLocalKeyAccounts() can surface them in the account switcher. | ||
| try { | ||
| val recovered = KeyStoreMigrationManager.recoverOrphanedKeystoreKeys() |
There was a problem hiding this comment.
This runs during AccountManager init, and buildOnChainKeyMap does a blocking FlowCadenceApi.getAccount() per known address, sequentially. First launch (and any launch while offline, since the hash intentionally isn't cached on network failure) eats N network round-trips on the cold-start path. Can we kick this to a background scope after init finishes?
There was a problem hiding this comment.
Recovery now runs in an ioScope after AccountManager initialization, so the on-chain lookups do not block the initialization path. Discovery returns successful matches together with any partial failures, and the previous global alias hash has been removed so offline or incomplete attempts remain naturally retryable.
Focused tests were added in 6021bc4. I could not execute them locally because the required Flow GitHub Packages dependencies return 401 Unauthorized, and this repository does not appear to run Android compilation/tests on PRs.
| ) | ||
|
|
||
| val currentAccounts = AccountCacheManager.read()?.toMutableList() ?: mutableListOf() | ||
| currentAccounts.removeAll { it.prefix == prefix } |
There was a problem hiding this comment.
Two concerns with injecting the synthetic account directly into the cache (which the NOTE already flags as a stopgap):
removeAll { it.prefix == prefix }will evict a real cached account with that prefix. The dedup check (isAliasAlreadyMapped) only looks atUserPrefixCacheManager, so an account that's in the account cache but missing a prefix entry gets replaced by the mock.- The avatar URL points at boringavatars — probably don't want an external service baked into account data.
Gating on the prefix being absent from both caches would close the first one.
There was a problem hiding this comment.
Addressed in 22decd2 and 1cff135.
Synthetic account creation and direct scanner-side cache injection have been removed entirely, including the external avatar. Discovery now returns evidence only, and AccountManager applies the recovered prefix to the matching existing real account.
This preserves the account’s wallet UID, username and profile identity, repairs the UID-to-prefix mapping, updates the in-memory account state immediately, and publishes through the existing account observers. Ambiguous multiple-prefix matches are left unresolved rather than guessed automatically.
Focused identity-preservation, publication, partial-state and idempotence tests were added in 6021bc4, subject to the same authenticated-build limitation noted above
a9bf55c to
6021bc4
Compare
|
The scoped rebuild is now pushed, the description is updated, and I’ve replied to the review threads. The remaining validation dependency is an authenticated Android build/test run, since the required GitHub Packages dependencies are unavailable to my local environment and no PR build workflow is configured. |
|
@Kay-Zee @zhouxl @jaymengxy @claude Following up on PR #3182. I addressed all review feedback and pushed the scoped rebuild on 17 July. The remaining validation dependency is an authenticated Android build/test run. Could you please review the latest revision and let me know what, if anything, is blocking validation, completion of the review, and a path to merge and release? As noted in the review, this recovery path is useful for the unresolved #2831 situation, and there are many other unresolved reports from users unable to sign, transfer, stake, or back up their accounts. I’m happy to address any concrete technical concerns promptly. |
|
Thanks for the patience — the scoped rebuild is in good shape. Re-reviewed the current head: all four review points are addressed, and the tests cover the right surface. On timing: we're intending to land this in the release right after 3.1.0 rather than stacking it in. How do you feel about that? |
All requested changes were addressed in the Jul 17 scoped rebuild; re-reviewed with no outstanding items. Held for release sequencing after r3.1.0, not for content.
|
Thanks @Kay-Zee the release immediately after 3.1.0 is fine with me. I appreciate you re-reviewing the narrowed implementation. The only remaining dependency on my side is the authenticated Android build/test run noted in the PR. Please let me know if that exposes anything or if you need changes when preparing the release. |
Summary
Recover legacy Flow accounts whose Android Keystore key still exists but whose
local prefix mapping was lost.
The scanner does not mutate caches, and recovery does not create synthetic accounts or identities.
Scope
This PR contains Android Keystore recovery only. Key-rotation WAL, mnemonic staging, staged signing, commit/finalize/discard APIs, algorithm metadata, and bridge compatibility remain outside this PR. NativeFRWBridge.kt is unchanged.
Tests
Focused JVM coverage for normalization, discovery, partial failures, accumulated persistence, identity preservation, publication, switching identity, partial state repair, and idempotence, plus Android Keystore instrumentation coverage.
Validation status
I added focused JVM and Android Keystore instrumentation tests, but I have not been able to compile or execute them locally because the required Flow GitHub Packages dependencies return 401 Unauthorized. This repository does not appear to run Android compilation or tests on pull requests, so the rebuilt branch still needs validation in an authenticated maintainer environment before merge.
Limitation
Recovery is intentionally limited to accounts whose Flow addresses remain available in local account state. It does not perform global public-key-to-address discovery. Ambiguous cases where multiple Keystore aliases could restore the same account are preserved for retry/manual handling rather than guessed automatically.