fix(system-health-check): add additional time providers and update URLs of existing providers#2611
fix(system-health-check): add additional time providers and update URLs of existing providers#2611
Conversation
CORS rules block accessing the `date` header on web from other domains unless it's been explicitly allowed.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Error: Could not generate a valid Mermaid diagram after multiple attempts. 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Visit the preview URL for this PR (updated for commit 3b6196e): https://walletrc--pull-2611-merge-o0slm7rg.web.app (expires Fri, 16 May 2025 14:35:39 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: f66a4ff03faa546f12f0ae5a841bd9eff2714dcc |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull Request Overview
This pull request enhances system time validation by introducing additional time providers, updates existing provider URLs, and removes unused providers. Key changes include adding new test suites for system health providers, integrating a new NTP dependency in pubspec.yaml, and refactoring the system health bloc and its associated repository and provider implementations.
Reviewed Changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test_units/main.dart | Added new system health test imports and grouped system health tests. |
| pubspec.yaml | Added ntp dependency. |
| Various views files | Removed unused imports related to system_health_state. |
| lib/services/initializer/app_bootstrapper.dart & lib/main.dart | Updated initialization to include mm2Api and ensure logger initialization. |
| lib/bloc/system_health/* | Refactored system health bloc, events, and states for improved periodic checks. |
| lib/bloc/system_health/system_clock_repository.dart | Refactored system clock validation logic to use provider registry. |
| lib/bloc/system_health/providers/* | Introduced and adjusted multiple time providers (NTP, HTTP Head, HTTP, Binance) with improved error handling. |
| lib/bloc/app_bloc_root.dart | Updated the system health bloc initialization to include the periodic check event. |
Comments suppressed due to low confidence (1)
lib/bloc/system_health/system_clock_repository.dart:42
- DateTime.timestamp() is not a valid method for obtaining the current time. Consider replacing it with DateTime.now() to correctly compute the time difference.
final localTime = DateTime.timestamp();
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (7)
lib/bloc/app_bloc_root.dart (1)
414-417: CoinIcon is marked as deprecatedThe pipeline failure indicates that CoinIcon is deprecated and shouldn't be used. Consider updating to use AssetIcon from the SDK's
komodo_uipackage instead.- await CoinIcon.precacheCoinIcon(context, abbr) + await AssetIcon.precacheCoinIcon(context, abbr)Note: You'll need to add the appropriate import for AssetIcon.
🧰 Tools
🪛 GitHub Actions: takenagain is validating code guidelines 🚀
[info] 415-415: 'CoinIcon' is deprecated and shouldn't be used. CoinIcon is deprecated. Use AssetIcon from the SDK's
komodo_uipackage insteadlib/bloc/system_health/providers/binance_time_provider.dart (1)
38-63: Consider adding delay between retries.While the retry mechanism is well-implemented with good error handling and logging, there's no delay between retry attempts. This could potentially overwhelm the server if it's experiencing temporary high load.
Consider adding an exponential backoff delay between retries:
while (retries < maxRetries) { try { final serverTime = await _fetchServerTime(); _logger.fine('Successfully retrieved time from Binance API'); return serverTime; } on SocketException catch (e, s) { _logger.warning('Socket error with Binance API', e, s); } on TimeoutException catch (e, s) { _logger.warning('Timeout with Binance API', e, s); } on FormatException catch (e, s) { _logger.severe('Failed to parse response from Binance API', e, s); } on Exception catch (e, s) { _logger.severe('Error fetching time from Binance API', e, s); } retries++; + // Add exponential backoff delay + if (retries < maxRetries) { + final delayDuration = Duration(milliseconds: 100 * (1 << retries)); + await Future.delayed(delayDuration); + } }test_units/tests/system_health/http_head_time_provider_test.dart (1)
66-80: Redundant test targets & unused importThe “all servers fail” case (status 500) duplicates the “status is not 200” test and does not add new behaviour checks.
Additionally, the explicitTimeoutExceptionimport is unused (the provider handles it internally), so it can be removed to keep the test clean.lib/bloc/system_health/providers/http_time_provider.dart (1)
69-75: Error responses leak into normal flow – prefer throwingReturning an artificial
http.Responsewith status 500 inside_httpGet()disguises network errors as “successful” HTTP calls, forcing the caller to inspectstatusCodeevery time and making debugging harder.
Consider re-throwing the original exception sogetCurrentUtcTime()can catch/log specific failures, keeping the happy path clean.lib/bloc/system_health/system_clock_repository.dart (1)
23-26: Idle_httpClientfield – unnecessary resource management
_httpClientis created and closed, yet never used after refactor (all providers manage their own clients). Removing it will simplify the class and avoid closing a client that providers might still rely on if the same instance was passed in.lib/bloc/system_health/providers/http_head_time_provider.dart (1)
11-18: Consider diversifying & randomising the default server listThe hard-coded list contains several CDNs that may block HEAD requests or omit the
Dateheader, causing repeated failures.
Randomising the order once per call (or shuffling on construction) will distribute load and reduce the chance of hitting a consistently bad first host.
Optionally allow the caller to inject their own list via constructor to simplify testing.lib/bloc/system_health/system_health_bloc.dart (1)
42-44: Minor: make the periodic interval configurable & testable
const Duration(seconds: 60)is baked into the bloc, which makes unit testing slow and prevents apps with different requirements from tweaking the cadence.
Expose the duration via the constructor with a sensible default:-class SystemHealthBloc extends Bloc<SystemHealthEvent, SystemHealthState> { - SystemHealthBloc(this._systemClockRepository, this._api) +class SystemHealthBloc extends Bloc<SystemHealthEvent, SystemHealthState> { + SystemHealthBloc( + this._systemClockRepository, + this._api, { + Duration checkInterval = const Duration(seconds: 60), + }) : _checkInterval = checkInterval, + super(SystemHealthInitial()) { @@ - _timer = Timer.periodic(const Duration(seconds: 60), (timer) { + _timer = Timer.periodic(_checkInterval, (timer) { add(SystemHealthCheckRequested()); }); } + final Duration _checkInterval;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
lib/bloc/app_bloc_root.dart(2 hunks)lib/bloc/system_health/providers/binance_time_provider.dart(1 hunks)lib/bloc/system_health/providers/http_head_time_provider.dart(1 hunks)lib/bloc/system_health/providers/http_time_provider.dart(1 hunks)lib/bloc/system_health/providers/ntp_time_provider.dart(1 hunks)lib/bloc/system_health/providers/time_provider.dart(1 hunks)lib/bloc/system_health/providers/time_provider_registry.dart(1 hunks)lib/bloc/system_health/system_clock_repository.dart(1 hunks)lib/bloc/system_health/system_health_bloc.dart(3 hunks)lib/bloc/system_health/system_health_event.dart(1 hunks)lib/bloc/system_health/system_health_state.dart(1 hunks)lib/main.dart(3 hunks)lib/services/initializer/app_bootstrapper.dart(1 hunks)lib/shared/ui/clock_warning_banner.dart(1 hunks)lib/views/bridge/bridge_exchange_form.dart(0 hunks)lib/views/dex/simple/form/maker/maker_form_trade_button.dart(0 hunks)lib/views/dex/simple/form/taker/taker_form_content.dart(0 hunks)lib/views/market_maker_bot/add_market_maker_bot_trade_button.dart(0 hunks)pubspec.yaml(1 hunks)test_units/main.dart(2 hunks)test_units/tests/system_health/http_head_time_provider_test.dart(1 hunks)test_units/tests/system_health/http_time_provider_test.dart(1 hunks)test_units/tests/system_health/ntp_time_provider_test.dart(1 hunks)test_units/tests/system_health/system_clock_repository_test.dart(1 hunks)test_units/tests/system_health/time_provider_registry_test.dart(1 hunks)
💤 Files with no reviewable changes (4)
- lib/views/market_maker_bot/add_market_maker_bot_trade_button.dart
- lib/views/bridge/bridge_exchange_form.dart
- lib/views/dex/simple/form/maker/maker_form_trade_button.dart
- lib/views/dex/simple/form/taker/taker_form_content.dart
🧰 Additional context used
🪛 GitHub Actions: takenagain is validating code guidelines 🚀
lib/bloc/app_bloc_root.dart
[info] 415-415: 'CoinIcon' is deprecated and shouldn't be used. CoinIcon is deprecated. Use AssetIcon from the SDK's komodo_ui package instead
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build Desktop (windows)
- GitHub Check: Build Mobile (Android)
- GitHub Check: Build Mobile (iOS)
🔇 Additional comments (27)
lib/services/initializer/app_bootstrapper.dart (1)
12-12: Method signature change adds Mm2Api parameterThe
ensureInitializedmethod now accepts an additionalMm2Api mm2Apiparameter. This change ensures that the bootstrapper has access to the MM2 API instance early in the initialization process.lib/main.dart (4)
5-5: Reordered import parameters without functional changeThe order of
kIsWasmandkIsWebhas been swapped in the import statement. This change has no functional impact.
32-32: Moved import statementThe import of
custom_feedback_form.darthas been moved to after the performance analytics imports, maintaining proper import organization.
58-63: Restructured initialization sequence to introduce MM2 API earlierThe initialization sequence has been improved:
- Added explanatory comments about foundational dependencies
- Created MM2 API instance immediately after KomodoDefiSdk initialization
- Passed both dependencies to the AppBootstrapper's ensureInitialized method
This restructuring aligns with the updated parameter requirements in AppBootstrapper and consolidates initialization logic.
65-65:Details
❓ Verification inconclusive
Good documentation of technical debt
The comment about "Strange inter-dependencies" appropriately documents a potential architectural issue with the current component relationships.
It would be valuable to understand the nature of these dependencies better:
🏁 Script executed:
#!/bin/bash # Look for any circular dependencies between these components echo "Checking dependency relationships between key components..." rg "TrezorRepo|TrezorCoinsBloc|CoinsRepo|WalletsRepository" -A 5 -B 5 --no-filename | grep -v "import" | sort | uniqLength of output: 33761
Dependency Analysis: No Direct Circular Dependencies Found
The grep across the codebase for
TrezorRepo,TrezorCoinsBloc,CoinsRepo, andWalletsRepositorydid not reveal any direct circular imports or references. That said, these repositories are injected extensively—in blocs likeBridgeBloc,MarketMakerTradeFormBloc,NftReceiveBloc, and many UI components—indicating tight coupling across modules.• Manually review the constructors and factory methods of these classes to catch any indirect cycles that the grep may have missed.
• Consider introducing facades or service layers (e.g. domain-specific interfaces) to encapsulate repository interactions and reduce cross-module coupling.pubspec.yaml (1)
164-164: Dependency added for NTP time providerThe addition of the
ntppackage (v2.0.0) supports the new NTP time provider mentioned in the PR objectives, which will be used for more accurate time synchronization on native desktop and mobile platforms.lib/shared/ui/clock_warning_banner.dart (1)
1-7: Import cleanup aligns with system health bloc refactoringThe import changes are part of a broader refactoring where system health state handling was centralized in
system_health_bloc.dart. TheSystemHealthStateclass is now accessed through this import rather than from a separate file.lib/bloc/app_bloc_root.dart (2)
287-287: SystemHealthBloc now starts periodic checks automaticallyThe SystemHealthBloc is now configured to immediately dispatch a
SystemHealthPeriodicCheckStartedevent upon creation, which initiates automatic periodic system health checks at startup. This is consistent with the new time provider system described in the PR objectives.
303-305: FaucetBloc creation reformatted for consistencyThe code has been reformatted to use a multi-line style without changing its logic or parameters.
lib/bloc/system_health/system_health_state.dart (2)
1-2: Refactored to use part directive for better organizationThe file has been converted to be a part of
system_health_bloc.dartusing the part directive, which improves organization by consolidating related components into a cohesive library structure.
11-12: Field position reorganized with no functional changeThe positioning of the
isValidfield was moved from above to below the constructor, maintaining the same functionality while improving code readability.lib/bloc/system_health/providers/time_provider.dart (1)
1-15: Well-designed abstract base class for time providersThis is a clean and well-structured abstract class that provides a clear contract for time providers. The class includes appropriate documentation, a well-defined async method to get UTC time, a name getter, and a disposable interface with a sensible default implementation.
A few observations:
- Good use of nullable return type to handle failure cases
- Clear documentation that explains the purpose of each method
- Proper disposal pattern included from the start
test_units/main.dart (2)
27-31: Appropriate imports for new system health test filesThe new imports for the system health tests are well organized and follow the existing import pattern in the file.
94-101: Well-structured test group for system health time providersThe new test group for 'SystemHealth' logically groups all the time provider tests together. The organization looks good and matches the pattern of other test groups in this file.
test_units/tests/system_health/ntp_time_provider_test.dart (1)
1-19: Good negative test case for NTP time providerThis test effectively verifies the failure handling of the NTP time provider by using an invalid server address with a short timeout and minimal retries.
I appreciate the informative comment about a true success test requiring a real NTP server. This shows good testing practice by acknowledging the limitations of unit tests for this component.
Consider adding a comment to this test file explaining why you're using such a short timeout (10ms). Is this intentionally short to ensure the test runs quickly, or is there another reason? This would help future developers understand the test configuration.
lib/bloc/system_health/system_health_event.dart (1)
5-12: Well-structured event classes with clear responsibilitiesThe refactoring from a single event to three specialized events improves the design by:
- Following better naming conventions (past tense for completed actions)
- Creating a clearer separation of concerns between one-time checks and periodic checks
- Providing explicit control over the periodic check lifecycle
This design allows for more granular control over system health checks and aligns with standard BLoC pattern practices.
test_units/tests/system_health/time_provider_registry_test.dart (2)
5-28: Good test coverage with clear structure.The test suite for TimeProviderRegistry provides comprehensive coverage of the core functionality - checking default providers, custom provider injection, and proper disposal of resources. The tests are well-structured with clear expectations and follow Dart testing best practices.
30-41: Good mock implementation.The _MockTimeProvider class is properly implemented with all required overrides from the TimeProvider abstract class. The optional onDispose callback is a nice touch that makes testing the disposal behavior straightforward.
lib/bloc/system_health/providers/time_provider_registry.dart (3)
9-18: Well-designed registry with proper dependency injection.The TimeProviderRegistry class follows good design principles by allowing injection of custom providers, HTTP client, and API timeout. Default values are provided when not specified, which makes the class flexible and easy to use.
28-68: Good platform-aware provider initialization with clear documentation.The provider initialization logic correctly handles platform differences between web/WASM and native platforms. The comments clearly explain the rationale behind platform-specific provider selection, which helps future maintainers understand why certain providers are excluded on specific platforms.
70-76: Proper resource cleanup in dispose method.The dispose method correctly iterates through all providers and calls their dispose methods, ensuring proper cleanup of resources like sockets or streams.
test_units/tests/system_health/http_time_provider_test.dart (2)
8-62: Comprehensive test coverage for HttpTimeProvider.The test suite thoroughly covers the functionality of HttpTimeProvider, including successful time retrieval, handling of non-200 responses, missing fields, invalid date formats, and exceptions. Good practice using proper setup and teardown for test isolation.
64-76: Clean MockClient implementation.The MockClient class is well-implemented with configurable response and exception behavior, making it easy to test various HTTP response scenarios.
lib/bloc/system_health/providers/binance_time_provider.dart (3)
11-18: Good configurable parameters with sensible defaults.The BinanceTimeProvider constructor allows for extensive customization while providing sensible defaults, making it both flexible and easy to use out of the box.
65-87: Well-implemented server time fetching with proper error handling.The _fetchServerTime method properly handles HTTP requests, validates responses, and parses the JSON data with appropriate error handling. The method correctly converts the Unix timestamp to a UTC DateTime object.
89-92: Proper resource cleanup in dispose method.The dispose method correctly closes the HTTP client to prevent resource leaks.
lib/bloc/system_health/providers/http_time_provider.dart (1)
112-122:⚠️ Potential issueCompile-time error: missing
breakcauses illegal fall-throughDart
switchstatements must end eachcasewithbreak,continue,return, orthrow.
TheTimeFormat.iso8601branch currently falls through intoTimeFormat.custom, which will trigger a compile-time error (and, if it compiled, would throw aFormatExceptionfor every ISO-8601 string).case TimeFormat.iso8601: if (formattedTime.endsWith('+00:00')) { formattedTime = formattedTime.replaceAll('+00:00', 'Z'); } else if (!formattedTime.endsWith('Z')) { formattedTime += 'Z'; } - case TimeFormat.custom: + break; + + case TimeFormat.custom: throw const FormatException('Custom time format not supported');⛔ Skipped due to learnings
Learnt from: takenagain PR: KomodoPlatform/komodo-wallet#2608 File: lib/views/fiat/fiat_form.dart:235-254 Timestamp: 2025-05-01T20:52:26.713Z Learning: In Dart, non-empty switch case statements do not require explicit `break` statements at the end as they don't fall through to the next case by default. Adding unnecessary breaks is against the "unnecessary_breaks" linter rule (https://dart.dev/tools/linter-rules/unnecessary_breaks).
|
|
||
| final timer = Stopwatch()..start(); | ||
| await logger.init(); | ||
| await initializeLogger(mm2Api); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Added initialization of logger with MM2 API
A new asynchronous call to initializeLogger(mm2Api) has been added after the base logger initialization. This integrates MM2 API logging into the bootstrap process.
Verify that the initializeLogger method properly handles any potential exceptions that might occur during initialization:
🏁 Script executed:
#!/bin/bash
# Check implementation of initializeLogger method
fd "initializeLogger" --type f -x cat {} \; | grep -A 15 "initializeLogger.*mm2Api"Length of output: 86
To inspect how initializeLogger handles errors, let’s locate its definition and surrounding logic:
🏁 Script executed:
#!/bin/bash
# Search for all definitions and usages of initializeLogger in Dart files
rg -n -C3 "initializeLogger" --glob "*.dart"Length of output: 1108
Let’s inspect the full initializeLogger implementation to confirm its error handling:
🏁 Script executed:
#!/bin/bash
# Display the implementation of initializeLogger with context
rg -n "Future<void> initializeLogger" -A50 --glob "*.dart" lib/services/logger/get_logger.dartLength of output: 2247
Ensure exception safety in initializeLogger
The current initializeLogger(mm2Api) implementation in lib/services/logger/get_logger.dart only guards the storage read call with .catchError, but leaves both the MM2 API version fetch and session metadata setup unprotected. Any exception thrown by mm2Api.version() or DragonLogs.setSessionMetadata would bubble up and could crash the bootstrap process.
Recommend wrapping the core initialization steps in a try/catch and handling/reporting failures gracefully.
• Affected file:
- lib/services/logger/get_logger.dart (around lines 33–49)
• Suggested pattern:
Future<void> initializeLogger(Mm2Api mm2Api) async {
try {
final platformInfo = PlatformInfo.getInstance();
final localeName = await getStorage()
.read('locale')
.catchError((_) => null) as String? ?? '';
final mm2Version = await mm2Api.version();
DragonLogs.setSessionMetadata({
'appVersion': packageInformation.packageVersion,
'mm2Version': mm2Version,
'appLanguage': localeName,
// …other metadata
});
Logger.root.level = kReleaseMode ? Level.INFO : Level.ALL;
Logger.root.onRecord.listen(_logToUniversalLogger);
} catch (e, st) {
// TODO: report to crashlytics or fallback cleanly
debugPrint('Logger initialization failed: $e');
}
}
test_units/tests/system_health/system_clock_repository_test.dart
Outdated
Show resolved
Hide resolved
previous check was a regression that wouldn't show the banner for incorrect clock if there were still peers connected
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull Request Overview
This PR improves the system health check functionality by adding new time providers, updating existing provider URLs, and removing dead ones. Key changes include adding Binance, NTP, and HTTP head time providers along with corresponding tests; updating the SystemHealthBloc to use periodic checks; and modifying dependency registration in the app bootstrap and main entry point.
Reviewed Changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test_units/main.dart | Added tests for new and updated system health components |
| pubspec.yaml | Added the ntp package dependency |
| lib/views/* | Removed unused system_health_state imports to avoid duplication |
| lib/services/initializer/app_bootstrapper.dart | Updated ensureInitialized signature and logger initialization |
| lib/main.dart | Updated bootstrap and removal of duplicate mm2Api instantiation |
| lib/bloc/system_health/system_health_state.dart | Updated state representation (duplicate field issue detected) |
| lib/bloc/system_health/system_health_bloc.dart | Refactored periodic check logic and event handling |
| lib/bloc/system_health/system_clock_repository.dart | Modified clock validation logic using time providers |
| lib/bloc/system_health/providers/* | Added new time provider implementations and registry |
Comments suppressed due to low confidence (1)
lib/bloc/system_health/system_clock_repository.dart:39
- Usage of 'DateTime.timestamp()' appears to be a mistake; use 'DateTime.now()' to get the current local time.
final localTime = DateTime.timestamp();
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
lib/bloc/system_health/providers/http_head_time_provider.dart (2)
26-28: Doc comment does not describe the fieldThe comment says “The name of the provider” but the field actually holds the logger instance.
- /// The name of the provider (for logging and identification) + /// Logger used for diagnostic output final Logger _logger;
76-88: Consider a GET fallback for servers that disallow HEADSome CDNs or WAF configurations reject
HEADrequests with 405 Method Not Allowed.
A lightweightGET(withrange: bytes=0-0oraccept: */*) as a fallback can salvage the time retrieval without noticeable overhead:- final response = await _httpClient.head(Uri.parse(url)).timeout(timeout); + http.Response response; + try { + response = await _httpClient.head(Uri.parse(url)).timeout(timeout); + } on HttpException catch (_) { + // Some servers return 405 for HEAD – retry with a minimal GET. + response = await _httpClient + .get(Uri.parse(url), headers: {'range': 'bytes=0-0'}) + .timeout(timeout); + }This is optional, yet it noticeably improves resilience in the wild.
If you adopt it, remember to keep the same status-code and header-parsing logic.lib/bloc/system_health/providers/http_time_provider.dart (1)
61-83: Minor:_parseTimeFromJsonneed not beasyncAfter the refactor above, every statement in this method is synchronous. You can:
-Future<DateTime> _parseTimeFromJson(Map<String, dynamic> jsonResponse) async { +DateTime _parseTimeFromJson(Map<String, dynamic> jsonResponse) { ... - return _parseDateTime(timeStr); + return _parseDateTime(timeStr); }Eliminating unnecessary
Futures reduces overhead and simplifies call-sites.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
lib/bloc/system_health/providers/binance_time_provider.dart(1 hunks)lib/bloc/system_health/providers/http_head_time_provider.dart(1 hunks)lib/bloc/system_health/providers/http_time_provider.dart(1 hunks)lib/bloc/system_health/providers/ntp_time_provider.dart(1 hunks)lib/bloc/system_health/providers/time_provider.dart(1 hunks)lib/bloc/system_health/system_clock_repository.dart(1 hunks)lib/bloc/system_health/system_health_bloc.dart(2 hunks)lib/services/initializer/app_bootstrapper.dart(1 hunks)test_units/tests/system_health/http_head_time_provider_test.dart(1 hunks)test_units/tests/system_health/http_time_provider_test.dart(1 hunks)test_units/tests/system_health/ntp_time_provider_test.dart(1 hunks)test_units/tests/system_health/system_clock_repository_test.dart(1 hunks)test_units/tests/system_health/time_provider_registry_test.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- lib/bloc/system_health/providers/time_provider.dart
- lib/bloc/system_health/providers/binance_time_provider.dart
- lib/services/initializer/app_bootstrapper.dart
- test_units/tests/system_health/time_provider_registry_test.dart
- test_units/tests/system_health/http_time_provider_test.dart
- lib/bloc/system_health/system_clock_repository.dart
- lib/bloc/system_health/system_health_bloc.dart
🧰 Additional context used
🧠 Learnings (2)
test_units/tests/system_health/system_clock_repository_test.dart (2)
Learnt from: takenagain
PR: KomodoPlatform/komodo-wallet#2611
File: test_units/tests/system_health/system_clock_repository_test.dart:72-77
Timestamp: 2025-05-08T19:08:38.518Z
Learning: In Dart, `DateTime.timestamp()` constructs a DateTime with the current UTC date and time, equivalent to `DateTime.now().toUtc()`. It was introduced in Dart 3.0.
Learnt from: takenagain
PR: KomodoPlatform/komodo-wallet#2611
File: lib/bloc/system_health/system_clock_repository.dart:41-45
Timestamp: 2025-05-08T19:06:13.560Z
Learning: `DateTime.timestamp()` constructor was introduced in Dart 3.0 and constructs a DateTime with the current UTC date and time. It's equivalent to `DateTime.now().toUtc()` but more concise.
lib/bloc/system_health/providers/http_time_provider.dart (1)
Learnt from: takenagain
PR: KomodoPlatform/komodo-wallet#2608
File: lib/views/fiat/fiat_form.dart:235-254
Timestamp: 2025-05-01T20:52:26.713Z
Learning: In Dart, non-empty switch case statements do not require explicit `break` statements at the end as they don't fall through to the next case by default. Adding unnecessary breaks is against the "unnecessary_breaks" linter rule (https://dart.dev/tools/linter-rules/unnecessary_breaks).
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build Mobile (Android)
- GitHub Check: Build Desktop (windows)
- GitHub Check: Build Mobile (iOS)
🔇 Additional comments (7)
test_units/tests/system_health/ntp_time_provider_test.dart (1)
5-22: Well-structured test for failure scenario.The test appropriately focuses on the error case that can be deterministically tested without external dependencies. I appreciate the clear comment explaining why a success test would be better as an integration test since it would require a real NTP server.
lib/bloc/system_health/providers/ntp_time_provider.dart (2)
8-36: Well-designed provider with good defaults and clear interface.This implementation provides good default NTP servers with appropriate fallbacks, configurable timeout and retry settings, and a clear interface following the TimeProvider contract.
37-76: Robust error handling with comprehensive logging.The getCurrentUtcTime method implementation demonstrates excellent practices:
- Server iteration with retries for each server
- Proper exception categorization and handling
- Detailed logging at appropriate levels (fine for success, warning for retries, severe for complete failure)
- Clear timeout exception if all servers fail
test_units/tests/system_health/http_head_time_provider_test.dart (2)
8-92: Comprehensive test coverage for HttpHeadTimeProvider.The test suite effectively covers all scenarios:
- Success case with valid date header
- Error cases: missing header, non-200 status, server failures, timeouts, and invalid date format
- Proper setup and teardown of test environment
This ensures robustness of the HttpHeadTimeProvider implementation.
94-111: Well-implemented MockClient for controlled testing.The MockClient class is elegantly designed to:
- Support configurable responses
- Simulate timeouts
- Override the necessary BaseClient methods
This enables deterministic testing of the HTTP interactions without relying on actual network calls.
test_units/tests/system_health/system_clock_repository_test.dart (2)
7-91: Well-structured tests with comprehensive scenario coverage.The test suite effectively verifies the SystemClockRepository behavior:
- Valid time provider returns true
- Invalid time (too different) returns false
- Provider exceptions are handled gracefully
- Provider ordering is respected (stops after first valid response)
This thorough testing ensures the repository behaves correctly in various scenarios.
93-131: Effective mock implementations for controlled testing.The mock classes (MockTimeProviderRegistry and MockTimeProvider) are well-designed:
- They implement just what's needed for testing
- Allow configuring return values and exceptions
- Track call counts to verify ordering behavior
This approach enables precise verification of the repository's interaction with its dependencies.
takenagain
force-pushed
the
bugfix/clock-api-errors
branch
from
df2e07e to
3b6196e
Compare
takenagain
requested review from
AndrewDelaney,
CharlVS,
ShantanuSharma9873 and
smk762
smk762
left a comment
smk762
left a comment
There was a problem hiding this comment.
LGTM!
Confirmed banner visible when time is over or under actual by 2 mins.
Confirmed banner removed once time is prperly sync'd
All platforms
Desktop and Mobile
CORS rules and restrictions on web require the server explicitly allow CORS for any domain and explicitly expose the
dateheader. For this reason, thedateheader extraction is only enabled on native, non-web, platforms (mobile & desktop).Testing
Requires a local debug build to view the DEX or Bridge pages that are hidden in the preview builds. Modifying system time by 2 or more minutes should trigger the banner. There might be a ~60-second delay, as the periodic system clock check happens once every 60 seconds.
Time Provider Availability by Platform
Provider Priority
Native Platforms (Desktop/Mobile)
Web Platforms
Notes
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores