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.

Sign up for GitHub

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

Jump to bottom

feat: implement binding specific errors [WPB-14352] #771

Merged
merged 9 commits into from
Nov 25, 2024
Merged

feat: implement binding specific errors [WPB-14352] #771

merged 9 commits into from
Nov 25, 2024

Conversation

coriolinus
Copy link
Contributor

@coriolinus coriolinus commented Nov 20, 2024 •
edited
Loading

What's new in this PR

Clients only care about a few error types, so let's surface those at the client interface and hide the rest.

(We can log the full detailed errors at the conversion point, but that's a separate task.)

  • binding-specific errors for WASM targets
  • binding-specific errors for uniffi targets
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.

@echoes-hq echoes-hq bot added the echoes: product-roadmap/feature Work contributing to adding a new feature as part of the product roadmap. label Nov 20, 2024
typfel
typfel reviewed Nov 21, 2024
View reviewed changes
crypto-ffi/src/wasm/mod.rs Outdated Show resolved Hide resolved
@coriolinus coriolinus force-pushed the prgn/feat/binding-specific-errors branch from 754fbe2 to 1d71062 Compare November 21, 2024 09:30
@codecov-commenter
Copy link

codecov-commenter commented Nov 21, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 85 lines in your changes missing coverage. Please review.

Project coverage is 71.61%. Comparing base (3c71dbc) to head (8c34db6).

Files with missing lines Patch % Lines
crypto-ffi/src/generic/mod.rs 0.00% 65 Missing ⚠️
crypto/src/error.rs 0.00% 12 Missing ⚠️
crypto-ffi/src/generic/context/proteus.rs 0.00% 5 Missing ⚠️
crypto-ffi/src/generic/context/mod.rs 0.00% 3 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #771      +/-   ##
==========================================
- Coverage   71.81%   71.61%   -0.21%     
==========================================
  Files         107      107              
  Lines       19840    19896      +56     
==========================================
  Hits        14249    14249              
- Misses       5591     5647      +56
Files with missing lines Coverage Δ
crypto-ffi/src/lib.rs 0.00% <ø> (ø)
crypto-ffi/src/generic/context/mod.rs 0.00% <0.00%> (ø)
crypto-ffi/src/generic/context/proteus.rs 0.00% <0.00%> (ø)
crypto/src/error.rs 19.23% <0.00%> (-4.58%) ⬇️
crypto-ffi/src/generic/mod.rs 0.00% <0.00%> (ø)

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 3c71dbc...8c34db6. Read the comment docs.

---- 🚨 Try these New Features:

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 21, 2024 •
edited
Loading

🐰 Bencher Report

Branchprgn/feat/binding-specific-errors
Testbedubuntu-latest
Click to view all benchmark results
BenchmarkLatencynanoseconds (ns)
Commit add f(group size)/cs1/mem/1002📈 view plot
🚷 view threshold		18,241,000.00
Commit add f(group size)/cs1/mem/2📈 view plot
🚷 view threshold		6,736,200.00
Commit add f(group size)/cs1/mem/202📈 view plot
🚷 view threshold		9,269,900.00
Commit add f(group size)/cs1/mem/402📈 view plot
🚷 view threshold		12,980,000.00
Commit add f(group size)/cs1/mem/602📈 view plot
🚷 view threshold		14,597,000.00
Commit add f(group size)/cs1/mem/802📈 view plot
🚷 view threshold		16,498,000.00
Commit add f(number clients)/cs1/mem/1002📈 view plot
🚷 view threshold		976,940,000.00
Commit add f(number clients)/cs1/mem/2📈 view plot
🚷 view threshold		6,750,500.00
Commit add f(number clients)/cs1/mem/202📈 view plot
🚷 view threshold		83,629,000.00
Commit add f(number clients)/cs1/mem/402📈 view plot
🚷 view threshold		218,180,000.00
Commit add f(number clients)/cs1/mem/602📈 view plot
🚷 view threshold		421,440,000.00
Commit add f(number clients)/cs1/mem/802📈 view plot
🚷 view threshold		670,940,000.00
Commit pending proposals f(group size)/cs1/mem/1002📈 view plot
🚷 view threshold		115,370,000.00
Commit pending proposals f(group size)/cs1/mem/2📈 view plot
🚷 view threshold		28,566,000.00
Commit pending proposals f(group size)/cs1/mem/202📈 view plot
🚷 view threshold		47,009,000.00
Commit pending proposals f(group size)/cs1/mem/402📈 view plot
🚷 view threshold		60,513,000.00
Commit pending proposals f(group size)/cs1/mem/602📈 view plot
🚷 view threshold		79,397,000.00
Commit pending proposals f(group size)/cs1/mem/802📈 view plot
🚷 view threshold		93,552,000.00
Commit pending proposals f(pending size)/cs1/mem/1📈 view plot
🚷 view threshold		18,725,000.00
Commit pending proposals f(pending size)/cs1/mem/101📈 view plot
🚷 view threshold		114,930,000.00
Commit pending proposals f(pending size)/cs1/mem/21📈 view plot
🚷 view threshold		35,718,000.00
Commit pending proposals f(pending size)/cs1/mem/41📈 view plot
🚷 view threshold		56,865,000.00
Commit pending proposals f(pending size)/cs1/mem/61📈 view plot
🚷 view threshold		75,199,000.00
Commit pending proposals f(pending size)/cs1/mem/81📈 view plot
🚷 view threshold		94,975,000.00
Commit remove f(group size)/cs1/mem/1002📈 view plot
🚷 view threshold		27,433,000.00
Commit remove f(group size)/cs1/mem/2📈 view plot
🚷 view threshold		6,628,800.00
Commit remove f(group size)/cs1/mem/202📈 view plot
🚷 view threshold		8,598,300.00
Commit remove f(group size)/cs1/mem/402📈 view plot
🚷 view threshold		11,762,000.00
Commit remove f(group size)/cs1/mem/602📈 view plot
🚷 view threshold		17,075,000.00
Commit remove f(group size)/cs1/mem/802📈 view plot
🚷 view threshold		21,731,000.00
Commit remove f(number clients)/cs1/mem/1002📈 view plot
🚷 view threshold		30,089,000.00
Commit remove f(number clients)/cs1/mem/2📈 view plot
🚷 view threshold		136,480,000.00
Commit remove f(number clients)/cs1/mem/202📈 view plot
🚷 view threshold		114,580,000.00
Commit remove f(number clients)/cs1/mem/402📈 view plot
🚷 view threshold		93,161,000.00
Commit remove f(number clients)/cs1/mem/602📈 view plot
🚷 view threshold		71,443,000.00
Commit remove f(number clients)/cs1/mem/802📈 view plot
🚷 view threshold		50,894,000.00
Commit update f(group size)/cs1/mem/1002📈 view plot
🚷 view threshold		136,750,000.00
Commit update f(group size)/cs1/mem/2📈 view plot
🚷 view threshold		6,896,400.00
Commit update f(group size)/cs1/mem/202📈 view plot
🚷 view threshold		33,402,000.00
Commit update f(group size)/cs1/mem/402📈 view plot
🚷 view threshold		59,281,000.00
Commit update f(group size)/cs1/mem/602📈 view plot
🚷 view threshold		87,265,000.00
Commit update f(group size)/cs1/mem/802📈 view plot
🚷 view threshold		111,840,000.00
Count KeyPackage/cs1/mem/1002📈 view plot
🚷 view threshold		10,175,000.00
Count KeyPackage/cs1/mem/2📈 view plot
🚷 view threshold		6,492,900.00
Count KeyPackage/cs1/mem/202📈 view plot
🚷 view threshold		7,097,100.00
Count KeyPackage/cs1/mem/402📈 view plot
🚷 view threshold		7,784,900.00
Count KeyPackage/cs1/mem/602📈 view plot
🚷 view threshold		8,532,900.00
Count KeyPackage/cs1/mem/802📈 view plot
🚷 view threshold		9,347,300.00
Create group/cs1/mem📈 view plot
🚷 view threshold		6,481,300.00
Decrypt f(msg size)/cs1/mem/10📈 view plot
🚷 view threshold		6,322,300.00
Decrypt f(msg size)/cs1/mem/10010📈 view plot
🚷 view threshold		6,313,000.00
Decrypt f(msg size)/cs1/mem/2010📈 view plot
🚷 view threshold		6,351,300.00
Decrypt f(msg size)/cs1/mem/4010📈 view plot
🚷 view threshold		6,307,200.00
Decrypt f(msg size)/cs1/mem/6010📈 view plot
🚷 view threshold		6,335,700.00
Decrypt f(msg size)/cs1/mem/8010📈 view plot
🚷 view threshold		6,411,400.00
Encrypt f(group size)/cs1/mem/1002📈 view plot
🚷 view threshold		8,695,300.00
Encrypt f(group size)/cs1/mem/2📈 view plot
🚷 view threshold		6,409,700.00
Encrypt f(group size)/cs1/mem/202📈 view plot
🚷 view threshold		6,960,300.00
Encrypt f(group size)/cs1/mem/402📈 view plot
🚷 view threshold		7,087,500.00
Encrypt f(group size)/cs1/mem/602📈 view plot
🚷 view threshold		7,530,600.00
Encrypt f(group size)/cs1/mem/802📈 view plot
🚷 view threshold		8,311,400.00
Encrypt f(msg size)/cs1/mem/10📈 view plot
🚷 view threshold		8,561,800.00
Encrypt f(msg size)/cs1/mem/10010📈 view plot
🚷 view threshold		8,887,200.00
Encrypt f(msg size)/cs1/mem/2010📈 view plot
🚷 view threshold		8,825,800.00
Encrypt f(msg size)/cs1/mem/4010📈 view plot
🚷 view threshold		8,924,300.00
Encrypt f(msg size)/cs1/mem/6010📈 view plot
🚷 view threshold		8,640,600.00
Encrypt f(msg size)/cs1/mem/8010📈 view plot
🚷 view threshold		8,866,500.00
Generate KeyPackage f(group size)/cs1/mem/1002📈 view plot
🚷 view threshold		245,880,000.00
Generate KeyPackage f(group size)/cs1/mem/2📈 view plot
🚷 view threshold		6,603,700.00
Generate KeyPackage f(group size)/cs1/mem/202📈 view plot
🚷 view threshold		33,276,000.00
Generate KeyPackage f(group size)/cs1/mem/402📈 view plot
🚷 view threshold		85,930,000.00
Generate KeyPackage f(group size)/cs1/mem/602📈 view plot
🚷 view threshold		138,490,000.00
Generate KeyPackage f(group size)/cs1/mem/802📈 view plot
🚷 view threshold		190,950,000.00
Join from external commit f(group size)/cs1/mem/1002📈 view plot
🚷 view threshold		241,040,000.00
Join from external commit f(group size)/cs1/mem/2📈 view plot
🚷 view threshold		7,546,000.00
Join from external commit f(group size)/cs1/mem/202📈 view plot
🚷 view threshold		54,223,000.00
Join from external commit f(group size)/cs1/mem/402📈 view plot
🚷 view threshold		100,830,000.00
Join from external commit f(group size)/cs1/mem/602📈 view plot
🚷 view threshold		148,560,000.00
Join from external commit f(group size)/cs1/mem/802📈 view plot
🚷 view threshold		194,080,000.00
Join from welcome f(group size)/cs1/mem/1002📈 view plot
🚷 view threshold		115,060,000.00
Join from welcome f(group size)/cs1/mem/2📈 view plot
🚷 view threshold		7,068,100.00
Join from welcome f(group size)/cs1/mem/202📈 view plot
🚷 view threshold		28,801,000.00
Join from welcome f(group size)/cs1/mem/402📈 view plot
🚷 view threshold		50,591,000.00
Join from welcome f(group size)/cs1/mem/602📈 view plot
🚷 view threshold		72,882,000.00
Join from welcome f(group size)/cs1/mem/802📈 view plot
🚷 view threshold		93,681,000.00
Mls vs Proteus: add/MLS/mem/1📈 view plot
🚷 view threshold		6,721,900.00
Mls vs Proteus: add/MLS/mem/101📈 view plot
🚷 view threshold		7,956,000.00
Mls vs Proteus: add/MLS/mem/21📈 view plot
🚷 view threshold		6,956,000.00
Mls vs Proteus: add/MLS/mem/41📈 view plot
🚷 view threshold		7,235,000.00
Mls vs Proteus: add/MLS/mem/61📈 view plot
🚷 view threshold		7,531,600.00
Mls vs Proteus: add/MLS/mem/81📈 view plot
🚷 view threshold		7,693,600.00
Mls vs Proteus: add/Proteus/mem/1📈 view plot
🚷 view threshold		6,381,300.00
Mls vs Proteus: add/Proteus/mem/101📈 view plot
🚷 view threshold		44,761,000.00
Mls vs Proteus: add/Proteus/mem/21📈 view plot
🚷 view threshold		13,998,000.00
Mls vs Proteus: add/Proteus/mem/41📈 view plot
🚷 view threshold		21,513,000.00
Mls vs Proteus: add/Proteus/mem/61📈 view plot
🚷 view threshold		29,448,000.00
Mls vs Proteus: add/Proteus/mem/81📈 view plot
🚷 view threshold		37,288,000.00
Mls vs Proteus: encrypt/MLS/mem/1📈 view plot
🚷 view threshold		6,240,400.00
Mls vs Proteus: encrypt/MLS/mem/101📈 view plot
🚷 view threshold		6,639,200.00
Mls vs Proteus: encrypt/MLS/mem/21📈 view plot
🚷 view threshold		6,460,100.00
Mls vs Proteus: encrypt/MLS/mem/41📈 view plot
🚷 view threshold		6,486,500.00
Mls vs Proteus: encrypt/MLS/mem/61📈 view plot
🚷 view threshold		6,559,200.00
Mls vs Proteus: encrypt/MLS/mem/81📈 view plot
🚷 view threshold		6,682,600.00
Mls vs Proteus: encrypt/Proteus/mem/1📈 view plot
🚷 view threshold		6,006,600.00
Mls vs Proteus: encrypt/Proteus/mem/101📈 view plot
🚷 view threshold		15,968,000.00
Mls vs Proteus: encrypt/Proteus/mem/21📈 view plot
🚷 view threshold		8,249,300.00
Mls vs Proteus: encrypt/Proteus/mem/41📈 view plot
🚷 view threshold		10,048,000.00
Mls vs Proteus: encrypt/Proteus/mem/61📈 view plot
🚷 view threshold		12,140,000.00
Mls vs Proteus: encrypt/Proteus/mem/81📈 view plot
🚷 view threshold		13,907,000.00
Mls vs Proteus: remove/MLS/mem/1📈 view plot
🚷 view threshold		20,225,000.00
Mls vs Proteus: remove/MLS/mem/101📈 view plot
🚷 view threshold		8,362,700.00
Mls vs Proteus: remove/MLS/mem/21📈 view plot
🚷 view threshold		17,830,000.00
Mls vs Proteus: remove/MLS/mem/41📈 view plot
🚷 view threshold		15,127,000.00
Mls vs Proteus: remove/MLS/mem/61📈 view plot
🚷 view threshold		12,868,000.00
Mls vs Proteus: remove/MLS/mem/81📈 view plot
🚷 view threshold		10,583,000.00
Mls vs Proteus: remove/Proteus/mem/1📈 view plot
🚷 view threshold		5,993,100.00
Mls vs Proteus: remove/Proteus/mem/101📈 view plot
🚷 view threshold		7,446,100.00
Mls vs Proteus: remove/Proteus/mem/21📈 view plot
🚷 view threshold		6,313,800.00
Mls vs Proteus: remove/Proteus/mem/41📈 view plot
🚷 view threshold		6,509,700.00
Mls vs Proteus: remove/Proteus/mem/61📈 view plot
🚷 view threshold		6,691,300.00
Mls vs Proteus: remove/Proteus/mem/81📈 view plot
🚷 view threshold		7,271,400.00
Mls vs Proteus: update/MLS/mem/1📈 view plot
🚷 view threshold		6,785,800.00
Mls vs Proteus: update/MLS/mem/101📈 view plot
🚷 view threshold		19,956,000.00
Mls vs Proteus: update/MLS/mem/21📈 view plot
🚷 view threshold		9,525,100.00
Mls vs Proteus: update/MLS/mem/41📈 view plot
🚷 view threshold		12,349,000.00
Mls vs Proteus: update/MLS/mem/61📈 view plot
🚷 view threshold		14,846,000.00
Mls vs Proteus: update/MLS/mem/81📈 view plot
🚷 view threshold		17,656,000.00
Mls vs Proteus: update/Proteus/mem/1📈 view plot
🚷 view threshold		6,387,400.00
Mls vs Proteus: update/Proteus/mem/101📈 view plot
🚷 view threshold		46,214,000.00
Mls vs Proteus: update/Proteus/mem/21📈 view plot
🚷 view threshold		14,681,000.00
Mls vs Proteus: update/Proteus/mem/41📈 view plot
🚷 view threshold		22,391,000.00
Mls vs Proteus: update/Proteus/mem/61📈 view plot
🚷 view threshold		30,442,000.00
Mls vs Proteus: update/Proteus/mem/81📈 view plot
🚷 view threshold		38,420,000.00
🐰 View full continuous benchmarking report in Bencher

@coriolinus coriolinus force-pushed the prgn/feat/binding-specific-errors branch 3 times, most recently from 6f4c511 to b9bf997 Compare November 22, 2024 13:42
@coriolinus coriolinus marked this pull request as ready for review November 22, 2024 14:37
@coriolinus coriolinus requested a review from a team as a code owner November 22, 2024 14:37
@coriolinus
feat: proteus error codes are Option<u16> not u32
d2e9307 
Returning an error code whose zero value indicates success is
a reasonable pattern, in C.

We are not programming in C.

Eventually we're going to get rid of this pattern entirely. For now,
we can at least use a real `Option` type to indicate whether an error
in fact exists.

Also, the Proteus implementation specifies that the error code is
a `u16`. No idea why that got widened to a `u32`, but that was
pointless, so it's back to a `u16` here.
@coriolinus coriolinus force-pushed the prgn/feat/binding-specific-errors branch 2 times, most recently from f99601c to 9e6b6e9 Compare November 22, 2024 15:02
SimonThormeyer
SimonThormeyer approved these changes Nov 22, 2024
View reviewed changes
Copy link
Contributor

@SimonThormeyer SimonThormeyer left a comment

Choose a reason for hiding this comment

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

Looks good! Just one thing I would have done differently in the tests

crypto-ffi/bindings/js/test/CoreCrypto.test.ts Outdated Show resolved Hide resolved
istankovic
istankovic reviewed Nov 22, 2024
View reviewed changes
crypto-ffi/Cargo.toml Outdated Show resolved Hide resolved
@coriolinus coriolinus force-pushed the prgn/feat/binding-specific-errors branch from 9e6b6e9 to fadf653 Compare November 22, 2024 16:24
@coriolinus
feat: refactor WASM error types
b13d88f 
Clients only care about a limited set of errors, which we enumerated
in the root issue. This commit refactors the client-visible errors
(in WASM) to focus only on the set they care about.
@coriolinus
feat: proteus error codes are Option<u16> not u32 outside wasm also
17ce552
@coriolinus coriolinus force-pushed the prgn/feat/binding-specific-errors branch from fadf653 to 11d071d Compare November 25, 2024 10:38
istankovic
istankovic reviewed Nov 25, 2024
View reviewed changes
README.md Show resolved Hide resolved
istankovic
istankovic approved these changes Nov 25, 2024
View reviewed changes
Copy link
Member

@istankovic istankovic left a comment

Choose a reason for hiding this comment

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

Left a few comments, looks good. 👍

@coriolinus
feat: refactor non-WASM error types
40b4576 
Clients only care about a limited set of errors, which we enumerated
in the root issue. This commit refactors the client-visible errors
(other than WASM) to focus only on the set they care about.
@coriolinus
test(wasm): fixup tests broken by recent changes
bb9ea00 
Turns out there is a whole human-written section in the TS bindings which parses the
output of our error types and turns it into a somewhat better error object.
That needed to be updated to handle the new, simpler output serialization.
@coriolinus
fix(ffi): build without error without default features
d5f2880
@coriolinus coriolinus force-pushed the prgn/feat/binding-specific-errors branch from 11d071d to 47bbc12 Compare November 25, 2024 12:26
@coriolinus
docs: improve platform-specific test instructions
6b752ee
@coriolinus
chore(uniffi/jvm): get kotlin to build _at all_
61c4f2f 
Kotlin tests are still failing due to previous changes, but at least
the fundamental build operation is now succeeding. It's a start!

Ran a Kotlin code formatter, intending only to hit the auto-generated files;
turns out there were a bunch of human-maintained files in the tree also.
Unfortunately, at that point, there were quite a lot of uncommitted changes.
Didn't want to go through the effort of going back and unformatting the
unrelated stuff, so that's all in here also.
@coriolinus
test(jvm): cause jvm kotlin tests to pass
8c34db6
@coriolinus coriolinus force-pushed the prgn/feat/binding-specific-errors branch from 47bbc12 to 8c34db6 Compare November 25, 2024 12:29
@coriolinus coriolinus merged commit 3b8d930 into main Nov 25, 2024
24 checks passed
@coriolinus coriolinus deleted the prgn/feat/binding-specific-errors branch November 25, 2024 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SimonThormeyer SimonThormeyer SimonThormeyer approved these changes

@typfel typfel typfel left review comments

@istankovic istankovic istankovic approved these changes

Assignees
No one assigned
Labels
echoes: product-roadmap/feature Work contributing to adding a new feature as part of the product roadmap.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@coriolinus @codecov-commenter @typfel @istankovic @SimonThormeyer