[PM-32809] feat: wire Bank Account through BitwardenSdk#2576
[PM-32809] feat: wire Bank Account through BitwardenSdk#2576SaintPatrck wants to merge 2 commits into
Conversation
SaintPatrck
added
t:feature-app
github-actions
Bot
added
t:feature
and removed
t:feature-app
SaintPatrck
added
the
ai-review-vnext
SaintPatrck
force-pushed
the
vault/pm-32809-ios-bank-account-foundation
branch
2 times, most recently
from
1d619c0 to
3f2113b
Compare
SaintPatrck
force-pushed
the
vault/pm-32809-ios-bank-account-sdk-bridge
branch
from
c69dd21 to
fd12e9a
Compare
|
Claude Code is reviewing this pull request... If this comment does not update with results, check the Actions log. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| /// The type of the bank account. | ||
| let accountType: BankAccountType? | ||
| let accountType: String? |
There was a problem hiding this comment.
❓ QUESTION: accountType switched from BankAccountType? to String? — is the typed enum still needed?
Details
The foundation PR introduced BankAccountType (checking, savings, certificateOfDeposit, lineOfCredit, investmentBrokerage, moneyMarket, other) as a typed Codable enum. After this PR's change to String?, BankAccountType is no longer referenced anywhere in production code — only in BankAccountTypeTests.swift self-references it.
If the SDK exposes accountType as a free-form String, a few options to consider:
- Remove
BankAccountType.swiftand its tests to avoid dead code, or - Keep
BankAccountTypeand map at the boundary (BankAccountType(rawValue: bankAccount.accountType)) so UI code can switch on a typed value, or - Use
RawRepresentable/ a custom decoder soaccountTypekeeps the typed contract while tolerating unknown server values.
Same question applies to the renamed bankPhone → bankContactPhone — confirming alignment with what the server's API contract actually emits (vs. what the SDK exposes) would be worthwhile, since Codable synthesis will use the property name as the JSON key.
There was a problem hiding this comment.
@claude Intentional — BankAccountType is staged for the Part 3/3 UI PR (it'll be mapped at the form/view-state boundary). Confirmed alignment with Android (bitwarden/android#6828,
VaultBankAccountType): network/SDK layer carries accountType: String? unchanged, the typed enum lives at the UI boundary with case-insensitive parsing + .other fallback.
Renames (accountType, bankContactPhone) match the SDK property names verified against BitwardenSdk.BankAccount.
There was a problem hiding this comment.
Claude finished @SaintPatrck's task in 32s —— View job
Acknowledged — BankAccountType staging confirmed
Thanks for the clarification, @SaintPatrck. The plan makes sense and matches Android's split:
- Network/SDK boundary (
CipherBankAccountModel.accountType: String?) — preserves whatever the server/SDK emits, no decode failures on unknown values. - UI boundary (
BankAccountTypeenum, Part 3/3) — case-insensitiveinit?(rawValue:)+.otherfallback, mapped at the form/view-state layer.
That cleanly resolves the "orphaned enum" concern — it's intentionally pre-staged. Confirmed BankAccountType is currently only referenced by its own tests (BankAccountType.swift, BankAccountTypeTests.swift), which is the expected state for this slice.
Renames (accountType, bankContactPhone) verified to match BitwardenSdk.BankAccount property names — agreed.
The remaining open item from the prior review is unrelated to this thread:
⚠️ BitwardenSdk+VaultMocking.swiftswitches overBitwardenSdk.CipherTypeneed a.bankAccountarm before the SDK bump (alsoCipherListView+Extensions.swift:133andCipherItemState.swift:548switch overBitwardenSdk.Cipher.typeand will hit the same exhaustiveness gap).
• Branch: vault/pm-32809-ios-bank-account-sdk-bridge
| case .bankAccount: | ||
| self = .bankAccount |
There was a problem hiding this comment.
BitwardenSdk.CipherType switches in BitwardenSdk+VaultMocking.swift are not updated and will block the planned drop-in SDK bump.
Details and fix
The PR description states: "Once available, one commit bumps project-common.yml + Package.resolved and this PR goes green." However, two test-helper files have switch cipher.type over BitwardenSdk.CipherType that handle only .card / .identity / .login / .secureNote / .sshKey:
BitwardenShared/Core/Vault/Services/TestHelpers/BitwardenSdk+VaultMocking.swift:89AuthenticatorShared/Core/Vault/Services/TestHelpers/BitwardenSdk+VaultMocking.swift:89
Once the SDK adds .bankAccount to its CipherType, both switches become non-exhaustive and the SDK-bump commit will fail to compile. To preserve the "one drop-in commit goes green" property, add a .bankAccount arm to each switch in this PR (or open an accompanying PR to be merged together). For the mocks, mapping to .identity (or any minimal valid case) until full UI support lands would be enough to restore exhaustiveness.
There was a problem hiding this comment.
@claude Good catch — added .bankAccount arms to both BitwardenSdk+VaultMocking.swift files mirroring production's CipherListViewType.bankAccount → .bankAccount mapping. Will land in
this PR so the SDK bump remains a single drop-in commit.
There was a problem hiding this comment.
Claude encountered an error —— View job
Verifying .bankAccount mocking arms 
- Check current state of branch / committed changes
- Inspect both
BitwardenSdk+VaultMocking.swiftfiles - Confirm exhaustiveness against
BitwardenSdk.CipherType - Note remaining production-side gap (
CipherListView+Extensions.swift,CipherItemState.swift)
• Branch: vault/pm-32809-ios-bank-account-sdk-bridge
There was a problem hiding this comment.
Claude finished @SaintPatrck's task in 2m 44s —— View job
Verified .bankAccount mocking arms ✅
- Check current state of branch / committed changes
- Inspect both
BitwardenSdk+VaultMocking.swiftfiles - Confirm mappings match production
CipherListViewType.bankAccount(BitwardenSdk+Vault.swift:240,257) - Note remaining production-side gaps
Confirmed commit 3fc9490 adds the arm cleanly to both files at the same offset:
// BitwardenShared & AuthenticatorShared TestHelpers/BitwardenSdk+VaultMocking.swift:90
case .bankAccount:
self = .bankAccountMirrors production's CipherType.init(_ type: BitwardenSdk.CipherListViewType) — once the SDK adds BitwardenSdk.CipherType.bankAccount, both test-helper switches and the two production extension switches in BitwardenSdk+Vault.swift will be exhaustive together. 👍
Remaining production-side BitwardenSdk.CipherType / CipherListViewType switches
Heads-up that the following still don't have .bankAccount arms — once the SDK lands, the bump commit will produce non-exhaustive switch errors here unless they're added in this PR:
| File | Line | Switched type |
|---|---|---|
BitwardenShared/Core/Vault/Extensions/CipherListView+Extensions.swift |
133 | BitwardenSdk.CipherType (init(cipherDecryptFailure:)) |
BitwardenShared/UI/Vault/VaultItem/CipherItemState.swift |
548 | BitwardenSdk.CipherType (var icon) |
BitwardenShared/UI/Vault/Helpers/TextAutofillOptionsHelper/TextAutofillOptionsHelperFactory.swift |
25 | cipherView.type |
BitwardenShared/UI/Vault/Extensions/Alert+Vault.swift |
307 | context.cipherView.type |
BitwardenShared/UI/Vault/Vault/VaultList/VaultListItem.swift |
127, 172 | CipherListView.type (CipherListViewType) |
If the intent is to let those break on the bump so the Part 3/3 UI PR is forced to flesh out real handling, that's a reasonable strategy — just want to flag explicitly that the "single drop-in commit" property only holds for the SDK-adapter layer, not the UI layer. Worth a quick sentence in the PR description either way so reviewers of the bump commit aren't surprised.
• Branch: vault/pm-32809-ios-bank-account-sdk-bridge
Stages the CipherType.bankAccount case and SDK bridge arms so the upcoming sdk-swift release (2.0.0-6370-96753eef) is a drop-in bump.
SaintPatrck
force-pushed
the
vault/pm-32809-ios-bank-account-sdk-bridge
branch
from
fd12e9a to
63e43de
Compare
Keeps the SDK bump a single drop-in commit by making the test-helper CipherListViewType switches exhaustive once BitwardenSdk.CipherType gains .bankAccount.
github-actions
Bot
added
the
app:authenticator
SaintPatrck
removed
hold
|
🤔 I'm seeing compile errors in e.g. |
|
|
||
| /// The phone number for contacting the bank. | ||
| let bankPhone: String? | ||
| let bankContactPhone: String? |
2 participants
🎟️ Tracking
2.0.0-6370-96753eef📔 Objective
Stage the
CipherType.bankAccountcase and the SDK bridge arms on top of the foundation PR so the eventual sdk-swift bump is a drop-in. Will not compile against the currently pinned SDK — held withholdlabel until the SDK release lands. Once available, one commit bumpsproject-common.yml+Package.resolvedand this PR goes green.