Skip to content

chore!: remove the cryptobox-migrate feature #942

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

istankovic
Copy link
Member

@istankovic istankovic commented Feb 28, 2025

All clients have moved away from cryptobox so it doesn't make sense to have this.

📓 This is a breaking change.

@istankovic istankovic requested a review from a team as a code owner February 28, 2025 16:48
@istankovic istankovic force-pushed the ivan/remove-cryptobox-migrate branch from 0ea8a1a to 0f562b9 Compare February 28, 2025 16:51
Copy link

github-actions bot commented Feb 28, 2025

🐰 Bencher Report

Branchivan/remove-cryptobox-migrate
Testbedubuntu-latest
Click to view all benchmark results
BenchmarkLatencymilliseconds (ms)
Commit add f(group size)/cs1/mem/1002📈 view plot
🚷 view threshold
20.43 ms
Commit add f(group size)/cs1/mem/2📈 view plot
🚷 view threshold
7.40 ms
Commit add f(group size)/cs1/mem/202📈 view plot
🚷 view threshold
10.39 ms
Commit add f(group size)/cs1/mem/402📈 view plot
🚷 view threshold
12.57 ms
Commit add f(group size)/cs1/mem/602📈 view plot
🚷 view threshold
16.21 ms
Commit add f(group size)/cs1/mem/802📈 view plot
🚷 view threshold
18.43 ms
Commit add f(number clients)/cs1/mem/1002📈 view plot
🚷 view threshold
1,001.50 ms
Commit add f(number clients)/cs1/mem/2📈 view plot
🚷 view threshold
7.45 ms
Commit add f(number clients)/cs1/mem/202📈 view plot
🚷 view threshold
86.89 ms
Commit add f(number clients)/cs1/mem/402📈 view plot
🚷 view threshold
226.35 ms
Commit add f(number clients)/cs1/mem/602📈 view plot
🚷 view threshold
431.94 ms
Commit add f(number clients)/cs1/mem/802📈 view plot
🚷 view threshold
682.08 ms
Commit pending proposals f(group size)/cs1/mem/1002📈 view plot
🚷 view threshold
127.07 ms
Commit pending proposals f(group size)/cs1/mem/2📈 view plot
🚷 view threshold
29.02 ms
Commit pending proposals f(group size)/cs1/mem/202📈 view plot
🚷 view threshold
47.40 ms
Commit pending proposals f(group size)/cs1/mem/402📈 view plot
🚷 view threshold
64.55 ms
Commit pending proposals f(group size)/cs1/mem/602📈 view plot
🚷 view threshold
82.09 ms
Commit pending proposals f(group size)/cs1/mem/802📈 view plot
🚷 view threshold
97.87 ms
Commit pending proposals f(pending size)/cs1/mem/1📈 view plot
🚷 view threshold
20.25 ms
Commit pending proposals f(pending size)/cs1/mem/101📈 view plot
🚷 view threshold
118.07 ms
Commit pending proposals f(pending size)/cs1/mem/21📈 view plot
🚷 view threshold
37.43 ms
Commit pending proposals f(pending size)/cs1/mem/41📈 view plot
🚷 view threshold
58.46 ms
Commit pending proposals f(pending size)/cs1/mem/61📈 view plot
🚷 view threshold
77.41 ms
Commit pending proposals f(pending size)/cs1/mem/81📈 view plot
🚷 view threshold
98.76 ms
Commit remove f(group size)/cs1/mem/1002📈 view plot
🚷 view threshold
30.07 ms
Commit remove f(group size)/cs1/mem/2📈 view plot
🚷 view threshold
6.89 ms
Commit remove f(group size)/cs1/mem/202📈 view plot
🚷 view threshold
9.65 ms
Commit remove f(group size)/cs1/mem/402📈 view plot
🚷 view threshold
13.55 ms
Commit remove f(group size)/cs1/mem/602📈 view plot
🚷 view threshold
18.37 ms
Commit remove f(group size)/cs1/mem/802📈 view plot
🚷 view threshold
22.80 ms
Commit remove f(number clients)/cs1/mem/1002📈 view plot
🚷 view threshold
34.04 ms
Commit remove f(number clients)/cs1/mem/2📈 view plot
🚷 view threshold
139.76 ms
Commit remove f(number clients)/cs1/mem/202📈 view plot
🚷 view threshold
117.91 ms
Commit remove f(number clients)/cs1/mem/402📈 view plot
🚷 view threshold
97.43 ms
Commit remove f(number clients)/cs1/mem/602📈 view plot
🚷 view threshold
75.08 ms
Commit remove f(number clients)/cs1/mem/802📈 view plot
🚷 view threshold
53.94 ms
Commit update f(group size)/cs1/mem/1002📈 view plot
🚷 view threshold
139.36 ms
Commit update f(group size)/cs1/mem/2📈 view plot
🚷 view threshold
7.24 ms
Commit update f(group size)/cs1/mem/202📈 view plot
🚷 view threshold
35.23 ms
Commit update f(group size)/cs1/mem/402📈 view plot
🚷 view threshold
60.60 ms
Commit update f(group size)/cs1/mem/602📈 view plot
🚷 view threshold
88.21 ms
Commit update f(group size)/cs1/mem/802📈 view plot
🚷 view threshold
113.95 ms
🐰 View full continuous benchmarking report in Bencher

@codecov-commenter
Copy link

codecov-commenter commented Feb 28, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.14%. Comparing base (39137b0) to head (d7d3e6c).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #942      +/-   ##
==========================================
- Coverage   79.27%   79.14%   -0.14%     
==========================================
  Files         113      113              
  Lines       20853    20713     -140     
==========================================
- Hits        16532    16393     -139     
+ Misses       4321     4320       -1     
Files with missing lines Coverage Δ
crypto-ffi/src/generic/context/proteus.rs 0.00% <ø> (ø)
crypto-ffi/src/generic/mod.rs 13.10% <ø> (+0.01%) ⬆️
crypto/src/error/mod.rs 100.00% <ø> (ø)
crypto/src/lib.rs 37.50% <ø> (ø)
crypto/src/proteus.rs 47.66% <ø> (-11.80%) ⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39137b0...d7d3e6c. Read the comment docs.

@istankovic istankovic force-pushed the ivan/remove-cryptobox-migrate branch from 0f562b9 to 7349166 Compare February 28, 2025 17:00
@istankovic istankovic marked this pull request as draft February 28, 2025 17:17
@istankovic istankovic marked this pull request as ready for review March 3, 2025 08:46
@istankovic istankovic force-pushed the ivan/remove-cryptobox-migrate branch from 7349166 to 0ac440f Compare March 3, 2025 08:52
All clients have moved away from cryptobox so it doesn't make sense to
have this.
@istankovic istankovic force-pushed the ivan/remove-cryptobox-migrate branch from 0ac440f to d7d3e6c Compare March 3, 2025 08:54
@istankovic istankovic changed the title chore: remove the cryptobox-migrate feature chore!: remove the cryptobox-migrate feature Mar 3, 2025
coriolinus
coriolinus previously approved these changes Mar 3, 2025
Copy link
Contributor

@coriolinus coriolinus left a comment

Choose a reason for hiding this comment

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

I love getting rid of obsolete features!

Copy link
Contributor

Choose a reason for hiding this comment

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

Great job that we can get rid of this entirely!

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, I misread the GH interface. We're not deleting the file entirely, but still, great to remove this much code from it.

@coriolinus coriolinus dismissed their stale review March 4, 2025 11:22

I hadn't previously understood the implications of removing the Cryptobox migration. This code isn't wrong, strictly, but should probably not be merged before August 2025.

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.

3 participants