feat(auth): poll trezor connection status and sign out when disconnected#126
feat(auth): poll trezor connection status and sign out when disconnected#126
Conversation
* Implement connection status stream * fix(types): use abstract classes for freezed models
|
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. 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 WalkthroughAdds Trezor connection-status RPC, parsed enum, repository polling/streaming, a TrezorConnectionMonitor class, wiring of monitoring into TrezorAuthService, new Freezed trezor types, a generic poll utility, test suites, and small example/packaging updates. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant TrezorAuthService
participant TrezorConnectionMonitor
participant TrezorRepository
participant RPCClient
App->>TrezorAuthService: signIn/register or auth stream
TrezorAuthService->>TrezorConnectionMonitor: _startConnectionMonitoring(devicePubkey?)
TrezorConnectionMonitor->>TrezorRepository: watchConnectionStatus(devicePubkey, pollInterval, maxDuration)
loop polling
TrezorRepository->>RPCClient: connectionStatus request
RPCClient-->>TrezorRepository: status response
TrezorRepository-->>TrezorConnectionMonitor: TrezorConnectionStatus
TrezorConnectionMonitor-->>TrezorAuthService: onStatusChanged / onConnectionLost / onConnectionRestored
alt onConnectionLost
TrezorAuthService->>TrezorAuthService: _signOutCurrentTrezorUser() / stop monitoring
end
end
App->>TrezorAuthService: signOut / dispose
TrezorAuthService->>TrezorConnectionMonitor: _stopConnectionMonitoring()/dispose()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Visit the preview URL for this PR (updated for commit 4ec1b11): https://komodo-defi-sdk--pr126-feat-trezor-connecti-wzjic6mx.web.app (expires Tue, 19 Aug 2025 18:46:58 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 7f9f5ac39928f333b6e8fcefb7138575e24ed347 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
packages/komodo_defi_local_auth/lib/src/trezor/trezor_initialization_state.dart (1)
26-69: Consider replacing fragile status string literals with a sealed enum
response.statusis compared against raw strings ('Ok','Error', …). A single typo or backend change will silently route the logic to thedefaultbranch and surface as a generic “Unknown status” error.- switch (response.status) { - case 'Ok': + switch (TrezorRpcStatus.from(response.status)) { + case TrezorRpcStatus.ok:Advantages:
• Compile-time exhaustiveness checks
• IDE autocomplete instead of magic strings
• Easier refactors when the backend adds a statusIf a direct enum mapping from the RPC layer isn’t feasible, at least centralise the string constants.
🧹 Nitpick comments (4)
packages/komodo_defi_types/lib/komodo_defi_type_utils.dart (1)
12-12: Export looks good – verify public API docs reference the new util
poll_utils.dartwill now be part of the public surface. Ensure the package-level README or API docs mention the newpollhelper so downstream packages know it exists.packages/komodo_defi_types/lib/src/trezor/trezor_device_info.dart (2)
4-4:non_abstract_class_inherits_abstract_memberignore is redundant
freezed-generated classes no longer require this suppression; leaving it in bloats lints.-// ignore_for_file: non_abstract_class_inherits_abstract_member
14-24: ExposetoJson()for symmetryThe class defines
fromJsonmanually but relies on the generatedtoJsonmethod implicitly. For clarity and discoverability, expose it explicitly:/// Construct a [TrezorDeviceInfo] from json. factory TrezorDeviceInfo.fromJson(JsonMap json) => _$TrezorDeviceInfoFromJson(json); + /// Convert this object to json. + JsonMap toJson() => _$TrezorDeviceInfoToJson(this);This prevents consumers from wondering whether a
toJsonexists and avoids relying on generated-member discovery.packages/komodo_defi_local_auth/lib/src/trezor/trezor_connection_status.dart (1)
19-32: Consider string casing consistency between parsing and display.The
fromStringmethod expects lowercase input ('connected', 'disconnected'), while thevaluegetter returns capitalized strings ('Connected', 'Disconnected'). This may be intentional for display purposes, but ensure this design is consistent with API expectations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
packages/komodo_defi_local_auth/lib/src/trezor/_trezor_index.dart(1 hunks)packages/komodo_defi_local_auth/lib/src/trezor/trezor_auth_service.dart(8 hunks)packages/komodo_defi_local_auth/lib/src/trezor/trezor_connection_monitor.dart(1 hunks)packages/komodo_defi_local_auth/lib/src/trezor/trezor_connection_status.dart(1 hunks)packages/komodo_defi_local_auth/lib/src/trezor/trezor_initialization_state.dart(1 hunks)packages/komodo_defi_local_auth/lib/src/trezor/trezor_repository.dart(2 hunks)packages/komodo_defi_rpc_methods/lib/src/rpc_methods/trezor/trezor_rpc_namespace.dart(4 hunks)packages/komodo_defi_types/lib/komodo_defi_type_utils.dart(1 hunks)packages/komodo_defi_types/lib/src/trezor/trezor_device_info.dart(1 hunks)packages/komodo_defi_types/lib/src/trezor/trezor_device_info.freezed.dart(1 hunks)packages/komodo_defi_types/lib/src/trezor/trezor_device_info.g.dart(1 hunks)packages/komodo_defi_types/lib/src/trezor/trezor_user_action_data.dart(1 hunks)packages/komodo_defi_types/lib/src/trezor/trezor_user_action_data.freezed.dart(1 hunks)packages/komodo_defi_types/lib/src/trezor/trezor_user_action_data.g.dart(1 hunks)packages/komodo_defi_types/lib/src/types.dart(1 hunks)packages/komodo_defi_types/lib/src/utils/poll_utils.dart(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/komodo_defi_rpc_methods/lib/src/rpc_methods/trezor/trezor_rpc_namespace.dart (1)
Learnt from: takenagain
PR: KomodoPlatform/komodo-defi-sdk-flutter#70
File: packages/komodo_defi_rpc_methods/lib/src/rpc_methods/swaps/legacy/cancel_order.dart:8-9
Timestamp: 2025-06-02T12:44:56.953Z
Learning: Legacy RPC methods in the Komodo DeFi SDK (located in `/legacy/` subdirectories) intentionally use `mmrpc: null` in their constructors, unlike modern RPC methods that use `mmrpc: "2.0"`. This is by design and should not be changed to maintain backward compatibility.
⏰ 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). (2)
- GitHub Check: build_and_preview_sdk_example_preview
- GitHub Check: build_and_preview_playground_preview
🔇 Additional comments (17)
packages/komodo_defi_local_auth/lib/src/trezor/trezor_initialization_state.dart (1)
4-4: Import correctly points to relocated types – good catchSwitching the
TrezorDeviceInfosource tokomodo_defi_typeskeeps the layer-boundaries clean after the refactor.packages/komodo_defi_types/lib/src/types.dart (1)
59-60: 👍 Centralising Trezor types improves reuseExporting
trezor_device_info.dartandtrezor_user_action_data.dartfrom the sharedtypespackage removes duplication across modules and simplifies dependency graphs.packages/komodo_defi_local_auth/lib/src/trezor/_trezor_index.dart (1)
7-8: New exports keep the private index in sync
trezor_connection_monitor.dartandtrezor_connection_status.dartare now reachable from the internal barrel. No issues spotted.packages/komodo_defi_types/lib/src/trezor/trezor_user_action_data.g.dart (1)
1-30: Generated JSON serialization code looks correct.The auto-generated JSON serialization and deserialization logic properly handles the enum mapping and nullable fields as expected.
packages/komodo_defi_types/lib/src/trezor/trezor_device_info.g.dart (1)
1-26: Generated JSON serialization code looks correct.The auto-generated JSON serialization and deserialization logic properly handles both required and optional fields with appropriate field mapping.
packages/komodo_defi_local_auth/lib/src/trezor/trezor_connection_status.dart (1)
50-61: Helper methods implement correct status logic.The boolean getters appropriately categorize connection states:
isAvailablecorrectly identifies only connected stateisUnavailableproperly includes disconnected and unreachable statesshouldContinueMonitoringcorrectly excludes only disconnected state for polling controlpackages/komodo_defi_types/lib/src/utils/poll_utils.dart (1)
10-52: Well-designed polling utility with robust error handling.The implementation correctly handles:
- Timeout enforcement with stopwatch
- Configurable backoff strategies
- Selective error continuation
- Proper attempt tracking and callbacks
The logic flow and error handling patterns are appropriate for a generic polling utility.
packages/komodo_defi_types/lib/src/trezor/trezor_user_action_data.dart (3)
9-17: Enum implementation with proper JSON serialization.The
TrezorUserActionTypeenum correctly uses@JsonEnumwithvalueFieldto map enum values to their string representations for JSON serialization.
22-26: Validation logic correctly enforces data consistency.The
@Assertannotation properly validates that:
- PIN is provided when
actionTypeisTrezorPin- Passphrase is provided when
actionTypeisTrezorPassphraseThis prevents invalid state combinations at compile time.
28-36: Well-structured data class with appropriate serialization.The
TrezorUserActionDataclass correctly uses:
@freezedfor immutable data structure@JsonSerializablewith snake_case field naming for API consistency- Proper factory method for JSON deserialization
packages/komodo_defi_types/lib/src/trezor/trezor_device_info.freezed.dart (1)
1-245: LGTM! Generated Freezed file.This is a properly generated Freezed file with standard implementations for immutable data classes.
packages/komodo_defi_types/lib/src/trezor/trezor_user_action_data.freezed.dart (1)
1-203: LGTM! Generated Freezed file with proper validation.This generated file includes appropriate validation logic ensuring PIN/passphrase requirements are met based on the action type.
packages/komodo_defi_local_auth/lib/src/trezor/trezor_auth_service.dart (1)
216-219: Potential redundant monitoring stop operations.When connection is lost,
_signOutCurrentTrezorUseris called (line 218), which stops monitoring (line 236). However, the monitor is still actively running when this happens. Consider whether this could cause any side effects.Verify the behavior when
stopMonitoringis called while the monitor is actively processing a disconnection event.Also applies to: 232-237
packages/komodo_defi_rpc_methods/lib/src/rpc_methods/trezor/trezor_rpc_namespace.dart (4)
3-4: LGTM! Import refactoring aligns with centralized type management.The imports properly reference the shared types from the
komodo_defi_typespackage, which improves code organization and reusability across the codebase.
110-121: LGTM! Well-documented connection status method.The implementation follows the established pattern of other RPC methods in the namespace:
- Proper documentation with clear description
- Consistent method signature with optional parameters
- Uses the correct request/response types
- Follows the same execution pattern as other methods
210-227: LGTM! Request class follows established conventions.The
TrezorConnectionStatusRequestclass properly:
- Extends
BaseRequestwith correct generic types- Uses modern MMRPC version
'2.0'(consistent with other non-legacy methods)- Handles optional
devicePubkeyparameter correctly in JSON serialization- Uses appropriate
fromJsonparsing method for the response
322-341: LGTM! Response class handles JSON parsing correctly.The
TrezorConnectionStatusResponseclass demonstrates good practices:
- Uses
valueOrNullfor potentially nullmmrpcfield- Uses
valuefor requiredstatusfield with proper error handling- Follows the expected JSON structure with
result.statusnesting- Implements proper
toJsonserialization
…nd error handling (#166) Co-authored-by: Cursor Agent <cursoragent@cursor.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/komodo_defi_local_auth/lib/src/trezor/trezor_repository.dart (1)
208-249: Add defensive check for nulllastvalue before the while loop.While the initial try-catch ensures
lastis assigned, adding a defensive check would make the code more robust against future modifications.+ if (last == null) { + // Defensive: should not happen, but ensures safety + yield TrezorConnectionStatus.unreachable; + return; + } + while (last.shouldContinueMonitoring && stopwatch.elapsed < maxDuration) {packages/komodo_defi_local_auth/lib/src/trezor/trezor_connection_monitor.dart (1)
66-71: Don't treat every stream error as a disconnection; classify errors before signing the user out.Right now any error while listening triggers onConnectionLost (unless already disposed), which risks false sign-outs from transient issues. Add an error classifier and only trigger sign-out for genuine connection-loss errors. This matches the prior feedback and avoids unnecessary logouts.
Apply this diff to gate the callback on connection-related errors:
- onError: (Object error, StackTrace stackTrace) { - _log.severe('Error monitoring Trezor connection: $error', error, stackTrace); - // Only call onConnectionLost if this is a real connection error, not a disposal - if (_connectionSubscription != null) { - onConnectionLost?.call(); - } - }, + onError: (Object error, StackTrace stackTrace) { + _log.severe('Error monitoring Trezor connection: $error', error, stackTrace); + // Only treat specific errors as connection lost (avoid false sign-outs) + if (_connectionSubscription != null && _isConnectionError(error)) { + onConnectionLost?.call(); + } else { + _log.fine('Non-critical error during monitoring; ignoring.'); + } + },Add this helper inside the class (outside the selected range):
bool _isConnectionError(Object error) { final msg = error.toString().toLowerCase(); // Tweak predicates to match your repo/RPC error surface return msg.contains('device not found') || msg.contains('disconnected') || msg.contains('unreachable') || msg.contains('no such device'); }packages/komodo_defi_local_auth/lib/src/trezor/trezor_auth_service.dart (1)
208-221: Good: devicePubkey is now threaded through to the monitor.This resolves the prior feedback about ignoring the devicePubkey parameter. Consider populating devicePubkey from the authenticated device context when available to ensure precise tracking if multiple devices can be connected.
If the repository exposes the active device's pubkey (e.g., via getConnectionStatus or a stored field after init), pass it here:
_startConnectionMonitoring(devicePubkey: activeDevicePubkey);Please verify whether such a getter exists in TrezorRepository or the initialization result so the monitor can filter to the correct device.
🧹 Nitpick comments (8)
packages/komodo_defi_sdk/example/pubspec.yaml (2)
11-12: Consider using a git reference or version for thedragon_logsdependency.The
dragon_logsdependency uses a local path reference. If this package is intended for broader use or CI/CD pipelines, consider using a git reference or published version to ensure reproducibility and avoid path resolution issues in different environments.
31-31: Consider using a more flexible logging version constraint.The logging dependency is pinned to exactly version 1.3.0. Consider using a version constraint like
^1.3.0to allow patch updates while maintaining compatibility.- logging: ^1.3.0 + logging: ^1.3.0packages/komodo_defi_local_auth/lib/src/trezor/trezor_repository.dart (1)
225-229: Consider yielding a disconnected status on initial failure instead of throwing.The current implementation throws an exception if the initial status check fails. For consistency with the polling loop error handling (which yields
disconnected), consider yielding a disconnected status instead.try { last = await getConnectionStatus(devicePubkey: devicePubkey); yield last; } catch (e) { - // If initial status check fails, we can't proceed - throw TrezorException( - 'Failed to get initial connection status', - e.toString(), - ); + // If initial status check fails, treat as disconnected + yield TrezorConnectionStatus.disconnected; + return; }packages/komodo_defi_local_auth/lib/src/trezor/trezor_connection_monitor.dart (2)
29-31: Monitoring timeout (30m) risks leaving users signed in but unmonitored; prefer indefinite or explicit auto-restart.After maxDuration elapses, onDone fires and monitoring stops with only a log line. If the user remains signed in, they won't be auto-signed out on later disconnections. Recommend one of:
- Make monitoring default indefinite (preferred), or
- Auto-restart monitoring onDone, or
- Treat unexpected onDone as connection loss and sign the user out.
Optional diffs to consider:
- Extend the default duration to a safer window (e.g., 24h):
- Duration maxDuration = const Duration(minutes: 30), + Duration maxDuration = const Duration(hours: 24),
- Auto-restart when the stream ends unexpectedly:
- onDone: () { - _log.info('Trezor connection monitoring stopped'); - }, + onDone: () { + _log.info('Trezor connection monitoring stopped'); + // Optional: auto-restart to avoid leaving user unmonitored + _log.fine('Auto-restarting Trezor connection monitoring'); + startMonitoring( + devicePubkey: devicePubkey, + pollInterval: pollInterval, + maxDuration: maxDuration, + onConnectionLost: onConnectionLost, + onConnectionRestored: onConnectionRestored, + onStatusChanged: onStatusChanged, + ); + },If the repository deliberately completes the stream (e.g., maintenance), confirm whether onDone should sign out the user instead. If you prefer indefinite monitoring, we can remove maxDuration entirely assuming repository supports it.
Also applies to: 73-76
80-87: Await subscription cancellation to avoid racey onError/onDone after stop.stopMonitoring currently ignores the Future returned by cancel(). Making it async and awaiting cancellation improves determinism and prevents post-stop events from racing with cleanup.
Apply this diff:
- /// Stop monitoring the Trezor connection status. - void stopMonitoring() { - if (_connectionSubscription != null) { - _log.info('Stopping Trezor connection monitoring'); - _connectionSubscription?.cancel(); - _connectionSubscription = null; - _lastStatus = null; - } - } + /// Stop monitoring the Trezor connection status. + Future<void> stopMonitoring() async { + if (_connectionSubscription != null) { + _log.info('Stopping Trezor connection monitoring'); + await _connectionSubscription?.cancel(); + _connectionSubscription = null; + _lastStatus = null; + } + }Note: This change requires awaiting stopMonitoring() at call sites (see suggestions in TrezorAuthService).
packages/komodo_defi_local_auth/lib/src/trezor/trezor_auth_service.dart (3)
111-114: Stop monitoring before sign-out is correct; consider awaiting if stop becomes async.The ordering is right. If you adopt the async stopMonitoring from the monitor, update this to await the stop.
- Future<void> signOut() async { - _stopConnectionMonitoring(); - await _authService.signOut(); - } + Future<void> signOut() async { + await _stopConnectionMonitoring(); + await _authService.signOut(); + }
223-228: Make _stopConnectionMonitoring async to await underlying cancellation.If you adopt the async stop in the monitor, mirror it here to ensure proper sequencing during teardown.
- /// Stop monitoring Trezor connection status. - void _stopConnectionMonitoring() { - if (_connectionMonitor.isMonitoring) { - _connectionMonitor.stopMonitoring(); - } - } + /// Stop monitoring Trezor connection status. + Future<void> _stopConnectionMonitoring() async { + if (_connectionMonitor.isMonitoring) { + await _connectionMonitor.stopMonitoring(); + } + }
230-241: Optional: move _stopConnectionMonitoring() outside try/catch to avoid swallowing unexpected failures.Unlikely to throw, but placing it outside the try keeps the catch focused on sign-out errors.
- try { - _stopConnectionMonitoring(); - await _authService.signOut(); - } catch (_) { + _stopConnectionMonitoring(); + try { + await _authService.signOut(); + } catch (_) { // ignore sign out errors }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/komodo_defi_sdk/example/pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
packages/komodo_defi_local_auth/lib/src/trezor/trezor_auth_service.dart(7 hunks)packages/komodo_defi_local_auth/lib/src/trezor/trezor_connection_monitor.dart(1 hunks)packages/komodo_defi_local_auth/lib/src/trezor/trezor_connection_status.dart(1 hunks)packages/komodo_defi_local_auth/lib/src/trezor/trezor_repository.dart(2 hunks)packages/komodo_defi_rpc_methods/lib/src/rpc_methods/trezor/trezor_rpc_namespace.dart(4 hunks)packages/komodo_defi_sdk/example/lib/blocs/auth/auth_bloc.dart(1 hunks)packages/komodo_defi_sdk/example/lib/blocs/auth/trezor_auth_mixin.dart(9 hunks)packages/komodo_defi_sdk/example/pubspec.yaml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/komodo_defi_local_auth/lib/src/trezor/trezor_connection_status.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). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (18)
packages/komodo_defi_sdk/example/pubspec.yaml (2)
19-20: LGTM! The komodo_defi_rpc_methods dependency is correctly configured.The new dependency enables the Trezor RPC functionality introduced in this PR.
39-40: LGTM! The integration_test dependency reordering is appropriate.The dependency was reordered but remains unchanged in functionality.
packages/komodo_defi_rpc_methods/lib/src/rpc_methods/trezor/trezor_rpc_namespace.dart (3)
3-4: LGTM! Import statement correctly updated to use shared types.The types have been properly migrated to the shared types package, promoting better code organization and reusability.
111-121: LGTM! Well-documented connection status method.The new
connectionStatusmethod is properly implemented and follows the established patterns in the codebase.
210-227: LGTM! Request class properly structured.The
TrezorConnectionStatusRequestclass follows the established request pattern with appropriate optional parameter handling.packages/komodo_defi_sdk/example/lib/blocs/auth/auth_bloc.dart (2)
9-9: LGTM! Logging import added for enhanced debugging.The logging package import enables better debugging and monitoring of the authentication flow.
14-14: LGTM! Part directive reordering is appropriate.The reordering places the mixin after the event definitions, which is a logical organization.
packages/komodo_defi_sdk/example/lib/blocs/auth/trezor_auth_mixin.dart (4)
7-7: LGTM! Logger instance properly configured.The static logger follows best practices with an appropriate name for identification.
15-20: LGTM! Event handler registration with appropriate logging.The setup method properly logs the registration of event handlers and includes the new cancellation handler.
68-69: LGTM! Improved error handling with stack trace logging.Capturing and logging the stack trace will help with debugging production issues.
187-195: LGTM! Cancellation handler properly implemented.The new cancellation handler correctly logs the event and transitions to the unauthenticated state.
packages/komodo_defi_local_auth/lib/src/trezor/trezor_repository.dart (2)
193-198: LGTM! Connection status method properly implemented.The method correctly delegates to the RPC client and follows the established pattern.
200-206: LGTM! Status parsing method is well-structured.The method properly converts the raw response to the enum type.
packages/komodo_defi_local_auth/lib/src/trezor/trezor_auth_service.dart (5)
19-21: Constructor wiring of TrezorConnectionMonitor looks correct.Using the repository param to initialize the monitor in the initializer list is sound and keeps responsibilities separated.
105-108: Dispose sequencing LGTM.Disposing the connection monitor before the composed auth service is the right order and avoids dangling callbacks.
136-141: Starting monitoring after password-based init is appropriate for this non-streamed path.This addresses the earlier concern about duplicate starts: signIn here uses _initializeTrezorWithPassphrase (not the streamed path), so starting monitoring here is necessary and not redundant with _authenticateTrezorStream.
168-173: Same as above: non-stream register path correctly starts monitoring post-auth.No duplication with stream path; this is the required spot for the imperative path.
328-328: Stream path startMonitoring is correct; ensure you don't accidentally start twice.Given the structure, this path is mutually exclusive with the imperative sign-in/register paths, so no double-start occurs. Keep it this way to avoid duplicate subscriptions.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds real-time Trezor connection monitoring with automatic sign-out functionality when the device becomes disconnected. It introduces connection status polling, new data models for Trezor operations, and a polling utility for robust background operations.
Key changes:
- Implements Trezor connection status monitoring with automatic sign-out on disconnect
- Adds new data models for TrezorDeviceInfo and TrezorUserActionData with proper validation
- Introduces a generic polling utility with configurable backoff strategies and error handling
Reviewed Changes
Copilot reviewed 29 out of 30 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/komodo_defi_types/lib/src/utils/poll_utils.dart | New polling utility with timeout, backoff, and error handling |
| packages/komodo_defi_types/test/utils/poll_utils_test.dart | Comprehensive test suite for the polling utility |
| packages/komodo_defi_types/lib/src/trezor/ | New data models for Trezor device info and user actions |
| packages/komodo_defi_local_auth/lib/src/trezor/ | Connection monitoring service and repository enhancements |
| packages/komodo_defi_rpc_methods/lib/src/rpc_methods/trezor/ | RPC method updates for connection status queries |
Comments suppressed due to low confidence (1)
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
packages/komodo_defi_local_auth/lib/src/trezor/trezor_repository.dart (2)
217-227: Error handling for initial status check is appropriate.The method properly handles initial connection failures by yielding disconnected status and terminating the stream.
229-247: Consider documenting the timeout behavior more clearly.The behavior when
maxDurationexpires (emittingunreachablestatus) should be documented to clarify that this doesn't necessarily mean the device is disconnected, just that monitoring has timed out.Consider adding a comment to clarify the timeout behavior:
if (maxDuration != null && stopwatch!.elapsed >= maxDuration) { + // Emit unreachable to indicate monitoring timeout, not actual disconnection yield TrezorConnectionStatus.unreachable; }packages/komodo_defi_local_auth/lib/src/trezor/trezor_auth_service.dart (4)
241-252: Potential circular dependency issue.The
_stopConnectionMonitoring()call inside_signOutCurrentTrezorUser()could create unexpected behavior since this method is also called from the connection monitor'sonConnectionLostcallback (line 225).Consider checking if monitoring is active before stopping to avoid potential issues:
Future<void> _signOutCurrentTrezorUser() async { final current = await _authService.getActiveUser(); if (current?.walletId.name == trezorWalletName) { _log.warning("Signing out current '${current?.walletId.name}' user"); - await _stopConnectionMonitoring(); + if (_connectionMonitor.isMonitoring) { + await _stopConnectionMonitoring(); + } try { await _authService.signOut(); } catch (_) { // ignore sign out errors } } }
147-151: Connection monitoring is redundantly started.The connection monitoring is started here and also in
_authenticateTrezorStream(line 338). Since_initializeTrezorWithPassphraseinternally uses_initializeTrezorAndAuthenticate, which yields states processed elsewhere, the monitoring here appears redundant.
179-183: Connection monitoring is redundantly started.Similar to the signIn method, connection monitoring is started here and also in
_authenticateTrezorStream.
220-231: Use the devicePubkey parameter.The
devicePubkeyparameter is accepted but not used when starting monitoring.
🧹 Nitpick comments (1)
packages/komodo_defi_local_auth/test/src/trezor/trezor_connection_monitor_test.dart (1)
198-217: Consider verifying the error handling behavior more thoroughly.The test verifies that onConnectionLost is called when an error occurs, but it might be worth also verifying that the monitoring continues after the error (unless explicitly stopped).
Consider extending the test to verify post-error behavior:
test('onError triggers onConnectionLost while monitoring', () async { final repo = _TestTrezorRepository(); final monitor = TrezorConnectionMonitor(repo); var lost = 0; - monitor.startMonitoring(onConnectionLost: () => lost++); + var statusCount = 0; + monitor.startMonitoring( + onConnectionLost: () => lost++, + onStatusChanged: (_) => statusCount++, + ); // Emit any status to set previousStatus repo.emit(TrezorConnectionStatus.connected); await Future<void>.delayed(const Duration(milliseconds: 5)); + expect(statusCount, 1); // Now emit error from repository stream repo.emitError(Exception('stream failure')); await Future<void>.delayed(const Duration(milliseconds: 5)); expect(lost, 1); + + // Verify monitoring continues after error if not stopped + repo.emit(TrezorConnectionStatus.busy); + await Future<void>.delayed(const Duration(milliseconds: 5)); + expect(statusCount, 2); await monitor.stopMonitoring(); await repo.close(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/komodo_defi_local_auth/.gitignore(1 hunks)packages/komodo_defi_local_auth/lib/src/trezor/trezor_auth_service.dart(7 hunks)packages/komodo_defi_local_auth/lib/src/trezor/trezor_connection_monitor.dart(1 hunks)packages/komodo_defi_local_auth/lib/src/trezor/trezor_repository.dart(5 hunks)packages/komodo_defi_local_auth/test/src/trezor/trezor_auth_service_test.dart(1 hunks)packages/komodo_defi_local_auth/test/src/trezor/trezor_connection_monitor_test.dart(1 hunks)packages/komodo_defi_local_auth/test/src/trezor/trezor_repository_test.dart(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/komodo_defi_local_auth/.gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/komodo_defi_local_auth/lib/src/trezor/trezor_connection_monitor.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: build_and_preview_playground_preview
- GitHub Check: build_and_preview_sdk_example_preview
- GitHub Check: Cursor Bugbot
🔇 Additional comments (54)
packages/komodo_defi_local_auth/test/src/trezor/trezor_connection_monitor_test.dart (13)
1-11: LGTM!The imports and test setup are properly structured.
8-11: LGTM!Clean implementation of the dummy API client for test isolation.
13-51: LGTM!Well-designed test double that provides control over stream emissions for testing the TrezorConnectionMonitor behavior.
55-82: LGTM!Comprehensive test that verifies status emissions, lastKnownStatus tracking, and monitoring state management.
84-120: LGTM!Excellent test that validates the nuanced connection loss/restoration callback behavior across different status transitions.
122-147: LGTM!Good test coverage for the onConnectionRestored callback, ensuring it only triggers on unavailable-to-available transitions.
149-172: LGTM!Validates that monitoring parameters are correctly forwarded to the repository.
174-196: LGTM!Important test that verifies proper cleanup behavior when monitoring is stopped.
219-244: LGTM!Verifies that starting a new monitoring session properly replaces the previous one.
246-256: LGTM!Simple but essential test for dispose behavior.
258-281: LGTM!Good test that verifies monitoring state is properly updated when the underlying stream completes.
283-301: LGTM!Validates that errors after stopping monitoring are properly ignored.
303-330: LGTM!Important edge case test that verifies no spurious callbacks occur when monitoring is started and stopped without any events.
packages/komodo_defi_local_auth/test/src/trezor/trezor_auth_service_test.dart (13)
1-11: LGTM!The imports are properly organized and include all necessary dependencies for testing.
12-15: LGTM!Clean implementation of the dummy API client for test isolation.
17-56: LGTM!Well-designed fake repository that provides control over initialization events and tracks method invocations for test assertions.
58-97: LGTM!Good fake implementation of TrezorConnectionMonitor that tracks monitoring state and method calls.
99-201: LGTM!Comprehensive fake implementation of IAuthService that provides necessary functionality for testing TrezorAuthService behavior.
205-230: LGTM!Good test that validates the service correctly rejects non-Trezor authentication options.
232-257: LGTM!Validates that register method also properly rejects non-Trezor authentication options.
259-346: LGTM!Comprehensive test that validates the complete sign-in flow including wallet registration, password generation and storage, passphrase forwarding, and monitoring startup.
348-404: LGTM!Good test coverage for the streaming sign-in path, verifying state emissions and monitor activation.
406-442: LGTM!Important test that verifies proper error handling and cleanup during sign-in failures.
444-488: LGTM!Good edge case test that validates proper error handling when a stored password is missing for an existing user.
490-512: LGTM!Simple but important test for password cleanup functionality.
514-535: LGTM!Validates that sign-out properly stops monitoring and delegates to the underlying auth service.
packages/komodo_defi_local_auth/lib/src/trezor/trezor_repository.dart (5)
51-54: Good improvement to timer management.The addition of Timer variable and comment clearly indicates the intent to prevent timer leaks when streams are cancelled by subscribers.
119-125: LGTM! Prevents race conditions.Good change to avoid immediate polling and potential race conditions with KDF task creation. Using the polling interval for the first status check is a sensible approach.
128-134: Error handling improvement.The change from throwing a TrezorException to yielding an error state provides better error handling consistency with the stream-based API.
135-144: Excellent timer cleanup logic.The finally block ensures the timer is always cancelled, preventing memory leaks even if the stream is cancelled by the subscriber or exits early.
194-202: LGTM!Clean implementation of getConnectionStatus that properly fetches and parses the connection status.
packages/komodo_defi_local_auth/lib/src/trezor/trezor_auth_service.dart (5)
19-29: Good dependency injection design.The constructor now properly accepts optional dependencies with sensible defaults, improving testability and flexibility.
115-118: Proper lifecycle management.The dispose method correctly disposes the connection monitor before the auth service.
121-124: LGTM!The signOut method properly stops monitoring before signing out.
234-238: LGTM!Clean helper method for stopping monitoring with proper state checking.
337-339: LGTM!Properly starts monitoring after successful authentication in the streaming flow.
packages/komodo_defi_local_auth/test/src/trezor/trezor_repository_test.dart (18)
1-11: LGTM!The imports are properly organized with appropriate test utilities.
13-48: Well-designed fake API client.The FakeApiClient provides excellent control over RPC responses for testing, with proper request tracking and error handling.
50-91: LGTM!Clean helper functions for creating API response structures.
94-174: Comprehensive initialization test.Excellent test that verifies the complete initialization flow including status mapping and device info extraction.
176-210: LGTM!Good test for error handling when status returns an error response.
212-245: LGTM!Important test that verifies stream error handling when the status call throws an exception.
248-280: LGTM!Good input validation tests for PIN and passphrase methods.
282-320: LGTM!Validates cancellation behavior including state emission and proper cleanup.
322-350: LGTM!Good test coverage for connection status enum mapping.
352-396: LGTM!Excellent test that verifies status change detection and stream termination on disconnect.
398-443: LGTM!Important test that verifies polling stops when the listener cancels the subscription.
445-495: LGTM!Validates that polling stops after receiving a disconnected status.
497-527: LGTM!Verifies the timeout behavior that emits unreachable status after maxDuration.
529-558: LGTM!Good test for error handling during polling that properly emits disconnected status.
561-596: LGTM!Validates that dispose properly cancels active initializations.
599-639: LGTM!Good test that verifies null maxDuration behavior continues polling until disconnected.
640-676: LGTM!Validates that no immediate polling occurs with long intervals, preventing unnecessary status checks.
678-731: LGTM!Excellent test that ensures timers are properly cancelled when streams are cancelled, preventing resource leaks.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
freezed by default adds all fields to toString overrides, which could result in user pin/passphrases being printed to logs
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…dk-flutter into feat/trezor-connection-status # Conflicts: # packages/komodo_defi_sdk/example/pubspec.yaml
Rename the dragon charts folder to be consistent with the package name. NO FUNCTIONAL CHANGES
Signs the user out if Trezor connectivity is lost -
trezor_connection_statusreports device disconnected.Summary by CodeRabbit
New Features
Improvements
Utilities
Tests