Skip to content

Conversation

Hinton
Copy link
Member

@Hinton Hinton commented Aug 21, 2025

🎟️ Tracking

📔 Objective

Provides a generic B64 and B64Url types we can use throughout the sdk.

⏰ 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

  • 👍 (:+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

Copy link
Contributor

github-actions bot commented Aug 21, 2025

Logo
Checkmarx One – Scan Summary & Details3736bc52-4844-46a5-94a0-1e3d3e428db6

Great job! No new security vulnerabilities introduced in this pull request

@Hinton Hinton marked this pull request as ready for review August 21, 2025 13:03
@Hinton Hinton requested review from a team as code owners August 21, 2025 13:03
Copy link

codecov bot commented Aug 21, 2025

Codecov Report

❌ Patch coverage is 90.81967% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.10%. Comparing base (a1654e8) to head (223f611).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
crates/bitwarden-encoding/src/uniffi_support.rs 0.00% 12 Missing ⚠️
crates/bitwarden-encoding/src/b64.rs 95.07% 7 Missing ⚠️
crates/bitwarden-core/src/auth/tde.rs 0.00% 3 Missing ⚠️
crates/bitwarden-encoding/src/serde.rs 80.00% 3 Missing ⚠️
crates/bitwarden-core/src/auth/auth_client.rs 0.00% 1 Missing ⚠️
crates/bitwarden-encoding/src/lib.rs 0.00% 1 Missing ⚠️
crates/bitwarden-uniffi/src/auth/mod.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #404      +/-   ##
==========================================
+ Coverage   75.88%   76.10%   +0.21%     
==========================================
  Files         259      264       +5     
  Lines       23732    24045     +313     
==========================================
+ Hits        18010    18299     +289     
- Misses       5722     5746      +24     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.


uniffi::custom_type!(B64, String, {
try_lift: |val| {
B64::try_from(val.as_str()).map_err(|e: NotB64Encoded| e.into())
Copy link
Member

Choose a reason for hiding this comment

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

Note that these conversions currently crash the ios app when they fail until #373 is fixed

@Hinton Hinton requested a review from dani-garcia August 21, 2025 16:24
dani-garcia
dani-garcia previously approved these changes Aug 21, 2025
gbubemismith
gbubemismith previously approved these changes Aug 21, 2025
Copy link
Contributor

@gbubemismith gbubemismith left a comment

Choose a reason for hiding this comment

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

Nice very handy

@Hinton Hinton dismissed stale reviews from gbubemismith and dani-garcia via 931afbf August 22, 2025 10:02
@Hinton Hinton requested a review from dani-garcia August 22, 2025 10:06
Copy link
Member Author

@Hinton Hinton Aug 22, 2025

Choose a reason for hiding this comment

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

@quexten We previously discussed extracting this to a lower level crate, which is done now and we should be able to update crypto to depend on this implementation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! I've made a ticket for now in the bitwarden-crypto backlog https://bitwarden.atlassian.net/browse/PM-25107

Copy link
Member Author

Choose a reason for hiding this comment

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

I added the change to this PR, seemed easier.

quexten
quexten previously approved these changes Aug 22, 2025
@Hinton Hinton requested a review from a team as a code owner August 22, 2025 11:58
@Hinton Hinton requested a review from a team as a code owner August 22, 2025 11:58
@Hinton Hinton requested a review from mzieniukbw August 22, 2025 11:58
Copy link

Copy link
Member

@dani-garcia dani-garcia left a comment

Choose a reason for hiding this comment

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

LGTM, should we now consider this crate the way to go, and migrate all uses of the base64 and data-encoding crates to it?

@Hinton
Copy link
Member Author

Hinton commented Aug 22, 2025

I think so.

@Hinton Hinton merged commit f52e521 into main Aug 22, 2025
51 checks passed
@Hinton Hinton deleted the arch/encoding branch August 22, 2025 13:32
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.

4 participants