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

Remove MatrixClient.initLegacyCrypto #4620

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

Conversation

florianduros
Copy link
Contributor

@florianduros florianduros commented Jan 16, 2025

Checklist

  • Tests written for new code (and old code if feasible).
  • New or updated public/exported symbols have accurate TSDoc documentation.
  • Linter and other CI checks pass.
  • Sign-off given on the changes (see CONTRIBUTING.md).

Task element-hq/element-web#26922
First step to delete definitively the legacy crypto stack. We deprecated during the past months the legacy crypto and now it is time to begin to delete it.

The removal will be in multiple phases and this PR has the unique goal to remove MatrixClient.initLegacyCrypto, the tests and documentation with direct references.

The legacy tests, integration tests, code will be removed or cleaned in other PRs.

@florianduros florianduros changed the title Remove MatrixClient.initLegacyCrypto Remove MatrixClient.initLegacyCrypto Jan 16, 2025
@florianduros florianduros force-pushed the florianduros/rip-out-legacy-crypto/remove-initLegacyCrypto branch from 8dcf73c to ba1ca5c Compare January 16, 2025 13:50
@florianduros florianduros marked this pull request as ready for review January 16, 2025 13:55
@florianduros florianduros requested review from a team as code owners January 16, 2025 13:55
@t3chguy
Copy link
Member

t3chguy commented Jan 16, 2025

I imagine that this will have very bad results on our coverage, as you are deleting the tests for code which is not yet deleted. Will this cause any breaches of contract? Do we still have any contracts to uphold coverage beyond a certain level? SonarCloud is only not complaining as it only looks at new code, of which there is none.

@florianduros
Copy link
Contributor Author

Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

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

IMO this should remove all related exported entrypoints, all deprecated & now-unused symbols. Otherwise we will need a 2nd follow-up Breaking Change release to clean up the tech debt to maintain the contract around semver.

.e.g. https://matrix-org.github.io/matrix-js-sdk/enums/matrix.CryptoEvent.html and most things in the crypto dir.

…-initLegacyCrypto

# Conflicts:
#	spec/integ/matrix-client-syncing.spec.ts
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

This looks broadly good to me, but I would rather see #4622 / #4624 land first.

I don't agree that we should rip out all of the legacy crypto code at once (and also, ftr, don't agree that removing things in the crypto directory would necessarily result in a major version bump, though it's true that updating CryptoEvent would), but I guess we should discuss that elsewhere.

…-initLegacyCrypto

# Conflicts:
#	spec/integ/sliding-sync-sdk.spec.ts
@florianduros
Copy link
Contributor Author

florianduros commented Jan 17, 2025

@richvdh I agree to land #4624 first (which did) however #4622 should land after in my opinion.

#4622 removes the support of the legacy crypto in the sync module. If we don't remove initLegacyCrypto before, we just break the sync in legacy crypto. Also, it is breaking existing legacy crypto tests.

@richvdh richvdh self-requested a review January 17, 2025 16:11
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.

3 participants