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 deprecated parameters and types in CryptoApi #4670

Draft
wants to merge 5 commits into
base: florianduros/rip-out-legacy-crypto/remove-legacy-crypto
Choose a base branch
from

Conversation

florianduros
Copy link
Contributor

@florianduros florianduros commented Jan 29, 2025

Remove deprecated parameters in CryptoApi and update rust-crypto .

@florianduros florianduros mentioned this pull request Jan 29, 2025
4 tasks
@florianduros florianduros changed the title Remove deprecated parameters in CryptoApi Remove deprecated parameters and types in CryptoApi Jan 29, 2025
@florianduros florianduros force-pushed the florianduros/rip-out-legacy-crypto/clean-up-crypto-api branch from 2693c9c to 644185d Compare January 29, 2025 13:43
@florianduros florianduros marked this pull request as ready for review January 29, 2025 13:53
@florianduros florianduros requested a review from a team as a code owner January 29, 2025 13:53
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.

I'm a bit nervous about this: it goes quite a long way beyond "remove legacy crypto", imho. For example findVerificationRequestDMInProgress and storeSessionBackupPrivateKey work ok in rust crypto.

Not saying we shouldn't plan to remove them, but I don't think we should do it as part of this change.

Removing the old DecryptionFailureCodes and CryptoCallbacks seems sensible.

Also, the tests are failing.

@florianduros
Copy link
Contributor Author

The legacy crypto tests are failing because of #4659.

If we plan to remove them, should be in the same release than the removal of the legacy crypto in order to avoir too many releases with breaking changes. Or do we want to do in some weeks/month?

@richvdh
Copy link
Member

richvdh commented Jan 30, 2025

If we plan to remove them, should be in the same release than the removal of the legacy crypto in order to avoir too many releases with breaking changes. Or do we want to do in some weeks/month?

I'm not really sure yet. Right now, I'm mostly nervous that we're making sweeping changes without effective CI.

To be honest, I think we should leave it for a release or two; maybe get a working vNext system going first so that we can do major-version housekeeping regularly without it being a massive headache.

Either way: not part of this PR, please?

@florianduros
Copy link
Contributor Author

I move it back to draft, we will resuscitate it if we need it later

@florianduros florianduros marked this pull request as draft January 30, 2025 15:59
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.

2 participants