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

feat: implement set_client_data() [WPB-10919] #757

Merged
merged 8 commits into from
Nov 12, 2024

Conversation

SimonThormeyer
Copy link
Contributor

@SimonThormeyer SimonThormeyer commented Nov 12, 2024

What's new in this PR

See title and corresponding Jira item


PR Submission Checklist for internal contributors
  • The PR Title
    • conforms to the style of semantic commits messages¹ supported in Wire's Github Workflow²
    • contains a reference JIRA issue number like SQPIT-764
    • answers the question: If merged, this PR will: ... ³
  1. https://sparkbox.com/foundry/semantic_commit_messages
  2. https://github.com/wireapp/.github#usage
  3. E.g. feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.

@SimonThormeyer SimonThormeyer requested a review from a team as a code owner November 12, 2024 09:18
@echoes-hq echoes-hq bot added the echoes: product-roadmap Work aligned with the customer-announced roadmap, targeting a specific release date. label Nov 12, 2024
@codecov-commenter
Copy link

codecov-commenter commented Nov 12, 2024

Codecov Report

Attention: Patch coverage is 5.08475% with 56 lines in your changes missing coverage. Please review.

Project coverage is 72.19%. Comparing base (acf727b) to head (8ac1aff).

Files with missing lines Patch % Lines
keystore/src/entities/platform/generic/general.rs 12.50% 21 Missing ⚠️
keystore/src/entities/mls.rs 0.00% 11 Missing ⚠️
crypto/src/context.rs 0.00% 10 Missing ⚠️
keystore/src/entities/general.rs 0.00% 7 Missing ⚠️
crypto-ffi/src/generic/context/mod.rs 0.00% 6 Missing ⚠️
keystore/src/transaction/dynamic_dispatch.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #757      +/-   ##
==========================================
- Coverage   72.39%   72.19%   -0.21%     
==========================================
  Files         104      106       +2     
  Lines       19650    19709      +59     
==========================================
+ Hits        14225    14228       +3     
- Misses       5425     5481      +56     
Files with missing lines Coverage Δ
keystore/src/entities/mod.rs 92.10% <ø> (ø)
keystore/src/error.rs 0.00% <ø> (ø)
keystore/src/transaction/mod.rs 89.26% <ø> (ø)
keystore/src/transaction/dynamic_dispatch.rs 93.02% <0.00%> (-1.10%) ⬇️
crypto-ffi/src/generic/context/mod.rs 0.00% <0.00%> (ø)
keystore/src/entities/general.rs 0.00% <0.00%> (ø)
crypto/src/context.rs 73.46% <0.00%> (-8.35%) ⬇️
keystore/src/entities/mls.rs 59.29% <0.00%> (-6.40%) ⬇️
keystore/src/entities/platform/generic/general.rs 12.50% <12.50%> (ø)

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 acf727b...8ac1aff. Read the comment docs.

Preparation for the set client data API (WPB-10919)
Preparation for the set client data API (WPB-10919)
These functions were previously unimplemented.
There is no reason to leave it this way, since we can provide meaningful implementations for unique entities.
Preparation for the set client data API (WPB-10919)
And in JVM wrapper.
Part of WPB-10919.
@SimonThormeyer SimonThormeyer force-pushed the simon/feat/set-client-data branch from c0fa4ad to 942ff79 Compare November 12, 2024 09:30
Copy link

github-actions bot commented Nov 12, 2024

🐰 Bencher Report

Branchsimon/feat/set-client-data
Testbedubuntu-latest
Click to view all benchmark results
BenchmarkLatencynanoseconds (ns)
Commit add f(group size)/cs1/mem/1002📈 view plot
🚷 view threshold
18,987,000.00
Commit add f(group size)/cs1/mem/2📈 view plot
🚷 view threshold
6,871,100.00
Commit add f(group size)/cs1/mem/202📈 view plot
🚷 view threshold
9,535,900.00
Commit add f(group size)/cs1/mem/402📈 view plot
🚷 view threshold
11,605,000.00
Commit add f(group size)/cs1/mem/602📈 view plot
🚷 view threshold
14,624,000.00
Commit add f(group size)/cs1/mem/802📈 view plot
🚷 view threshold
16,804,000.00
Commit add f(number clients)/cs1/mem/1002📈 view plot
🚷 view threshold
983,690,000.00
Commit add f(number clients)/cs1/mem/2📈 view plot
🚷 view threshold
6,885,800.00
Commit add f(number clients)/cs1/mem/202📈 view plot
🚷 view threshold
84,701,000.00
Commit add f(number clients)/cs1/mem/402📈 view plot
🚷 view threshold
219,540,000.00
Commit add f(number clients)/cs1/mem/602📈 view plot
🚷 view threshold
426,270,000.00
Commit add f(number clients)/cs1/mem/802📈 view plot
🚷 view threshold
676,070,000.00
Commit pending proposals f(group size)/cs1/mem/1002📈 view plot
🚷 view threshold
116,950,000.00
Commit pending proposals f(group size)/cs1/mem/2📈 view plot
🚷 view threshold
28,639,000.00
Commit pending proposals f(group size)/cs1/mem/202📈 view plot
🚷 view threshold
46,246,000.00
Commit pending proposals f(group size)/cs1/mem/402📈 view plot
🚷 view threshold
61,373,000.00
Commit pending proposals f(group size)/cs1/mem/602📈 view plot
🚷 view threshold
79,325,000.00
Commit pending proposals f(group size)/cs1/mem/802📈 view plot
🚷 view threshold
94,293,000.00
Commit pending proposals f(pending size)/cs1/mem/1📈 view plot
🚷 view threshold
18,279,000.00
Commit pending proposals f(pending size)/cs1/mem/101📈 view plot
🚷 view threshold
115,120,000.00
Commit pending proposals f(pending size)/cs1/mem/21📈 view plot
🚷 view threshold
35,268,000.00
Commit pending proposals f(pending size)/cs1/mem/41📈 view plot
🚷 view threshold
56,872,000.00
Commit pending proposals f(pending size)/cs1/mem/61📈 view plot
🚷 view threshold
75,544,000.00
Commit pending proposals f(pending size)/cs1/mem/81📈 view plot
🚷 view threshold
95,192,000.00
Commit remove f(group size)/cs1/mem/1002📈 view plot
🚷 view threshold
27,324,000.00
Commit remove f(group size)/cs1/mem/2📈 view plot
🚷 view threshold
6,804,700.00
Commit remove f(group size)/cs1/mem/202📈 view plot
🚷 view threshold
8,889,700.00
Commit remove f(group size)/cs1/mem/402📈 view plot
🚷 view threshold
11,991,000.00
Commit remove f(group size)/cs1/mem/602📈 view plot
🚷 view threshold
16,675,000.00
Commit remove f(group size)/cs1/mem/802📈 view plot
🚷 view threshold
21,771,000.00
Commit remove f(number clients)/cs1/mem/1002📈 view plot
🚷 view threshold
29,617,000.00
Commit remove f(number clients)/cs1/mem/2📈 view plot
🚷 view threshold
137,570,000.00
Commit remove f(number clients)/cs1/mem/202📈 view plot
🚷 view threshold
115,120,000.00
Commit remove f(number clients)/cs1/mem/402📈 view plot
🚷 view threshold
93,638,000.00
Commit remove f(number clients)/cs1/mem/602📈 view plot
🚷 view threshold
71,927,000.00
Commit remove f(number clients)/cs1/mem/802📈 view plot
🚷 view threshold
50,398,000.00
Commit update f(group size)/cs1/mem/1002📈 view plot
🚷 view threshold
137,530,000.00
Commit update f(group size)/cs1/mem/2📈 view plot
🚷 view threshold
6,970,700.00
Commit update f(group size)/cs1/mem/202📈 view plot
🚷 view threshold
33,794,000.00
Commit update f(group size)/cs1/mem/402📈 view plot
🚷 view threshold
59,794,000.00
Commit update f(group size)/cs1/mem/602📈 view plot
🚷 view threshold
86,848,000.00
Commit update f(group size)/cs1/mem/802📈 view plot
🚷 view threshold
112,130,000.00
Count KeyPackage/cs1/mem/1002📈 view plot
🚷 view threshold
10,397,000.00
Count KeyPackage/cs1/mem/2📈 view plot
🚷 view threshold
6,427,100.00
Count KeyPackage/cs1/mem/202📈 view plot
🚷 view threshold
7,226,800.00
Count KeyPackage/cs1/mem/402📈 view plot
🚷 view threshold
8,191,300.00
Count KeyPackage/cs1/mem/602📈 view plot
🚷 view threshold
8,790,100.00
Count KeyPackage/cs1/mem/802📈 view plot
🚷 view threshold
9,338,800.00
Create group/cs1/mem📈 view plot
🚷 view threshold
6,647,200.00
Decrypt f(msg size)/cs1/mem/10📈 view plot
🚷 view threshold
6,142,500.00
Decrypt f(msg size)/cs1/mem/10010📈 view plot
🚷 view threshold
6,277,300.00
Decrypt f(msg size)/cs1/mem/2010📈 view plot
🚷 view threshold
6,330,500.00
Decrypt f(msg size)/cs1/mem/4010📈 view plot
🚷 view threshold
6,330,100.00
Decrypt f(msg size)/cs1/mem/6010📈 view plot
🚷 view threshold
6,250,200.00
Decrypt f(msg size)/cs1/mem/8010📈 view plot
🚷 view threshold
6,303,900.00
Encrypt f(group size)/cs1/mem/1002📈 view plot
🚷 view threshold
8,866,200.00
Encrypt f(group size)/cs1/mem/2📈 view plot
🚷 view threshold
6,276,600.00
Encrypt f(group size)/cs1/mem/202📈 view plot
🚷 view threshold
7,041,700.00
Encrypt f(group size)/cs1/mem/402📈 view plot
🚷 view threshold
7,523,300.00
Encrypt f(group size)/cs1/mem/602📈 view plot
🚷 view threshold
7,901,100.00
Encrypt f(group size)/cs1/mem/802📈 view plot
🚷 view threshold
8,331,100.00
Encrypt f(msg size)/cs1/mem/10📈 view plot
🚷 view threshold
8,343,800.00
Encrypt f(msg size)/cs1/mem/10010📈 view plot
🚷 view threshold
8,788,100.00
Encrypt f(msg size)/cs1/mem/2010📈 view plot
🚷 view threshold
8,652,700.00
Encrypt f(msg size)/cs1/mem/4010📈 view plot
🚷 view threshold
8,957,400.00
Encrypt f(msg size)/cs1/mem/6010📈 view plot
🚷 view threshold
8,510,500.00
Encrypt f(msg size)/cs1/mem/8010📈 view plot
🚷 view threshold
8,633,100.00
Generate KeyPackage f(group size)/cs1/mem/1002📈 view plot
🚷 view threshold
241,960,000.00
Generate KeyPackage f(group size)/cs1/mem/2📈 view plot
🚷 view threshold
6,573,500.00
Generate KeyPackage f(group size)/cs1/mem/202📈 view plot
🚷 view threshold
33,123,000.00
Generate KeyPackage f(group size)/cs1/mem/402📈 view plot
🚷 view threshold
85,190,000.00
Generate KeyPackage f(group size)/cs1/mem/602📈 view plot
🚷 view threshold
137,420,000.00
Generate KeyPackage f(group size)/cs1/mem/802📈 view plot
🚷 view threshold
189,880,000.00
Join from external commit f(group size)/cs1/mem/1002📈 view plot
🚷 view threshold
239,430,000.00
Join from external commit f(group size)/cs1/mem/2📈 view plot
🚷 view threshold
7,492,600.00
Join from external commit f(group size)/cs1/mem/202📈 view plot
🚷 view threshold
53,830,000.00
Join from external commit f(group size)/cs1/mem/402📈 view plot
🚷 view threshold
100,440,000.00
Join from external commit f(group size)/cs1/mem/602📈 view plot
🚷 view threshold
147,470,000.00
Join from external commit f(group size)/cs1/mem/802📈 view plot
🚷 view threshold
192,900,000.00
Join from welcome f(group size)/cs1/mem/1002📈 view plot
🚷 view threshold
141,950,000.00
Join from welcome f(group size)/cs1/mem/2📈 view plot
🚷 view threshold
7,269,800.00
Join from welcome f(group size)/cs1/mem/202📈 view plot
🚷 view threshold
29,403,000.00
Join from welcome f(group size)/cs1/mem/402📈 view plot
🚷 view threshold
51,472,000.00
Join from welcome f(group size)/cs1/mem/602📈 view plot
🚷 view threshold
74,228,000.00
Join from welcome f(group size)/cs1/mem/802📈 view plot
🚷 view threshold
96,413,000.00
Mls vs Proteus: add/MLS/mem/1📈 view plot
🚷 view threshold
6,780,900.00
Mls vs Proteus: add/MLS/mem/101📈 view plot
🚷 view threshold
8,425,200.00
Mls vs Proteus: add/MLS/mem/21📈 view plot
🚷 view threshold
7,062,500.00
Mls vs Proteus: add/MLS/mem/41📈 view plot
🚷 view threshold
7,444,900.00
Mls vs Proteus: add/MLS/mem/61📈 view plot
🚷 view threshold
7,568,300.00
Mls vs Proteus: add/MLS/mem/81📈 view plot
🚷 view threshold
7,794,500.00
Mls vs Proteus: add/Proteus/mem/1📈 view plot
🚷 view threshold
6,458,100.00
Mls vs Proteus: add/Proteus/mem/101📈 view plot
🚷 view threshold
44,485,000.00
Mls vs Proteus: add/Proteus/mem/21📈 view plot
🚷 view threshold
14,068,000.00
Mls vs Proteus: add/Proteus/mem/41📈 view plot
🚷 view threshold
22,130,000.00
Mls vs Proteus: add/Proteus/mem/61📈 view plot
🚷 view threshold
29,344,000.00
Mls vs Proteus: add/Proteus/mem/81📈 view plot
🚷 view threshold
36,882,000.00
Mls vs Proteus: encrypt/MLS/mem/1📈 view plot
🚷 view threshold
6,543,900.00
Mls vs Proteus: encrypt/MLS/mem/101📈 view plot
🚷 view threshold
6,838,900.00
Mls vs Proteus: encrypt/MLS/mem/21📈 view plot
🚷 view threshold
6,318,400.00
Mls vs Proteus: encrypt/MLS/mem/41📈 view plot
🚷 view threshold
6,550,900.00
Mls vs Proteus: encrypt/MLS/mem/61📈 view plot
🚷 view threshold
6,538,600.00
Mls vs Proteus: encrypt/MLS/mem/81📈 view plot
🚷 view threshold
6,877,200.00
Mls vs Proteus: encrypt/Proteus/mem/1📈 view plot
🚷 view threshold
6,114,500.00
Mls vs Proteus: encrypt/Proteus/mem/101📈 view plot
🚷 view threshold
15,945,000.00
Mls vs Proteus: encrypt/Proteus/mem/21📈 view plot
🚷 view threshold
8,199,100.00
Mls vs Proteus: encrypt/Proteus/mem/41📈 view plot
🚷 view threshold
12,251,000.00
Mls vs Proteus: encrypt/Proteus/mem/61📈 view plot
🚷 view threshold
12,212,000.00
Mls vs Proteus: encrypt/Proteus/mem/81📈 view plot
🚷 view threshold
14,822,000.00
Mls vs Proteus: remove/MLS/mem/1📈 view plot
🚷 view threshold
20,227,000.00
Mls vs Proteus: remove/MLS/mem/101📈 view plot
🚷 view threshold
8,197,300.00
Mls vs Proteus: remove/MLS/mem/21📈 view plot
🚷 view threshold
17,658,000.00
Mls vs Proteus: remove/MLS/mem/41📈 view plot
🚷 view threshold
15,085,000.00
Mls vs Proteus: remove/MLS/mem/61📈 view plot
🚷 view threshold
12,815,000.00
Mls vs Proteus: remove/MLS/mem/81📈 view plot
🚷 view threshold
10,714,000.00
Mls vs Proteus: remove/Proteus/mem/1📈 view plot
🚷 view threshold
6,064,200.00
Mls vs Proteus: remove/Proteus/mem/101📈 view plot
🚷 view threshold
7,540,900.00
Mls vs Proteus: remove/Proteus/mem/21📈 view plot
🚷 view threshold
6,247,400.00
Mls vs Proteus: remove/Proteus/mem/41📈 view plot
🚷 view threshold
6,439,700.00
Mls vs Proteus: remove/Proteus/mem/61📈 view plot
🚷 view threshold
6,707,900.00
Mls vs Proteus: remove/Proteus/mem/81📈 view plot
🚷 view threshold
7,234,500.00
Mls vs Proteus: update/MLS/mem/1📈 view plot
🚷 view threshold
6,941,100.00
Mls vs Proteus: update/MLS/mem/101📈 view plot
🚷 view threshold
20,280,000.00
Mls vs Proteus: update/MLS/mem/21📈 view plot
🚷 view threshold
9,942,400.00
Mls vs Proteus: update/MLS/mem/41📈 view plot
🚷 view threshold
12,387,000.00
Mls vs Proteus: update/MLS/mem/61📈 view plot
🚷 view threshold
15,044,000.00
Mls vs Proteus: update/MLS/mem/81📈 view plot
🚷 view threshold
17,884,000.00
Mls vs Proteus: update/Proteus/mem/1📈 view plot
🚷 view threshold
6,519,700.00
Mls vs Proteus: update/Proteus/mem/101📈 view plot
🚷 view threshold
46,230,000.00
Mls vs Proteus: update/Proteus/mem/21📈 view plot
🚷 view threshold
14,538,000.00
Mls vs Proteus: update/Proteus/mem/41📈 view plot
🚷 view threshold
22,554,000.00
Mls vs Proteus: update/Proteus/mem/61📈 view plot
🚷 view threshold
30,597,000.00
Mls vs Proteus: update/Proteus/mem/81📈 view plot
🚷 view threshold
38,499,000.00
🐰 View full continuous benchmarking report in Bencher

@SimonThormeyer SimonThormeyer force-pushed the simon/feat/set-client-data branch 2 times, most recently from e7baa2b to c02f4de Compare November 12, 2024 10:03
@SimonThormeyer SimonThormeyer force-pushed the simon/feat/set-client-data branch from c02f4de to 8ac1aff Compare November 12, 2024 10:03
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.

Had some questions, but I believe this should be good to go.

@@ -0,0 +1,4 @@
CREATE TABLE consumer_data (
id INTEGER PRIMARY KEY CHECK ( id = 0 ),
Copy link
Contributor

Choose a reason for hiding this comment

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

This appears to enforce that id is only ever equal to 0. Is this to enforce that there is only ever one row in the table?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, exactly. This pattern can also be seen in other tables where we only ever expect one record, such as e2ei_acme_ca.

(identifier_16, ProteusPrekey),
(identifier_17, ProteusIdentity),
(identifier_18, ProteusSession)
(identifier_17, ProteusPrekey),
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know how these identifiers are used. Are these identifiers persisted at all? Can anything go wrong by moving identifier_16 from ProteusPrekey to ConsumerData?

Copy link
Contributor Author

@SimonThormeyer SimonThormeyer Nov 12, 2024

Choose a reason for hiding this comment

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

These identifiers are merely used to hold the data from the in-memory cache before it is committed. So it doesn't matter what their name is/was at all.

If we can create them internally in the macro instead of passing them in, I would like doing so!

Copy link
Contributor

Choose a reason for hiding this comment

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

Creating idents like this within a macro is tricky but not impossible.

In the future, it will be easier: combine the ${index()} metafunction with the paste! macro and we should be able to generate all these idents automatically.

For now, ${index()} is still not stable, so you have to use recursive macro tricks to build up a unary number for each index and then count how many symbols appear in that number. This approach is complicated and opaque enough I'd recommend not reworking the existing macro until ${index()} stabilizes.

@SimonThormeyer SimonThormeyer merged commit 07b97f5 into main Nov 12, 2024
24 checks passed
@SimonThormeyer SimonThormeyer deleted the simon/feat/set-client-data branch November 12, 2024 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
echoes: product-roadmap Work aligned with the customer-announced roadmap, targeting a specific release date.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants