Skip to content

Updating Crypto API to v3#1789

Open
Velin92 wants to merge 5 commits intodevelopfrom
mauroromito/updating_crypto_endpoint_to_v3
Open

Updating Crypto API to v3#1789
Velin92 wants to merge 5 commits intodevelopfrom
mauroromito/updating_crypto_endpoint_to_v3

Conversation

@Velin92
Copy link
Copy Markdown
Member

@Velin92 Velin92 commented May 30, 2023

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

codecov bot commented May 30, 2023

Codecov Report

❌ Patch coverage is 31.57895% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 35.54%. Comparing base (258bdb8) to head (5fcbf18).
⚠️ Report is 282 commits behind head on develop.

Files with missing lines Patch % Lines
MatrixSDK/MXRestClient.m 31.57% 12 Missing and 1 partial ⚠️
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     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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

*/
NSString *const kMXContentUriScheme = @"mxc://";
NSString *const kMXContentPrefixPath = @"_matrix/media/r0";
NSString *const kMXContentPrefixPath = @"_matrix/media/v3";
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.

It's not related to crypto?

@Velin92
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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