mirror of
https://github.com/vitorpamplona/amethyst.git
synced 2026-10-06 03:38:23 +00:00
Claude
e8af62717e
A review pass over the full range turned up eleven things. The two that lose user data: **A deleted account came back.** deleteAccount cleared the legacy file, the key store and the secrets store, but never the account's plain DataStore — deleteUserPreferenceFile sweeps shared_prefs/ and that store lives in filesDir/datastore/. AccountPreferenceStores.removeAccount existed and had no caller. Before this branch the orphan was merely litter, because nothing read it; now it holds nostr_pubkey, so re-adding the same npub found a live identity, setDefaultAccount's downgrade guard saw a writeable account and restored the one the user had just deleted. Everything else in there — cached contact and mute lists, 31 follow-list filters, dismissed polls — also stayed on disk unencrypted for good. **The notifications Global -> Selected migration stamped itself done and threw the result away.** It wrote the corrected filter to the legacy DEFAULT_NOTIFICATION_FOLLOW_LIST, which nothing reads once the follow-list copy marker is set, while the stamp went somewhere that persists. End a session with no save and the next launch skips the migration and reads Global back out of the DataStore — the account stays on raw Global notifications permanently. It now writes through followListStore and stamps only after that write succeeds. Two more that would have bitten later: - The SAVED_ACCOUNTS upgrade branch wrote ALL_ACCOUNT_INFO to the legacy file without mirroring it into the roster store, whose own copy had already run against a key that did not exist yet and whose marker was already set. Those installs would open as a fresh install the moment the legacy write goes — the failure AccountRoster's KDoc calls the most consequential to get wrong. - deletePrivateKey gated its removal on a decrypting read, so a rotated or wiped keystore — exactly when the value is unreadable — skipped the delete and left a deleted account's key on disk. It now tests presence without decrypting, via a new EncryptedDataStore.contains. Two crashes from DataStore's one-store-per-path registry, which only releases on scope cancellation: desktopChessDismissedGamesStore() and Android's SecureKeyStorage both built a store per call over a fixed path. The chess one is reachable today — the view model builds one in its constructor under a remember(account), so reopening that screen threw from an unhandled scope.launch and took the screen's scope down with it. Both are now one store per process. EncryptedSharedPreferences.create was idempotent, which is why neither needed this before. Chess dismissals were fire-and-forget saves of a read-modify-write snapshot, so two in quick succession could land out of order and drop one, and a dismissal racing the async seed wrote a snapshot missing everything already stored. They now go through a single conflated channel with one consumer that writes the current set, and the seed asks for a write when it finds it unioned into a set someone had already persisted. Cost, on paths that run constantly: - saveSecrets did ten separate encrypted-file rewrites per account save, none of which DataStore could skip since AES-GCM re-randomises the IV so the ciphertext differs even when the value does not. One edit now — which also makes the marker mean what LegacyPreferenceCleanup reads it as, since it can no longer exist without the values beside it. loadSecrets likewise reads one snapshot instead of ten flow collections. - SecretEncryption was default-constructed per store: eight AndroidKeyStore loads and eight per-thread Cipher caches for one key alias. One shared instance; both actuals document concurrent use. - updateSavedAccounts compared a MutableStateFlow to a List, so the guard was unconditionally true and every call rewrote both stores. Pre-existing, but this branch put an encrypt and an encrypted-store write behind it. - The cleanup ran inside the mutex that serialises every account load, so once enabled each account on a multi-account cold start would wait for the previous one's full pass. It now runs outside the lock, once, on the call that did the loading. And one design flaw in the new cleanup itself: it compared the stored secrets against the legacy file, justified by their being dual-written — but the check only runs once LEGACY_WRITES_RETIRED turns that off, from which point the legacy copy is frozen. Any account that re-paired a bunker after upgrading would have differed forever and never had its file deleted. Secrets are now gated on the migration marker, like the plain groups. The private-key comparison stays: an npub is derived from its key, so that one cannot legitimately change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AXvKXakvup4inNFfAhhr4L