-
Notifications
You must be signed in to change notification settings - Fork 10
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
Update rust-sdk ver and bootstrapCrossSigning
response
#49
Conversation
For consistency with matrix-rust-sdk.
IMHO it was more confusing than helpful. It was only used in one place, and there just to hold a temporary result. Let's make its one useful method a free function, and get rid of it.
`bootstrapCrossSigning` now returns its own type, which can contain two or three nested requests. Create a new response type wrapper and return it.
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.
LGTM!
matrix-sdk-common = { git = "https://github.com/matrix-org/matrix-rust-sdk", rev = "7440ce0a0cfecf6c5e121e019470fde6ff38fefd", features = ["js"] } | ||
matrix-sdk-indexeddb = { git = "https://github.com/matrix-org/matrix-rust-sdk", rev = "7440ce0a0cfecf6c5e121e019470fde6ff38fefd" } | ||
matrix-sdk-qrcode = { git = "https://github.com/matrix-org/matrix-rust-sdk", rev = "7440ce0a0cfecf6c5e121e019470fde6ff38fefd", optional = true } |
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.
If you don't pin a revision, I'm worried this may try to use the latest one, and update the Cargo.lock
for next users, so I'd recommend to keep on pinning a commit hash here (unless you've tested this and it doesn't change behavior even after new commits to upstream have landed).
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.
We discussed this in the sdk dev room (https://matrix.to/#/!qSsPTKDfMGYqhgiLPJ:flipdot.org/$Qd_cwgzB-POjqR7HicHup0jj-_n6GoxyTEAgmopBtkY?via=flipdot.org&via=matrix.org&via=element.io).
AIUI, Cargo.lock
will only be updated if you do a cargo update
or similar. I think that's fine.
(I just thought that an alternative design would be to create a large promise wrapping the three others: if first is present, run it and mark it as sync; then run second; then run third — and expose only that large promise to JS. It would simplify the end user API.) |
I'm not really following this. We do need the details of the actual requests, because it's up to the JS side to actually make those requests over HTTP: rust-sdk-crypto-wasm itself does no networking. |
Ah yes, i lacked that context, nevermind me. |
bootstrapCrossSigning
now returns its own type, which can contain two or three nested requests. Create a new response type wrapper and return it.Some other cleanups while we are here:
Get rid of
OutgoingRequests
wrapper, which was more confusing than helpful.Rename
SigningKeysUploadRequest
toUploadSigningKeysRequest
for consistency with matrix-rust-sdk.