Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes introduce HTTP client reset capability throughout the framework to recover from stale connections and transport failures, enhance KDF health verification in the auth service with concurrency control and forced restart logic, inject KMD-specific withdrawal parameters, and make the KmdRewards claimedByMe field optional. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant AuthService
participant KomodoDefiFramework
participant KdfOperations
User->>AuthService: signIn()
AuthService->>AuthService: log login attempt
AuthService->>AuthService: ensureKdfHealthy() with timeout
alt Health check in progress (single-flight)
AuthService->>AuthService: wait for ongoing check
else First check or cooldown expired
AuthService->>AuthService: _performHealthCheck()
AuthService->>KomodoDefiFramework: isHealthy()
KomodoDefiFramework->>KdfOperations: executeRpc(version)
alt Success
KdfOperations-->>KomodoDefiFramework: result
KomodoDefiFramework-->>AuthService: true
else Fatal transport error (e.g., ECONNRESET)
KdfOperations->>KdfOperations: detect SocketException
KdfOperations->>KomodoDefiFramework: resetHttpClient()
KomodoDefiFramework->>KdfOperations: resetHttpClient()
KdfOperations->>KdfOperations: close + reinit HTTP client
KdfOperations-->>KomodoDefiFramework: error logged & rethrown
end
alt Still unhealthy after check
AuthService->>AuthService: _forceStartKdf()
AuthService->>AuthService: _verifyKdfHealthy()
alt Still fails after retry
AuthService-->>User: throw AuthException (KDF unavailable)
else Healthy after restart
AuthService->>AuthService: emit logged-out state (if user was signed in)
AuthService-->>User: proceed with signIn
end
else Healthy
AuthService-->>User: proceed with signIn
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Suggested labels
Poem
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (10)
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 |
There was a problem hiding this comment.
Pull Request Overview
This PR implements robust KDF health check mechanisms and KMD withdrawal support improvements. It addresses iOS backgrounding issues where KDF becomes unresponsive but reports as running, causing authentication failures.
- Enhanced KDF health checking with double verification, single-flight guards, and cooldown mechanisms
- Made
KmdRewards.claimedByMenullable to handle optional API responses - Added automatic KMD rewards parameters to withdrawal requests
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
withdrawal_types.dart |
Made claimedByMe field optional/nullable in KmdRewards class |
withdraw_request.dart |
Added automatic KMD rewards parameters for KMD coin withdrawals |
auth_service.dart |
Implemented comprehensive health check with single-flight guards, cooldown, and proactive KDF verification before login |
komodo_defi_framework.dart |
Enhanced version RPC with timing logs, improved isHealthy() to rely only on RPC verification, added HTTP client reset and transport error detection |
kdf_operations_*.dart |
Implemented resetHttpClient() across all operation implementations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| final errorType = errorString.contains('errno = 32') || errorString.contains('broken pipe') ? 'EPIPE (32)' : | ||
| errorString.contains('errno = 54') || errorString.contains('connection reset') ? 'ECONNRESET (54)' : | ||
| errorString.contains('errno = 60') || errorString.contains('operation timed out') ? 'ETIMEDOUT (60)' : | ||
| 'ECONNREFUSED (61)'; |
There was a problem hiding this comment.
The nested ternary operator chain for error type mapping is difficult to read and maintain. Consider refactoring to use a helper function or if-else statements for better clarity.
| final now = DateTime.now(); | ||
| if (_lastHealthCheckCompleted != null) { | ||
| final timeSinceLastCheck = now.difference(_lastHealthCheckCompleted!); | ||
| if (timeSinceLastCheck.inSeconds < 2) { |
There was a problem hiding this comment.
The cooldown period of 2 seconds is a magic number. Consider extracting it as a named constant (e.g., _kHealthCheckCooldownSeconds) to improve maintainability and make the purpose clear.
| onTimeout: () { | ||
| _logger.warning('[$_sessionId] signIn: Health check timed out after 3s'); | ||
| return false; | ||
| }, |
There was a problem hiding this comment.
The timeout duration of 3 seconds is hardcoded in multiple places (lines 140, 152, 578, 590, 618, 652). Consider extracting these timeout values as named constants to avoid inconsistencies and improve maintainability.
| // Add 200ms delay after restart before verification to avoid race where | ||
| // native status reports "up" but HTTP listener hasn't bound yet | ||
| _logger.info('[$_sessionId] _performHealthCheck: Waiting 200ms for HTTP listener to bind'); | ||
| await Future.delayed(const Duration(milliseconds: 200)); |
There was a problem hiding this comment.
The 200ms delay after restart is a magic number. Consider extracting it as a named constant (e.g., _kHttpListenerBindDelayMs) with a clear name explaining why this delay is necessary.
|
Visit the preview URL for this PR (updated for commit fed0e3c): https://kdf-sdk--pr267-dev-5dgxb221.web.app (expires Wed, 05 Nov 2025 01:12:26 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 9c1b6e6c010cf0b965c455ba7a69c4aedafa8a1d |
|
Visit the preview URL for this PR (updated for commit fed0e3c): https://komodo-playground--pr267-dev-dsd1z56b.web.app (expires Wed, 05 Nov 2025 01:14:44 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 2bfedd77fdea45b25ba7c784416e81f177aa5c47 |
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements