-
Notifications
You must be signed in to change notification settings - Fork 54
Reduce UI lag: gate redundant callbacks in accountbased engines #1033
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.
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
base: master
Are you sure you want to change the base?
Changes from all commits
fc9acad
ab97b94
2679dbb
6f96cb4
93692e0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,7 @@ import { | |
| EdgeLog, | ||
| EdgeMetaToken, | ||
| EdgeSpendInfo, | ||
| EdgeStakingStatus, | ||
| EdgeSubscribedAddress, | ||
| EdgeSyncStatus, | ||
| EdgeToken, | ||
|
|
@@ -196,6 +197,7 @@ export class CurrencyEngine< | |
| publicKey: '', | ||
| totalBalances: {}, | ||
| numTransactions: {}, | ||
| detectedTokenIds: {}, | ||
| unactivatedTokenIds: [], | ||
| otherData: undefined | ||
| } | ||
|
|
@@ -655,6 +657,24 @@ export class CurrencyEngine< | |
| this.syncTracker.balanceComplete?.(tokenId) | ||
| } | ||
|
|
||
| private lastStakingStatusJson: string = '' | ||
|
|
||
| protected reportStakingStatus(status: EdgeStakingStatus): void { | ||
| const json = JSON.stringify(status) | ||
| if (json === this.lastStakingStatusJson) return | ||
| this.lastStakingStatusJson = json | ||
| this.currencyEngineCallbacks.onStakingStatusChanged(status) | ||
| } | ||
|
|
||
| reportDetectedTokens(tokenIds: string[]): void { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Warning: Recommendation: Mark |
||
| 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)) | ||
| } | ||
|
Comment on lines
+669
to
+676
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Warning: No test coverage for Recommendation: Add unit tests covering deduplication, cumulative emission, and walletLocalDataDirty flag. |
||
|
|
||
| updateConfirmations(tx: EdgeTransaction): boolean { | ||
| // No update needed for these status | ||
| switch (tx.confirmations) { | ||
|
|
@@ -717,10 +737,12 @@ export class CurrencyEngine< | |
|
|
||
| this.walletLocalData.blockHeight = blockHeight | ||
| this.walletLocalDataDirty = true | ||
| this.currencyEngineCallbacks.onBlockHeightChanged(blockHeight) | ||
|
|
||
| // Update confirmations directly on all in-memory transactions and emit | ||
| // any that changed via onTransactions. Confirmations are owned by the engine; | ||
| // core-js learns of changes only when we send txs with updated confirmations | ||
| // via onTransactions (i.e. the deprecated onBlockHeightChanged is not called). | ||
| const activeTokenIds = [null, ...this.enabledTokenIds] | ||
|
|
||
| for (const tokenId of activeTokenIds) { | ||
| const txList = this.transactionList[tokenId ?? ''] ?? [] | ||
| for (let i = 0; i < txList.length; i++) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,8 @@ | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Warning: Global throttle shared across all wallet engines. Recommendation: Move |
||
|
|
||
| /** | ||
| * Abstracts the ability to return a sync status, | ||
| * since different chains track their sync status in different ways. | ||
|
|
@@ -43,6 +46,7 @@ export function makeTokenSyncTracker(engine: SyncEngine): TokenSyncTracker { | |
| // Each tokenId can be a 0-1 value: | ||
| const balanceRatios = new Map<EdgeTokenId, number>() | ||
| const historyRatios = new Map<EdgeTokenId, number>() | ||
| let lastSyncStatus: EdgeSyncStatus | undefined | ||
|
|
||
| function getSyncStatus(): EdgeSyncStatus { | ||
| const activeTokenIds = [null, ...engine.enabledTokenIds] | ||
|
|
@@ -66,10 +70,29 @@ export function makeTokenSyncTracker(engine: SyncEngine): TokenSyncTracker { | |
| return { totalRatio } | ||
| } | ||
|
|
||
| function sendSyncStatusIfChanged(): void { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Warning: No tests cover the new Recommendation: Add test cases with fake timers covering: (1) duplicate ratios are suppressed, (2) |
||
| const currentStatus = getSyncStatus() | ||
| if ( | ||
| lastSyncStatus == null || | ||
| lastSyncStatus.totalRatio !== currentStatus.totalRatio | ||
| ) { | ||
| lastSyncStatus = currentStatus | ||
|
|
||
| if (currentStatus.totalRatio !== 1) { | ||
| const now = Date.now() | ||
| if (now - ssLastEmitTime < 500) return | ||
| ssLastEmitTime = now | ||
| } | ||
|
Comment on lines
+79
to
+85
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Warning: Silent data loss from throttle ordering: Recommendation: Move There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Throttled sync status updates are permanently lostMedium Severity
|
||
|
|
||
| engine.sendSyncStatus(currentStatus) | ||
| } | ||
| } | ||
|
|
||
| const out: TokenSyncTracker = { | ||
| resetSync() { | ||
| balanceRatios.clear() | ||
| historyRatios.clear() | ||
| lastSyncStatus = undefined | ||
| }, | ||
|
|
||
| balanceComplete(tokenId) { | ||
|
|
@@ -78,12 +101,12 @@ export function makeTokenSyncTracker(engine: SyncEngine): TokenSyncTracker { | |
|
|
||
| setBalanceRatios(tokenIds, ratio) { | ||
| for (const tokenId of tokenIds) balanceRatios.set(tokenId, ratio) | ||
| engine.sendSyncStatus(getSyncStatus()) | ||
| sendSyncStatusIfChanged() | ||
| }, | ||
|
|
||
| setHistoryRatios(tokenIds, ratio) { | ||
| for (const tokenId of tokenIds) historyRatios.set(tokenId, ratio) | ||
| engine.sendSyncStatus(getSyncStatus()) | ||
| sendSyncStatusIfChanged() | ||
| }, | ||
|
|
||
| updateBalanceRatio(tokenId, ratio) { | ||
|
|
@@ -94,7 +117,7 @@ export function makeTokenSyncTracker(engine: SyncEngine): TokenSyncTracker { | |
| if (ratio <= lastRatio) return | ||
|
|
||
| balanceRatios.set(tokenId, ratio) | ||
| engine.sendSyncStatus(getSyncStatus()) | ||
| sendSyncStatusIfChanged() | ||
| }, | ||
|
|
||
| updateHistoryRatio(tokenId, ratio, minStep) { | ||
|
|
@@ -108,7 +131,7 @@ export function makeTokenSyncTracker(engine: SyncEngine): TokenSyncTracker { | |
| if (minStep != null && ratio - lastRatio < minStep && ratio < 1) return | ||
|
|
||
| historyRatios.set(tokenId, ratio) | ||
| engine.sendSyncStatus(getSyncStatus()) | ||
| sendSyncStatusIfChanged() | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| import { | ||
| asArray, | ||
| asBoolean, | ||
| asCodec, | ||
| asEither, | ||
| asMaybe, | ||
|
|
@@ -60,6 +61,7 @@ export const asWalletLocalData = asObject({ | |
| asObject(asNumber), | ||
| () => ({}) | ||
| ), | ||
| detectedTokenIds: asMaybe(asObject(asBoolean), () => ({})), | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Recommendation: Consider using |
||
| unactivatedTokenIds: asMaybe(asArray(asString), () => []), | ||
| otherData: asOptional(asUnknown, () => ({})) | ||
| }) | ||
|
|
||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion:
JSON.stringifyforreportStakingStatuscomparison 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.