Skip to content

Reduce UI lag: gate redundant Monero callbacks #102

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
paullinator wants to merge 3 commits into
base: master
Choose a base branch
from
+19 −12
Open

Reduce UI lag: gate redundant Monero callbacks #102

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
47426d8
Remove stale onSubscribeAddresses from test callbacks
paullinator Feb 25, 2026
cb39e6a
Gate Monero empty transaction callbacks
paullinator Feb 25, 2026
f6ffd9a
Gate Monero onSeenTxCheckpoint to only fire on advancement
paullinator Feb 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 19 additions & 11 deletions src/MoneroEngine.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,9 @@ const SAVE_DATASTORE_MILLISECONDS = 10000

const PRIMARY_CURRENCY_TOKEN_ID = null

// Global throttle: max 1 onAddressesChecked per 500ms; ratio=1 always passes.
Copy link
Copy Markdown

@eddy-edge eddy-edge Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let lastSyncEmitTime = 0
Copy link
Copy Markdown

@eddy-edge eddy-edge Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

@cursor cursor bot Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Fix in Cursor Fix in Web


export class MoneroEngine implements EdgeCurrencyEngine {
apiKey: string
walletInfo: SafeWalletInfo
Expand Down Expand Up @@ -169,6 +172,9 @@ export class MoneroEngine implements EdgeCurrencyEngine {
return
}
if (numTx !== totalTxs) {
const now = Date.now()
if (now - lastSyncEmitTime < 500) return
Copy link
Copy Markdown

@eddy-edge eddy-edge Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

lastSyncEmitTime = now
Comment on lines 174 to +177
Copy link
Copy Markdown

@eddy-edge eddy-edge Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const progress = numTx / totalTxs
this.edgeTxLibCallbacks.onAddressesChecked(progress)
} else {
Expand Down Expand Up @@ -304,8 +310,10 @@ export class MoneroEngine implements EdgeCurrencyEngine {
}

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 = []
}
Comment on lines 312 to +316
Copy link
Copy Markdown

@eddy-edge eddy-edge Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


return blockHeight
}
Expand Down Expand Up @@ -343,11 +351,13 @@ export class MoneroEngine implements EdgeCurrencyEngine {
}

this.updateOnAddressesChecked(transactions.length, transactions.length)
// Update the seenTxCheckpoint state:
this.seenTxCheckpoint = seenTxCheckpoint
this.edgeTxLibCallbacks.onSeenTxCheckpoint(
wasSeenTxCheckpoint(this.seenTxCheckpoint)
)
// Only update the seenTxCheckpoint if it actually advanced:
Copy link
Copy Markdown

@eddy-edge eddy-edge Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 newCheckpoint = wasSeenTxCheckpoint(seenTxCheckpoint)
const oldCheckpoint = wasSeenTxCheckpoint(this.seenTxCheckpoint)
Comment on lines +355 to +356
Copy link
Copy Markdown

@eddy-edge eddy-edge Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (newCheckpoint !== oldCheckpoint) {
this.seenTxCheckpoint = seenTxCheckpoint
this.edgeTxLibCallbacks.onSeenTxCheckpoint(newCheckpoint)
}
} catch (e) {
this.log.error('checkTransactionsInnerLoop', e)
}
Expand Down Expand Up @@ -457,10 +467,8 @@ export class MoneroEngine implements EdgeCurrencyEngine {
doInitialCallbacks(): void {
for (const tokenId of this.walletLocalData.enabledTokens) {
try {
this.edgeTxLibCallbacks.onTokenBalanceChanged(
tokenId,
this.walletLocalData.totalBalances.get(tokenId) ?? '0'
)
const _bal = this.walletLocalData.totalBalances.get(tokenId) ?? '0'
this.edgeTxLibCallbacks.onTokenBalanceChanged(tokenId, _bal)
Comment on lines +470 to +471
Copy link
Copy Markdown

@eddy-edge eddy-edge Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} catch (e) {
this.log.error('Error for currencyCode', tokenId, e)
}
Expand Down
1 change: 0 additions & 1 deletion test/engine/engine.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,6 @@ for (const fixture of fixtures) {
},
onUnactivatedTokenIdsChanged() {},
onStakingStatusChanged() {},
onSubscribeAddresses() {},
onWcNewContractCall(payload) {
emitter.emit('wcNewContractCall', payload)
},
Expand Down
Loading