Skip to content

Port to matrix-rust-sdk-crypto#820

Merged
KitsuneRal merged 1 commit into
devfrom
tobias/rust-sdk-crypto
Apr 20, 2026
Merged

Port to matrix-rust-sdk-crypto#820
KitsuneRal merged 1 commit into
devfrom
tobias/rust-sdk-crypto

Conversation

@TobiasFella

@TobiasFella TobiasFella commented Oct 26, 2024

Copy link
Copy Markdown
Member

No description provided.

@TobiasFella
TobiasFella force-pushed the tobias/rust-sdk-crypto branch 3 times, most recently from 2bf6831 to f01f524 Compare November 4, 2024 21:12
@TobiasFella
TobiasFella force-pushed the tobias/rust-sdk-crypto branch 3 times, most recently from 0796120 to c76e066 Compare November 8, 2024 21:21
@TobiasFella
TobiasFella force-pushed the tobias/rust-sdk-crypto branch 4 times, most recently from a07274a to 8c33951 Compare November 16, 2024 12:31
@TobiasFella
TobiasFella force-pushed the tobias/rust-sdk-crypto branch 2 times, most recently from 453f6cc to 8e4ad29 Compare November 28, 2024 21:28
@TobiasFella
TobiasFella force-pushed the tobias/rust-sdk-crypto branch 14 times, most recently from 214ccab to d3814ed Compare December 14, 2024 12:06
@TobiasFella
TobiasFella force-pushed the tobias/rust-sdk-crypto branch 2 times, most recently from 0c7f343 to b300831 Compare December 14, 2024 12:33
@TobiasFella
TobiasFella force-pushed the tobias/rust-sdk-crypto branch 2 times, most recently from 0f8fc8b to fea61b9 Compare December 29, 2024 21:38
@TobiasFella
TobiasFella force-pushed the tobias/rust-sdk-crypto branch 3 times, most recently from c43999c to dd68852 Compare September 25, 2025 17:08
@TobiasFella
TobiasFella force-pushed the tobias/rust-sdk-crypto branch from b31e9ba to 5e9f114 Compare November 22, 2025 20:38
@TobiasFella
TobiasFella force-pushed the tobias/rust-sdk-crypto branch 4 times, most recently from 85ec3d8 to 0a6f2e8 Compare December 28, 2025 14:05
@TobiasFella
TobiasFella force-pushed the tobias/rust-sdk-crypto branch 5 times, most recently from c16795f to 812256d Compare January 6, 2026 17:10

@redstrate redstrate left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Initial review drop, will do more when I'm feeling better

Comment thread .github/workflows/ci.yml
Comment thread .github/workflows/ci.yml
Comment thread .github/workflows/ci.yml Outdated
Comment thread CMakeLists.txt Outdated
Comment thread Quotient/connection.cpp Outdated
Comment thread Quotient/keyverificationsession.cpp Outdated
Comment thread Quotient/crypto-sdk/Cargo.toml Outdated
Comment thread Quotient/crypto-sdk/src/cryptomachine.rs
Comment thread Quotient/crypto-sdk/src/cryptomachine.rs Outdated
Comment thread Quotient/crypto-sdk/src/verification.rs
Comment thread Quotient/crypto-sdk/src/cryptomachine.rs Outdated
impl BackupRequest {
pub(crate) fn transaction_id(&self) -> String {
self.transaction_id
.clone()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: Do these functions (and ditto) need to use clone? Clippy would tell you I guess.

Comment thread Quotient/keyverificationsession.h
Comment thread Quotient/keyverificationsession.cpp Outdated
@redstrate

Copy link
Copy Markdown
Collaborator

Do we want #863 in before this or can they be merged unrelated to each other?

@TobiasFella

Copy link
Copy Markdown
Member Author

Do we want #863 in before this or can they be merged unrelated to each other?

I guess that doesn't matter too much, whichever goes in later must be adapted

@nvrWhere

Copy link
Copy Markdown
Collaborator

So currently when I try to verify my session with NeoChat it all starts fine but no emojis appear once both sides have started

@nvrWhere

nvrWhere commented Apr 8, 2026

Copy link
Copy Markdown
Collaborator

I've been using this for months and haven't noticed which in this case is great. It's worlds better than where we are now. I think we ship this.

nvrWhere
nvrWhere previously approved these changes Apr 8, 2026

@KitsuneRal KitsuneRal left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Functionally, this looks very reasonable overall by now. Thanks for the huge work done! The only thing I discovered is actually in the build configuration. There's something off with the path to lib.rs.h because my initial attempt to build the PR failed because the compiler couldn't find the header (despite it having been generated obviously). Also, the fact that it relies on a very new Corrosion version that has to be manually installed is a bit unfortunate; but I really don't know if we should wait for it to land in distro repos or just commit the generated files instead... which might solve the lib.rs.h path problem along the way, too?

@TobiasFella

Copy link
Copy Markdown
Member Author

Ugh, thought the header generation thing was fixed. Weird that it works fine on the CI, then... I'll have a look tomorrow.

WRT corrosion, i think we should just ship it like it is now. The best way to make a distro update a dependency is to just enforce it.

@TobiasFella

Copy link
Copy Markdown
Member Author

I really don't know what's up with lib.rs.h... It works completely fine here. Is there anything you can say about when it is (and isn't) failing for you?

@KitsuneRal

Copy link
Copy Markdown
Member

Right; I really should've given more than just a "doesn't build here" note. Buut, now that I tried to build it from clean slate, one more thing surfaced:

   Compiling matrix-sdk-sqlite v0.16.0
error[E0308]: mismatched types
    --> src/cryptomachine.rs:1887:37
     |
1887 | ...                   pickle: session.pickle(),
     |                               ^^^^^^^^^^^^^^^^ expected `SessionPickle`, found `vodozemac::olm::SessionPickle`
     |
note: there are multiple different versions of crate `vodozemac` in the dependency graph
    --> /var/home/axy/dev/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/vodozemac-0.9.0/src/olm/session/mod.rs:481:1
     |
 481 | pub struct SessionPickle {
     | ^^^^^^^^^^^^^^^^^^^^^^^^ this is the expected type
     |
    ::: /var/home/axy/dev/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/vodozemac-0.10.0/src/olm/session/mod.rs:510:1
     |
 510 | pub struct SessionPickle {
     | ------------------------ this is the found type
     = help: you can use `cargo tree` to explore your dependency tree

cargo tree shows that indeed, we get the older vodozemac with matrix-sdk crates but then also get vodozemac-0.10 via our own Cargo.toml. That was what I could see from absent ~/.cargo, no Cargo.lock, and with a freshly done CMake configuration. If I manually remove the new vodozemac from Cargo.lock then the build goes through (even without that former error about lib.rs.h I've got, which is also peculiar). In any case, that definitely needs fixing: we should only pull the version of vodozemac that other crates are using, not just anything with 0 major version. I'm not sure if pinning down vodozemac 0.9 in Cargo.toml is the best way but that's as good as I can come up with for now.

@redstrate

Copy link
Copy Markdown
Collaborator

Without testing, IIRC you can specify a "*" and Cargo should be smart enough to pick the version used by the other crates.

@TobiasFella

Copy link
Copy Markdown
Member Author

Fixed by not depending on vodozemac directly.

@KitsuneRal

Copy link
Copy Markdown
Member

Hm, that seemed to work although when I tried the same locally before it was complaining about not finding the crate. Whatever works.
I'll give it a couple more runs but (aside from panics in case of some errors) this does seem good to go.

@KitsuneRal

Copy link
Copy Markdown
Member

I don't want to hold it off for another month while I'm away; so I guess that's it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement A feature or change request for the library

Projects

Status: 0.10 - Done

Development

Successfully merging this pull request may close these issues.

4 participants