feat(sdk-api,sdk-core): add password rotation progress callback with truthful keychain total by noegutierrez5 · Pull Request #9825 · BitGo/BitGoJS

noegutierrez5 · 2026-09-25T20:40:04Z

Summary

BitGoAPI.changePassword() re-encrypts every wallet keychain in the caller's process and had no progress hook, so the retail dialog could only show an indeterminate spinner for a multi-minute operation. This adds an optional progressCallback to ChangePasswordOptions and threads a truthful total through it.
The callback is observational: it cannot change rotation success/failure. It is invoked synchronously inside a guard (sdk-api/src/bitgoAPI.ts), never awaited, and a throwing observer is swallowed so a UI/state-handler exception cannot abort a rotation. Callers that omit it see identical behavior.
One reporter covers both v1 (sdk-api/src/v1/keychains.ts) and v2 (sdk-core/src/bitgo/keychain/keychains.ts) processing, so consumers get a single monotonic counter rather than separate v1/v2 sequences. keychains/started fires before the first attempt; finalizing/started before batch uploads and the final account-password POST; finalizing/completed only after that request succeeds.
Batch retries emit nothing — they are transport work, and counting them would inflate the user-visible total. No private key material is exposed; currentKeychainId carries only a pub/id.
sdk-core captures totalCount from the first v2 key-list page and passes it through every event as total. When the backend does not supply it, total stays undefined and consumers render indeterminate progress — no denominator is ever fabricated.
A v1-only account emits no v2 event, so after the v2 walk returns with zero events the SDK emits one terminal keychains/updated event carrying the client-known v1 set size (n of n), immediately before finalizing. Guarded on the v2 event count so mixed accounts keep the single stable denominator instead of one that jumps mid-rotation (2 of 2, then 34 of 34).

Counting unit

The WCN-2084 scope doc left the denominator to product/SDK owners. This implements option A — a generic list total: the backend's totalCount counts the page's own query (BitGoJS#9825's companion wallet-platform PR), and the walk reports an outcome for every listed record — updated for re-encrypted material, skipped for records with no serialized encryptedPrv (and for known decrypt failures). completed therefore reaches total on any account composition, and the SDK never needs to know which records the serializer suppresses: a bitgo-source, nitro-trust, or placeholder record is simply a listed record that reports skipped.

Test plan

modules/sdk-api unit suite — 251 passing, 0 failing
modules/bitgo v1 keychains — 4 passing, 0 failing
modules/bitgo v2 keychains — 104 passing (8 failures pre-exist on master post-rebase: the async validation refactor 792216e000 broke its own sync-throw expectations, kaspa/starknet joined an xpub-expecting seed test, and the OFC rotate test expects a default encryptionVersion that does not exist. BitGoJS PR CI runs no unit tests, so these do not gate)
New coverage: event ordering across pages, encryptedTotalCount threading, records without encryptedPrv emitting nothing, known decrypt-failure skips, fatal errors still propagating, callback-exception isolation, finalization ordering, batching-check / batching-POST / retry contracts
Verified in a browser against a real staging account — 32 encrypted keychains out of 50 key records produced a counter that ran 1→32 and terminated exactly

Companion work

This PR is the SDK contract only. The other two pieces of WCN-2084:

Repo	PR	State
bitgo-microservices	#63086	ready for review — supplies `encryptedTotalCount`
bitgo-retail	#10191	draft — the dialog that renders it

Downstream

Merging publishes a new @bitgo-beta/sdk-api + @bitgo-beta/sdk-core pair. bitgo-retail must bump to that pair before wallet-platform's encryptedTotalCount reaches any environment — an older SDK emits progress for records the new count excludes.

Ticket: WCN-2084

linear-code · 2026-09-25T20:40:16Z

WCN-2084

github-actions · 2026-09-25T20:47:57Z

⚠️ Unit tests are failing on Node 26.x (Current release line, non-blocking). This is not an LTS version yet, so it does not block merge, but it signals an incompatibility to fix before Node 26.x becomes LTS.

View run

pranavjain97

lgtm overall, blocked on the mixed v1/v2 counter overshooting the total

pranavjain97 · 2026-09-28T21:46:08Z

+          phase: 'keychains',
+          status: 'updated',
+          ...counters,
+          total: progress.total,


completed counts v1 and v2 but total only comes from the v2 list, a mixed account with 2 sjcl + 32 v2 keychains finishes at 34 of 32. lets scope the counters per keychainVersion or cover both sets in the total

vibhavgo

Thank you for taking this up!

I think pranavs comment is addressed but there's one more case:
For a user having only v1 keychains, the total always remains undefined
Consider emitting an event like
{ phase: 'keychains', status: 'updated', ...counters, total: v1KeychainCount }
after v1 password update

Might need to rebase for merge conflicts

LGTM otherwise

Adds an optional, failure-isolated progressCallback to changePassword() so callers (e.g. bitgo-retail's account-password dialog) can surface truthful indeterminate progress through v1/v2 keychain re-encryption and a finalizing phase, instead of a silent multi-minute operation. - sdk-core: KeychainPasswordUpdateProgress/-Callback on UpdatePasswordOptions; v1 and v2 updatePassword() emit one updated/skipped outcome per nonfatal keychain, observer exceptions are isolated and never affect rotation results or abort on fatal errors. - sdk-api: PasswordRotationProgress/-Callback on ChangePasswordOptions; BitGoAPI.changePassword() aggregates lower-level outcomes into a single keychains/started -> keychains/updated* -> finalizing/started -> finalizing/completed lifecycle with monotonic counters, without changing the existing batching/legacy POST fallback contract. - Unit tests cover v1/v2 outcome ordering, missing-encryptedPrv and known-decrypt-failure skips, unchanged fatal-error propagation, event-order/counter assertions, callback-throws isolation, and the batching-check-failure / batching-POST-failure / retry contracts. Ticket: WCN-2084

…progress Completes the WCN-2084 progress contract by giving the password-rotation callback a truthful denominator and fixing the counting unit. sdk-core: capture `encryptedTotalCount` from the first v2 key-list page and pass it through every progress event. Records without an `encryptedPrv` no longer emit progress at all. sdk-api: forward `total` from the lower-level keychain callbacks into the aggregated `PasswordRotationProgress` events. Denominator decision (the choice WCN-2084's scope doc left to product/SDK owners): option B, "encrypted wallet keychains". The unit is keychain records that hold encrypted user key material, matching the backend's `encryptedTotalCount`, which counts `users[]` entries with `encryptedPrv` set. The alternative, "records inspected", was rejected: it would count placeholder key records that no rotation ever touches. Emitting progress for those while the backend excluded them made `completed` overshoot `total` — an account with 50 key records of which 32 are encrypted rendered "50 of 32". Records that do hold ciphertext but fail to decrypt still emit `skipped`, so the counter always reaches the denominator exactly. Tests: v2 key without `encryptedPrv` emits nothing and keeps `total` intact; existing ordering, pagination, decrypt-failure, callback-exception, and finalization coverage unchanged. Ticket: WCN-2084

The changePassword progress counters are shared across the v1 and v2 sets, but the total came only from the v2 list's encryptedTotalCount — a mixed account with 2 SJCL and 32 v2 keychains finished the counter at 34 of 32. The v1 set has no server-provided count, but its size is knowable client side: every /user/encrypted entry emits exactly one progress event, so the returned re-encryption map's size is the v1 contribution. Captured after the v1 pass and added to every v2 event's total, so the mixed account above completes at 34 of 34, exactly. v1 events stay indeterminate: surfacing the client-side v1 count mid-flight would make the denominator jump the moment v2's server count arrives, and a denominator that moves is worse than none. When the backend supplies no encryptedTotalCount the total stays undefined — never fabricated. Test: the 2 v1 + 32 v2 account now asserts v1 events indeterminate, v2 events at total 34, and the final event at completed 34 of total 34; the total-forwarding test accounts for the combined denominator. Ticket: WCN-2084

Review follow-up: an account with only v1 keychains never receives a v2 event, so the total stayed undefined for the whole rotation and the observer could never render "n of n". After the v2 walk completes with zero events, emit one terminal keychains/updated event carrying the client-known v1 set size as the denominator (2 of 2), immediately before finalizing. Guarded on v2EventCount === 0 so mixed accounts keep the single stable total from encryptedTotalCount — surfacing the v1 size unconditionally would make the denominator jump mid-rotation (2 of 2, then 34 of 34). Also rebases onto latest master, where the safe-mode password rotation landed in sdk-core (safeId on UpdatePasswordOptions, batch persistence via PUT /api/v2/key/bulk). The UpdatePasswordOptions conflict resolves additively: progressCallback remains a legacy-mode observer and the safe-mode walk does not invoke it. Local suites: sdk-api bitgoAPI 74 passing; my keychain progress tests in modules/bitgo v1 (2) and v2 (4) passing. Pre-existing master-side failures in modules/bitgo's own keychain suites are unchanged by this diff and do not run in PR CI. Ticket: WCN-2084

zahin-mohammad

Defering review ti

zahin-mohammad · 2026-10-01T18:32:39Z

+    oldPassword,
+    newPassword,
+    encryptionVersion,
+    progressCallback,


Suggested change

oldPassword,

newPassword,

encryptionVersion,

progressCallback,

oldPassword,

newPassword,

encryptionVersion,

keychainCallbacks: { onProgress, onError },

maybe something like this? The onError hook lets callers decide to handle cases. This can be called on the catch block

zahin-mohammad · 2026-10-01T18:32:53Z

+    oldPassword,
+    newPassword,
+    encryptionVersion,
+    progressCallback,


Also params need to be documented in the jsDocs.

Review direction (WCN-2084): the total is the generic list count (totalCount from the wallet-platform companion PR), so the walk emits skipped for listed records with no serialized encryptedPrv instead of silently dropping them — completed reaches the total on any account composition, and the SDK no longer needs to know which records the serializer suppresses. Renames the list-result field to totalCount. Ticket: WCN-2084

pranavjain97

lgtm

… comment Ticket: WCN-2084

noegutierrez5 requested review from a team as code owners September 25, 2026 20:40

noegutierrez5 requested review from pranavjain97 and vibhavgo September 25, 2026 20:40

noegutierrez5 requested a review from zahin-mohammad September 25, 2026 20:48

pranavjain97 requested changes Sep 28, 2026

View reviewed changes

vibhavgo reviewed Sep 30, 2026

View reviewed changes

noegutierrez5 added 4 commits September 30, 2026 08:45

noegutierrez5 force-pushed the noegutierrez/wcn-2084-add-progress-indication-for-login-password-rotation branch from 9e0c612 to 245aa94 Compare September 30, 2026 15:59

noegutierrez5 requested review from pranavjain97 and vibhavgo September 30, 2026 17:06

zahin-mohammad reviewed Oct 1, 2026

View reviewed changes

pranavjain97 previously approved these changes Oct 1, 2026

View reviewed changes

test(sdk-api): fix stale encryptedTotalCount mention in mixed-account…

f7ee764

… comment Ticket: WCN-2084

noegutierrez5 dismissed pranavjain97’s stale review via f7ee764 October 2, 2026 07:28

Navigation Menu

feat(sdk-api,sdk-core): add password rotation progress callback with truthful keychain total - #9825

feat(sdk-api,sdk-core): add password rotation progress callback with truthful keychain total#9825
noegutierrez5 wants to merge 6 commits into
masterfrom
noegutierrez/wcn-2084-add-progress-indication-for-login-password-rotation

noegutierrez5 commented Sep 25, 2026 •

edited

Loading

Uh oh!

linear-code Bot commented Sep 25, 2026

Uh oh!

github-actions Bot commented Sep 25, 2026 •

edited

Loading

Uh oh!

pranavjain97 left a comment

Uh oh!

pranavjain97 Sep 28, 2026

Uh oh!

vibhavgo left a comment

Uh oh!

zahin-mohammad left a comment

Uh oh!

zahin-mohammad Oct 1, 2026

Uh oh!

zahin-mohammad Oct 1, 2026

Uh oh!

pranavjain97 left a comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

noegutierrez5 commented Sep 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Counting unit

Test plan

Companion work

Downstream

Uh oh!

linear-code Bot commented Sep 25, 2026

Uh oh!

github-actions Bot commented Sep 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pranavjain97 left a comment

Choose a reason for hiding this comment

Uh oh!

pranavjain97 Sep 28, 2026

Choose a reason for hiding this comment

Uh oh!

vibhavgo left a comment

Choose a reason for hiding this comment

Uh oh!

zahin-mohammad left a comment

Choose a reason for hiding this comment

Uh oh!

zahin-mohammad Oct 1, 2026

Choose a reason for hiding this comment

Uh oh!

zahin-mohammad Oct 1, 2026

Choose a reason for hiding this comment

Uh oh!

pranavjain97 left a comment

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

noegutierrez5 commented Sep 25, 2026 •

edited

Loading

github-actions Bot commented Sep 25, 2026 •

edited

Loading