-
Notifications
You must be signed in to change notification settings - Fork 17
β¨ (dmk) [DSDK-945]: Improve refresher after new intent queue impl #1114
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
ed19525 to
823e2fe
Compare
823e2fe to
8e5907f
Compare
8e5907f to
f061f08
Compare
f061f08 to
f9de41d
Compare
f9de41d to
7a95546
Compare
e4d3b3f to
99d6e1b
Compare
| intentQueueServiceFactory: ( | ||
| sessionEventDispatcher: DeviceSessionEventDispatcher, | ||
| ) => IntentQueueService = (sessionEventDispatcher) => | ||
| new IntentQueueService(loggerModuleFactory, sessionEventDispatcher), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[ASK] Why are you using a factory here when you instantiate the class immediately
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's easier to test it as I can just inject a mock
packages/device-management-kit/src/internal/send/use-case/SendApduUseCase.ts
Outdated
Show resolved
Hide resolved
99d6e1b to
89be3e5
Compare
89be3e5 to
037be01
Compare
037be01 to
412c59a
Compare
412c59a to
7fcfbca
Compare
ofreyssinet-ledger
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changes requested in #1111 to either do there first or here
287db77 to
6bfc631
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR improves device session management by introducing a new intent queue system that coordinates with the device session refresher. The main enhancement is that the refresher now intelligently avoids pinging the device when it's already busy processing operations, preventing conflicts and unnecessary network calls.
Key Changes:
- Replaces
MutexServicewith a newIntentQueueServicethat provides better coordination between operations and the refresher through event dispatching - Removes
disableRefresherfromInternalApias the refresher is now smart enough to automatically skip refreshes during active operations - Implements a pending device status system in
DeviceSessionStateHandlerto better track device state transitions
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
packages/device-management-kit/src/internal/device-session/service/IntentQueueService.ts |
New intent queue implementation with comprehensive documentation and FIFO sequential processing |
packages/device-management-kit/src/internal/device-session/service/IntentQueueService.test.ts |
Comprehensive test coverage for the new intent queue service |
packages/device-management-kit/src/internal/device-session/model/DeviceSession.ts |
Refactored sendApdu, sendCommand, and executeDeviceAction to use the intent queue; added bypass capability for testing |
packages/device-management-kit/src/internal/device-session/model/DeviceSession.test.ts |
New comprehensive tests for DeviceSession functionality |
packages/device-management-kit/src/internal/device-session/model/DeviceSessionRefresher.ts |
Enhanced to track device busy state and skip refreshes during intent processing |
packages/device-management-kit/src/internal/device-session/model/DeviceSessionStateHandler.ts |
Implements pending status system for better state tracking; adds exhaustive switch case handling |
packages/device-management-kit/src/internal/device-session/model/DeviceSessionEventDispatcher.ts |
Adds NEW_STATE and DEVICE_STATE_UPDATE_UNKNOWN events for improved state coordination |
packages/device-management-kit/src/internal/device-session/service/MutexService.ts |
Removed - replaced by IntentQueueService |
packages/device-management-kit/src/internal/device-session/service/MutexService.test.ts |
Removed - no longer needed |
packages/device-management-kit/src/internal/device-session/use-case/UnsafeBypassIntentQueueUseCase.ts |
New use case to bypass intent queue for testing purposes |
packages/device-management-kit/src/api/device-action/DeviceAction.ts |
Removes disableRefresher from InternalApi interface |
packages/device-management-kit/src/api/DeviceManagementKit.ts |
Adds _unsafeBypassIntentQueue public API method |
packages/device-management-kit/src/api/transport/model/Errors.ts |
Adds SendCommandTimeoutError for command timeout handling |
packages/device-management-kit/src/api/secure-channel/task/ConnectToSecureChannelTask.ts |
Removes manual refresher disabling as it's now automatic |
packages/device-management-kit/src/internal/send/use-case/SendApduUseCase.test.ts |
Refactored to use mocks instead of full service instantiation |
packages/device-management-kit/src/internal/discovery/use-case/ConnectUseCase.test.ts |
Updated to use proper mocking and added test for TransportNotSupportedError |
packages/device-management-kit/src/api/logger-subscriber/service/WebLogsExporterLogger.test.ts |
Uses deviceSessionStubBuilder instead of direct DeviceSession instantiation |
packages/signer/signer-{solana,eth,btc}/src/internal/app-binder/device-action/__test-utils__/makeInternalApi.ts |
Removes disableRefresher mock from test utilities |
.changeset/*.md |
Changeset files documenting the breaking and patch changes |
π‘ Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/device-management-kit/src/internal/device-session/model/DeviceSession.ts
Show resolved
Hide resolved
packages/device-management-kit/src/internal/device-session/model/DeviceSession.ts
Outdated
Show resolved
Hide resolved
packages/device-management-kit/src/internal/device-session/model/DeviceSession.ts
Outdated
Show resolved
Hide resolved
packages/device-management-kit/src/internal/device-session/model/DeviceSessionStateHandler.ts
Outdated
Show resolved
Hide resolved
packages/device-management-kit/src/internal/device-session/model/DeviceSession.ts
Show resolved
Hide resolved
packages/device-management-kit/src/internal/device-session/model/DeviceSession.ts
Outdated
Show resolved
Hide resolved
packages/device-management-kit/src/internal/device-session/model/DeviceSession.ts
Show resolved
Hide resolved
to cleanup the transport state machine
2647e60 to
c6d5239
Compare
π Description
This PR improves the device session refresher to work better with the new intent queue implementation. The main improvement is that the refresher now avoids unnecessary pings when the device is already busy processing an intent.
What changed:
disableRefresherfrom InternalApi since it's no longer needed, the refresher is smart enough to know when to runThis makes the device communication more efficient by avoiding conflicts between the refresher and ongoing operations.
Warning:
InternalApitype changed.β Context
β Checklist
Pull Requests must pass CI checks and undergo code review. Set the PR as Draft if it is not yet ready for review.
π§ Checklist for the PR Reviewers