-
Notifications
You must be signed in to change notification settings - Fork 12
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
Add bindings for dehydrated devices #104
Conversation
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.
Just a few comments
} | ||
} | ||
|
||
impl TryFrom<OriginalPutDehydratedDeviceRequest> for PutDehydratedDeviceRequest { |
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.
I think there is a macro to do that.
Like request!(KeysBackupRequest from OriginalKeysBackupRequest extracts version: string and groups rooms);
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.
I think that the macro only works for requests that have an ID (which is why UploadSigningKeys
is also not done using the macro).
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 feels weird to me that this has to happen in the bindings, macro or not. Surely turning the request object into the relevant JSON is something that all applications will need to do, so it should be in the main SDK?
I appreciate it's following a pattern, so this is maybe something to fix another time, but still, it seems wrong.
#[wasm_bindgen] | ||
#[derive(Debug)] | ||
/// Struct collecting methods to create and rehydrate dehydrated devices. | ||
pub struct DehydratedDevices { |
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.
Was a bit surprised by the plural here, but it's the same as in the SDK.
It's more a Factory? or a Manager?
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.
Sort of a manager, but not quite. Yeah, I'm not super happy with the naming, but it's what the SDK uses.
// Assume we have 50 keys on the server, until we get a sync that says fewer. | ||
uploaded_signed_key_count: 50, | ||
creation_local_time: MilliSecondsSinceUnixEpoch::now(), | ||
fallback_key_creation_timestamp: None, |
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.
This change is to make it work with latest version of matrix-sdk-crypto
|
||
OutgoingRequests::KeysBackup(request) => { | ||
JsValue::from(KeysBackupRequest::try_from((request_id, request))?) | ||
} |
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.
OutgoingRequests::KeysBackup
was deleted in latest version of matrix-sdk-crypto
(matrix-org/matrix-rust-sdk@3ccd2e9)
Test failure is because the latest matrix-sdk-crypto includes time-based fallback key rotation, which uses SystemTime to calculate the expiry, which doesn't work with wasm. matrix-org/matrix-rust-sdk#3249 fixes this. So once that's merged, the |
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.
A couple of nits, but lgtm otherwise
CHANGELOG.md
Outdated
- Add `OlmMachine.dehydratedDevices()` and `DehydratedDevices` class to | ||
support dehydrated devices. | ||
([#104](https://github.com/matrix-org/matrix-rust-sdk-crypto-wasm/pull/104)) | ||
|
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.
please can we put more recent changelog entries at the top?
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.
Oh, I purposely put it at the bottom, because I mis-remembered the convention. 🤦 Fixed now
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.
yeahh... my thinking goes, if new versions go at the top, then by extension, so should individual entries, so that the newest stuff is always at the top.
src/requests.rs
Outdated
@@ -597,6 +596,69 @@ impl TryFrom<&OriginalUploadSigningKeysRequest> for UploadSigningKeysRequest { | |||
} | |||
} | |||
|
|||
#[wasm_bindgen(getter_with_clone)] | |||
#[derive(Debug)] | |||
/// Upload a dehydrated device to the server. |
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.
/// Upload a dehydrated device to the server. | |
/// A request that will upload a dehydrated device to the server. |
} | ||
} | ||
|
||
impl TryFrom<OriginalPutDehydratedDeviceRequest> for PutDehydratedDeviceRequest { |
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 feels weird to me that this has to happen in the bindings, macro or not. Surely turning the request object into the relevant JSON is something that all applications will need to do, so it should be in the main SDK?
I appreciate it's following a pattern, so this is maybe something to fix another time, but still, it seems wrong.
Depends on matrix-org/matrix-rust-sdk#3164