perf(market-data): add update backoff strategy and guard against fetching tx history of unactivated assets#3162
Conversation
TODO: ensure that the portfolio growth event is only fired once to avoid restarting the periodic updates on every user input (likely the case currently)
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughIntroduces exponential backoff-based periodic updates for portfolio growth and profit/loss blocs, adds coin/asset filtering and aggregation utilities, updates wallet asset retrieval and activation flows via SDK, adjusts defaults to 2-minute intervals, refactors version info state updates, and enhances coins bloc with initial activation guards and threshold-based refresh triggering. Adds unit/integration tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant B as PortfolioGrowth/ProfitLoss Bloc
participant BS as BackoffStrategy
participant R as Repository
participant SDK as KomodoDefiSdk
U->>B: LoadRequested(coins, period, updateFrequency=2m)
B->>B: Filter coins (withoutTestCoins + supported)
alt No supported coins
B-->>U: Emit LoadFailure
else Supported coins exist
B->>SDK: waitForEnabledCoinsToPassThreshold()
SDK-->>B: Threshold reached/timeout
B->>R: Fetch initial chart (filtered coins)
R-->>B: Chart data
B-->>U: Emit LoadSuccess
loop Periodic updates (backoff)
B->>BS: getNextInterval()
BS-->>B: Interval (capped, paired)
B->>R: Fetch chart (active coins)
R-->>B: Chart data or error
alt Success
B-->>U: Emit UpdateSuccess
else Error
B-->>U: Emit LoadFailure
end
B->>B: Wait interval (async)
end
end
sequenceDiagram
autonumber
actor U as User Login
participant CB as CoinsBloc
participant SDK as KomodoDefiSdk
participant M as Monitors
U->>CB: Login
CB->>CB: _isInitialActivationInProgress = true
CB->>SDK: Activate coins (async)
par Schedule refresh
CB->>SDK: waitForEnabledCoinsToPassThreshold(>=80%)
SDK-->>CB: Done/Empty/Timeout
CB->>CB: Trigger balance refresh
CB->>M: Start balance monitoring
and Activation
SDK-->>CB: Activation result
CB->>CB: Clear in-progress flag
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull Request Overview
This PR implements a performance optimization for market data updates by introducing an exponential backoff strategy and preventing API calls for unactivated assets. The changes aim to reduce unnecessary API requests while maintaining data freshness.
- Implements an exponential backoff strategy to reduce API call frequency over time
- Guards against fetching transaction history for unactivated assets to avoid wasted API calls
- Updates default update frequencies from 1 minute to 2 minutes across portfolio components
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
test_units/bloc/cex_market_data/common/update_frequency_backoff_strategy_test.dart |
Unit tests for the new backoff strategy implementation |
test_integration/bloc/cex_market_data/common/update_frequency_backoff_strategy_integration_test.dart |
Integration tests demonstrating backoff strategy behavior in realistic scenarios |
lib/model/kdf_auth_metadata_extension.dart |
Enhanced wallet asset retrieval with improved error handling for missing assets |
lib/bloc/version_info/version_info_bloc.dart |
Refactored to use cleaner variable assignment pattern |
lib/bloc/coins_bloc/coins_repo.dart |
Simplified asset retrieval logic and reduced retry attempts from 30 to 15 |
lib/bloc/coins_bloc/coins_bloc.dart |
Added initial activation tracking and improved coin activation flow |
lib/bloc/coins_bloc/asset_coin_extension.dart |
Added utility methods for filtering supported coins and portfolio calculations |
lib/bloc/cex_market_data/profit_loss/profit_loss_event.dart |
Updated default update frequency to 2 minutes |
lib/bloc/cex_market_data/profit_loss/profit_loss_bloc.dart |
Integrated backoff strategy for periodic updates |
lib/bloc/cex_market_data/portfolio_growth/portfolio_growth_event.dart |
Updated default update frequency to 2 minutes |
lib/bloc/cex_market_data/portfolio_growth/portfolio_growth_bloc.dart |
Integrated backoff strategy and moved portfolio calculations to extensions |
lib/bloc/cex_market_data/common/update_frequency_backoff_strategy.dart |
Core backoff strategy implementation with exponential intervals |
lib/bloc/assets_overview/bloc/asset_overview_bloc.dart |
Added coin filtering to prevent processing unsupported assets |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (11)
lib/bloc/coins_bloc/asset_coin_extension.dart (2)
244-261: Avoid double SDK calls for active assetsBoth methods fetch activated assets separately. If called back‑to‑back, cache the ID set to cut I/O.
+Future<Set<AssetId>> _activeAssetIds(KomodoDefiSdk sdk) async => + (await sdk.assets.getActivatedAssets()).map((e) => e.id).toSet(); @@ - Future<List<Asset>> removeInactiveAssets(KomodoDefiSdk sdk) async { - final activeAssets = await sdk.assets.getActivatedAssets(); - final activeAssetsMap = activeAssets.map((e) => e.id).toSet(); + Future<List<Asset>> removeInactiveAssets(KomodoDefiSdk sdk) async { + final activeAssetsMap = await _activeAssetIds(sdk); @@ - Future<List<Asset>> removeActiveAssets(KomodoDefiSdk sdk) async { - final activeAssets = await sdk.assets.getActivatedAssets(); - final activeAssetsMap = activeAssets.map((e) => e.id).toSet(); + Future<List<Asset>> removeActiveAssets(KomodoDefiSdk sdk) async { + final activeAssetsMap = await _activeAssetIds(sdk);
239-242: Remove empty extension or add TODOEmpty
CoinListOpsadds noise.-extension CoinListOps on List<Coin> { - -} +// TODO: Add List<Coin>-specific ops or remove if unused. +extension CoinListOps on List<Coin> {}lib/bloc/assets_overview/bloc/asset_overview_bloc.dart (4)
97-106: Wait on threshold for active coins onlyCall the threshold wait after removing inactive coins to avoid waiting on coins that cannot activate.
- final supportedCoins = await event.coins.filterSupportedCoins(); - if (supportedCoins.isEmpty) { + final supportedCoins = await event.coins.filterSupportedCoins(); + if (supportedCoins.isEmpty) { _log.warning('No supported coins to load portfolio overview for'); return; } - - await _sdk.waitForEnabledCoinsToPassThreshold(supportedCoins); - - final activeCoins = await supportedCoins.removeInactiveCoins(_sdk); + final activeCoins = await supportedCoins.removeInactiveCoins(_sdk); + await _sdk.waitForEnabledCoinsToPassThreshold(activeCoins);
145-152: Verify totalValue: using profit amount looks wrong
totalValueis set toFiatValue.usd(profitAmount)(same asprofitAmount). Likely meant current portfolio value.- totalValue: FiatValue.usd(profitAmount), + // If profitLoss = currentValue - totalInvestment + totalValue: FiatValue.usd(totalInvestment.value + profitAmount),If semantics differ, please confirm intended definition of totalValue.
225-237: Guard divide-by-zero in percentage increaseWhen
oldestValue == 0, division explodes. Return 0 or define convention.double _calculatePercentageIncrease(List<ProfitLoss> profitLosses) { if (profitLosses.length < 2) return 0.0; @@ - if (oldestValue < 0 && newestValue >= 0) { + if (oldestValue == 0) { + return 0.0; + } else if (oldestValue < 0 && newestValue >= 0) { final double totalChange = newestValue + oldestValue.abs(); return (totalChange / oldestValue.abs()) * 100; } else { return ((newestValue - oldestValue) / oldestValue.abs()) * 100; }
239-251: Avoid divide-by-zero when totalProfit is 0Computing per-asset portions with
totalProfit == 0yieldsNaN/Infinity.Map<String, double> _calculateAssetPortionPercentages( List<List<ProfitLoss>> profitLosses, double totalProfit, ) { final Map<String, double> assetPortionPercentages = {}; + if (totalProfit == 0) return assetPortionPercentages;lib/model/kdf_auth_metadata_extension.dart (2)
32-35: Defensive typing of activated_coins readEnsure a
List<String>regardless of metadata serialization.- Future<List<String>> getWalletCoinIds() async { - final user = await auth.currentUser; - return user?.metadata.valueOrNull<List<String>>('activated_coins') ?? []; - } + Future<List<String>> getWalletCoinIds() async { + final user = await auth.currentUser; + final raw = + user?.metadata.valueOrNull<List>('activated_coins') ?? const []; + return List<String>.from(raw); + }
113-115: Strongly type merged coins; avoid<dynamic>Use
Set<String>to dedupe; consider sorting for stable persistence.- final mergedCoins = <dynamic>{...existingCoins, ...coins}.toList(); + final mergedCoins = <String>{...existingCoins, ...coins}.toList()..sort(); await auth.setOrRemoveActiveUserKeyValue('activated_coins', mergedCoins);lib/bloc/version_info/version_info_bloc.dart (1)
51-55: State management pattern changed to mutable variable.The code now uses
var currentInfoinstead of a finalbasicInfoand re-assignscurrentInfoon each update. WhilecopyWithcreates new instances (preserving immutability of the state objects themselves), the mutable local variable pattern differs from typical Dart/bloc conventions that favor immutable bindings.Consider whether the original pattern (creating distinct named variables like
basicInfo,withApiVersion,withCoinsCommits) might be clearer, or document the rationale for the mutable local variable approach.lib/bloc/cex_market_data/profit_loss/profit_loss_bloc.dart (2)
232-259: Suggest adding cancellation mechanism for periodic updates.The infinite loop should respect bloc closure to prevent resource leaks. Consider checking
isClosedin the loop condition.Apply this diff:
/// Run periodic updates with exponential backoff strategy Future<void> _runPeriodicUpdates( ProfitLossPortfolioChartLoadRequested event, Emitter<ProfitLossState> emit, ) async { - while (true) { + while (!isClosed) { try { await Future.delayed(_backoffStrategy.getNextInterval()); + + // Check again after delay in case bloc was closed during wait + if (isClosed) break; final supportedCoins = await event.coins.filterSupportedCoins();
54-65: Clarify the dual emptiness check logic.The method checks both
supportedCoins.isEmptyandfilteredEventCoins.length <= 1(line 59). The comment suggests this handles single-coin detail charts, but the logic is not immediately clear.Consider adding a more detailed comment explaining when each condition triggers and why both are needed, or refactor to make the intent more explicit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
lib/bloc/assets_overview/bloc/asset_overview_bloc.dart(2 hunks)lib/bloc/cex_market_data/common/update_frequency_backoff_strategy.dart(1 hunks)lib/bloc/cex_market_data/portfolio_growth/portfolio_growth_bloc.dart(11 hunks)lib/bloc/cex_market_data/portfolio_growth/portfolio_growth_event.dart(2 hunks)lib/bloc/cex_market_data/profit_loss/profit_loss_bloc.dart(8 hunks)lib/bloc/cex_market_data/profit_loss/profit_loss_event.dart(2 hunks)lib/bloc/coins_bloc/asset_coin_extension.dart(3 hunks)lib/bloc/coins_bloc/coins_bloc.dart(7 hunks)lib/bloc/coins_bloc/coins_repo.dart(5 hunks)lib/bloc/version_info/version_info_bloc.dart(3 hunks)lib/model/kdf_auth_metadata_extension.dart(2 hunks)test_integration/bloc/cex_market_data/common/update_frequency_backoff_strategy_integration_test.dart(1 hunks)test_units/bloc/cex_market_data/common/update_frequency_backoff_strategy_test.dart(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-04-01T15:51:37.060Z
Learnt from: takenagain
PR: KomodoPlatform/komodo-wallet#2566
File: lib/bloc/coins_bloc/coins_bloc.dart:10-10
Timestamp: 2025-04-01T15:51:37.060Z
Learning: In the Komodo Wallet project, part files share imports with their parent files. The import for `app_config.dart` in `coins_bloc.dart` is necessary because the part file `coins_state.dart` uses `excludedAssetList` from that package.
Applied to files:
lib/bloc/coins_bloc/coins_bloc.dartlib/bloc/cex_market_data/portfolio_growth/portfolio_growth_bloc.dartlib/bloc/coins_bloc/asset_coin_extension.dartlib/bloc/coins_bloc/coins_repo.dart
📚 Learning: 2025-04-01T15:51:37.060Z
Learnt from: takenagain
PR: KomodoPlatform/komodo-wallet#2566
File: lib/bloc/coins_bloc/coins_bloc.dart:10-10
Timestamp: 2025-04-01T15:51:37.060Z
Learning: The `excludedAssetList` from `app_config.dart` is used in the `_filterExcludedAssets` method in `coins_state.dart`. Since `coins_state.dart` is a part file of `coins_bloc.dart`, the import needs to be in the parent file even though it's not directly used there. In Dart, part files share the namespace and imports of their parent files.
Applied to files:
lib/bloc/coins_bloc/coins_bloc.dartlib/bloc/cex_market_data/portfolio_growth/portfolio_growth_bloc.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit_tests
- GitHub Check: Test web-app-linux-profile
- GitHub Check: Test web-app-macos
🔇 Additional comments (21)
lib/model/kdf_auth_metadata_extension.dart (1)
59-66: Duplicate assets for a config ID: fail-fast vs graceful handlingYou
shoutand throwStateError. Confirm this is the desired UX; alternatively, log and skip the ID to keep wallet usable.lib/bloc/coins_bloc/asset_coin_extension.dart (2)
332-346: InvalidRational.fromInt(1, 100); add proper small-balance guard
fromInttakes one arg. Use parse (or Rational(BigInt.one, BigInt.from(100))).- Future<Rational> percentageChange24h(KomodoDefiSdk sdk) async { - final double totalBalance = totalLastKnownUsdBalance(sdk); - final Rational totalBalanceRational = Rational.parse( - totalBalance.toString(), - ); - final Rational totalChange = await totalChange24h(sdk); - - // Avoid division by zero or very small balances - if (totalBalanceRational <= Rational.fromInt(1, 100)) { - return Rational.zero; - } - - // Return the percentage change - return (totalChange / totalBalanceRational) * Rational.fromInt(100); - } + Future<Rational> percentageChange24h(KomodoDefiSdk sdk) async { + final totalBalance = totalLastKnownUsdBalance(sdk); + final totalBalanceR = Rational.parse(totalBalance.toString()); + final totalChange = await totalChange24h(sdk); + final min = Rational.parse('0.01'); // 1/100 + if (totalBalanceR <= min) return Rational.zero; + return (totalChange / totalBalanceR) * Rational.fromInt(100); + }Likely an incorrect or invalid review comment.
272-287:staticnot allowed in extensions; default predicate should be a local closureDart extensions can’t declare static members. Also the tear‑off to
_alwaysSupportedwon’t resolve here.Future<List<Coin>> filterSupportedCoins([ Future<bool> Function(Coin coin)? isSupported, ]) async { - final predicate = isSupported ?? _alwaysSupported; + final predicate = isSupported ?? ((_) async => true); @@ - static Future<bool> _alwaysSupported(Coin _) async => true;Likely an incorrect or invalid review comment.
test_integration/bloc/cex_market_data/common/update_frequency_backoff_strategy_integration_test.dart (1)
1-136: LGTM: Comprehensive integration test coverage.The integration test suite effectively validates real-world scenarios:
- Backoff progression over 20 attempts with correct 2-minute pair intervals
- API call reduction verification (confirms <50% of fixed-interval calls over 24h)
- Reset recovery behavior
- Custom interval configurations
- Portfolio update use case simulation
The tests are well-structured with clear expectations and helpful debug output. The 24-hour simulation (lines 35-60) provides valuable quantitative evidence of the backoff strategy's efficiency gains.
test_units/bloc/cex_market_data/common/update_frequency_backoff_strategy_test.dart (1)
1-164: LGTM: Thorough unit test coverage.The unit tests comprehensively validate the backoff strategy's core behavior:
- Initial state (attempt count 0)
- Base interval for first two attempts
- Exponential doubling pattern (2,2,4,4,8,8,16,16)
- Maximum interval capping
- Reset functionality
- Cache update behavior
- Preview functionality (non-mutating state inspection)
- Custom base/max interval configurations
All test cases are well-isolated, use clear assertions, and verify both state changes and return values.
lib/bloc/cex_market_data/portfolio_growth/portfolio_growth_event.dart (2)
20-20: Default update frequency adjusted to 2 minutes.The updateFrequency default has been changed from 1 to 2 minutes, aligning with the new backoff strategy's base interval. This reduces initial API call frequency while maintaining reasonable responsiveness.
44-44: Consistent default update frequency.The updateFrequency default mirrors the change in
PortfolioGrowthLoadRequested(line 20), ensuring consistency across portfolio growth events.lib/bloc/version_info/version_info_bloc.dart (2)
66-67: State update correctly uses copyWith and re-emits.The pattern
currentInfo = currentInfo.copyWith(...)followed byemit(currentInfo)correctly creates a new state instance and emits it. The logic preserves incremental state updates across the three phases (basic info, API version, SDK commits).
87-91: Final state update maintains consistency.The SDK coins commits update follows the same pattern as the API version update (lines 66-67), correctly updating
currentInfoand emitting the new state.lib/bloc/cex_market_data/profit_loss/profit_loss_event.dart (2)
20-20: Default update frequency aligned with portfolio growth events.The updateFrequency default has been changed from 1 to 2 minutes, mirroring the change in
portfolio_growth_event.dart(line 20) and aligning with the new backoff strategy's base interval.
42-42: Consistent default across profit/loss events.The updateFrequency default maintains consistency with
ProfitLossPortfolioChartLoadRequested(line 20), ensuring uniform behavior across profit/loss portfolio events.lib/bloc/coins_bloc/coins_bloc.dart (6)
65-82: LGTM! Activation guard and coin-not-found check improve safety.The early returns prevent pubkey requests during initial activation and when coins haven't been pre-populated yet. The logging provides useful diagnostics.
339-339: LGTM! Cleaner abstraction for state reset.Using the
_resetInitialActivationState()helper improves consistency and maintainability.
353-401: LGTM! Well-structured threshold-based refresh scheduling.The method correctly:
- Guards against closed bloc state
- Waits for 80% of coins to activate before refreshing balances
- Handles timeouts gracefully
- Provides clear logging for debugging
- Uses
unawaitedappropriately for non-blocking executionThe empty-coin handling (lines 362-366) ensures immediate refresh when no coins need activation.
403-405: LGTM! Clean helper for state reset.Simple, focused helper method that improves code organization.
305-332: Activation flag handling is correct. Every path that sets_isInitialActivationInProgressto true is paired with a reset to false (lines 326, 330, and 404).
49-49: Activation guard is already safe; adjust transformer if needed
Dart event handlers run on a single isolate and the_isInitialActivationInProgressflag is set synchronously before anyawait, so there’s no race. If the goal is to prevent overlapping pubkey fetches, changeCoinsPubkeysRequestedfromconcurrent()todroppable()orsequential().Likely an incorrect or invalid review comment.
lib/bloc/coins_bloc/coins_repo.dart (3)
183-186: LGTM! Migration to SDK-based asset retrieval.The change from wallet config to
_kdfSdk.getWalletAssets()aligns with the SDK-centric architecture and provides a cleaner asset management approach.
530-553: Excellent improvement to error handling!The updated
getFirstPubkeyimplementation adds robust checks for:
- Empty asset lists (returns
nullgracefully)- Multiple assets (throws explicit
StateErrorwith clear message)This prevents silent failures and provides better diagnostics compared to the previous implementation.
580-580: LGTM! Consistent SDK-based asset retrieval.The change aligns with the broader migration to SDK-based asset management.
lib/bloc/cex_market_data/profit_loss/profit_loss_bloc.dart (1)
186-195: LGTM! Robust coin filtering and validation.The added checks for supported and active coins with appropriate logging prevent unnecessary processing and provide clear diagnostics.
lib/bloc/cex_market_data/portfolio_growth/portfolio_growth_bloc.dart
Outdated
Show resolved
Hide resolved
* fix(screenshot-sensitivity): notify listeners in postFrameCallback fixes "setState() or markNeedsBuild() called when widget tree was locked. This ScreenshotSensitivity widget cannot be marked as needing to build because the framework is locked." and "DartError: setState() or markNeedsBuild() called during build. This ScreenshotSensitivity widget cannot be marked as needing to build because the framework is already in the process of building widgets. A widget can be marked as needing to be built during the build phase only if one of its ancestors is currently building. This exception is allowed because the framework builds parent widgets before children, which means a dirty descendant will always be built. Otherwise, the framework might not visit this widget during this build phase." * chore(sdk): switch to zhtlc branch * feat(zhtlc): add configuration and param download dialogs * refactor: generate localisations, and fix theming on popup dialog * refactor: use repositoryprovider and adjust status bar width * perf(coins-manager): add input debouncer and cache heavy operations - cache heavy operations that aren't expected to change regularly. - add debounce on search input * fix(activation): swap page activation, improve list and disposal management * fix(zhtlc): update activation warning, add padding, and stealth text * feat(orderbook): migrate to v2 orderbook RPC via SDK * perf(dex): remove enabled assets rpc call from taker validator With ZHTLC enabled, the `get_enabled_coins` RPC takes long - not yet sure why - and the taker form calls the validator regularly, resulting in long loading times and input delays * style(zhtlc): improve dialog layout, add transaction note, fix hidden spinner * fix(market-metrics): filter out inactive coins before starting calcs fetching tx history of activating coins wrecks havok on startup, especially for ARRR * fix(zhtlc): workaround "Task is finished" error with retry on activation Activation throws a bunch of uncaught errors, takes, long, but with this the coin should not remain in suspended state when it does finally activate. TODO(takenagain): investigate and fix cause in SDK * feat(zhtlc): add activation status to banner * fix(withdraw): add recipient address and memo (if not empty) * fix(zhtlc): filter out active assets before attempting zhtlc activation This logic was mistakenly not copied over from the normal coin activation function * fix(wallet): add resilience to delisted coins for wallet coin metadata * fix(version-info): ensure that apiversion and commit has persist * perf(market-data): add update backoff strategy and guard against fetching tx history of unactivated assets (#3162) * fix(wallet): add resilience to delisted coins for wallet coin metadata * fix(version-info): ensure that apiversion and commit has persist * perf(market-metrics): add backoff strategy for periodic updates TODO: ensure that the portfolio growth event is only fired once to avoid restarting the periodic updates on every user input (likely the case currently) * refactor(market-metrics): extract common coin filtering for test coin removal * refactor(market-metrics): extract calculation functions into extension * perf(coins-bloc): add initialisation status tracking for startup * refactor: add loop exits, update comments, remove unused event member * perf(dex): reduce overhead in coin selection and filtering * fix(dex): only update sell amount on order selection if no value present also cap to the available sell amount * fix(zhtlc): track ongoing activations to prevent duplicate requests * fix(taker-validator): revert to getting active assets from API getWalletAssets was used to improve form validation speed to fix hitching and lengthy delays, but it is not correct and allowed for form submissions with inactive coins, resulting in unexpected error messages * chore(sdk): switch back to dev branch (sdk branch merged)
Summary by CodeRabbit