Add docstrings for 7 more Rust API modules #287

cosmicexplorer · 2021-05-02T03:04:03Z

This is the second in a set of planned PRs to add docstrings to the Rust API, see #285.

I took care to make these changes highly orthogonal, and have split each module's changes into its own commit.

Notably, this does not depend on #286!

jrose-signal

Many comments, apologies for the onslaught. :-)

jrose-signal · 2021-05-04T00:37:55Z

rust/protocol/src/consts.rs

-pub const MAX_RECEIVER_CHAINS: usize = 5;
-pub const ARCHIVED_STATES_MAX_LENGTH: usize = 40;
-pub const MAX_SENDER_KEY_STATES: usize = 5;
+//! Magic numbers.


I don't think any of these types or values need to be made public. That doesn't mean it's not worth documenting them, but I'm more inclined to filter them back into the relevant files that reference them than add more structure.

Ok! I have marked both of crate::consts::{types,limits} as pub(crate) in 2ff52da. Does that address your concern? I think that the tiny little module structure here is useful for documentation of these values. If you're not seeing it, we can totally do away with that as well though!

I still tend to prefer keeping types and constants close to their primary use site, but clearly the code isn't doing that today. I'll come back to this.

rust/protocol/src/crypto.rs

rust/protocol/src/storage/traits.rs

jrose-signal · 2021-05-04T01:14:42Z

rust/protocol/src/storage/traits.rs

@@ -62,6 +85,7 @@ pub trait PreKeyStore {
    async fn remove_pre_key(&mut self, prekey_id: PreKeyId, ctx: Context) -> Result<()>;
 }

+/// Interface for storing signed pre-keys downloaded from a server. (TODO: ???)


I'll come back with an explanation of pre-keys and signed pre-keys later; for now feel free to leave this generic.

Thanks! Done!

[marking this unresolved so I'm less likely to forget about it]

rust/protocol/src/storage/traits.rs

rust/protocol/src/identity_key.rs

jrose-signal · 2021-05-04T01:18:36Z

cc @rubdos and @Be-ing, also interested in using libsignal-client as a Rust library directly.

rubdos · 2021-05-04T07:53:09Z

cc @gferon and @boxdot, thanks for the heads up @jrose-signal.

@cosmicexplorer You may be interested in the libraries and work at https://github.com/whisperfish. Feel free to join our Matrix channel to coordinate some other work; we are planning to submit some PRs into here wrt. some crypto code. Cfr also #152.

@jrose-signal would you appreciate a review from me and Gabriel on #286 and this one? I'm not sure whether we can catch any more than you already did, but I'd rather ask whether it'd be appreciated or not.

jrose-signal · 2021-05-04T17:34:25Z

@jrose-signal would you appreciate a review from me and Gabriel on #286 and this one? I'm not sure whether we can catch any more than you already did, but I'd rather ask whether it'd be appreciated or not.

I think it's reasonable for multiple perspectives, but I don't want to stress out @cosmicexplorer either. @cosmicexplorer, what do you think?

cosmicexplorer · 2021-05-04T19:03:45Z

I'd be wholly unstressed for people to give this a more thorough review!

rust/protocol/src/identity_key.rs

rust/protocol/src/consts.rs

Be-ing · 2021-05-04T19:28:05Z

Thanks for working on this! And yes please join us in the Whisperfish Matrix room at #whisperfish:rubdos.be. I'm not sure what your goal is with using this library, but we're working towards a GUI client written in Rust and QML with the qmetaobject-rs crate. Currently Whisperfish uses Jolla's proprietary QML libraries so it only works on Sailfish OS, but I just got a PinePhone so I'll be working on adapting it to also work with KDE's Kirigami QML libraries without Sailfish OS. At the moment Whisperfish only supports text messages and GroupsV2 but we intend to make it feature complete. I presume you're probably writing a Signal client, in which case you'll probably be interested in the libsignal-service-rs library which now uses this libsignal-client library. @gferon is also working on using libsignal-service-rs for CLI clients and bots with Presage.

cosmicexplorer · 2021-05-04T22:42:43Z

@Be-ing I'm working on what should become a CLI tool very analogous to gpg that performs forward-secret encrypted messaging and document signing! I had been working on a Rust wrapper to libsignal-protocol-c a while ago before to my amazement the whole thing was rewritten in Rust! I had been expecting to have to write a lot more stuff myself too so diverted my excitement towards documenting what was already done here instead.

I can see there are already a lot of extensions in the whisperfish org you linked above, so I'm seeing whether any of those solve more problems I thought I had.

rubdos

This is already such an improvement! Thanks for taking this on ❤️

I've added 22 comments; most of them are really nit-picky. I figured, if we're doing this, let's do this the best we can already.

rust/protocol/src/crypto.rs

rust/protocol/src/storage/inmem.rs

rust/protocol/src/storage/traits.rs

rubdos · 2021-05-05T08:08:30Z

rust/protocol/src/storage/traits.rs

 // SPDX-License-Identifier: AGPL-3.0-only
 //

+//! Traits defining several stores used throughout the Signal Protocol.


I would add a word about the usage of async-trait, including the fact that it's async_trait(?Send). Maybe some example/dummy implementation would be really great here.

Honestly the ?Send part has been a bit weird in practice; probably these should all require Send after all. That means newtype-ing Context but that's also a good idea.

*cough* but that's for another PR. I agree that some mention of async-trait is a good idea if only so people know that they have to use that when implementing the traits.

I started to look into having the store traits require Send, just to see how far I could go, would you mind if I open an issue so that we can discuss it in more details there?

@gferon That sounds great, thank you!

- requires the `unstable_internals` feature to be enabled for doctests to work

- [NON RUST/RENAME] rename "DJB_TYPE" to "CURVE_25519_TYPE" in java and swift - [1+CRYPTO CRATE] make swift/build-ffi.sh --generate-ffi work - [1+CURVE] add docs and the Keyed trait to `curve*.rs`

cosmicexplorer · 2022-06-27T08:03:32Z

This is now closed in favor of #475 and the other remaining tasks on the tracking issue #467!

cosmicexplorer mentioned this pull request May 2, 2021

tracking issue for some Rust API learnability improvements #285

Closed

3 tasks

jrose-signal reviewed May 4, 2021

View reviewed changes

cosmicexplorer force-pushed the docs-2 branch 2 times, most recently from 5dcfd7d to 2ff52da Compare May 4, 2021 19:02

Be-ing reviewed May 4, 2021

View reviewed changes

rust/protocol/src/identity_key.rs Outdated Show resolved Hide resolved

Be-ing reviewed May 4, 2021

View reviewed changes

rust/protocol/src/consts.rs Outdated Show resolved Hide resolved

cosmicexplorer force-pushed the docs-2 branch from 2ff52da to 55c9c65 Compare May 4, 2021 19:14

cosmicexplorer force-pushed the docs-2 branch 9 times, most recently from 54dbdd7 to eea8618 Compare May 4, 2021 22:12

cosmicexplorer force-pushed the docs-2 branch 4 times, most recently from e46ceca to f649b9a Compare May 5, 2021 04:54

rubdos reviewed May 5, 2021

View reviewed changes

cosmicexplorer force-pushed the docs-2 branch 3 times, most recently from eeddf75 to 201a3fb Compare May 8, 2021 16:58

This was referenced Jun 27, 2022

make doctests fail on any warnings #466

Closed

module documentation tracking issue #467

Open

cosmicexplorer force-pushed the docs-2 branch from 056a556 to b39b7c1 Compare June 27, 2022 03:52

cosmicexplorer mentioned this pull request Jun 27, 2022

add docs to storage module #468

Closed

cosmicexplorer force-pushed the docs-2 branch 2 times, most recently from d91b9fe to 718d70c Compare June 27, 2022 04:05

This was referenced Jun 27, 2022

add docs to utils and expose constant_time_cmp() to consumers #469

Closed

add docs to identity_key.rs #470

Closed

cosmicexplorer force-pushed the docs-2 branch from 718d70c to 1d903eb Compare June 27, 2022 04:12

cosmicexplorer mentioned this pull request Jun 27, 2022

add documentation for error cases #471

Closed

cosmicexplorer force-pushed the docs-2 branch from 1d903eb to 3fa739a Compare June 27, 2022 04:16

This was referenced Jun 27, 2022

remove Debug from SessionRecord #472

Closed

document a few constants #473

Closed

cosmicexplorer force-pushed the docs-2 branch 4 times, most recently from 7b96a07 to ab70e19 Compare June 27, 2022 04:32

cosmicexplorer mentioned this pull request Jun 27, 2022

remove unnecessary Result-wrapping in crypto.rs #474

Closed

1 task

cosmicexplorer force-pushed the docs-2 branch 4 times, most recently from 59efa3a to 19e11fe Compare June 27, 2022 07:44

[0+UTILS] add docs to utils.rs

990766f

- requires the `unstable_internals` feature to be enabled for doctests to work

cosmicexplorer force-pushed the docs-2 branch from 19e11fe to 503d336 Compare June 27, 2022 07:52

[1/rename] rename everything from "djb" to just "curve25519"

30216e0

- [NON RUST/RENAME] rename "DJB_TYPE" to "CURVE_25519_TYPE" in java and swift - [1+CRYPTO CRATE] make swift/build-ffi.sh --generate-ffi work - [1+CURVE] add docs and the Keyed trait to `curve*.rs`

cosmicexplorer force-pushed the docs-2 branch from 503d336 to 30216e0 Compare June 27, 2022 07:52

cosmicexplorer mentioned this pull request Jun 27, 2022

rename "djb" type curve to "curve25519" everywhere #475

Closed

1 task

cosmicexplorer closed this Jun 27, 2022

cosmicexplorer mentioned this pull request Jun 27, 2022

set imports_granularity = "Module" #476

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add docstrings for 7 more Rust API modules #287

Add docstrings for 7 more Rust API modules #287

cosmicexplorer commented May 2, 2021

jrose-signal left a comment

jrose-signal May 4, 2021

cosmicexplorer May 4, 2021

jrose-signal May 5, 2021

jrose-signal May 4, 2021

cosmicexplorer May 4, 2021

jrose-signal May 5, 2021

jrose-signal commented May 4, 2021

rubdos commented May 4, 2021 •

edited

Loading

jrose-signal commented May 4, 2021

cosmicexplorer commented May 4, 2021

Be-ing commented May 4, 2021 •

edited

Loading

cosmicexplorer commented May 4, 2021 •

edited

Loading

rubdos left a comment

rubdos May 5, 2021

jrose-signal May 6, 2021

gferon May 6, 2021

jrose-signal May 7, 2021

cosmicexplorer commented Jun 27, 2022

Add docstrings for 7 more Rust API modules #287

Add docstrings for 7 more Rust API modules #287

Conversation

cosmicexplorer commented May 2, 2021

jrose-signal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrose-signal commented May 4, 2021

rubdos commented May 4, 2021 • edited Loading

jrose-signal commented May 4, 2021

cosmicexplorer commented May 4, 2021

Be-ing commented May 4, 2021 • edited Loading

cosmicexplorer commented May 4, 2021 • edited Loading

rubdos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cosmicexplorer commented Jun 27, 2022

rubdos commented May 4, 2021 •

edited

Loading

Be-ing commented May 4, 2021 •

edited

Loading

cosmicexplorer commented May 4, 2021 •

edited

Loading