From 67e0e3abb9d6a643f00ece197106955fafb54331 Mon Sep 17 00:00:00 2001 From: Noe Gutierrez Date: Wed, 23 Sep 2026 10:24:04 -0700 Subject: [PATCH 1/7] feat(sdk-api,sdk-core): add progress callback for password rotation 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 --- modules/bitgo/test/unit/keychains.ts | 57 +++++++ modules/bitgo/test/v2/unit/keychains.ts | 100 ++++++++++++ modules/sdk-api/src/bitgoAPI.ts | 88 +++++++++-- modules/sdk-api/src/types.ts | 21 +++ modules/sdk-api/src/v1/keychains.ts | 12 ++ modules/sdk-api/test/unit/bitgoAPI.ts | 143 ++++++++++++++++++ .../sdk-core/src/bitgo/keychain/iKeychains.ts | 11 ++ .../sdk-core/src/bitgo/keychain/keychains.ts | 19 +++ 8 files changed, 437 insertions(+), 14 deletions(-) diff --git a/modules/bitgo/test/unit/keychains.ts b/modules/bitgo/test/unit/keychains.ts index a817dbfd1bb..b53c747a5b5 100644 --- a/modules/bitgo/test/unit/keychains.ts +++ b/modules/bitgo/test/unit/keychains.ts @@ -73,6 +73,63 @@ describe('Keychains', function v2keychains() { } result.should.hasOwnProperty('version'); }); + + it('should emit progressCallback outcomes for updated and skipped keychains', async function () { + const oldPassword = 'oldPassword'; + const newPassword = 'newPassword'; + const otherPassword = 'otherPassword'; + + const encryptedXprv1 = await bitgo.encrypt({ input: 'xprv1', password: oldPassword }); + const encryptedXprv2 = await bitgo.encrypt({ input: 'xprv2', password: otherPassword }); + + nock(bgUrl) + .post('/api/v1/user/encrypted') + .reply(200, { + keychains: { + xpub1: encryptedXprv1, + xpub2: encryptedXprv2, + }, + version: 1, + }); + + const events: Array<{ status: string; currentKeychainId?: string }> = []; + await keychains.updatePassword({ + oldPassword, + newPassword, + progressCallback: (progress) => events.push(progress), + }); + + events.should.have.length(2); + events.should.containEql({ status: 'updated', currentKeychainId: 'xpub1' }); + events.should.containEql({ status: 'skipped', currentKeychainId: 'xpub2' }); + }); + + it('should not let a throwing progressCallback affect the rotation result', async function () { + const oldPassword = 'oldPassword'; + const newPassword = 'newPassword'; + + const encryptedXprv1 = await bitgo.encrypt({ input: 'xprv1', password: oldPassword }); + + nock(bgUrl) + .post('/api/v1/user/encrypted') + .reply(200, { + keychains: { + xpub1: encryptedXprv1, + }, + version: 1, + }); + + const result = await keychains.updatePassword({ + oldPassword, + newPassword, + progressCallback: () => { + throw new Error('observer boom'); + }, + }); + + const decryptedPrv = await bitgo.decrypt({ input: result.keychains.xpub1, password: newPassword }); + decryptedPrv.should.equal('xprv1'); + }); }); after(function afterKeychains() { diff --git a/modules/bitgo/test/v2/unit/keychains.ts b/modules/bitgo/test/v2/unit/keychains.ts index 1847d0a43e0..c12980ac229 100644 --- a/modules/bitgo/test/v2/unit/keychains.ts +++ b/modules/bitgo/test/v2/unit/keychains.ts @@ -567,6 +567,106 @@ describe('V2 Keychains', function () { const keys = await keychains.updatePassword({ oldPassword: oldPassword, newPassword: newPassword }); await validateKeys(keys, newPassword, 1); }); + + it('should emit progressCallback outcomes in cursor order across every page', async function () { + const prevId = 'prevId'; + const encXprv1 = await bitgo.encrypt({ input: 'xprv1', password: oldPassword }); + const encXprv2 = await bitgo.encrypt({ input: 'xprv2', password: otherPassword }); + const encXprv3 = await bitgo.encrypt({ input: 'xprv3', password: oldPassword }); + nock(bgUrl) + .get('/api/v2/tltc/key') + .query(true) + .reply(200, { + nextBatchPrevId: prevId, + keys: [ + { pub: 'xpub1', encryptedPrv: encXprv1 }, + { pub: 'xpub2', encryptedPrv: encXprv2 }, + ], + }); + nock(bgUrl) + .get('/api/v2/tltc/key') + .query(function queryNextPageMatch(queryObject) { + return queryObject.prevId === prevId; + }) + .reply(200, { + keys: [{ pub: 'xpub3', encryptedPrv: encXprv3 }], + }); + + const events: Array<{ status: string; currentKeychainId?: string }> = []; + await keychains.updatePassword({ + oldPassword, + newPassword, + progressCallback: (progress) => events.push(progress), + }); + + events.should.deepEqual([ + { status: 'updated', currentKeychainId: 'xpub1' }, + { status: 'skipped', currentKeychainId: 'xpub2' }, + { status: 'updated', currentKeychainId: 'xpub3' }, + ]); + }); + + it('should emit skipped when a key has no encryptedPrv', async function () { + const encXprv1 = await bitgo.encrypt({ input: 'xprv1', password: oldPassword }); + nock(bgUrl) + .get('/api/v2/tltc/key') + .query(true) + .reply(200, { + keys: [{ pub: 'xpub1', encryptedPrv: encXprv1 }, { pub: 'xpub2' }], + }); + + const events: Array<{ status: string; currentKeychainId?: string }> = []; + await keychains.updatePassword({ + oldPassword, + newPassword, + progressCallback: (progress) => events.push(progress), + }); + + events.should.containEql({ status: 'updated', currentKeychainId: 'xpub1' }); + events.should.containEql({ status: 'skipped', currentKeychainId: 'xpub2' }); + }); + + it('should emit skipped for a known decrypt-failure and never mislabel a fatal error as skipped', async function () { + const encXprv1 = await bitgo.encrypt({ input: 'xprv1', password: oldPassword }); + const encXprv2 = await bitgo.encrypt({ input: 'xprv2', password: otherPassword }); + nock(bgUrl) + .get('/api/v2/tltc/key') + .query(true) + .reply(200, { + keys: [ + { pub: 'xpub1', encryptedPrv: encXprv1 }, + { pub: 'xpub2', encryptedPrv: encXprv2 }, + ], + }); + + const events: Array<{ status: string; currentKeychainId?: string }> = []; + await keychains.updatePassword({ + oldPassword, + newPassword, + progressCallback: (progress) => events.push(progress), + }); + + events.should.containEql({ status: 'updated', currentKeychainId: 'xpub1' }); + events.should.containEql({ status: 'skipped', currentKeychainId: 'xpub2' }); + + const sandbox = sinon.createSandbox(); + try { + nock(bgUrl) + .get('/api/v2/tltc/key') + .query(true) + .reply(200, { + keys: [{ pub: 'xpub3', encryptedPrv: encXprv1 }], + }); + sandbox.stub(keychains, 'updateSingleKeychainPassword').throws(new Error('some random error')); + const fatalEvents: Array<{ status: string; currentKeychainId?: string }> = []; + await keychains + .updatePassword({ oldPassword, newPassword, progressCallback: (progress) => fatalEvents.push(progress) }) + .should.be.rejectedWith('some random error'); + fatalEvents.should.have.length(0); + } finally { + sandbox.restore(); + } + }); }); describe('Create TSS Keychains', function () { diff --git a/modules/sdk-api/src/bitgoAPI.ts b/modules/sdk-api/src/bitgoAPI.ts index 797889c164a..42cf0c91d35 100644 --- a/modules/sdk-api/src/bitgoAPI.ts +++ b/modules/sdk-api/src/bitgoAPI.ts @@ -68,6 +68,7 @@ import { GetUserOptions, ListWebhookNotificationsOptions, LoginResponse, + PasswordRotationProgress, PingOptions, ProcessedAuthenticationOptions, ReconstitutedSecret, @@ -2010,7 +2011,12 @@ export class BitGoAPI implements BitGoBase { * @param oldPassword {String} - the current password * @param newPassword {String} - the new password */ - async changePassword({ oldPassword, newPassword, encryptionVersion }: ChangePasswordOptions): Promise { + async changePassword({ + oldPassword, + newPassword, + encryptionVersion, + progressCallback, + }: ChangePasswordOptions): Promise { if (!_.isString(oldPassword)) { throw new Error('expected string oldPassword'); } @@ -2029,6 +2035,39 @@ export class BitGoAPI implements BitGoBase { throw new Error('the provided oldPassword is incorrect'); } + const emitProgress = (progress: PasswordRotationProgress) => { + if (!_.isFunction(progressCallback)) { + return; + } + try { + progressCallback(progress); + } catch (e) { + // ignore observer exceptions so a throwing callback never affects rotation results + } + }; + + // Single counter set shared by both the v1 and v2 lower-level keychain callbacks below. + const counters = { attempted: 0, completed: 0, succeeded: 0, skipped: 0 }; + emitProgress({ phase: 'keychains', status: 'started', ...counters }); + + const makeKeychainProgressCallback = + (keychainVersion: 'v1' | 'v2') => (progress: { status: 'updated' | 'skipped'; currentKeychainId?: string }) => { + counters.attempted++; + counters.completed++; + if (progress.status === 'updated') { + counters.succeeded++; + } else { + counters.skipped++; + } + emitProgress({ + phase: 'keychains', + status: 'updated', + ...counters, + currentKeychainId: progress.currentKeychainId, + keychainVersion, + }); + }; + // it doesn't matter which coin we choose because the v2 updatePassword functions updates all v2 keychains // we just need to choose a coin that exists in the current environment const coin = common.Environments[this.getEnv()].network === 'bitcoin' ? 'btc' : 'tbtc'; @@ -2038,14 +2077,24 @@ export class BitGoAPI implements BitGoBase { const encryptionSession = encryptionVersion === 2 ? await this.createEncryptionSession(newPassword, encryptionVersion) : undefined; try { - const updateKeychainPasswordParams = { + const v1KeychainUpdatePWResult = await this.keychains().updatePassword({ oldPassword, newPassword, encryptionVersion, encryptionSession, - }; - const v1KeychainUpdatePWResult = await this.keychains().updatePassword(updateKeychainPasswordParams); - const v2Keychains = await this.coin(coin).keychains().updatePassword(updateKeychainPasswordParams); + progressCallback: makeKeychainProgressCallback('v1'), + }); + const v2Keychains = await this.coin(coin) + .keychains() + .updatePassword({ + oldPassword, + newPassword, + encryptionVersion, + encryptionSession, + progressCallback: makeKeychainProgressCallback('v2'), + }); + + emitProgress({ phase: 'finalizing', status: 'started' }); const [hmacOldPassword, hmacNewPassword] = await Promise.all([ this._hmacAuthStrategy.calculateHMAC(user.username, oldPassword), @@ -2065,6 +2114,7 @@ export class BitGoAPI implements BitGoBase { const payloadSizeKB = Math.ceil(payloadSizeBytes / 1024); // Check if batching flow is enabled + let useBatchingFlow = false; try { const batchingFlowCheck = await this.get(this.url('/user/checkBatchingPasswordFlow', 2)) .query({ payloadSize: payloadSizeKB.toString() }) @@ -2077,20 +2127,30 @@ export class BitGoAPI implements BitGoBase { batchingFlowCheck.maxBatchSizeKB, 3 ); - // Call changepassword API without keychains for batching flow - return this.post(this.url('/user/changepassword')) - .send({ - version: updatePasswordParams.version, - oldPassword: updatePasswordParams.oldPassword, - password: updatePasswordParams.password, - }) - .result(); + useBatchingFlow = true; } } catch (error) { // batching flow check failed } - return this.post(this.url('/user/changepassword')).send(updatePasswordParams).result(); + if (useBatchingFlow) { + // Call changepassword API without keychains for batching flow. Awaited outside the + // check/upload try-catch above, so a rejection here still propagates to the caller + // with no legacy-POST fallback, exactly as before. + const result = await this.post(this.url('/user/changepassword')) + .send({ + version: updatePasswordParams.version, + oldPassword: updatePasswordParams.oldPassword, + password: updatePasswordParams.password, + }) + .result(); + emitProgress({ phase: 'finalizing', status: 'completed' }); + return result; + } + + const result = await this.post(this.url('/user/changepassword')).send(updatePasswordParams).result(); + emitProgress({ phase: 'finalizing', status: 'completed' }); + return result; } finally { encryptionSession?.destroy(); } diff --git a/modules/sdk-api/src/types.ts b/modules/sdk-api/src/types.ts index 17c20c1b69a..29d7a1af772 100644 --- a/modules/sdk-api/src/types.ts +++ b/modules/sdk-api/src/types.ts @@ -265,6 +265,25 @@ export interface GetEcdhSecretOptions { eckey: ECPairInterface; } +export type PasswordRotationProgress = + | { + phase: 'keychains'; + status: 'started' | 'updated'; + completed: number; + total?: number; + attempted: number; + succeeded: number; + skipped: number; + currentKeychainId?: string; + keychainVersion?: 'v1' | 'v2'; + } + | { + phase: 'finalizing'; + status: 'started' | 'completed'; + }; + +export type PasswordRotationProgressCallback = (progress: PasswordRotationProgress) => void; + export interface ChangePasswordOptions { oldPassword: string; newPassword: string; @@ -274,6 +293,8 @@ export interface ChangePasswordOptions { * Argon2id upgrade for v1 (SJCL) keychains once the caller is ready. */ encryptionVersion?: EncryptionVersion; + /** Optional observer invoked as the password rotation progresses through keychains and finalization. */ + progressCallback?: PasswordRotationProgressCallback; } /** diff --git a/modules/sdk-api/src/v1/keychains.ts b/modules/sdk-api/src/v1/keychains.ts index bc557563a13..bc8d98d38cf 100644 --- a/modules/sdk-api/src/v1/keychains.ts +++ b/modules/sdk-api/src/v1/keychains.ts @@ -188,6 +188,16 @@ Keychains.prototype.updatePassword = function (params, callback) { const newKeychains = {}; // @ts-expect-error - no implicit this const self = this; + const notifyProgress = (status: 'updated' | 'skipped', currentKeychainId: string) => { + if (!_.isFunction(params.progressCallback)) { + return; + } + try { + params.progressCallback({ status, currentKeychainId }); + } catch (e) { + // ignore observer exceptions so a throwing callback never affects rotation results + } + }; for (const [xpub, oldEncryptedXprv] of Object.entries((encrypted as any).keychains)) { try { const decryptedPrv = await self.bitgo.decrypt({ @@ -201,9 +211,11 @@ Keychains.prototype.updatePassword = function (params, callback) { encryptionVersion, }); newKeychains[xpub] = newEncryptedPrv; + notifyProgress('updated', xpub); } catch (e) { // decrypting the keychain with the old password didn't work so we just keep it the way it is newKeychains[xpub] = oldEncryptedXprv as string; + notifyProgress('skipped', xpub); } } return { keychains: newKeychains, version: (encrypted as any).version }; diff --git a/modules/sdk-api/test/unit/bitgoAPI.ts b/modules/sdk-api/test/unit/bitgoAPI.ts index 9c6666b8688..020ff027c74 100644 --- a/modules/sdk-api/test/unit/bitgoAPI.ts +++ b/modules/sdk-api/test/unit/bitgoAPI.ts @@ -6,6 +6,7 @@ import * as sinon from 'sinon'; import nock from 'nock'; import type { IHmacAuthStrategy } from '@bitgo/sdk-hmac'; import type { IEncryptionSession } from '@bitgo/sdk-core'; +import type { PasswordRotationProgress } from '../../src/types'; describe('Constructor', function () { describe('cookiesPropagationEnabled argument', function () { @@ -1189,6 +1190,148 @@ describe('Constructor', function () { }); sinon.assert.calledOnce(destroy); }); + + it('emits keychains/started, monotonic keychains/updated events, then finalizing/started and finalizing/completed', async function () { + nock(ROOT).get('/api/v2/user/checkBatchingPasswordFlow').query(true).reply(200, { isBatchingFlowEnabled: false }); + nock(ROOT) + .post('/api/v1/user/changepassword', (body: any) => !!body.keychains && !!body.v2_keychains) + .reply(200, {}); + + v1UpdatePasswordStub.callsFake(async (params: { progressCallback?: (p: unknown) => void }) => { + params.progressCallback?.({ status: 'updated', currentKeychainId: 'xpub1' }); + params.progressCallback?.({ status: 'skipped', currentKeychainId: 'xpub2' }); + return { keychains: { k1: 'v1enc' }, version: 25 }; + }); + v2UpdatePasswordStub.callsFake(async (params: { progressCallback?: (p: unknown) => void }) => { + params.progressCallback?.({ status: 'updated', currentKeychainId: 'xpub3' }); + return { v2k1: 'v2enc' }; + }); + + const events: PasswordRotationProgress[] = []; + await bitgo.changePassword({ + oldPassword: 'oldpw', + newPassword: 'newpw', + progressCallback: (progress) => events.push(progress), + }); + + events.should.deepEqual([ + { phase: 'keychains', status: 'started', completed: 0, attempted: 0, succeeded: 0, skipped: 0 }, + { + phase: 'keychains', + status: 'updated', + completed: 1, + attempted: 1, + succeeded: 1, + skipped: 0, + currentKeychainId: 'xpub1', + keychainVersion: 'v1', + }, + { + phase: 'keychains', + status: 'updated', + completed: 2, + attempted: 2, + succeeded: 1, + skipped: 1, + currentKeychainId: 'xpub2', + keychainVersion: 'v1', + }, + { + phase: 'keychains', + status: 'updated', + completed: 3, + attempted: 3, + succeeded: 2, + skipped: 1, + currentKeychainId: 'xpub3', + keychainVersion: 'v2', + }, + { phase: 'finalizing', status: 'started' }, + { phase: 'finalizing', status: 'completed' }, + ]); + }); + + it('does not let a throwing progressCallback abort the rotation', async function () { + nock(ROOT).get('/api/v2/user/checkBatchingPasswordFlow').query(true).reply(200, { isBatchingFlowEnabled: false }); + const changePassScope = nock(ROOT) + .post('/api/v1/user/changepassword', (body: any) => !!body.keychains && !!body.v2_keychains) + .reply(200, {}); + + await bitgo.changePassword({ + oldPassword: 'oldpw', + newPassword: 'newpw', + progressCallback: () => { + throw new Error('observer boom'); + }, + }); + + changePassScope.isDone().should.be.true(); + }); + + it('emits no finalizing/completed and no legacy fallback when the batching-path final POST fails', async function () { + nock(ROOT) + .get('/api/v2/user/checkBatchingPasswordFlow') + .query(true) + .reply(200, { isBatchingFlowEnabled: true, maxBatchSizeKB: 900 }); + nock(ROOT).put('/api/v2/user/keychains').reply(200, {}); + const changePassScope = nock(ROOT).post('/api/v1/user/changepassword').reply(500, { error: 'boom' }); + + const events: PasswordRotationProgress[] = []; + await bitgo + .changePassword({ + oldPassword: 'oldpw', + newPassword: 'newpw', + progressCallback: (progress) => events.push(progress), + }) + .should.be.rejected(); + + changePassScope.isDone().should.be.true(); + events.some((e) => e.phase === 'finalizing' && e.status === 'completed').should.be.false(); + }); + + it('completes exactly once via legacy fallback when the batching check fails', async function () { + nock(ROOT).get('/api/v2/user/checkBatchingPasswordFlow').query(true).reply(503, { error: 'service unavailable' }); + nock(ROOT) + .post('/api/v1/user/changepassword', (body: any) => !!body.keychains && !!body.v2_keychains) + .reply(200, {}); + + const events: PasswordRotationProgress[] = []; + await bitgo.changePassword({ + oldPassword: 'oldpw', + newPassword: 'newpw', + progressCallback: (progress) => events.push(progress), + }); + + events.filter((e) => e.phase === 'finalizing' && e.status === 'completed').should.have.length(1); + }); + + it('does not add keychain events or double-count when a transport batch is retried', async function () { + nock(ROOT) + .get('/api/v2/user/checkBatchingPasswordFlow') + .query(true) + .reply(200, { isBatchingFlowEnabled: true, maxBatchSizeKB: 900 }); + nock(ROOT).put('/api/v2/user/keychains').reply(500, { error: 'transient' }); + nock(ROOT).put('/api/v2/user/keychains').reply(200, {}); + nock(ROOT) + .post('/api/v1/user/changepassword', (body: any) => !body.keychains && !body.v2_keychains) + .reply(200, {}); + + v1UpdatePasswordStub.callsFake(async (params: { progressCallback?: (p: unknown) => void }) => { + params.progressCallback?.({ status: 'updated', currentKeychainId: 'xpub1' }); + return { keychains: { k1: 'v1enc' }, version: 25 }; + }); + + const events: PasswordRotationProgress[] = []; + await bitgo.changePassword({ + oldPassword: 'oldpw', + newPassword: 'newpw', + progressCallback: (progress) => events.push(progress), + }); + + const keychainEvents = events.filter((e) => e.phase === 'keychains' && e.status === 'updated'); + keychainEvents.should.have.length(1); + keychainEvents[0].should.have.properties({ succeeded: 1, skipped: 0, attempted: 1, completed: 1 }); + }); }); describe('createUserEcdhKeychain - encryptionVersion threading', function () { diff --git a/modules/sdk-core/src/bitgo/keychain/iKeychains.ts b/modules/sdk-core/src/bitgo/keychain/iKeychains.ts index 0868ef73e31..a2fdd2fbcb9 100644 --- a/modules/sdk-core/src/bitgo/keychain/iKeychains.ts +++ b/modules/sdk-core/src/bitgo/keychain/iKeychains.ts @@ -102,6 +102,15 @@ export interface ListKeychainOptions { safeId?: string; } +export type KeychainPasswordUpdateStatus = 'updated' | 'skipped'; + +export interface KeychainPasswordUpdateProgress { + status: KeychainPasswordUpdateStatus; + currentKeychainId?: string; +} + +export type KeychainPasswordUpdateProgressCallback = (progress: KeychainPasswordUpdateProgress) => void; + export interface UpdatePasswordOptions { oldPassword: string; newPassword: string; @@ -131,6 +140,8 @@ export interface UpdatePasswordOptions { * per-keychain source versions. */ safeId?: string; + /** Optional observer invoked once per nonfatal keychain outcome during password rotation. */ + progressCallback?: KeychainPasswordUpdateProgressCallback; } export interface UpdateSingleKeychainPasswordOptions { diff --git a/modules/sdk-core/src/bitgo/keychain/keychains.ts b/modules/sdk-core/src/bitgo/keychain/keychains.ts index eb4207dc70a..43f11df1040 100644 --- a/modules/sdk-core/src/bitgo/keychain/keychains.ts +++ b/modules/sdk-core/src/bitgo/keychain/keychains.ts @@ -155,6 +155,18 @@ export class Keychains implements IKeychains { return this.updateSafePassword(params as UpdatePasswordOptions & { safeId: string }); } const changedKeys: ChangedKeychains = {}; + const notifyProgress = (status: 'updated' | 'skipped', currentKeychainId?: string) => { + if (!_.isFunction(params.progressCallback)) { + return; + } + try { + params.progressCallback({ status, currentKeychainId }); + } catch (e) { + // ignore observer exceptions so a throwing callback never affects rotation results + } + }; + const observationId = (key: Keychain): string | undefined => + key.type === 'tss' || Keychains.isMultiUserKey(key) ? key.id : key.pub; let prevId; let keysLeft = true; while (keysLeft) { @@ -162,6 +174,7 @@ export class Keychains implements IKeychains { for (const key of result.keys) { const oldEncryptedPrv = key.encryptedPrv; if (_.isUndefined(oldEncryptedPrv)) { + notifyProgress('skipped', observationId(key)); continue; } try { @@ -180,7 +193,12 @@ export class Keychains implements IKeychains { : updatedKeychain.pub; if (changedKeyIdentifier) { changedKeys[changedKeyIdentifier] = updatedKeychain.encryptedPrv; + notifyProgress('updated', changedKeyIdentifier); + } else { + notifyProgress('skipped', observationId(key)); } + } else { + notifyProgress('skipped', observationId(key)); } } catch (e) { // A decrypt failure is usually not a wrong password — the keychain may be @@ -192,6 +210,7 @@ export class Keychains implements IKeychains { ) { throw e; } + notifyProgress('skipped', observationId(key)); } } if (result.nextBatchPrevId) { From f2a647a769496c9d2a6fd2dfca2774bab2e29231 Mon Sep 17 00:00:00 2001 From: Noe Gutierrez Date: Fri, 25 Sep 2026 12:25:18 -0700 Subject: [PATCH 2/7] feat(sdk-api,sdk-core): thread truthful keychain total into rotation progress MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- modules/bitgo/test/v2/unit/keychains.ts | 45 +++++++++++++++---- modules/sdk-api/src/bitgoAPI.ts | 4 +- modules/sdk-api/test/unit/bitgoAPI.ts | 30 +++++++++++++ .../sdk-core/src/bitgo/keychain/iKeychains.ts | 9 ++++ .../sdk-core/src/bitgo/keychain/keychains.ts | 11 ++++- 5 files changed, 88 insertions(+), 11 deletions(-) diff --git a/modules/bitgo/test/v2/unit/keychains.ts b/modules/bitgo/test/v2/unit/keychains.ts index c12980ac229..c2bcc406f01 100644 --- a/modules/bitgo/test/v2/unit/keychains.ts +++ b/modules/bitgo/test/v2/unit/keychains.ts @@ -600,19 +600,47 @@ describe('V2 Keychains', function () { }); events.should.deepEqual([ - { status: 'updated', currentKeychainId: 'xpub1' }, - { status: 'skipped', currentKeychainId: 'xpub2' }, - { status: 'updated', currentKeychainId: 'xpub3' }, + { status: 'updated', currentKeychainId: 'xpub1', total: undefined }, + { status: 'skipped', currentKeychainId: 'xpub2', total: undefined }, + { status: 'updated', currentKeychainId: 'xpub3', total: undefined }, ]); }); - it('should emit skipped when a key has no encryptedPrv', async function () { + it('threads encryptedTotalCount from the first page into total on every emitted event', async function () { + const encXprv1 = await bitgo.encrypt({ input: 'xprv1', password: oldPassword }); + const encXprv2 = await bitgo.encrypt({ input: 'xprv2', password: oldPassword }); + nock(bgUrl) + .get('/api/v2/tltc/key') + .query(true) + .reply(200, { + keys: [ + { pub: 'xpub1', encryptedPrv: encXprv1 }, + { pub: 'xpub2', encryptedPrv: encXprv2 }, + ], + encryptedTotalCount: 2, + }); + + const events: Array<{ status: string; currentKeychainId?: string; total?: number }> = []; + await keychains.updatePassword({ + oldPassword, + newPassword, + progressCallback: (progress) => events.push(progress), + }); + + events.should.deepEqual([ + { status: 'updated', currentKeychainId: 'xpub1', total: 2 }, + { status: 'updated', currentKeychainId: 'xpub2', total: 2 }, + ]); + }); + + it('should not emit progress for a key with no encryptedPrv (outside the encryptedTotalCount unit)', async function () { const encXprv1 = await bitgo.encrypt({ input: 'xprv1', password: oldPassword }); nock(bgUrl) .get('/api/v2/tltc/key') .query(true) .reply(200, { keys: [{ pub: 'xpub1', encryptedPrv: encXprv1 }, { pub: 'xpub2' }], + encryptedTotalCount: 1, }); const events: Array<{ status: string; currentKeychainId?: string }> = []; @@ -622,8 +650,9 @@ describe('V2 Keychains', function () { progressCallback: (progress) => events.push(progress), }); - events.should.containEql({ status: 'updated', currentKeychainId: 'xpub1' }); - events.should.containEql({ status: 'skipped', currentKeychainId: 'xpub2' }); + // Only the encrypted record is in the unit; the placeholder record must not + // emit, or `completed` would overshoot `encryptedTotalCount` in the UI. + events.should.deepEqual([{ status: 'updated', currentKeychainId: 'xpub1', total: 1 }]); }); it('should emit skipped for a known decrypt-failure and never mislabel a fatal error as skipped', async function () { @@ -646,8 +675,8 @@ describe('V2 Keychains', function () { progressCallback: (progress) => events.push(progress), }); - events.should.containEql({ status: 'updated', currentKeychainId: 'xpub1' }); - events.should.containEql({ status: 'skipped', currentKeychainId: 'xpub2' }); + events.should.containEql({ status: 'updated', currentKeychainId: 'xpub1', total: undefined }); + events.should.containEql({ status: 'skipped', currentKeychainId: 'xpub2', total: undefined }); const sandbox = sinon.createSandbox(); try { diff --git a/modules/sdk-api/src/bitgoAPI.ts b/modules/sdk-api/src/bitgoAPI.ts index 42cf0c91d35..b572f99f7d8 100644 --- a/modules/sdk-api/src/bitgoAPI.ts +++ b/modules/sdk-api/src/bitgoAPI.ts @@ -2051,7 +2051,8 @@ export class BitGoAPI implements BitGoBase { emitProgress({ phase: 'keychains', status: 'started', ...counters }); const makeKeychainProgressCallback = - (keychainVersion: 'v1' | 'v2') => (progress: { status: 'updated' | 'skipped'; currentKeychainId?: string }) => { + (keychainVersion: 'v1' | 'v2') => + (progress: { status: 'updated' | 'skipped'; currentKeychainId?: string; total?: number }) => { counters.attempted++; counters.completed++; if (progress.status === 'updated') { @@ -2063,6 +2064,7 @@ export class BitGoAPI implements BitGoBase { phase: 'keychains', status: 'updated', ...counters, + total: progress.total, currentKeychainId: progress.currentKeychainId, keychainVersion, }); diff --git a/modules/sdk-api/test/unit/bitgoAPI.ts b/modules/sdk-api/test/unit/bitgoAPI.ts index 020ff027c74..a201296c7f9 100644 --- a/modules/sdk-api/test/unit/bitgoAPI.ts +++ b/modules/sdk-api/test/unit/bitgoAPI.ts @@ -1223,6 +1223,7 @@ describe('Constructor', function () { attempted: 1, succeeded: 1, skipped: 0, + total: undefined, currentKeychainId: 'xpub1', keychainVersion: 'v1', }, @@ -1233,6 +1234,7 @@ describe('Constructor', function () { attempted: 2, succeeded: 1, skipped: 1, + total: undefined, currentKeychainId: 'xpub2', keychainVersion: 'v1', }, @@ -1243,6 +1245,7 @@ describe('Constructor', function () { attempted: 3, succeeded: 2, skipped: 1, + total: undefined, currentKeychainId: 'xpub3', keychainVersion: 'v2', }, @@ -1251,6 +1254,33 @@ describe('Constructor', function () { ]); }); + it('forwards a truthful total from the v2 lower-level callback into every keychains/updated event', async function () { + nock(ROOT).get('/api/v2/user/checkBatchingPasswordFlow').query(true).reply(200, { isBatchingFlowEnabled: false }); + nock(ROOT) + .post('/api/v1/user/changepassword', (body: any) => !!body.keychains && !!body.v2_keychains) + .reply(200, {}); + + v2UpdatePasswordStub.callsFake(async (params: { progressCallback?: (p: unknown) => void }) => { + params.progressCallback?.({ status: 'updated', currentKeychainId: 'xpub1', total: 2 }); + params.progressCallback?.({ status: 'updated', currentKeychainId: 'xpub2', total: 2 }); + return { v2k1: 'v2enc', v2k2: 'v2enc2' }; + }); + + const events: PasswordRotationProgress[] = []; + await bitgo.changePassword({ + oldPassword: 'oldpw', + newPassword: 'newpw', + progressCallback: (progress) => events.push(progress), + }); + + const v2Events = events.filter( + (e): e is Extract => + e.phase === 'keychains' && e.status === 'updated' && e.keychainVersion === 'v2' + ); + v2Events.should.have.length(2); + v2Events.every((e) => e.total === 2).should.be.true(); + }); + it('does not let a throwing progressCallback abort the rotation', async function () { nock(ROOT).get('/api/v2/user/checkBatchingPasswordFlow').query(true).reply(200, { isBatchingFlowEnabled: false }); const changePassScope = nock(ROOT) diff --git a/modules/sdk-core/src/bitgo/keychain/iKeychains.ts b/modules/sdk-core/src/bitgo/keychain/iKeychains.ts index a2fdd2fbcb9..7b7aeed4432 100644 --- a/modules/sdk-core/src/bitgo/keychain/iKeychains.ts +++ b/modules/sdk-core/src/bitgo/keychain/iKeychains.ts @@ -81,6 +81,8 @@ export interface ChangedKeychains { export interface ListKeychainsResult { keys: Keychain[]; nextBatchPrevId?: string; + /** Count of this user's keychains with encrypted private key material set (WCN-2084 total-count support). */ + encryptedTotalCount?: number; } export interface GetKeychainOptions { @@ -107,6 +109,13 @@ export type KeychainPasswordUpdateStatus = 'updated' | 'skipped'; export interface KeychainPasswordUpdateProgress { status: KeychainPasswordUpdateStatus; currentKeychainId?: string; + /** + * Truthful total of keychains with encrypted user key material (`encryptedTotalCount` + * from the backend, WCN-2084). Records without an `encryptedPrv` are outside this + * unit and emit no progress event — the denominator must never be undershot or + * overshot by `completed`. + */ + total?: number; } export type KeychainPasswordUpdateProgressCallback = (progress: KeychainPasswordUpdateProgress) => void; diff --git a/modules/sdk-core/src/bitgo/keychain/keychains.ts b/modules/sdk-core/src/bitgo/keychain/keychains.ts index 43f11df1040..f67f0dd60c4 100644 --- a/modules/sdk-core/src/bitgo/keychain/keychains.ts +++ b/modules/sdk-core/src/bitgo/keychain/keychains.ts @@ -155,12 +155,13 @@ export class Keychains implements IKeychains { return this.updateSafePassword(params as UpdatePasswordOptions & { safeId: string }); } const changedKeys: ChangedKeychains = {}; + let total: number | undefined; const notifyProgress = (status: 'updated' | 'skipped', currentKeychainId?: string) => { if (!_.isFunction(params.progressCallback)) { return; } try { - params.progressCallback({ status, currentKeychainId }); + params.progressCallback({ status, currentKeychainId, total }); } catch (e) { // ignore observer exceptions so a throwing callback never affects rotation results } @@ -171,10 +172,16 @@ export class Keychains implements IKeychains { let keysLeft = true; while (keysLeft) { const result: ListKeychainsResult = await this.list({ limit: 500, prevId }); + if (total === undefined && result.encryptedTotalCount !== undefined) { + total = result.encryptedTotalCount; + } for (const key of result.keys) { const oldEncryptedPrv = key.encryptedPrv; if (_.isUndefined(oldEncryptedPrv)) { - notifyProgress('skipped', observationId(key)); + // Outside the progress unit (WCN-2084 option B): the denominator is + // `encryptedTotalCount` — records with no encryptedPrv are not counted + // by the backend and must not emit progress, or `completed` would + // overshoot `total` on any page containing placeholder records. continue; } try { From f3bce88c5c709a6f4b235c59c6250f58ccdae951 Mon Sep 17 00:00:00 2001 From: Noe Gutierrez Date: Tue, 29 Sep 2026 09:35:00 -0700 Subject: [PATCH 3/7] fix(sdk-api): cover both keychain sets in the rotation total MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- modules/sdk-api/src/bitgoAPI.ts | 16 +++++++- modules/sdk-api/test/unit/bitgoAPI.ts | 53 +++++++++++++++++++++++++-- 2 files changed, 65 insertions(+), 4 deletions(-) diff --git a/modules/sdk-api/src/bitgoAPI.ts b/modules/sdk-api/src/bitgoAPI.ts index b572f99f7d8..0b0b73ab98d 100644 --- a/modules/sdk-api/src/bitgoAPI.ts +++ b/modules/sdk-api/src/bitgoAPI.ts @@ -2048,6 +2048,11 @@ export class BitGoAPI implements BitGoBase { // Single counter set shared by both the v1 and v2 lower-level keychain callbacks below. const counters = { attempted: 0, completed: 0, succeeded: 0, skipped: 0 }; + // The v1 set has no server-provided count; its size (the /user/encrypted map) is + // known only once v1 processing returns. Captured so v2 events can add it to the + // backend-served encryptedTotalCount — without this, a mixed account finishes at + // `completed > total` (2 v1 + 32 v2 keychains read "34 of 32"). + let v1KeychainCount = 0; emitProgress({ phase: 'keychains', status: 'started', ...counters }); const makeKeychainProgressCallback = @@ -2064,7 +2069,13 @@ export class BitGoAPI implements BitGoBase { phase: 'keychains', status: 'updated', ...counters, - total: progress.total, + // v1 events stay indeterminate: the v1 set has no server count, and surfacing + // its client-side size mid-flight would make the denominator jump the moment + // v2's encryptedTotalCount arrives. v2 events add the v1 set size so the + // counter reaches the denominator exactly on mixed v1/v2 accounts. When the + // backend supplies no count, the total stays undefined (never fabricated). + total: + keychainVersion === 'v2' && progress.total !== undefined ? v1KeychainCount + progress.total : undefined, currentKeychainId: progress.currentKeychainId, keychainVersion, }); @@ -2086,6 +2097,9 @@ export class BitGoAPI implements BitGoBase { encryptionSession, progressCallback: makeKeychainProgressCallback('v1'), }); + // Every v1 entry emits exactly one progress event, so the returned map's size is + // the v1 contribution to the denominator; v2 events below add it to their total. + v1KeychainCount = Object.keys(v1KeychainUpdatePWResult.keychains).length; const v2Keychains = await this.coin(coin) .keychains() .updatePassword({ diff --git a/modules/sdk-api/test/unit/bitgoAPI.ts b/modules/sdk-api/test/unit/bitgoAPI.ts index a201296c7f9..25c3c5ec5fe 100644 --- a/modules/sdk-api/test/unit/bitgoAPI.ts +++ b/modules/sdk-api/test/unit/bitgoAPI.ts @@ -1200,7 +1200,8 @@ describe('Constructor', function () { v1UpdatePasswordStub.callsFake(async (params: { progressCallback?: (p: unknown) => void }) => { params.progressCallback?.({ status: 'updated', currentKeychainId: 'xpub1' }); params.progressCallback?.({ status: 'skipped', currentKeychainId: 'xpub2' }); - return { keychains: { k1: 'v1enc' }, version: 25 }; + // one map entry per emitted event, since the map's size now feeds the v2 total + return { keychains: { k1: 'v1enc', k2: 'v1enc2' }, version: 25 }; }); v2UpdatePasswordStub.callsFake(async (params: { progressCallback?: (p: unknown) => void }) => { params.progressCallback?.({ status: 'updated', currentKeychainId: 'xpub3' }); @@ -1277,8 +1278,54 @@ describe('Constructor', function () { (e): e is Extract => e.phase === 'keychains' && e.status === 'updated' && e.keychainVersion === 'v2' ); - v2Events.should.have.length(2); - v2Events.every((e) => e.total === 2).should.be.true(); + // the v1 stub keeps its default 1-entry map, so v2 events forward 1 (v1) + 2 (v2) = 3 + v2Events.every((e) => e.total === 3).should.be.true(); + }); + + it('covers both keychain sets in the total so mixed v1/v2 accounts complete exactly (2 v1 + 32 v2 = 34 of 34)', async function () { + nock(ROOT).get('/api/v2/user/checkBatchingPasswordFlow').query(true).reply(200, { isBatchingFlowEnabled: false }); + nock(ROOT) + .post('/api/v1/user/changepassword', (body: any) => !!body.keychains && !!body.v2_keychains) + .reply(200, {}); + + v1UpdatePasswordStub.callsFake(async (params: { progressCallback?: (p: unknown) => void }) => { + params.progressCallback?.({ status: 'updated', currentKeychainId: 'xpub1' }); + params.progressCallback?.({ status: 'updated', currentKeychainId: 'xpub2' }); + return { keychains: { k1: 'v1enc', k2: 'v1enc2' }, version: 25 }; + }); + v2UpdatePasswordStub.callsFake(async (params: { progressCallback?: (p: unknown) => void }) => { + for (let i = 1; i <= 32; i++) { + params.progressCallback?.({ status: 'updated', currentKeychainId: `v2key-${i}`, total: 32 }); + } + return { v2k1: 'v2enc' }; + }); + + const events: PasswordRotationProgress[] = []; + await bitgo.changePassword({ + oldPassword: 'oldpw', + newPassword: 'newpw', + progressCallback: (progress) => events.push(progress), + }); + + const keychainEvents = events.filter( + (e): e is Extract => + e.phase === 'keychains' && e.status === 'updated' + ); + const v1Events = keychainEvents.filter((e) => e.keychainVersion === 'v1'); + const v2Events = keychainEvents.filter((e) => e.keychainVersion === 'v2'); + + // v1 events stay indeterminate: the v1 set has no server-provided count + v1Events.should.have.length(2); + v1Events.every((e) => e.total === undefined).should.be.true(); + + // v2 events carry the combined denominator: 2 (v1 set) + 32 (encryptedTotalCount) + v2Events.should.have.length(32); + v2Events.every((e) => e.total === 34).should.be.true(); + + // the counter completes exactly instead of overshooting the total + const last = v2Events[v2Events.length - 1]; + last.completed.should.equal(34); + last.total.should.equal(34); }); it('does not let a throwing progressCallback abort the rotation', async function () { From 245aa94823dae79f68578fe34d2934ec09d6b433 Mon Sep 17 00:00:00 2001 From: Noe Gutierrez Date: Wed, 30 Sep 2026 08:58:10 -0700 Subject: [PATCH 4/7] fix(sdk-api): emit terminal total for v1-only accounts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- modules/sdk-api/src/bitgoAPI.ts | 15 +++++++++ modules/sdk-api/test/unit/bitgoAPI.ts | 45 +++++++++++++++++++++++++-- 2 files changed, 58 insertions(+), 2 deletions(-) diff --git a/modules/sdk-api/src/bitgoAPI.ts b/modules/sdk-api/src/bitgoAPI.ts index 0b0b73ab98d..19b8f51c90e 100644 --- a/modules/sdk-api/src/bitgoAPI.ts +++ b/modules/sdk-api/src/bitgoAPI.ts @@ -2053,6 +2053,9 @@ export class BitGoAPI implements BitGoBase { // backend-served encryptedTotalCount — without this, a mixed account finishes at // `completed > total` (2 v1 + 32 v2 keychains read "34 of 32"). let v1KeychainCount = 0; + // A v1-only account emits no v2 event at all, so nothing would ever carry a total; + // tracked so the terminal denominator below can fire for those accounts. + let v2EventCount = 0; emitProgress({ phase: 'keychains', status: 'started', ...counters }); const makeKeychainProgressCallback = @@ -2060,6 +2063,9 @@ export class BitGoAPI implements BitGoBase { (progress: { status: 'updated' | 'skipped'; currentKeychainId?: string; total?: number }) => { counters.attempted++; counters.completed++; + if (keychainVersion === 'v2') { + v2EventCount++; + } if (progress.status === 'updated') { counters.succeeded++; } else { @@ -2110,6 +2116,15 @@ export class BitGoAPI implements BitGoBase { progressCallback: makeKeychainProgressCallback('v2'), }); + // A v1-only account emits no v2 event, so its total would stay undefined for the + // whole rotation and the observer could never render "n of n". Emit the client-known + // v1 set size as the terminal denominator — guarded on zero v2 events so mixed + // accounts keep the single stable total from encryptedTotalCount instead of one + // that jumps mid-rotation. + if (v2EventCount === 0 && v1KeychainCount > 0) { + emitProgress({ phase: 'keychains', status: 'updated', ...counters, total: v1KeychainCount }); + } + emitProgress({ phase: 'finalizing', status: 'started' }); const [hmacOldPassword, hmacNewPassword] = await Promise.all([ diff --git a/modules/sdk-api/test/unit/bitgoAPI.ts b/modules/sdk-api/test/unit/bitgoAPI.ts index 25c3c5ec5fe..f3aa8a49bdf 100644 --- a/modules/sdk-api/test/unit/bitgoAPI.ts +++ b/modules/sdk-api/test/unit/bitgoAPI.ts @@ -1325,7 +1325,44 @@ describe('Constructor', function () { // the counter completes exactly instead of overshooting the total const last = v2Events[v2Events.length - 1]; last.completed.should.equal(34); - last.total.should.equal(34); + last.total!.should.equal(34); + }); + + it('emits a terminal total for v1-only accounts so the counter can render n of n', async function () { + nock(ROOT).get('/api/v2/user/checkBatchingPasswordFlow').query(true).reply(200, { isBatchingFlowEnabled: false }); + nock(ROOT) + .post('/api/v1/user/changepassword', (body: any) => !!body.keychains && !!body.v2_keychains) + .reply(200, {}); + + v1UpdatePasswordStub.callsFake(async (params: { progressCallback?: (p: unknown) => void }) => { + params.progressCallback?.({ status: 'updated', currentKeychainId: 'xpub1' }); + params.progressCallback?.({ status: 'updated', currentKeychainId: 'xpub2' }); + return { keychains: { k1: 'v1enc', k2: 'v1enc2' }, version: 25 }; + }); + // a v1-only account: the v2 walk finds nothing and emits nothing + v2UpdatePasswordStub.callsFake(async () => ({})); + + const events: PasswordRotationProgress[] = []; + await bitgo.changePassword({ + oldPassword: 'oldpw', + newPassword: 'newpw', + progressCallback: (progress) => events.push(progress), + }); + + const keychainEvents = events.filter( + (e): e is Extract => + e.phase === 'keychains' && e.status === 'updated' + ); + const v1Events = keychainEvents.filter((e) => e.keychainVersion === 'v1'); + v1Events.should.have.length(2); + v1Events.every((e) => e.total === undefined).should.be.true(); + + // no v2 event ever arrived, so the terminal keychains/updated event carries the + // client-known v1 set size as the denominator: 2 of 2, immediately before finalizing + const last = keychainEvents[keychainEvents.length - 1]; + last.completed.should.equal(2); + last.total!.should.equal(2); + events[events.length - 2].should.deepEqual({ phase: 'finalizing', status: 'started' }); }); it('does not let a throwing progressCallback abort the rotation', async function () { @@ -1406,8 +1443,12 @@ describe('Constructor', function () { }); const keychainEvents = events.filter((e) => e.phase === 'keychains' && e.status === 'updated'); - keychainEvents.should.have.length(1); + // one real keychain event plus the terminal denominator event for this v1-only + // account — the retried transport must contribute neither a duplicate event + // nor a double-count to either + keychainEvents.should.have.length(2); keychainEvents[0].should.have.properties({ succeeded: 1, skipped: 0, attempted: 1, completed: 1 }); + keychainEvents[1].should.have.properties({ succeeded: 1, skipped: 0, attempted: 1, completed: 1, total: 1 }); }); }); From 2c456e9a3a1e23f28bec3dcc50ea6a557e7f2413 Mon Sep 17 00:00:00 2001 From: Noe Gutierrez Date: Thu, 1 Oct 2026 12:53:14 -0700 Subject: [PATCH 5/7] feat(sdk-core): report an outcome for every listed keychain (option A) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- modules/bitgo/test/v2/unit/keychains.ts | 20 +++++++++++-------- modules/sdk-api/src/bitgoAPI.ts | 6 +++--- .../sdk-core/src/bitgo/keychain/iKeychains.ts | 11 +++++----- .../sdk-core/src/bitgo/keychain/keychains.ts | 14 +++++++------ 4 files changed, 28 insertions(+), 23 deletions(-) diff --git a/modules/bitgo/test/v2/unit/keychains.ts b/modules/bitgo/test/v2/unit/keychains.ts index c2bcc406f01..382a322a246 100644 --- a/modules/bitgo/test/v2/unit/keychains.ts +++ b/modules/bitgo/test/v2/unit/keychains.ts @@ -606,7 +606,7 @@ describe('V2 Keychains', function () { ]); }); - it('threads encryptedTotalCount from the first page into total on every emitted event', async function () { + it('threads totalCount from the first page into total on every emitted event', async function () { const encXprv1 = await bitgo.encrypt({ input: 'xprv1', password: oldPassword }); const encXprv2 = await bitgo.encrypt({ input: 'xprv2', password: oldPassword }); nock(bgUrl) @@ -617,7 +617,7 @@ describe('V2 Keychains', function () { { pub: 'xpub1', encryptedPrv: encXprv1 }, { pub: 'xpub2', encryptedPrv: encXprv2 }, ], - encryptedTotalCount: 2, + totalCount: 2, }); const events: Array<{ status: string; currentKeychainId?: string; total?: number }> = []; @@ -633,26 +633,30 @@ describe('V2 Keychains', function () { ]); }); - it('should not emit progress for a key with no encryptedPrv (outside the encryptedTotalCount unit)', async function () { + it('emits skipped for a listed record with no encryptedPrv so completed reaches totalCount (option A)', async function () { const encXprv1 = await bitgo.encrypt({ input: 'xprv1', password: oldPassword }); nock(bgUrl) .get('/api/v2/tltc/key') .query(true) .reply(200, { keys: [{ pub: 'xpub1', encryptedPrv: encXprv1 }, { pub: 'xpub2' }], - encryptedTotalCount: 1, + totalCount: 2, }); - const events: Array<{ status: string; currentKeychainId?: string }> = []; + const events: Array<{ status: string; currentKeychainId?: string; total?: number }> = []; await keychains.updatePassword({ oldPassword, newPassword, progressCallback: (progress) => events.push(progress), }); - // Only the encrypted record is in the unit; the placeholder record must not - // emit, or `completed` would overshoot `encryptedTotalCount` in the UI. - events.should.deepEqual([{ status: 'updated', currentKeychainId: 'xpub1', total: 1 }]); + // Every listed record reports an outcome (updated | skipped): the placeholder + // record is counted by the list total, so it MUST report skipped — a silent + // drop would leave `completed` one short of `totalCount` in the UI. + events.should.deepEqual([ + { status: 'updated', currentKeychainId: 'xpub1', total: 2 }, + { status: 'skipped', currentKeychainId: 'xpub2', total: 2 }, + ]); }); it('should emit skipped for a known decrypt-failure and never mislabel a fatal error as skipped', async function () { diff --git a/modules/sdk-api/src/bitgoAPI.ts b/modules/sdk-api/src/bitgoAPI.ts index 19b8f51c90e..d44967d9870 100644 --- a/modules/sdk-api/src/bitgoAPI.ts +++ b/modules/sdk-api/src/bitgoAPI.ts @@ -2050,7 +2050,7 @@ export class BitGoAPI implements BitGoBase { const counters = { attempted: 0, completed: 0, succeeded: 0, skipped: 0 }; // The v1 set has no server-provided count; its size (the /user/encrypted map) is // known only once v1 processing returns. Captured so v2 events can add it to the - // backend-served encryptedTotalCount — without this, a mixed account finishes at + // backend-served totalCount — without this, a mixed account finishes at // `completed > total` (2 v1 + 32 v2 keychains read "34 of 32"). let v1KeychainCount = 0; // A v1-only account emits no v2 event at all, so nothing would ever carry a total; @@ -2077,7 +2077,7 @@ export class BitGoAPI implements BitGoBase { ...counters, // v1 events stay indeterminate: the v1 set has no server count, and surfacing // its client-side size mid-flight would make the denominator jump the moment - // v2's encryptedTotalCount arrives. v2 events add the v1 set size so the + // v2's totalCount arrives. v2 events add the v1 set size so the // counter reaches the denominator exactly on mixed v1/v2 accounts. When the // backend supplies no count, the total stays undefined (never fabricated). total: @@ -2119,7 +2119,7 @@ export class BitGoAPI implements BitGoBase { // A v1-only account emits no v2 event, so its total would stay undefined for the // whole rotation and the observer could never render "n of n". Emit the client-known // v1 set size as the terminal denominator — guarded on zero v2 events so mixed - // accounts keep the single stable total from encryptedTotalCount instead of one + // accounts keep the single stable total from totalCount instead of one // that jumps mid-rotation. if (v2EventCount === 0 && v1KeychainCount > 0) { emitProgress({ phase: 'keychains', status: 'updated', ...counters, total: v1KeychainCount }); diff --git a/modules/sdk-core/src/bitgo/keychain/iKeychains.ts b/modules/sdk-core/src/bitgo/keychain/iKeychains.ts index 7b7aeed4432..696449191bc 100644 --- a/modules/sdk-core/src/bitgo/keychain/iKeychains.ts +++ b/modules/sdk-core/src/bitgo/keychain/iKeychains.ts @@ -81,8 +81,8 @@ export interface ChangedKeychains { export interface ListKeychainsResult { keys: Keychain[]; nextBatchPrevId?: string; - /** Count of this user's keychains with encrypted private key material set (WCN-2084 total-count support). */ - encryptedTotalCount?: number; + /** Total of the listed keychains — the count of the list's own query (WCN-2084 total-count support). */ + totalCount?: number; } export interface GetKeychainOptions { @@ -110,10 +110,9 @@ export interface KeychainPasswordUpdateProgress { status: KeychainPasswordUpdateStatus; currentKeychainId?: string; /** - * Truthful total of keychains with encrypted user key material (`encryptedTotalCount` - * from the backend, WCN-2084). Records without an `encryptedPrv` are outside this - * unit and emit no progress event — the denominator must never be undershot or - * overshot by `completed`. + * Truthful total of the listed keychains (`totalCount` from the backend, WCN-2084). + * Every listed record reports an outcome (updated | skipped), so `completed` reaches + * this total exactly on any account composition. */ total?: number; } diff --git a/modules/sdk-core/src/bitgo/keychain/keychains.ts b/modules/sdk-core/src/bitgo/keychain/keychains.ts index f67f0dd60c4..56f9b7140a0 100644 --- a/modules/sdk-core/src/bitgo/keychain/keychains.ts +++ b/modules/sdk-core/src/bitgo/keychain/keychains.ts @@ -172,16 +172,18 @@ export class Keychains implements IKeychains { let keysLeft = true; while (keysLeft) { const result: ListKeychainsResult = await this.list({ limit: 500, prevId }); - if (total === undefined && result.encryptedTotalCount !== undefined) { - total = result.encryptedTotalCount; + if (total === undefined && result.totalCount !== undefined) { + total = result.totalCount; } for (const key of result.keys) { const oldEncryptedPrv = key.encryptedPrv; if (_.isUndefined(oldEncryptedPrv)) { - // Outside the progress unit (WCN-2084 option B): the denominator is - // `encryptedTotalCount` — records with no encryptedPrv are not counted - // by the backend and must not emit progress, or `completed` would - // overshoot `total` on any page containing placeholder records. + // Option A counting unit (WCN-2084): every listed record reports an outcome, + // so `completed` reaches the server's `totalCount` on any account composition. + // A record with no serialized material is reported as skipped rather than + // silently dropped — it is counted by the list total, and the walk still + // leaves it untouched (no re-encryption, no entry in the changedKeys map). + notifyProgress('skipped', observationId(key)); continue; } try { From f7ee764174c897e73f5a2541e6f1f0819ea69bbc Mon Sep 17 00:00:00 2001 From: Noe Gutierrez Date: Fri, 2 Oct 2026 00:27:53 -0700 Subject: [PATCH 6/7] test(sdk-api): fix stale encryptedTotalCount mention in mixed-account comment Ticket: WCN-2084 --- modules/sdk-api/test/unit/bitgoAPI.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/sdk-api/test/unit/bitgoAPI.ts b/modules/sdk-api/test/unit/bitgoAPI.ts index f3aa8a49bdf..f887e2ed24b 100644 --- a/modules/sdk-api/test/unit/bitgoAPI.ts +++ b/modules/sdk-api/test/unit/bitgoAPI.ts @@ -1318,7 +1318,7 @@ describe('Constructor', function () { v1Events.should.have.length(2); v1Events.every((e) => e.total === undefined).should.be.true(); - // v2 events carry the combined denominator: 2 (v1 set) + 32 (encryptedTotalCount) + // v2 events carry the combined denominator: 2 (v1 set) + 32 (totalCount) v2Events.should.have.length(32); v2Events.every((e) => e.total === 34).should.be.true(); From 05aad3a4661f627a02086257f5d453251f480c07 Mon Sep 17 00:00:00 2001 From: Noe Gutierrez Date: Tue, 6 Oct 2026 01:35:19 -0700 Subject: [PATCH 7/7] docs(sdk-api,sdk-core): document changePassword progress options Document encryptionVersion and progressCallback on changePassword, state that safe mode never invokes the callback, and drop a design label from one comment and one test title. No behavior change. Ticket: WCN-2084 --- modules/bitgo/test/v2/unit/keychains.ts | 2 +- modules/sdk-api/src/bitgoAPI.ts | 11 +++++++++++ modules/sdk-core/src/bitgo/keychain/iKeychains.ts | 6 +++++- modules/sdk-core/src/bitgo/keychain/keychains.ts | 4 ++-- 4 files changed, 19 insertions(+), 4 deletions(-) diff --git a/modules/bitgo/test/v2/unit/keychains.ts b/modules/bitgo/test/v2/unit/keychains.ts index 382a322a246..caf1269742e 100644 --- a/modules/bitgo/test/v2/unit/keychains.ts +++ b/modules/bitgo/test/v2/unit/keychains.ts @@ -633,7 +633,7 @@ describe('V2 Keychains', function () { ]); }); - it('emits skipped for a listed record with no encryptedPrv so completed reaches totalCount (option A)', async function () { + it('emits skipped for a listed record with no encryptedPrv so completed reaches totalCount', async function () { const encXprv1 = await bitgo.encrypt({ input: 'xprv1', password: oldPassword }); nock(bgUrl) .get('/api/v2/tltc/key') diff --git a/modules/sdk-api/src/bitgoAPI.ts b/modules/sdk-api/src/bitgoAPI.ts index d44967d9870..e3d3d750d94 100644 --- a/modules/sdk-api/src/bitgoAPI.ts +++ b/modules/sdk-api/src/bitgoAPI.ts @@ -2010,6 +2010,17 @@ export class BitGoAPI implements BitGoBase { * given oldPassword. Returns nothing on success. * @param oldPassword {String} - the current password * @param newPassword {String} - the new password + * @param encryptionVersion {EncryptionVersion} - optional envelope version for the re-encrypted keychains; + * defaults to preserving each keychain's existing envelope version + * @param progressCallback {PasswordRotationProgressCallback} - optional observer for rotation progress. + * Invoked synchronously and never awaited; an exception thrown by the callback is swallowed, so it can + * never change the outcome of the rotation. Events, in order: `keychains/started`, one `keychains/updated` + * per listed keychain record (`updated` or `skipped`), then `finalizing/started` and `finalizing/completed` + * (only after the final request succeeds). `total` is the backend's first-page `totalCount` plus the size of + * the v1 keychain set; it stays `undefined` while the backend supplies no count, except that an account with + * only v1 keychains receives one terminal `keychains/updated` event carrying the v1 set size. Consumers should + * render indeterminate progress while `total` is `undefined`. `completed` reaches `total` unless keychains are + * added or removed during the rotation, because `total` is latched from the first v2 page. */ async changePassword({ oldPassword, diff --git a/modules/sdk-core/src/bitgo/keychain/iKeychains.ts b/modules/sdk-core/src/bitgo/keychain/iKeychains.ts index 696449191bc..fe40604b18a 100644 --- a/modules/sdk-core/src/bitgo/keychain/iKeychains.ts +++ b/modules/sdk-core/src/bitgo/keychain/iKeychains.ts @@ -148,7 +148,11 @@ export interface UpdatePasswordOptions { * per-keychain source versions. */ safeId?: string; - /** Optional observer invoked once per nonfatal keychain outcome during password rotation. */ + /** + * Optional observer invoked once per listed keychain record (`updated` or `skipped`) during password + * rotation. Legacy mode (no `safeId`) only: safe mode persists through the bulk key endpoint and never + * invokes it. + */ progressCallback?: KeychainPasswordUpdateProgressCallback; } diff --git a/modules/sdk-core/src/bitgo/keychain/keychains.ts b/modules/sdk-core/src/bitgo/keychain/keychains.ts index 56f9b7140a0..ec226f0a059 100644 --- a/modules/sdk-core/src/bitgo/keychain/keychains.ts +++ b/modules/sdk-core/src/bitgo/keychain/keychains.ts @@ -178,8 +178,8 @@ export class Keychains implements IKeychains { for (const key of result.keys) { const oldEncryptedPrv = key.encryptedPrv; if (_.isUndefined(oldEncryptedPrv)) { - // Option A counting unit (WCN-2084): every listed record reports an outcome, - // so `completed` reaches the server's `totalCount` on any account composition. + // Every listed record reports an outcome, so `completed` reaches the server's + // `totalCount` on any account composition. // A record with no serialized material is reported as skipped rather than // silently dropped — it is counted by the list total, and the walk still // leaves it untouched (no re-encryption, no entry in the changedKeys map).