Skip to content

Expose QR login related structs and methods #118

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

Merged
merged 3 commits into from
May 21, 2024
Merged

Conversation

poljar
Copy link
Contributor

@poljar poljar commented May 15, 2024

This PR depends on #116 and #117 so opening it as a draft. It adds all the functionality necessary to make QR code login work in the js-sdk.

A review commit by commit is recommended.

@poljar poljar force-pushed the poljar/qr-login-pr branch 2 times, most recently from 09f02f3 to 706e913 Compare May 16, 2024 09:20
@poljar poljar marked this pull request as ready for review May 16, 2024 09:26
@poljar poljar requested a review from a team as a code owner May 16, 2024 09:26
@poljar poljar requested a review from uhoreg May 16, 2024 09:26
Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. My comments are mostly nits or suggestions for renaming, so nothing that should prevent from merging this PR once those comments are addressed. Thanks!

#[derive(Clone)]
#[wasm_bindgen]
pub struct EstablishedEcies {
inner: Arc<Mutex<ecies::EstablishedEcies>>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, are the arc/mutex needed here, since this is wasm and in theory mono-threaded? (unless the consumer is making use of shared array buffers)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIR yes, leaving them out did not work. On WASM they will be compiled into a no-op, but the Rust compiler complains about... Err I forgot when/where it complained.

const bundle = await firstMachine.exportSecretsBundle();

const alice = new Ecies();
const bob = new Ecies();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it really a bob or rather an alice2?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's Alice's second device called Bob.

Copy link
Member

@BillCarsonFr BillCarsonFr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a comment on doc

@poljar poljar force-pushed the poljar/qr-login-pr branch from 94020b6 to 86f43b7 Compare May 21, 2024 12:21
@poljar poljar requested a review from bnjbvr May 21, 2024 12:25
Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Not re-reading, I trust you made the necessary changes, thanks!)

@poljar poljar force-pushed the poljar/qr-login-pr branch from 86f43b7 to c699380 Compare May 21, 2024 13:19
@poljar poljar enabled auto-merge (rebase) May 21, 2024 13:20
@poljar poljar merged commit 3c707d0 into main May 21, 2024
3 checks passed
@poljar poljar deleted the poljar/qr-login-pr branch May 21, 2024 13:37
Comment on lines +46 to +55
/// Create new [`QrCodeData`] from a given public key, a rendezvous URL and,
/// optionally, a homeserver URL.
///
/// If a homeserver URL is given, then the [`QrCodeData`] mode will be
/// [`QrCodeMode::Reciprocate`], i.e. the QR code will contain data for the
/// existing device to display the QR code.
///
/// If no homeserver is given, the [`QrCodeData`] mode will be
/// [`QrCodeMode::Login`], i.e. the QR code will contain data for the
/// new device to display the QR code.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please bear in mind that [`QrCodeData`] doesn't work in the documentation generated by wasm_bindgen: see https://matrix-org.github.io/matrix-rust-sdk-crypto-wasm/classes/QrCodeData.html#constructor.

You need to use tsdoc-style {@link ... } links.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the style in: 2c9bd6f.

} else {
None
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we're adding new methods to the WASM bindings, please can we use #[wasm_bindgen(js_name)] to give them camelCase names, to be consistent with the rest of the bindings?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants