-
Notifications
You must be signed in to change notification settings - Fork 20
[PM-23785] Use actual error types in UniFFI #424
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
Conversation
6303db3
to
f40f671
Compare
This comment was marked as resolved.
This comment was marked as resolved.
f40f671
to
cdfbe13
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Err(e) => Err(e), | ||
} | ||
} | ||
) -> Result<bool, $crate::platform::repository::RepositoryError>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latest UniFFI doesn't allow default trait methods, so I've removed it. We weren't using it anyway.
|
||
quote! { | ||
#[derive(serde::Serialize)] | ||
#[cfg_attr(feature = "uniffi", derive(uniffi::Error))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this is different than what WASM is doing. The WASM part is generating the code based on the wasm
feature flag of the macro crate itself, while the UniFFI is always emmitting the cfg_attr
code, which means it's the feature flag of the final crate that gets evaluated.
I've needed to do this because I couldn't quite get it working the other way, I think the workspace feature unification was causing issues and generating the UniFFI code all the time, even when not needed (like for the bitwarden-ipc crate).
Doing it this new way has one negative effect, which is that it's not obvious that the final crate has to have a uniffi
feature, and otherwise you'll get a warning (that I've had to silence in the root cargo.toml because of wasm-only crates like bitwarden-ipc)
# Conflicts: # crates/bitwarden-crypto/src/safe/password_protected_key_envelope.rs # crates/bitwarden-fido/src/authenticator.rs # crates/bitwarden-fido/src/client_fido.rs # crates/bitwarden-fido/src/lib.rs # crates/bitwarden-fido/src/types.rs # crates/bitwarden-vault/src/cipher/cipher.rs
# Conflicts: # crates/bitwarden-uniffi/src/crypto.rs # crates/bitwarden-uniffi/src/platform/mod.rs
# Conflicts: # crates/bitwarden-uniffi/src/error.rs # crates/bitwarden-uniffi/src/lib.rs
## 🎟️ Tracking https://bitwarden.atlassian.net/browse/PM-23512 ## 📔 Objective Create the bitwarden-pm crate that will sit between bitwarden-core and bitwarden-wasm/uniffi and will encapsulate all the password manager functionality. Note that uniffi is still mostly reimplementing the API due to #424, so this doesn't change that much. I've also made the bitwarden-pm crate reexport the clients from the feature crates, which allows us to remove them from the wasm-crate. ## ⏰ Reminders before review - Contributor guidelines followed - All formatters and local linters executed and passed - Written new unit and / or integration tests where applicable - Protected functional changes with optionality (feature flags) - Used internationalization (i18n) for all UI strings - CI builds passed - Communicated to DevOps any deployment requirements - Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team ## 🦮 Reviewer guidelines <!-- Suggested interactions but feel free to use (or not) as you desire! --> - 👍 (`:+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 confirmed issue 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
|
…tual error types in UniFFI (bitwarden/sdk-internal#424)
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-23785
📔 Objective
Currently all the UniFFI errors get serialized as a string. This PR updates UniFFI to the latest main changes to add support for proper errors. This requires a couple of changes:
uniffi::Error
and added support to the bitwarden-error macro for uniffi.How this looks on the mobile apps:
Android:
iOS:
⏰ 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