Make Channel async, and return caBLE UX updates (fix #113) #126
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors caBLE (Hybrid) connections to improve error handling and UX by making Channels async and breaking connection establishment into 3 reusable stages. The changes move connection logic to channel handles and implement proper timeout separation - operations first wait for connection establishment without timeout, then apply operation timeouts.
Key changes:
- Refactored caBLE connection flow into proximity check, connection, and handshake stages
- Made Channel trait generic over UX update types with async operations
- Separated connection establishment from operation timeouts in caBLE channels
Reviewed Changes
Copilot reviewed 32 out of 34 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| libwebauthn/src/lib.rs | Renamed UxUpdate to UvUpdate for user verification updates |
| libwebauthn/src/transport/channel.rs | Made Channel trait generic with UxUpdate associated type and async send methods |
| libwebauthn/src/transport/cable/connection_stages.rs | New module implementing 3-stage connection process for caBLE |
| libwebauthn/src/transport/cable/channel.rs | Added connection state management and timeout separation logic |
| libwebauthn/src/transport/cable/tunnel.rs | Refactored connection logic and added rustls initialization |
| Multiple example files | Updated to use UvUpdate instead of UxUpdate |
Comments suppressed due to low confidence (1)
libwebauthn/src/transport/cable/advertisement.rs:34
- [nitpick] The field name _nonce suggests it's unused, but if it needs to be stored for potential future use, consider a more descriptive name like unused_nonce or remove it entirely if truly unnecessary.
_nonce: nonce,
| .await; | ||
|
|
||
| ux_sender | ||
| .set_connection_state(ConnectionState::Connected) |
There was a problem hiding this comment.
Do we want to set the state, before sending out the update of the changed state? I'm not fully sure what intricacies there are in terms of race condition possibilities.
There was a problem hiding this comment.
TL;DR - I think this will be fine as the ConnectionState is only consumed internally, whilst UX updates are only consumed externally.
For each connection state transition:
- The
Connectedstate is only used to allow enqueuing CBOR operations, and starting the operation timeout timer. No behaviour that depends on the UX state. - The
Terminatedstate is only used to fail operations early. If a new operation is queued after disconnection actually occurs, but before the state is transitioned toTerminated: the CBOR request will never be dequeued leading to a timeout.
msirringhaus
left a comment
msirringhaus
left a comment
There was a problem hiding this comment.
Looking good from what I can tell. Added one question.
|
Rebased. |
Requires #126. Changes: * Moves UX updates to `Channel`, so that `Device::channel()` no longer needs to return a tuple (Channel, Channel::UxUpdate) * Changes updates channel implementation from multi-producer, single-consumer `mpsc` to `broadcast`, allowing subscribing more than once. This allows subscribing to events without borrowing the channel mutably * Wraps PIN entry callback with `Arc`, and makes all `UxUpdate`s `Clone`, as `broadcast` requires the message to be `Clone`
Requires #127.
Changes:
Next steps: