[PM-26534] Remove non-generic wrapper for PasswordProtectedKeyEnvelope #488
Conversation
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #488 +/- ##
==========================================
- Coverage 78.34% 78.33% -0.01%
==========================================
Files 287 286 -1
Lines 28095 28084 -11
==========================================
- Hits 22010 21999 -11
Misses 6085 6085 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
quexten
changed the title
| } | ||
|
|
||
| #[cfg(feature = "wasm")] | ||
| impl FromWasmAbi for PasswordProtectedKeyEnvelope { |
There was a problem hiding this comment.
concern FromWasmAbi is marked as unstable. https://docs.rs/wasm-bindgen/0.2.104/wasm_bindgen/convert/trait.FromWasmAbi.html#-unstable
There was a problem hiding this comment.
Previously, because we had the newtype wrapper, we could use tsify to auto-generate it. For just working with the type directly, I did not find a good way to use tsify other than creating a newtype in the same file, which did not seem like the right solution.
That said, tsify just generates FromWasmAbi. https://github.com/madonoharu/tsify/blob/ab399fe86761e94f99bdb57a0b18a78d54b08671/tsify-macros/src/wasm_bindgen.rs#L202
There was a problem hiding this comment.
Wasm bindgen explicitly documents this also here: https://wasm-bindgen.github.io/wasm-bindgen/contributing/design/rust-type-conversions.html#from-js-to-rust.
There was a problem hiding this comment.
Thank you for the context. I guess if it breaks in the future we can just follow whatever tsify comes up with to solve for it. I would like someone with more wasm bindgen experience than me to review.
There was a problem hiding this comment.
Leaving this comment open until the review from @bitwarden/team-sdk-sme
There was a problem hiding this comment.
Yea it's unstable but a lot of things depends on it and we'll know if it stops compiling.
crates/bitwarden-crypto/src/safe/password_protected_key_envelope.rs
Outdated
Show resolved
Hide resolved
…sdk-internal into km/remove-non-generic-wrapper
quexten
dismissed stale reviews from addisonbeck, Thomas-Avery, and Hinton
via
00dcbe4
|
4 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-26534
📔 Objective
The initial implementation assumed that the struct of of the PasswordProtectedKeyEnvelope in crypto had to be generic, because the key context is generic w.r.t. the keys used, and the keys are only then provided by core. Because uniffy / wasm-bindgen can't handle generics, I added a non-generic wrapper struct in core, leaving us with two different "PasswordProtectedKeyEnvelopes".
This understanding was not correct. You don't actually need to make the struct generic, having just two specific functions on the impl generic to the key id's suffices, allowing us to drop the "non generic wrapper" entirely, vastly simplifing our code here. The changes are mostly removing code and moving the uniffi mapping to the crypto crate.
⏰ Reminders before review
team
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmedissue and could potentially benefit from discussion
:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes