Skip to content
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

Switch to Rust crypto #2467

Closed
wants to merge 30 commits into from
Closed

Conversation

robintown
Copy link
Member

@robintown robintown commented Jul 5, 2024

We're not really using crypto at the moment, but with it being this easy, we should still make the switch!

Based on #2466

Here I've implemented an MVP for the new unified grid layout, which scales smoothly up to arbitrarily many participants. It doesn't yet have a special 1:1 layout, so in spotlight mode and 1:1s, we will still fall back to the legacy grid systems.

Things that happened along the way:
- The part of VideoTile that is common to both spotlight and grid tiles, I refactored into MediaView
- VideoTile renamed to GridTile
- Added SpotlightTile for the new, glassy spotlight designs
- NewVideoGrid renamed to Grid, and refactored to be even more generic
- I extracted the media name logic into a custom React hook
- Deleted the BigGrid experiment
Also removing some unused settings along the way.
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 3.24570% with 2981 lines in your changes missing coverage. Please review.

Project coverage is 17.02%. Comparing base (8cced48) to head (0a561a6).
Report is 554 commits behind head on livekit.

Files Patch % Lines
src/grid/Grid.tsx 0.00% 481 Missing ⚠️
src/room/InCallView.tsx 0.00% 388 Missing ⚠️
src/tile/SpotlightTile.tsx 0.00% 323 Missing ⚠️
src/tile/GridTile.tsx 0.00% 301 Missing ⚠️
src/state/CallViewModel.ts 0.00% 297 Missing ⚠️
src/state/MediaViewModel.ts 0.00% 160 Missing ⚠️
src/grid/CallLayout.ts 0.00% 152 Missing ⚠️
src/grid/GridLayout.tsx 0.00% 135 Missing ⚠️
src/tile/MediaView.tsx 0.00% 127 Missing ⚠️
src/grid/SpotlightPortraitLayout.tsx 0.00% 119 Missing ⚠️
... and 26 more
Additional details and impacted files
@@             Coverage Diff             @@
##           livekit    #2467      +/-   ##
===========================================
- Coverage    25.02%   17.02%   -8.00%     
===========================================
  Files           47      142      +95     
  Lines         2382    19251   +16869     
  Branches       434      249     -185     
===========================================
+ Hits           596     3278    +2682     
- Misses        1735    15973   +14238     
+ Partials        51        0      -51     
Flag Coverage Δ
unittests 17.02% <3.24%> (-8.00%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@toger5
Copy link
Contributor

toger5 commented Jul 5, 2024

As you mention in the description, this is: Based on #2466.
Should this PR be the standalone change?

Copy link
Member

@AndrewFerr AndrewFerr left a comment

Choose a reason for hiding this comment

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

This really is easy 😃

If you want, you could rebase this onto livekit to make it independent of your other PRs, and it could be merged without conflicts much sooner.

@robintown
Copy link
Member Author

Yeah, I guess I could un-stack this one, but it's like… I know that it would create conflicts for some other things in the stack, requiring me to rebase N other changes, and it's not a very impactful or time-sensitive change, so I'd rather just keep it on the stack.

react-rxjs is the library we've been using to connect our React components to view models and consume observables. However, after spending some time with react-rxjs, I feel that it's a very heavy-handed solution. It requires us to sprinkle <Subscribe /> and <RemoveSubscribe /> components all throughout the code, and makes React go through an extra render cycle whenever we mount a component that binds to a view model. What I really want is a lightweight React hook that just gets the current value out of a plain observable, without any extra setup. Luckily the observable-hooks library with its useObservableEagerState hook seems to do just that—and it's more actively maintained, too!
Because we were hiding even the local participant during initial connection, there would be no participants, and therefore nothing to put in the spotlight. The designs don't really tell us what the connecting state should look like, so I've taken the liberty of restoring it to its former glory of showing the local participant immediately.
Includes the mobile UX optimizations and the tweaks we've made to cut down on wasted space, but does not yet include the change to embed the spotlight tile within the grid.
Codecov hasn't been working recently because Vitest doesn't report coverage by default.
This way we benefit from not having to maintain the same directory structure twice, and our linters etc. will actually lint test files by default.
Vitest provides globals primarily to make the transition from Jest more smooth. But importing its functions explicitly is considered a better pattern, and we have so few tests right now that it's trivial to migrate them all.
We no longer use Storybook.
We're not really using crypto at the moment, but with it being this easy, we should still make the switch!
@toger5
Copy link
Contributor

toger5 commented Jul 30, 2024

Is this actually still blocked?

@toger5
Copy link
Contributor

toger5 commented Aug 5, 2024

Also blocked on missing rust encrypted to device sending matrix-org/matrix-js-sdk#3304

@robintown
Copy link
Member Author

We're not using that API currently (though it being missing would slow our adoption of a new individual sender keys encryption scheme)

@toger5
Copy link
Contributor

toger5 commented Aug 7, 2024

I am wondering how we currently distribute megolm keys without toDevice? Is it just custom toDevice messages that are missing in the wasm bindings?

@fkwp
Copy link
Contributor

fkwp commented Aug 7, 2024

that's abstracted away by the js-sdk. the rust js-sdk is internally using toDevice msg but the API is not exposed to the js layer.

@hughns
Copy link
Member

hughns commented Aug 19, 2024

As this is now on the critical path for looking at sender key distribution I have made #2571 which has just the Rust crypto changes in it to make review easier.

@hughns hughns closed this Aug 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants