Skip to content

WCI-1358: Forward enterprise on tss/settings and keychain add calls - #9543

Open
Marzooqa wants to merge 1 commit into
masterfrom
marzooqakather498/wci-1358-bitgojs-add-enterprise-wallettype-params-to-tsssettings-and
Open

WCI-1358: Forward enterprise on tss/settings and keychain add calls#9543
Marzooqa wants to merge 1 commit into
masterfrom
marzooqakather498/wci-1358-bitgojs-add-enterprise-wallettype-params-to-tsssettings-and

Conversation

@Marzooqa

Copy link
Copy Markdown
Contributor

Summary

  • Keychains.createMpc() and all 5 Wallets call sites that hit GET /api/v2/tss/settings (hot, cold, custodial, external-signer, ECDSA version-bump) now send enterprise as a query param when the caller supplied one. Previously this value was computed/available but silently dropped, never sent over the wire.
  • EddsaMPCv2Utils threads enterprise into the keychains.add(...) calls for both the standard and external-signer EdDSA MPCv2 key-creation paths — including fixing a pre-existing gap where the backup keychain never received enterprise even though the user keychain did.
  • Fully backward compatible: enterprise remains optional everywhere; omitting it produces the exact same request as before this PR.
  • Companion wallet-platform change (already merged/in review): BitGo/bitgo-microservices#61993 (WCI-830), which is what actually evaluates enterpriseId-scoped Flipt segments once this reaches the server.

Test plan

  • keychains.ts, wallets.ts (bitgo module) and eddsaMPCv2.ts/eddsaExternalSigner.ts/walletsExternalSigner.ts (sdk-core) all pass locally
  • Two new dedicated tests in keychains.ts prove the actual behavior: enterprise is forwarded when supplied (exact query match, not a loosened matcher), and omitted entirely (no query string) when it isn't
  • Verify against a staging enterprise-scoped Flipt segment (WCI-831/WCI-830) end-to-end once all three PRs are live

🤖 Generated with Claude Code

@linear-code

linear-code Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

WCI-1358

…alls

BitGoJS already had enterprise available at every call site into GET
/api/v2/tss/settings and the subsequent EdDSA MPCv2 keychains.add(...)
calls, but never sent it, so per-enterprise Flipt rollouts on the
wallet-platform side (WCI-830) can never take effect for SDK-driven
wallet/key creation. Thread it through:

- Keychains.createMpc() and all 5 Wallets GET /tss/settings call
  sites now send enterprise as a query param when available.
- EddsaMPCv2Utils threads enterprise into the user/backup keychain
  add() calls for both the standard and external-signer paths,
  including a pre-existing gap where the backup keychain never
  received it even though the user keychain did.

Fully backward compatible: enterprise remains optional everywhere,
and omitting it produces the exact same request as before.

Existing nock/sinon mocks for GET /tss/settings expected no query
string; loosened to .query(true) since none of those tests were
asserting on query params, plus two new dedicated tests proving
enterprise is forwarded when supplied and omitted when it isn't.

Ticket: WCI-1358
@Marzooqa
Marzooqa force-pushed the marzooqakather498/wci-1358-bitgojs-add-enterprise-wallettype-params-to-tsssettings-and branch from d376a1d to 25f747b Compare August 24, 2026 10:53
@Marzooqa

Copy link
Copy Markdown
Contributor Author

@claude

@github-actions

github-actions Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Claude finished @Marzooqa's task —— View job


Code Review for WCI-1358

Todo List:

  • Read and analyze all 6 changed files
  • Review implementation against PR description
  • Check backward compatibility
  • Verify test coverage
  • Provide comprehensive feedback

Review Summary

This PR successfully implements forwarding the enterprise parameter through TSS settings and keychain API calls. The changes are well-structured, backward compatible, and properly tested.

Strengths

1. Comprehensive Coverage

  • keychains.ts:377-379: Added enterprise query parameter to TSS settings call in createMpc()
  • wallets.ts: Added enterprise parameter to all 5 TSS settings calls across different wallet creation methods (hot, cold, custodial, external-signer, ECDSA)
  • eddsaMPCv2.ts:309,375,442: Properly threaded enterprise through both user and backup keychain creation paths, fixing the pre-existing gap mentioned in the PR description

2. Backward Compatibility

The implementation correctly maintains backward compatibility:

  • enterprise parameter is optional everywhere
  • When enterprise is undefined, no query parameter is added (not even an empty one)
  • Existing callers without enterprise will produce identical requests

3. Test Coverage

Excellent test coverage with two dedicated behavioral tests in keychains.ts:

  • Test with enterprise parameter: Uses exact query matching ({ enterprise: 'enterprise-123' }) to prove the parameter is actually forwarded
  • Test without enterprise: Verifies no query parameters are sent when omitted
  • Updated existing tests to use .query(true) for flexibility where exact matching isn't the focus

🔍 Minor Observations

1. Consistent Pattern

The implementation follows a consistent pattern across all files:

.query({ enterprise: params.enterprise })

This is clean and handles the undefined case properly since query() ignores undefined values.

2. EddsaMPCv2 Fix

The fix in eddsaMPCv2.ts:309 addresses the pre-existing gap where backup keychains weren't receiving the enterprise parameter:

// Before: only user keychain got enterprise implicitly
// After: both user and backup keychains explicitly get enterprise
keychains.add({ source: 'user', keyType: 'tss' as KeyType, commonKeychain, isMPCv2: true, enterprise }),
keychains.add({ source: 'backup', keyType: 'tss' as KeyType, commonKeychain, isMPCv2: true, enterprise }),

3. Test Quality

The new tests in keychains.ts:604-634 are particularly well-designed:

  • They use exact query matching instead of loose matchers to prove the fix
  • They test both presence and absence of the parameter
  • They're focused and minimal

🎯 Recommendations

No Issues Found

This is a clean, well-implemented change that:

  • ✅ Matches the PR description exactly
  • ✅ Maintains full backward compatibility
  • ✅ Includes comprehensive test coverage
  • ✅ Follows existing code patterns
  • ✅ Fixes the identified pre-existing gap

Final Assessment

✅ APPROVE - This PR is ready to merge. The implementation is correct, well-tested, and maintains backward compatibility while successfully forwarding the enterprise parameter through the TSS/keychain workflow.


View Job Run | View Branch

@Marzooqa
Marzooqa marked this pull request as ready for review August 25, 2026 08:53
@Marzooqa
Marzooqa requested review from a team as code owners August 25, 2026 08:53
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.

2 participants