Reduce UI lag: gate redundant Monero callbacks#102
Reduce UI lag: gate redundant Monero callbacks#102paullinator wants to merge 3 commits intomasterfrom
Conversation
The onSubscribeAddresses property was removed from EdgeCurrencyEngineCallbacks but remained in the test mock, causing a TypeScript compilation error.
MoneroEngine called onTransactions unconditionally, generating ~70 Redux dispatches and bridge messages per 10 seconds for nothing. Now gates the call on a non-empty array.
Monero fired onSeenTxCheckpoint 5-6 times per 10 seconds with identical checkpoint values. Comparing the new checkpoint to the previous before calling eliminates redundant emissions. Also throttles onAddressesChecked to a maximum of one emission per 500ms (ratio=1 always passes through) to cap aggregate sync-status volume.
eddy-edge
left a comment
eddy-edge
left a comment
There was a problem hiding this comment.
Additional Findings
- warning
test/engine/engine.ts:51: No tests were added for the three new behavioral changes: (1) 500ms throttle ononAddressesChecked, (2) empty-array guard before callingonTransactions, and (3)onSeenTxCheckpointgating on checkpoint advancement. The existing tests are almost entirely skipped integration tests that don't exercise these code paths.- Add unit tests for
updateOnAddressesCheckedandprocessMoneroTransaction. For the throttle, mockDate.nowto verify calls within 500ms are suppressed. For the checkpoint gate, verifyonSeenTxCheckpointis only called when the checkpoint changes.
- Add unit tests for
| const PRIMARY_CURRENCY_TOKEN_ID = null | ||
|
|
||
| // Global throttle: max 1 onAddressesChecked per 500ms; ratio=1 always passes. | ||
| let lastSyncEmitTime = 0 |
There was a problem hiding this comment.
Issue: lastSyncEmitTime is a module-level global shared across all MoneroEngine instances. If a user has multiple Monero wallets syncing concurrently, one wallet's throttle will suppress progress callbacks for another wallet, causing its sync progress bar to appear stuck.
Recommendation: Move lastSyncEmitTime to an instance property on MoneroEngine (e.g. private lastSyncEmitTime = 0 in the class body) so each wallet throttles independently.
| if (numTx !== totalTxs) { | ||
| const now = Date.now() | ||
| if (now - lastSyncEmitTime < 500) return | ||
| lastSyncEmitTime = now |
There was a problem hiding this comment.
Warning: The throttle drops progress callbacks entirely with no trailing-edge emit. The UI may jump from e.g. 30% directly to 100% if the last sub-100% update is suppressed. For wallets with many transactions where progress updates happen rapidly, intermediate progress values are silently discarded.
Recommendation: Consider a trailing-edge throttle (schedule a deferred emit when one is suppressed) so the most recent progress value is always delivered before the final 100% callback.
| } | ||
| if (numTx !== totalTxs) { | ||
| const now = Date.now() | ||
| if (now - lastSyncEmitTime < 500) return |
There was a problem hiding this comment.
Warning: The 500ms throttle interval is a magic number. It's not discoverable at the top of the file alongside the other interval constants (SYNC_INTERVAL_MILLISECONDS, SAVE_DATASTORE_MILLISECONDS).
Recommendation: Extract to a named constant: const SYNC_PROGRESS_THROTTLE_MS = 500 next to the other interval constants at the top of the file.
| this.saveTransactionState(PRIMARY_CURRENCY_TOKEN_ID, edgeTransaction) | ||
| this.edgeTxLibCallbacks.onTransactions(this.transactionEventArray) | ||
| this.transactionEventArray = [] | ||
| if (this.transactionEventArray.length > 0) { | ||
| this.edgeTxLibCallbacks.onTransactions(this.transactionEventArray) | ||
| this.transactionEventArray = [] | ||
| } |
There was a problem hiding this comment.
Warning: processMoneroTransaction fires onTransactions after every single transaction that produces events. The empty-array guard helps avoid no-op calls, but still fires one callback per new/updated transaction rather than batching. Since this PR aims to reduce UI lag, batching would be more impactful.
Recommendation: Consider moving the onTransactions flush out of processMoneroTransaction and into checkTransactionsInnerLoop after the for-loop (or every N iterations), to batch transaction events and reduce callback frequency.
| const newCheckpoint = wasSeenTxCheckpoint(seenTxCheckpoint) | ||
| const oldCheckpoint = wasSeenTxCheckpoint(this.seenTxCheckpoint) |
There was a problem hiding this comment.
Suggestion: The checkpoint comparison serializes both values via wasSeenTxCheckpoint (number→string) before comparing. Comparing the raw numeric values (seenTxCheckpoint !== this.seenTxCheckpoint) would be clearer and avoids unnecessary serialization.
Recommendation: Compare seenTxCheckpoint !== (this.seenTxCheckpoint ?? 0) directly, then call wasSeenTxCheckpoint only when emitting the callback.
|
|
||
| const PRIMARY_CURRENCY_TOKEN_ID = null | ||
|
|
||
| // Global throttle: max 1 onAddressesChecked per 500ms; ratio=1 always passes. |
There was a problem hiding this comment.
Suggestion: The comment 'ratio=1 always passes' uses jargon that doesn't map to the code's variable names (numTx/totalTxs). It takes a moment to understand this means the final 100% callback bypasses the throttle.
Recommendation: Reword to: // Global throttle: max 1 onAddressesChecked call per 500ms for in-progress syncs. The final 100% callback is never throttled.
| this.edgeTxLibCallbacks.onSeenTxCheckpoint( | ||
| wasSeenTxCheckpoint(this.seenTxCheckpoint) | ||
| ) | ||
| // Only update the seenTxCheckpoint if it actually advanced: |
There was a problem hiding this comment.
Suggestion: The comment says 'if it actually advanced' but the comparison checks for any change (!==), not strictly advancement (increase). If seenTxCheckpoint could decrease (e.g. on a reorg), the comment would be misleading.
Recommendation: Change to: // Only update the seenTxCheckpoint if it actually changed:
| const _bal = this.walletLocalData.totalBalances.get(tokenId) ?? '0' | ||
| this.edgeTxLibCallbacks.onTokenBalanceChanged(tokenId, _bal) |
There was a problem hiding this comment.
Suggestion: The _bal variable uses an underscore prefix, which conventionally signals an unused or private member. This is a normal local variable. Additionally, this is a cosmetic-only change unrelated to fixing UI lag.
Recommendation: Rename to balance or bal. Consider dropping this change from this PR or moving it to a separate cleanup commit.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| const PRIMARY_CURRENCY_TOKEN_ID = null | ||
|
|
||
| // Global throttle: max 1 onAddressesChecked per 500ms; ratio=1 always passes. | ||
| let lastSyncEmitTime = 0 |
There was a problem hiding this comment.
Global throttle variable shared across all engine instances
Medium Severity
lastSyncEmitTime is a module-level variable shared across all MoneroEngine instances. When multiple Monero wallets sync concurrently, one wallet's onAddressesChecked emission resets the shared timer, starving progress callbacks for other wallets. This needs to be an instance property on MoneroEngine rather than a module-scoped let.
Additional Locations (1)
|
Bugbot Autofix prepared fixes for 1 of the 1 bugs found in the latest run.
Or push these changes by commenting: Preview (f5769749fb)diff --git a/src/MoneroEngine.ts b/src/MoneroEngine.ts
--- a/src/MoneroEngine.ts
+++ b/src/MoneroEngine.ts
@@ -66,9 +66,6 @@
const PRIMARY_CURRENCY_TOKEN_ID = null
-// Global throttle: max 1 onAddressesChecked per 500ms; ratio=1 always passes.
-let lastSyncEmitTime = 0
-
export class MoneroEngine implements EdgeCurrencyEngine {
apiKey: string
walletInfo: SafeWalletInfo
@@ -91,6 +88,7 @@
engineFetch: EdgeFetchFunction
seenTxCheckpoint: number | undefined
loginPromise: Promise<void> | null = null
+ lastSyncEmitTime: number = 0
constructor(
env: EdgeCorePluginOptions,
@@ -173,8 +171,8 @@
}
if (numTx !== totalTxs) {
const now = Date.now()
- if (now - lastSyncEmitTime < 500) return
- lastSyncEmitTime = now
+ if (now - this.lastSyncEmitTime < 500) return
+ this.lastSyncEmitTime = now
const progress = numTx / totalTxs
this.edgeTxLibCallbacks.onAddressesChecked(progress)
} else { |
2 participants
CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
none
Description
Reduces RN JS thread starvation from Monero engine callbacks. Three fixes:
onSubscribeAddressesfrom test mock: Fixes a pre-existing TypeScript error intest/engine/engine.tswhere the mockEdgeCurrencyEngineCallbacksstill included a property removed from the type.onTransactionsis now only called whentransactionEventArrayis non-empty, preventing no-op bridge crossings on quiet blocks.onSeenTxCheckpoint+ throttleonAddressesChecked:onSeenTxCheckpointis only emitted when the checkpoint value actually advances.onAddressesCheckedis globally rate-limited to 1 emission per 500ms (sync-complete always passes).Note
Low Risk
Behavior changes are limited to callback frequency/conditions (no transaction creation/broadcast logic changes), but could affect UI sync progress and downstream listeners that implicitly relied on extra emissions.
Overview
Reduces React Native bridge/JS-thread pressure from Monero sync by gating redundant engine callbacks.
MoneroEnginenow throttlesonAddressesCheckedprogress emissions to at most once every 500ms (while still always emitting the final1), only callsonTransactionswhen there are actual transaction events to deliver, and only emitsonSeenTxCheckpointwhen the checkpoint value advances. Tests update theEdgeCurrencyEngineCallbacksmock by removing the staleonSubscribeAddressescallback to match the current type.Written by Cursor Bugbot for commit f6ffd9a. This will update automatically on new commits. Configure here.