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.

Sign up for GitHub

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

Jump to bottom

Add support for new dehydrated device format #199

Merged
merged 1 commit into from
Jan 17, 2025
Merged

Add support for new dehydrated device format #199

merged 1 commit into from
Jan 17, 2025

Conversation

uhoreg
Copy link
Member

@uhoreg uhoreg commented Nov 27, 2024 •
edited
Loading

as documented in the current version of MSC3814

Fixes #196

@uhoreg uhoreg force-pushed the dehydration_format branch 2 times, most recently from 97c798f to fbf8e39 Compare December 4, 2024 02:56
@uhoreg uhoreg marked this pull request as ready for review December 4, 2024 03:00
@uhoreg uhoreg requested review from dkasak and poljar as code owners December 4, 2024 03:00
@poljar
Copy link
Collaborator

poljar commented Dec 5, 2024

Gonna close and reopen this to see if the codecov thing kicks in.

@poljar poljar closed this Dec 5, 2024
@poljar poljar reopened this Dec 5, 2024
@codecov-commenter
Copy link

codecov-commenter commented Dec 5, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 91.66667% with 7 lines in your changes missing coverage. Please review.

Project coverage is 90.38%. Comparing base (bf01e8b) to head (210fb77).

Files with missing lines Patch % Lines
src/olm/account/mod.rs 91.78% 6 Missing ⚠️
src/types/ed25519.rs 87.50% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #199      +/-   ##
==========================================
+ Coverage   90.28%   90.38%   +0.09%     
==========================================
  Files          34       34              
  Lines        1905     1986      +81     
==========================================
+ Hits         1720     1795      +75     
- Misses        185      191       +6

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

@uhoreg uhoreg mentioned this pull request Dec 16, 2024
Merged
1 task
@uhoreg
Copy link
Member Author

uhoreg commented Dec 17, 2024

@poljar can I get a review on this PR? Thanks

poljar
poljar requested changes Dec 18, 2024
View reviewed changes
Copy link
Collaborator

@poljar poljar left a comment

Choose a reason for hiding this comment

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

Hi, sorry for the late reply, we miscommunicated on who would do the first review.

A good start but needs a bit more polish.

uhoreg
uhoreg commented Jan 2, 2025
View reviewed changes
uhoreg
uhoreg commented Jan 2, 2025
View reviewed changes
@uhoreg uhoreg requested a review from poljar January 6, 2025 14:05
@uhoreg
Copy link
Member Author

uhoreg commented Jan 9, 2025

The clippy failure seems to be in code that I did not change. I'm guessing that I should create a new PR to fix that, rather than adding the fixes to this PR.

@poljar
Copy link
Collaborator

poljar commented Jan 9, 2025

The clippy failure seems to be in code that I did not change. I'm guessing that I should create a new PR to fix that, rather than adding the fixes to this PR.

Should be fixed in this PR: #200.

@uhoreg uhoreg force-pushed the dehydration_format branch 2 times, most recently from f421d84 to 800719a Compare January 15, 2025 03:02
@uhoreg uhoreg requested a review from poljar January 15, 2025 03:06
poljar
poljar requested changes Jan 15, 2025
View reviewed changes
Copy link
Collaborator

@poljar poljar left a comment

Choose a reason for hiding this comment

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

Found a couple of minor things, but I think after that it's good to go.

I'm fuzzing the new target so we have a large corpus that we can run as part of the unit tests (We do that for every fuzz target).

@uhoreg
Copy link
Member Author

uhoreg commented Jan 17, 2025

I think that the clippy errors are (again) unrelated to this PR

@uhoreg uhoreg requested a review from poljar January 17, 2025 02:15
@poljar
Copy link
Collaborator

poljar commented Jan 17, 2025

I think that the clippy errors are (again) unrelated to this PR

Yeah, that lint might not have been the best idea: #203.

@poljar poljar force-pushed the dehydration_format branch from d7240c2 to 8c15db1 Compare January 17, 2025 13:32
@poljar
Copy link
Collaborator

poljar commented Jan 17, 2025

Rebased to get the CI unblocked.

@poljar poljar force-pushed the dehydration_format branch from f1f130f to b04448a Compare January 17, 2025 14:23
@uhoreg @poljar
feat: Add support for the new dehydrated device format
210fb77 
Changelog: Added support for [MSC3814](matrix-org/matrix-spec-proposals#3814),
    allowing users to export an Olm Account in a serialized format. The
    serialized account can be uploaded to a server for safekeeping until
    needed.
@poljar poljar force-pushed the dehydration_format branch from b04448a to 210fb77 Compare January 17, 2025 14:24
poljar
poljar approved these changes Jan 17, 2025
View reviewed changes
Copy link
Collaborator

@poljar poljar left a comment

Choose a reason for hiding this comment

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

I added some minor fixes and cleaned up the history which now includes a changelog entry as well.

Thanks for working patiently on this.

@poljar poljar enabled auto-merge (rebase) January 17, 2025 14:26
@poljar poljar merged commit 3d655ad into matrix-org:main Jan 17, 2025
19 checks passed
poljar
poljar reviewed Jan 17, 2025
View reviewed changes
src/olm/account/mod.rs
private_ed25519_key: account
.signing_key
.unexpanded_secret_key()
.expect("Cannot dehydrate an account created from a libolm pickle"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, @uhoreg, won't this pose a problem? In any case I don't think we should use expect() here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So the expectation is that we ourselves will always create a new Account when we want to create a dehydrated device, but people may misuse this.

I think we should convert this into a TryFrom and let the person be aware of this limitation. In the Rust SDK itself we can guarantee that we don't misuse the API and call expect() there.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Will open a PR.

@poljar poljar mentioned this pull request Jan 17, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@poljar poljar poljar approved these changes

@dkasak dkasak Awaiting requested review from dkasak dkasak is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Dehydrated Device: Serialize and deserialize new format
3 participants
@uhoreg @poljar @codecov-commenter