Update / indexed db account ops integration - #2513
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
PetromirDev
left a comment
There was a problem hiding this comment.
I love the idea of using indexeddb but don't think that investing much time into a POC is that viable. IMO you cannot compare inserting a single account op without serializing it to serializing up to 1000 ops per network for each account and then persisting that with chrome.storage.local. Let's have the three of us discuss this (@gergana95, @superKalo and me) and ship this. I have some considerations with the current state of the POC, mainly about migrations and not having an abstraction over storage. I won't share them yet because I know that this is a POC but want to discuss them and proceed with this one quicker as I think that it will be a great improvement.
|
/review |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PetromirDev
left a comment
There was a problem hiding this comment.
Also, please update the description of the PR
| // The 'phishing' store holds a single document keyed by this constant. | ||
| // All reads and writes target this one record. | ||
| // | ||
| // NOTE: the store is deliberately NOT in AMBIRE_IDB_SCHEMA. Adding it would require a | ||
| // dbVersion bump, and a shipped bump cannot be rolled back — so it is not worth | ||
| // carrying for a store nothing reads yet. Add it in the same change that wires | ||
| // PhishingController to this backend, not before. | ||
| const STORE_NAME = 'phishing' |
There was a problem hiding this comment.
"Adding it would require a
// dbVersion bump, and a shipped bump cannot be rolled back" - it is not shipped yet so I see no reason to not add it there
Issue is https://github.com/AmbireTech/ambire-app/issues/7264
Related ambire-app PR https://github.com/AmbireTech/ambire-app/pull/7697
Move transaction history to IndexedDB
Makes IndexedDB the source of truth for
accountsOps(transaction history), with aone-time migration from the legacy
chrome.storage.localblob.Why
accountsOpswas stored as a single key-value blob, so every new transaction rewrote theentire history. That cost grows without bound — a heavy account is tens of MB re-serialised
on every write, and the whole blob is read on every service-worker wake-up.
IndexedDB gives row-level writes and a bounded startup read: pending ops in full, plus the
20 most recent finalized ops per (account, chain). Older history is lazy-loaded on demand.
Mobile is unaffected — it has no IndexedDB and keeps using the key-value backend.
What reviewers should focus on
In rough order of "if this is wrong, it matters most":
ActivityController.#load()— migration must complete before the first read, and#load()must never reject. It is launched from the constructor and assigned to#initialLoadPromise, which every public method awaits; a rejection there breaks thecontroller for the whole session and raises an unhandled rejection.
#mergeOpsIntoCache— the lazy-load merges by id and keeps the existing objecton a collision. Replacing the array would drop ops that are only in memory and detach
objects that
updateAccountsOpsStatusesmutates in place across long provider awaits.addAccountOp— persistence comes last, afteremitUpdate(). On thekey-value backend
putSingleOprewrites the whole blob, so awaiting it earlier blocksthe UI on a full serialisation. There is a comment saying not to "fix" this; please leave
it alone.
#fallBackToKeyValueForThisSession()— if the migration fails, the session must readand write the legacy blob. Reading the blob while still writing to IndexedDB puts one
row in the empty store, which makes the
isEmpty()guard skip the retry forever.idbDatabase.ts). To transform rows you mustchain off the read, not
awaitit — awaiting a non-IDB promise lets the versionchangetransaction commit and the writes vanish silently. Verified in Chrome and Firefox
against 14,000 rows. There is a comment warning against tidying it into an
await.getTotalOpsCountForAccount()existsbecause
BannerControllerevaluatesminTxnsTotal/maxTxnsTotalin a synchronouscallback that cannot query IndexedDB, and the in-memory lengths are only the startup
window. On the key-value backend the count is summed live instead — caching there could
only ever be staler, since that backend's in-memory blob is the whole history. The
cache is refreshed by recount, not by
+1on write, becauseputSingleOpcan evict arow on its own and make the net change zero.
Design decisions worth knowing
AMBIRE_IDB_SCHEMAis the single source of truth;reconcileSchema()creates any missing store or index on every upgrade, idempotently. Soa purely additive change needs only a manifest entry plus a
dbVersionbump. Migrationhandlers exist for transforming existing rows, not for creating structure.
wrong. It is frozen at migration time, so it is never a source of truth. Open question
for this PR: when do we delete it? Keeping it forever means every migrated user stores
the history twice.
activityIdbMigratedrecords that history lives in IndexedDB. Nothing reads ittoday — it is written (
#recordHistoryLivesInIdb) because it can only be recorded whileIndexedDB is working, and a session that cannot open IndexedDB can no longer distinguish
"this user never had transactions" from "this user's history is in IndexedDB and
unreachable". Keeping the writer means the marker is in place for whatever consumes it
later. Deliberately accessed via narrow casts rather than added to
StorageProps, becauseit is provisional. Reviewers: flag this if you would rather not carry a write-only key.
dbVersionbump is effectively unrollbackable. A build pinned to the old versioncannot open an upgraded database.
dbVersionis 1 here — thephishingstore wasprepared and then intentionally left out of the manifest, so this PR ships a single
version. Add it in the same change that wires
PhishingController.Manual QA
The whole checklist below has been run and passes — on Chrome, and B1/B2 on Firefox too.
The checkboxes are left unticked on purpose so it stays usable as a fresh checklist for
anyone re-verifying. The table here is the record of what was actually covered.
Status — all green
blocking()closes,terminated()drops the cache,#openTxreopens a dead handlePART A — Setup (required)
yarn setupfrom the repo root.npm installinsidesrc/ambire-commonbefore building. The Jest suiteneeds that nested
node_modules, but the LavaMoat build fails on it(unknown package directory for
@babel/runtime) — which is exactly whysetup.shdeletes it. Tests and builds want opposite states; run
yarn setupagain after testing.yarn build:web:webkitbuild/webkit-prod/manifest.jsonexists.cannot open a v3 database (
VersionError) and would silently fall back tokey-value storage.
v0 → v1(not v3), noVersionError.that sends a transaction.
PART B — Fixes you can verify in a browser
B1 — A broadcast transaction is no longer dropped from memory ⭐ most important
This is the bug that could stop a transaction being polled to confirmation. It needed
an active chain-filtered session and a group inside the 20-op startup window.
This is what creates the filtered session that used to destroy the new op.
pending, and progresses to confirmed on its own.
Fail: it appears briefly then vanishes, or stays pending forever — that is the
old bug (in-memory op discarded → never polled).
B2 — Pending ops no longer block the lazy-load
The old gate was
inMemoryCount > 20. Pending ops are exempt from the 20-op cap, so agroup could arrive at 25 and be mistaken for "already fully expanded", freezing
pagination at 25 of 35.
fin-29)and check the total/page count reflects 35.
Fail: only ~25 visible with no further pages — the old heuristic.
always loaded in full).
B3 — The migration-completion flag is recorded for users with no legacy blob
Previously the flag was only set after migrating a legacy blob, so it was never recorded for
anyone who installed after IDB became the default — the one moment it can be written is
while IDB still works, and those users skipped it entirely.
No transaction needed. Ops are written straight into IDB below, which is exactly the
state a post-IDB user reaches naturally. Note that clearing
accountsOpsdoes notremove your account — only the transaction history.
B3a — A genuinely empty wallet must NOT arm the flag
(unset). With no history there is nothing to warn about, so a brand-newwallet must not be told it lost something.
B3b — Ops in IDB (no legacy blob) DO arm the flag
true. History now lives in IDB, so the net is armed.(unset)—#recordHistoryLivesInIdbis not running.B3c — IDB unavailable degrades to the legacy blob without breaking
There is deliberately no banner and no error toast for this state. The condition is
detected inside
#load(), i.e. on service-worker startup, where errors only reach the UIthrough a live port — no window is open, so a toast is emitted into nothing. A banner was
implemented and then removed: the user cannot act on it, and the retained legacy blob means
history still renders. What must hold is that the fallback is quiet and functional.
To make IDB genuinely unavailable, put the database at a version the build cannot open —
deleting it is not enough, that leaves IDB usable and merely empty.
no error banner, no error toast, no
[Emitted error…]line in the console.ActivityControllernever resolving — that would mean#load()rejected.both reads and writes, not a mixed state).
open it, so the extension stays on the key-value fallback until you remove it:
[AmbireIdb] Upgrading "ambire" v0 → v1.B4 — Removing an account clears its expansion markers
A stale marker would make a re-added account look already-expanded and permanently
show only the startup window.
filter to that chain, and page to the end so the group is fully expanded.
Fail: only ~20 visible with no further pages — a stale marker.
B5 — Nothing regressed
poisoning warning.
InvalidStateErrororVersionErrorin the SW console.B6 — Firefox
yarn build:web:gecko, load viaabout:debugging.PART C — Fixes that are NOT browser-reachable
Each needs a specific call to fail on demand, which cannot be induced from the console
without patching the build. Listed so the gap is explicit.
migrateFromStorageto throwa failed migration keeps writes out of IDB…#loadnever rejects (post-load guard)storage.get('activityIdbMigrated')to throwinit survives the post-load history checks throwingloadStartupOpsto throwa failed startup read does not permanently mark…updateOpsaborts on a malformed optimestampreachingupdateOpsa malformed op aborts the batch…putSingleOpto fail while the op stays in memoryan op that failed to persist still survives…All five were verified by removing the fix and confirming the test fails, so they are
not resting on a green-suite assumption.
Automated tests
193 tests, 7 suites — 167 of them new in this PR.
activityIdb.test.tsidbIntegration.test.tsreconcileSchema,applyMigrations, manifest↔handler consistency guards, handler row-transformationactivityIdbMigration.test.ts#load()wiring: migration ordering, legacy retention, the flag, fallback, expansion, method interactionsphishingIdb.test.tsidbDatabase.test.tsblocking()activity.test.tsexternalAccountOps.test.tsactivity.test.tsis worth calling out: it constructs the controller without anidbargument, so every one of its 20 tests runs the key-value backend. It passing unchanged is
the main evidence that mobile behaviour is untouched.
Every new test was sabotage-verified
Each fix was removed individually to confirm the intended test fails. This caught four
tests that passed with the fix deleted — each because the test setup happened to satisfy
the precondition the test depended on, so the scenario never reached the code under test. One
example: a test meant to prove the startup window is respected used a load helper that
expanded the full history as a side effect.
A green suite gives no signal about that, so please apply the same check to anything added on
top of this.
One test is knowingly not sabotage-isolable and says so in a comment: "the mobile count
reflects a newly added op". It passes with its guard removed, because a second gate upstream
makes the guarded branch unreachable on that backend. It is kept as a behavioural pin rather
than deleted or dressed up as verified.
Known limitations, deliberately not fixed
None of these are known-broken behaviour — they are accepted trade-offs, each documented at
the code that implements it.
isEmpty()migration guard. It cannot distinguish "never migrated" from "migrated,wiped, then partially repopulated". If IndexedDB is wiped while running and anything is
written before the next restart, that row makes the guard skip and the legacy copy is
never read again. Wipe → restart with no write in between does recover, which is the
realistic path — the extension has
unlimitedStorage, so routine eviction is not afactor. Documented at
activityIdb.tsensureMigrated()and incontrollers/AGENTS.md.activityIdbMigratedis write-only. Nothing reads it. It is written because it canonly be recorded while IndexedDB works — a session that cannot open IndexedDB can no
longer tell "never had transactions" from "history is in IndexedDB and unreachable", and
by then it is too late to write the flag that would have distinguished them. Keeping the
writer means the marker is in place for whatever consumes it later.
Reviewers: flag this if you would rather not carry a write-only key.
hasAccountOpsSentTostill expands full history. It answers "have I sent here?" andthe address-poisoning lookalike check, both properties of the whole history, so on a miss
it loads everything into memory — for every account when
accountIdis empty. The fastpath (
sentToHistory.recipients) is only populated byaddAccountOp, and there is nobackfill from existing history, so for users with pre-existing history the expensive
path runs on recipients they have used many times before. This is the largest remaining
optimization; see
services/storage/README.mdfor the full write-up and the fix.#refreshTotalOpsCountruns afteremitUpdate()inaddAccountOp(persistence must come last), so a banner gated onminTxnsTotalsees the new total on the next update. Refreshing earlier would block theUI on an IndexedDB read; emitting a second update would re-serialize the whole controller
state per broadcast. The thresholds are coarse buckets.
aggregate view of how many users land on the fallback. The right place is where
openAmbireIdb()fails inbackground.ts, which currently only logs to console.MAX_IDB_GROUP_SIZEcannot converge downward —putSingleOpadds one row anddeletes at most one, so an oversized group holds steady rather than shrinking.
Unreachable today because the in-memory
trim()keeps groups at ≤1000.Suggested follow-ups
migration has proven itself over a release).
sentToHistory.recipientsfrom full history at migration time, then drop#ensureFullHistoryLoaded()from the address-poisoning path — this removes a mechanismrather than adding one. Deliberately not in this PR: it changes security-relevant
behaviour and deserves its own review.
PhishingControllertoPhishingIdbStorageand add the store to the manifest (inone change — the
dbVersionbump is unrollbackable).Rollout note
There is no feature flag: the read-path flip lands for every existing user at once. The
retained legacy blob makes it recoverable, but recovery means shipping again. Worth deciding
whether to gate it or stage the rollout.