Reduce UI lag: gate redundant callbacks in accountbased engines#1033
Reduce UI lag: gate redundant callbacks in accountbased engines#1033paullinator wants to merge 5 commits intomasterfrom
Conversation
Accountbased engines called onBlockHeightChanged on every new block (polygon ~1/sec, fantom ~1/sec, etc.), which forced core-js to iterate ALL transactions to recompute confirmations. By setting confirmations directly on transactions via updateConfirmations and emitting them through onTransactions, we eliminate ~130 expensive callbacks per 10s.
EVM engines called onNewTokens(detectedTokenIds) on every balance-check cycle — ~25 times per 10 seconds — even when the token list hadn't changed. A sorted-array comparison against the last-reported set eliminates all redundant calls. Adds reportDetectedTokens() to the base class with a per-wallet detectedTokenIds cache (persisted to walletLocalData) and updates all engine callsites to use the new method.
Tron and FIO engines called onStakingStatusChanged on every poll cycle (1-3 per 10s) with identical staking data. A JSON.stringify comparison against the previous status in reportStakingStatus() eliminates redundant dispatches.
Engines that were already fully synced (ratio=1.0) kept firing onSyncStatusChanged hundreds of times. Solana alone produced 129 calls per 10 seconds at steady state. sendSyncStatusIfChanged suppresses callbacks when the ratio hasn't changed since the last emission.
Even with per-engine dedup, 30+ accountbased engines syncing simultaneously produced ~42 onSyncStatusChanged calls per 10 seconds. A global rate limiter (max 1 per 500ms, totalRatio=1 always passes) limits aggregate volume during the post-login sync burst.
eddy-edge
left a comment
eddy-edge
left a comment
There was a problem hiding this comment.
Additional Findings
- warning
src/common/CurrencyEngine.ts:735: Removal ofonBlockHeightChangedcallback. The old code dispatchedonBlockHeightChanged(blockHeight)which core-js uses to updatewalletState.heightin redux. This code no longer calls it, meaningwalletState.heightin core-js will never update for this plugin. Consumers readingEdgeCurrencyWallet.blockHeightwill see a stale value. The@deprecatedannotation in core-js types suggests a replacement is planned but may not be in place yet.- Verify that edge-core-js has an alternative path to learn the block height (e.g. via
onSyncStatusChanged). If not, continue callingonBlockHeightChangedfor the height update even if confirmation handling is done engine-side. Check the minimum core-js version this plugin targets.
- Verify that edge-core-js has an alternative path to learn the block height (e.g. via
- warning
src/zano/ZanoEngine.ts:89: IfdeleteWalletfails, the error is only logged andneedsNativeStorageClearis already reset tofalse(line 90). The resync proceeds with stale wallet files, silently defeating the purpose ofresyncBlockchain.- Consider leaving
needsNativeStorageClear = trueuntil deletion succeeds (move the reset to afterdeleteWalletreturns), or re-throw the error so the lifecycle manager'sonErrorpath is triggered.
- Consider leaving
- warning
CHANGELOG.md:3: The## Unreleasedsection is empty, but this PR contains significant performance improvements (SyncTracker 500ms throttle, staking status dedup, token detection dedup, removal of onBlockHeightChanged) that are not documented in any changelog entry. These are the core changes implied by the branch namepaul/fixUiLag.- Add changelog entries documenting the performance improvements, e.g.:
fixed: Throttle sync status updates to reduce UI re-rendersfixed: Gate staking status callbacks to only fire on actual changesfixed: Deduplicate detected token reportschanged: Remove deprecated onBlockHeightChanged callback- warning
CHANGELOG.md:22: Grammar:Use correct Alchemy URL's— the apostrophe is incorrect for a plural.- Change to
Use correct Alchemy URLs.
- Change to
- suggestion
CHANGELOG.md:5: Four version bumps (4.74.2, 4.75.0, 4.75.1, 4.75.2) are batched into a single PR.- Document in the PR description why these versions are batched, if this is a changelog catch-up.
- suggestion
CHANGELOG.md:5: The removal ofonBlockHeightChangedin favor of engine-side confirmation handling is a behavioral/API change that warrants at least achanged:changelog entry.- Add a
changed:entry and consider whether a minor version bump is appropriate for semver compliance.
- Add a
| lastSyncStatus = currentStatus | ||
|
|
||
| if (currentStatus.totalRatio !== 1) { | ||
| const now = Date.now() | ||
| if (now - ssLastEmitTime < 500) return | ||
| ssLastEmitTime = now | ||
| } |
There was a problem hiding this comment.
Warning: Silent data loss from throttle ordering: lastSyncStatus is updated (line 79) before the throttle check (lines 81-84). If the call is throttled via return, the status is recorded but never sent. Subsequent calls with the same ratio will be deduplicated against the unsent value and also never sent. Example: ratio goes 0.3→0.5 (throttled, saved but not sent) → 0.5 again (deduped, never sent). The UI would remain stuck at 0.3.
Recommendation: Move lastSyncStatus = currentStatus to just before engine.sendSyncStatus(currentStatus) (after the throttle gate), so only actually-emitted statuses are recorded for deduplication.
| import type { EdgeSyncStatus, EdgeTokenId } from 'edge-core-js/types' | ||
|
|
||
| // Global throttle: max 1 sendSyncStatus per 500ms; totalRatio=1 always passes. | ||
| let ssLastEmitTime = 0 |
There was a problem hiding this comment.
Warning: Global throttle shared across all wallet engines. ssLastEmitTime is module-level, so if wallet A emits at T=0, wallet B is blocked until T=500ms. This couples independent wallets' sync status reporting and can cause stale progress bars during multi-wallet sync (e.g. initial login with many wallets).
Recommendation: Move ssLastEmitTime into the per-tracker closure (next to lastSyncStatus on line 49), so each wallet engine has its own independent 500ms throttle. If a global cross-wallet limit is truly desired, document it explicitly.
| this.currencyEngineCallbacks.onStakingStatusChanged(status) | ||
| } | ||
|
|
||
| reportDetectedTokens(tokenIds: string[]): void { |
There was a problem hiding this comment.
Warning: reportDetectedTokens has implicit public visibility, while the adjacent reportStakingStatus (line 662) is protected. Inconsistent access modifiers.
Recommendation: Mark reportDetectedTokens as public explicitly (since EthereumNetwork.ts accesses it from outside the class hierarchy).
| return { totalRatio } | ||
| } | ||
|
|
||
| function sendSyncStatusIfChanged(): void { |
There was a problem hiding this comment.
Warning: No tests cover the new sendSyncStatusIfChanged deduplication logic or the 500ms global throttle. The module-level ssLastEmitTime variable persists across test cases and may make existing SyncTracker tests flaky when they run in rapid succession.
Recommendation: Add test cases with fake timers covering: (1) duplicate ratios are suppressed, (2) totalRatio=1 bypasses the throttle, (3) rapid non-1 updates within 500ms are throttled. Export a test-only reset function for ssLastEmitTime or make the clock injectable.
| reportDetectedTokens(tokenIds: string[]): void { | ||
| const known = this.walletLocalData.detectedTokenIds | ||
| const newTokenIds = tokenIds.filter(id => known[id] == null) | ||
| if (newTokenIds.length === 0) return | ||
| for (const id of newTokenIds) known[id] = true | ||
| this.walletLocalDataDirty = true | ||
| this.currencyEngineCallbacks.onNewTokens(Object.keys(known)) | ||
| } |
There was a problem hiding this comment.
Warning: No test coverage for reportDetectedTokens deduplication logic (only truly new token IDs trigger onNewTokens, repeated calls are no-ops, cumulative set is always emitted).
Recommendation: Add unit tests covering deduplication, cumulative emission, and walletLocalDataDirty flag.
| private lastStakingStatusJson: string = '' | ||
|
|
||
| protected reportStakingStatus(status: EdgeStakingStatus): void { | ||
| const json = JSON.stringify(status) |
There was a problem hiding this comment.
Suggestion: JSON.stringify for reportStakingStatus comparison is technically sensitive to key ordering. While V8 uses insertion order, the spec doesn't fully guarantee it across all JS engines (e.g. Hermes, JSC in React Native).
Recommendation: Consider a shallow-comparison helper or fast-deep-equal (if in the dependency tree) for more robust change detection.
| asObject(asNumber), | ||
| () => ({}) | ||
| ), | ||
| detectedTokenIds: asMaybe(asObject(asBoolean), () => ({})), |
There was a problem hiding this comment.
Suggestion: asObject(asBoolean) works but is semantically misleading. Values are only ever set to true; the boolean value itself is never inspected (lookup checks known[id] == null). This is effectively a Set<string> serialized as a record.
Recommendation: Consider using asObject(asTrue) from cleaners to narrow the type, or document that values are always true. Minor style concern — not a correctness issue.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| const now = Date.now() | ||
| if (now - ssLastEmitTime < 500) return | ||
| ssLastEmitTime = now | ||
| } |
There was a problem hiding this comment.
Throttled sync status updates are permanently lost
Medium Severity
lastSyncStatus is assigned at line 79 before the global throttle check at line 83. When the throttle triggers an early return, the dedup state records the status as "already sent" even though engine.sendSyncStatus was never called. On subsequent calls with the same totalRatio, the dedup guard at line 77 filters it out, so the value is permanently dropped. Moving lastSyncStatus = currentStatus to just before engine.sendSyncStatus(currentStatus) would ensure throttled values are retried on the next invocation.
|
Bugbot Autofix prepared fixes for 1 of the 1 bugs found in the latest run.
Or push these changes by commenting: Preview (26e9ea7da9)diff --git a/src/common/SyncTracker.ts b/src/common/SyncTracker.ts
--- a/src/common/SyncTracker.ts
+++ b/src/common/SyncTracker.ts
@@ -76,14 +76,13 @@
lastSyncStatus == null ||
lastSyncStatus.totalRatio !== currentStatus.totalRatio
) {
- lastSyncStatus = currentStatus
-
if (currentStatus.totalRatio !== 1) {
const now = Date.now()
if (now - ssLastEmitTime < 500) return
ssLastEmitTime = now
}
+ lastSyncStatus = currentStatus
engine.sendSyncStatus(currentStatus)
}
} |
2 participants
CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
none
Description
Reduces RN JS thread starvation by eliminating redundant callbacks from accountbased currency engines. Five targeted fixes:
onBlockHeightChanged: EVM/accountbased engines no longer callonBlockHeightChanged, removing ~130 expensive core-js callbacks per 10s (polygon/fantom fire ~1/sec). Confirmations are now updated directly on transactions and emitted viaonTransactions.onNewTokensviareportDetectedTokens: Adds adetectedTokenIdscache (persisted towalletLocalData) in the base class. Engines calledonNewTokenson every poll cycle even when the token list hadn't changed. Only fires when genuinely new tokens are discovered.onStakingStatusChangedviareportStakingStatus: Deduplicates staking status callbacks using JSON.stringify comparison. Tron and FIO engines were firing on every poll cycle with identical data.SyncTrackerper-engine dedup: AddssendSyncStatusIfChangedto suppressonSyncStatusChangedwhen thetotalRatiohasn't changed. Solana alone produced 129 redundant calls per 10s at steady state.sendSyncStatus: Limits aggregate sync status volume to 2/sec across all 30+ accountbased engines during the post-login burst.Note
Medium Risk
Touches shared engine plumbing and callback emission semantics across many chains, so regressions could affect token discovery, staking UI updates, or confirmation/sync progress reporting despite being performance-focused.
Overview
Reduces JS-thread/UI lag by deduplicating high-volume callbacks emitted by account-based currency engines.
Adds base
CurrencyEnginehelpersreportDetectedTokens(with persistedwalletLocalData.detectedTokenIds) andreportStakingStatus(JSON-based dedupe), and migrates multiple engines (Algorand, Cosmos, Ripple, Solana, Sui, Tron, Zano, andEthereumNetwork) to use them instead of firingonNewTokens/onStakingStatusChangedrepeatedly.Stops emitting the deprecated
onBlockHeightChangedcallback;updateBlockHeightnow updates in-memory tx confirmations and emits changes viaonTransactions.SyncTrackernow suppresses unchangedonSyncStatusChangedupdates and applies a global 500ms throttle for non-totalRatio=1statuses.Written by Cursor Bugbot for commit 93692e0. This will update automatically on new commits. Configure here.