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

Updating Crypto API to v3 #1789

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

Velin92
Copy link
Member

@Velin92 Velin92 commented May 30, 2023

@Velin92 Velin92 requested a review from BillCarsonFr May 30, 2023 09:38
@codecov
Copy link

codecov bot commented May 30, 2023

Codecov Report

Patch coverage: 31.57% and no project coverage change.

Comparison is base (258bdb8) 35.54% compared to head (5fcbf18) 35.54%.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #1789   +/-   ##
========================================
  Coverage    35.54%   35.54%           
========================================
  Files          617      617           
  Lines        97208    97208           
  Branches     41489    41491    +2     
========================================
+ Hits         34548    34549    +1     
+ Misses       61768    61762    -6     
- Partials       892      897    +5     
Impacted Files Coverage Δ
MatrixSDK/MXRestClient.m 28.55% <31.57%> (-0.07%) ⬇️

... and 8 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@manuroe manuroe requested review from a team and nimau and removed request for a team June 13, 2023 09:39
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.

Looks like we should check /versions to know what version of the spec is supported by the homeserver then use the correct end point.
Some details here https://github.com/matrix-org/matrix-spec-proposals/blob/old_master/proposals/2844-global-versioning.md ? not sure if safe to move all to v3

@@ -22,7 +22,7 @@
Matrix content respository path
*/
NSString *const kMXContentUriScheme = @"mxc://";
NSString *const kMXContentPrefixPath = @"_matrix/media/r0";
NSString *const kMXContentPrefixPath = @"_matrix/media/v3";
Copy link
Member

Choose a reason for hiding this comment

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

It's not related to crypto?

@Velin92
Copy link
Member Author

Velin92 commented Jun 13, 2023

Looks like we should check /versions to know what version of the spec is supported by the homeserver then use the correct end point. Some details here https://github.com/matrix-org/matrix-spec-proposals/blob/old_master/proposals/2844-global-versioning.md ? not sure if safe to move all to v3

Okay I understand, this PR was actually suggested by a member of the community, but maybe we should actually discard it and schedule a separate piece of work on this MSC that you linked.
I imagine that v3 is not backward compatible with r0 right?
What do you think? @manuroe

@nisbet-hubbard
Copy link

nisbet-hubbard commented Jun 13, 2023

The PR was originally suggested because the Conduit homeserver implementation had moved to the v3 endpoints, rendering all crypto-related functionality unusable on the Element client: famedly/conduit#292.

FWIW, the changes I proposed were based on what FluffyChat did: famedly/fluffychat#986. Of course, it’d be perfectly reasonable for Element to go with a different solution.

Whatever solution ends up getting adopted, I hope it makes Element clients work well with Conduit soon.

@Velin92
Copy link
Member Author

Velin92 commented Jun 13, 2023

The PR was originally suggested because the Conduit homeserver implementation had moved to the v3 endpoints, rendering all crypto-related functionality unusable on the Element client: famedly/conduit#292.

FWIW, the changes I proposed were based on what FluffyChat did: famedly/fluffychat#986. Of course, it’d be perfectly reasonable for Element to go with a different solution.

Whatever solution ends up getting adopted, I hope it makes Element clients work well with Conduit soon.

Hello thanks again for your help, we probably need to decide first from a product perspective if it's okay to leave behind any server that would not support the v3 endpoints or if we should instead have a way to support all of them based on the version check.

@nisbet-hubbard
Copy link

we probably need to decide first from a product perspective if it's okay to leave behind any server that would not support the v3 endpoints or if we should instead have a way to support all of them based on the version check

That makes sense! I agree this is a good approach.

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.

Use v3 API for crypto endpoints Use v3 API for crypto endpoints
3 participants