Skip to content

feat: add BitGo TSS MPC signing driver - #2163

Open
ravibitgo wants to merge 1 commit into
canton-network:mainfrom
ravibitgo:signing-bitgo
Open

feat: add BitGo TSS MPC signing driver#2163
ravibitgo wants to merge 1 commit into
canton-network:mainfrom
ravibitgo:signing-bitgo

Conversation

@ravibitgo

Copy link
Copy Markdown

Summary

Adds a new signing driver (@canton-network/core-signing-bitgo) that integrates BitGo's TSS MPC custodial wallets as a Canton signing provider.

  • Key model: A BitGo custodial wallet maps 1:1 to a Canton key. walletId is used as the stable key identifier (publicKey) throughout — wallet creation is the createKey flow, and the walletId is passed as keyIdentifier.id on every signing request.
  • Async signing: signTransaction submits a message signing request (POST /msgrequests) and returns immediately with status: pending. The gateway polls getTransaction until messages[0].state === 'signed', at which point the Ed25519 signature and Canton signer fingerprint are extracted from the signed txHash (a hex-encoded JSON blob).
  • Restart resilience: An in-memory txRequestId → walletId map is populated at sign time for fast lookups. On process restart, the enterprise txrequests endpoint (GET /api/v2/enterprise/{id}/txrequests) serves as a fallback — no Redis required, consistent with other drivers.

Changes

  • core/signing-bitgo/ — new package: BitGoHandler (API client) + BitGoSigningDriver (implements SigningDriverInterface), 61 tests, test scripts, README
  • core/signing-lib/src/config/schema.ts — adds BITGO = 'bitgo' to SigningProvider enum
  • wallet-gateway/remote/src/env.ts + init.ts — wires up BitGo env vars (BITGO_ACCESS_TOKEN, BITGO_API_URL, BITGO_ENTERPRISE_ID, BITGO_COIN) and registers the driver

Design notes for reviewers

  • publicKey === walletId is intentional. Unlike Fireblocks (which returns a real Ed25519 public key), BitGo's MPC model doesn't expose a single public key upfront. The walletId is the stable identifier used to route signing requests.
  • apiVersion=full and latest=true are both required on txrequest fetches. Without full, messages[0].state is absent; without latest, you get a stale version of the txRequest.
  • Message-level state takes precedence: Canton signing is complete when messages[0].state === 'signed', even if the txRequest itself is still in pendingDelivery. This was discovered empirically during integration testing.

Known limitations

  • getTransactions with publicKeys fetches only the first page of txrequests per wallet (no cursor/pagination yet). Fine for current usage; can be followed up.

Test plan

  • yarn test in core/signing-bitgo — 61 tests pass
  • yarn build in core/signing-bitgo
  • Manual connectivity: BITGO_ACCESS_TOKEN= BITGO_ENTERPRISE_ID= npx tsx scripts/test-connectivity.ts
  • Manual sign round-trip: npx tsx scripts/test-sign.ts (submit mode, then check mode)

@ravibitgo
ravibitgo requested a review from a team as a code owner July 17, 2026 10:21
@ravibitgo
ravibitgo force-pushed the signing-bitgo branch 2 times, most recently from 57bc6a9 to c990ec3 Compare July 17, 2026 10:55
Copilot AI review requested due to automatic review settings July 21, 2026 03:40

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Adds a new BitGo-backed signing provider to the Wallet Gateway, enabling asynchronous Canton transaction signing via BitGo TSS MPC custodial wallets, and wires it into the existing signing-provider framework.

Changes:

  • Introduces @canton-network/core-signing-bitgo (BitGo API handler + SigningDriverInterface implementation) with tests, scripts, and README.
  • Extends signing-provider configuration to include SigningProvider.BITGO.
  • Wires BitGo env vars and driver registration into wallet-gateway/remote.

Reviewed changes

Copilot reviewed 15 out of 16 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
yarn.lock Adds BitGo SDK (and related) dependencies and bumps various packages.
wallet-gateway/remote/src/init.ts Registers BitGo signing driver when BITGO_ACCESS_TOKEN is set.
wallet-gateway/remote/src/env.ts Adds BitGo-related environment variable accessors.
wallet-gateway/remote/package.json Adds workspace dependency on @canton-network/core-signing-bitgo.
core/signing-lib/src/config/schema.ts Adds BITGO = 'bitgo' to SigningProvider enum.
core/signing-bitgo/vitest.config.ts Adds Vitest config + coverage thresholds for the new package.
core/signing-bitgo/tsup.config.ts Adds tsup build config for the new package.
core/signing-bitgo/tsconfig.json Adds TypeScript config for the new package.
core/signing-bitgo/src/index.ts Implements BitGoSigningDriver (controller methods, config get/set).
core/signing-bitgo/src/index.test.ts Adds unit tests for BitGoSigningDriver.
core/signing-bitgo/src/bitgo.ts Implements BitGoHandler (BitGo REST calls, tx request formatting).
core/signing-bitgo/src/bitgo.test.ts Adds unit tests for BitGoHandler including state/signature extraction.
core/signing-bitgo/scripts/test-sign.ts Adds a manual submit/check script for signing requests.
core/signing-bitgo/scripts/test-connectivity.ts Adds a manual connectivity script (getKeys/createKey).
core/signing-bitgo/README.md Documents usage, configuration, and operational behavior.
core/signing-bitgo/package.json Defines the new package’s build/test setup and dependencies.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread core/signing-bitgo/src/bitgo.ts
Comment thread core/signing-bitgo/src/bitgo.ts Outdated
Comment thread core/signing-bitgo/src/index.ts
Copilot AI review requested due to automatic review settings July 22, 2026 05:43

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 15 out of 16 changed files in this pull request and generated 3 comments.

Comment thread core/signing-bitgo/src/bitgo.ts
Comment thread core/signing-bitgo/src/bitgo.ts Outdated
Comment thread core/signing-bitgo/README.md Outdated
Copilot AI review requested due to automatic review settings July 22, 2026 05:48

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 15 out of 16 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

core/signing-bitgo/README.md:26

  • The README’s coin auto-detection description doesn’t match the implementation (bitgo.ts defaults to tcanton only when baseUrl includes "bitgo-test.com", otherwise canton). This can mislead users configuring a proxy/non-test URL.
| `coin`         | No       | Canton coin identifier. Auto-detected: `canton` for `*.bitgo.com`, `tcanton` otherwise.                                               |

Comment thread core/signing-bitgo/src/index.ts Outdated
Comment thread core/signing-bitgo/src/bitgo.ts
Comment thread wallet-gateway/remote/src/init.ts
Copilot AI review requested due to automatic review settings July 22, 2026 05:55

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 15 out of 16 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

core/signing-bitgo/src/bitgo.ts:273

  • formatTxRequest can return status='signed' without a signature when txReq.state maps to signed (e.g. delivered/signed) but messages[0].txHash is absent/undefined. This is inconsistent with the driver’s own intent to avoid "signed-without-signature" and can cause the gateway to stop polling without a usable signature. Consider always attempting signature extraction whenever the mapped status is 'signed', and downgrade to 'pending' if extraction fails.
        const mappedStatus: SigningStatus =
            messageState === 'signed'
                ? 'signed'
                : (BITGO_STATE_TO_CANTON[txReq.state] ?? 'pending')
        // Only extract when signed status comes from message-level state (txRequest still in

Comment thread core/signing-bitgo/tsup.config.ts Outdated
Copilot AI review requested due to automatic review settings July 22, 2026 06:11

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 15 out of 16 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

core/signing-bitgo/src/bitgo.ts:275

  • formatTxRequest can return status 'signed' without a signature when txReq.state maps to 'signed' (e.g. delivered/signed) but messages[0].txHash is missing/empty. The current fallback-to-pending guard only triggers when messageState === 'signed', so callers may observe a signed tx with no signature.
        // Prefer message-level state: for Canton message signing, the crypto is complete once
        // messages[0].state === 'signed', even if the txRequest is still in 'pendingDelivery'.
        const messageState = txReq.messages?.[0]?.state
        const mappedStatus: SigningStatus =
            messageState === 'signed'

Comment thread core/signing-bitgo/src/bitgo.ts Outdated
Copilot AI review requested due to automatic review settings July 22, 2026 06:20

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 15 out of 16 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

core/signing-bitgo/src/bitgo.ts:284

  • formatTxRequest can return status: 'signed' without a signature when BitGo reports a terminal txRequest state (delivered/signed) but messages[0].txHash is missing or cannot be parsed. This contradicts the nearby intent comment (“prevents signed-without-signature from reaching the gateway”) and will cause runtime errors in the gateway (it treats signed-without-signature as fatal).
        // Only extract when signed status comes from message-level state (txRequest still in
        // pendingDelivery). When the txRequest itself is terminal (delivered/signed), trust it.
        const signedFromMessage =
            messageState === 'signed' && mappedStatus === 'signed'
        const signedData = signedFromMessage

wallet-gateway/remote/src/init.ts:334

  • The BitGo driver is registered here, but the gateway still rejects SigningProvider.BITGO in core flows: TransactionService.sign() has a provider switch that does not include BITGO (wallet-gateway/remote/src/ledger/transaction-service.ts:70-107), and WalletAllocationService similarly lacks BITGO (wallet-gateway/remote/src/ledger/wallet-allocation/wallet-allocation-service.ts:108-173). As a result, wallets configured with signingProviderId: 'bitgo' will throw "Unsupported signing provider" despite the driver being available.
    if (Env.BITGO_ACCESS_TOKEN()) {
        drivers[SigningProvider.BITGO] = new BitGoSigningProvider({
            accessToken: Env.BITGO_ACCESS_TOKEN()!,
            baseUrl: Env.BITGO_API_URL('https://app.bitgo.com'),
            enterpriseId: Env.BITGO_ENTERPRISE_ID(),

Copilot AI review requested due to automatic review settings July 23, 2026 05:14

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 15 out of 16 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

core/signing-bitgo/src/bitgo.ts:286

  • formatTxRequest may return status: 'signed' without a signature when the txRequest is terminal (state: delivered/signed) but messages[0].state/txHash is missing (or not parseable). Since BitGo’s signature is carried in messages[0].txHash (no dedicated signature field), this can surface a signed transaction that the gateway can’t execute. Consider extracting signed data whenever mappedStatus === 'signed', and falling back to pending if extraction fails (same protection you already have for message-level signed).
        const signedFromMessage =
            messageState === 'signed' && mappedStatus === 'signed'
        const signedData = signedFromMessage
            ? this.extractSignedData(txReq.messages?.[0]?.txHash)
            : {}

wallet-gateway/remote/src/init.ts:334

  • The driver is registered here, but core gateway flows still treat SigningProvider.BITGO as unsupported. For example, wallet-gateway/remote/src/ledger/transaction-service.ts throws in the default branch of sign()/execute() when the provider isn’t one of the existing cases (BITGO isn’t included), and wallet-gateway/remote/src/ledger/wallet-allocation/wallet-allocation-service.ts similarly rejects unknown providers in createWallet()/allocateParty(). As-is, selecting bitgo will fail even though the driver is instantiated.
    if (Env.BITGO_ACCESS_TOKEN()) {
        drivers[SigningProvider.BITGO] = new BitGoSigningProvider({
            accessToken: Env.BITGO_ACCESS_TOKEN()!,
            baseUrl: Env.BITGO_API_URL('https://app.bitgo.com'),
            enterpriseId: Env.BITGO_ENTERPRISE_ID(),

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 20 out of 21 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (4)

core/signing-bitgo/src/index.ts:68

  • signTransaction relies on an in-memory keyMap when only keyIdentifier.publicKey is provided. After a process restart (or in a fresh instance), this map will be empty and the driver will return key_not_found even for valid keys, forcing callers to always provide keyIdentifier.id and undermining restart resilience.

Consider refreshing the keyMap from BitGo (via getKeys()) once on cache miss when publicKey is provided, then retrying the lookup before failing. Also, the current error text implies publicKey is a walletId, but in this driver it is the derived Ed25519 key used to resolve a walletId.

                    // id is the BitGo walletId (preferred); fall back to keyMap lookup by publicKey.
                    const walletId =
                        params.keyIdentifier.id ??
                        this.handler.getWalletId(params.keyIdentifier.publicKey)
                    if (!walletId) {
                        return {
                            error: 'key_not_found',
                            error_description:
                                'The provided key identifier must include an id or publicKey (BitGo walletId).',
                        }
                    }

core/signing-bitgo/src/bitgo.ts:327

  • getKeys() currently returns publicKey: w.id when a BitGo wallet has no keychain. That value is not an Ed25519 public key and will lead to incorrect fingerprints / topology generation if a caller tries to use it, and it also cannot be resolved back to a walletId via keyMap.

Safer behavior is to skip non-TSS / incomplete wallets in this driver’s key listing (or to surface them in a separate, explicitly-marked way).

    async getKeys(): Promise<Key[]> {
        const response = await this.request<{ wallets: BitGoWallet[] }>(
            'GET',
            `/api/v2/wallets?coin=${this.coin}&type=custodial`
        )
        return Promise.all(
            response.wallets.map(async (w) => {
                if (!w.keys[0]) {
                    // Non-TSS or incomplete wallet — return walletId as publicKey fallback.
                    return { id: w.id, name: w.label, publicKey: w.id }
                }

core/signing-bitgo/README.md:11

  • The README states that the BitGo wallet ID becomes the Canton key identifier (publicKey), but createKey() in this package returns id = walletId and publicKey = derived Ed25519 public key. This can confuse integrators and doesn’t match how the gateway/allocator uses the driver.

Update the doc to reflect the actual returned fields and how walletId is used (as keyIdentifier.id).

1. **Key creation** — a BitGo custodial wallet is created per Canton party (`POST /api/v2/{coin}/wallet`). The wallet ID becomes the stable Canton key identifier (`publicKey`).
2. **Sign request** — the Canton transaction is submitted as a message signing request (`POST /api/v2/wallet/{walletId}/msgrequests`) and returns a `txRequestId` immediately with status `pending`.
3. **Polling** — the wallet gateway polls `getTransaction(txRequestId)` until `status === 'signed'`. The Ed25519 signature and Canton signer fingerprint are extracted from the signed txRequest response.

wallet-gateway/remote/src/init.ts:342

  • BitGo is registered whenever BITGO_ACCESS_TOKEN is set, but BITGO_ENTERPRISE_ID materially affects functionality (required for createKey, and for restart-safe txrequest lookup via the enterprise endpoint). With only the access token set, the provider will appear "enabled" but key creation / restart recovery will fail at runtime.

Consider warning explicitly when BITGO_ENTERPRISE_ID is missing so operators understand the limitation.

    if (Env.BITGO_ACCESS_TOKEN()) {
        drivers[SigningProvider.BITGO] = new BitGoSigningProvider({
            accessToken: Env.BITGO_ACCESS_TOKEN()!,
            baseUrl: Env.BITGO_API_URL('https://app.bitgo.com'),
            enterpriseId: Env.BITGO_ENTERPRISE_ID(),
            coin: Env.BITGO_COIN(),
        })

Copilot AI review requested due to automatic review settings July 29, 2026 10:07

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 20 out of 21 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

core/signing-bitgo/src/index.ts:68

  • signTransaction assumes keyIdentifier.publicKey is a BitGo walletId and only consults the in-memory keyMap. After a process restart (or if keyMap was never populated), calls that provide only publicKey will return key_not_found even though the wallet exists. Also, the error_description is misleading: publicKey here is an Ed25519 public key (not a walletId). Consider refreshing keys (via getKeys()) once when the walletId cannot be resolved, and adjust the error text accordingly.
                    // id is the BitGo walletId (preferred); fall back to keyMap lookup by publicKey.
                    const walletId =
                        params.keyIdentifier.id ??
                        this.handler.getWalletId(params.keyIdentifier.publicKey)
                    if (!walletId) {

core/signing-bitgo/README.md:9

  • The README states that the BitGo wallet ID becomes the Canton publicKey, but the implementation returns the derived Ed25519 public key as publicKey and uses the wallet ID as id (see BitGoHandler.createKey() / BitGoSigningDriver.createKey). Please update this to match the actual key model to avoid confusion for integrators.
1. **Key creation** — a BitGo custodial wallet is created per Canton party (`POST /api/v2/{coin}/wallet`). The wallet ID becomes the stable Canton key identifier (`publicKey`).

Copilot AI review requested due to automatic review settings July 30, 2026 06:04

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Not ready to approve

The BitGo driver’s signTransaction path can fail after cold start/restart due to missing walletId resolution (no keyMap refresh), which can break signing in production.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Comments suppressed due to low confidence (5)

core/signing-bitgo/src/bitgo.ts:315

  • getKeys() returns { publicKey: walletId } for wallets without keychains, but it doesn't populate keyMap, so later signing/key lookups by that returned publicKey cannot be resolved. Consider always caching walletId -> walletId for each wallet returned by getKeys() (in addition to derived-key mappings).
            response.wallets.map(async (w) => {
                if (!w.keys[0]) {
                    // Non-TSS or incomplete wallet — return walletId as publicKey fallback.
                    return { id: w.id, name: w.label, publicKey: w.id }
                }

core/signing-bitgo/src/bitgo.ts:202

  • createKey populates keyMap only for the derived Ed25519 public key. If callers follow the documented pattern of using walletId as the stable key identifier, getWalletId(walletId) will still be undefined. Consider also caching walletId -> walletId so keyIdentifier.publicKey can safely be the walletId when needed.

This issue also appears on line 311 of the same file.

        const publicKey = await this.derivePublicKey(keychain.commonKeychain)
        this.keyMapSet(publicKey, wallet.id)
        return { id: wallet.id, name: wallet.label, publicKey }

core/signing-bitgo/src/index.ts:68

  • signTransaction only resolves the BitGo walletId via the in-memory keyMap. After a process restart (or any cold start), keyMap will be empty, so signing with only keyIdentifier.publicKey will return key_not_found even for valid wallets. Consider refreshing keyMap from BitGo (via getKeys()) once on cache miss and retrying resolution.
                    // id is the BitGo walletId (preferred); fall back to keyMap lookup by publicKey.
                    const walletId =
                        params.keyIdentifier.id ??
                        this.handler.getWalletId(params.keyIdentifier.publicKey)
                    if (!walletId) {
                        return {
                            error: 'key_not_found',
                            error_description:
                                'The provided key identifier must include an id or publicKey (BitGo walletId).',
                        }
                    }

core/signing-bitgo/README.md:9

  • The README states that the BitGo walletId becomes the Canton publicKey, but the driver currently derives an Ed25519 public key from the wallet keychain and returns the walletId separately as key.id. Please align the documentation with the actual Key fields to avoid confusion for integrators.
1. **Key creation** — a BitGo custodial wallet is created per Canton party (`POST /api/v2/{coin}/wallet`). The wallet ID becomes the stable Canton key identifier (`publicKey`).

wallet-gateway/remote/src/init.ts:338

  • BitGo is enabled when BITGO_ACCESS_TOKEN is set, but createKey requires BITGO_ENTERPRISE_ID and restart-safe tx lookup also relies on enterpriseId. Consider logging a dedicated warning when BITGO_ENTERPRISE_ID is missing so operators understand wallet creation / restart recovery limitations up front.
    if (Env.BITGO_ACCESS_TOKEN()) {
        drivers[SigningProvider.BITGO] = new BitGoSigningProvider({
            accessToken: Env.BITGO_ACCESS_TOKEN()!,
            baseUrl: Env.BITGO_API_URL('https://app.bitgo.com'),
            enterpriseId: Env.BITGO_ENTERPRISE_ID(),
  • Files reviewed: 20/21 changed files
  • Comments generated: 0 new
  • Review effort level: Low

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

Copilot AI review requested due to automatic review settings July 30, 2026 09:26

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Human review recommended

It introduces a new external signing provider integration and API client behavior whose correctness and production operational characteristics warrant final human review.

Review details

Comments suppressed due to low confidence (3)

core/signing-bitgo/src/index.ts:74

  • The key_not_found error_description is misleading: publicKey may be an Ed25519 key that can be resolved to a BitGo walletId via getKeys() (not necessarily a walletId itself). Clarifying the message helps callers supply the correct identifier(s) and reduces confusion during restarts/cold-start keyMap refresh.
                    if (!walletId) {
                        return {
                            error: 'key_not_found',
                            error_description:
                                'The provided key identifier must include an id or publicKey (BitGo walletId).',
                        }

core/signing-bitgo/src/bitgo.ts:172

  • derivePublicKey dynamically imports @bitgo/sdk-lib-mpc and calls Ed25519Bip32HdTree.initialize() on every invocation. Since getKeys() may call this once per wallet, this leads to repeated (potentially expensive) initialization work. Consider caching the imported module and initialized tree and reusing them across calls.
    private async derivePublicKey(commonKeychain: string): Promise<string> {
        const {
            Ed25519Bip32HdTree,
            bigIntFromBufferLE,
            bigIntFromBufferBE,
            bigIntToBufferLE,
        } = await import('@bitgo/sdk-lib-mpc')
        const tree = await Ed25519Bip32HdTree.initialize()

core/signing-bitgo/src/bitgo.ts:330

  • getKeys() uses Promise.all(response.wallets.map(...)), which will fire one keychain fetch per wallet concurrently. In environments with many custodial wallets this can create large request bursts and increase the risk of hitting BitGo rate limits/timeouts. Consider iterating sequentially (or applying a concurrency limit) to keep request volume bounded.
    async getKeys(): Promise<Key[]> {
        const response = await this.request<{ wallets: BitGoWallet[] }>(
            'GET',
            `/api/v2/wallets?coin=${this.coin}&type=custodial`
        )
        return Promise.all(
            response.wallets.map(async (w) => {
                if (!w.keys[0]) {
                    // Non-TSS or incomplete wallet — walletId serves as both id and publicKey.
                    this.keyMapSet(w.id, w.id)
                    return { id: w.id, name: w.label, publicKey: w.id }
                }
                const keychain = await this.request<BitGoKeychain>(
  • Files reviewed: 20/21 changed files
  • Comments generated: 0 new
  • Review effort level: Low

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

Copilot AI review requested due to automatic review settings July 30, 2026 09:33

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Human review recommended

It introduces a new external signing integration and operational flow changes that require careful human validation of correctness, security, and real-world BitGo API behavior.

Review details

Comments suppressed due to low confidence (1)

core/signing-bitgo/src/index.ts:75

  • The key_not_found error message implies publicKey must be a BitGo walletId, but this driver also supports resolving an Ed25519 public key to a walletId via the internal key map (including a cold-start refresh via getKeys()). This message is misleading for callers and makes debugging harder.
                    // id is the BitGo walletId (preferred); fall back to keyMap lookup by publicKey.
                    let walletId =
                        params.keyIdentifier.id ??
                        this.handler.getWalletId(params.keyIdentifier.publicKey)
                    if (!walletId && params.keyIdentifier.publicKey) {
                        // keyMap may be empty after a restart — refresh once and retry.
                        await this.handler.getKeys()
                        walletId = this.handler.getWalletId(
                            params.keyIdentifier.publicKey
                        )
                    }
                    if (!walletId) {
                        return {
                            error: 'key_not_found',
                            error_description:
                                'Could not resolve a BitGo walletId from the provided keyIdentifier. Pass keyIdentifier.id (walletId) directly, or keyIdentifier.publicKey (Ed25519 base64) so the driver can look it up via keyMap.',
                        }
  • Files reviewed: 20/21 changed files
  • Comments generated: 0 new
  • Review effort level: Low

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

Copilot AI review requested due to automatic review settings July 30, 2026 09:40

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Not ready to approve

The BitGo driver currently overloads Transaction.publicKey with walletId, which is inconsistent with Key.publicKey semantics and risks breaking consumers that correlate transactions back to keys by public key.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Comments suppressed due to low confidence (1)

core/signing-bitgo/src/bitgo.ts:423

  • formatTxRequest sets publicKey to the BitGo walletId. In this driver, Key.publicKey is the derived Ed25519 public key (used elsewhere for party allocation / fingerprint), so returning walletId in the Transaction.publicKey field makes it hard for consumers to correlate transactions back to keys and is inconsistent with other signing drivers.

Consider not overloading publicKey here: omit it (or only set it when you can provide the derived Ed25519 public key), and expose walletId via metadata instead.

        return {
            txId,
            status,
            ...(signature !== undefined && { signature }),
            publicKey: walletId,
  • Files reviewed: 20/21 changed files
  • Comments generated: 0 new
  • Review effort level: Low

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

Signed-off-by: Ravi Hegde <ravihegde348@bitgo.com>
Copilot AI review requested due to automatic review settings July 31, 2026 06:01

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Not ready to approve

The gateway persists and passes BitGo key identifiers in a way that appears to contradict the PR’s stated “walletId-as-stable-identifier” design, which can degrade restart behavior and correctness unless aligned.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (3)

wallet-gateway/remote/src/ledger/wallet-allocation/signing-providers/bitgo-wallet-allocator.ts:86

  • PR description says BitGo walletId should be the stable key identifier used on signing requests. This allocator persists publicKey: key.publicKey (derived Ed25519) instead of the BitGo walletId (key.id), which prevents the gateway from reliably passing walletId on future signing requests (and forces expensive keyMap/getKeys fallback after restarts). If walletId-as-publicKey is the intended design, store key.id here and keep using key.publicKey only for fingerprint/topology generation.
            signingProviderId: SigningProvider.BITGO,
            networkId: network.id,
            primary,
            publicKey: key.publicKey,
            externalTxId: txId,

wallet-gateway/remote/src/ledger/transaction-service.ts:737

  • signWithBitgo currently calls signTransaction with only keyIdentifier.publicKey. If the BitGo integration is meant to use walletId as the stable identifier (per PR description / driver docs), the gateway should pass that walletId as keyIdentifier.id to avoid reliance on the driver's in-memory keyMap and the restart-time getKeys() refresh path.
                .signTransaction({
                    tx: tx.preparedTransaction,
                    txHash: tx.preparedTransactionHash,
                    keyIdentifier: {
                        publicKey: wallet.publicKey,
                    },
                })

wallet-gateway/remote/src/ledger/wallet-allocation/wallet-allocation-service.test.ts:1526

  • These BitGo tests stub the BitGo driver using a helper named createDfnsDriver, which is misleading and makes it harder to understand what behavior is actually under test (and risks BitGo-specific defaults drifting from Dfns defaults over time). Consider introducing a provider-agnostic driver stub (or a createBitGoDriver wrapper) and using that here.
        it('createWallet returns initialized when signTransaction returns pending', async () => {
            const serviceWithBitGo = createService({
                [SigningProvider.BITGO]: createDfnsDriver({
                    signTransactionResult: { status: 'pending', txId: 'tx-1' },
                    getTransactionResult: { status: 'pending', txId: 'tx-1' },
                }),
  • Files reviewed: 20/21 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

txId,
status,
...(signature !== undefined && { signature }),
publicKey: walletId,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Still returns walletId instead of public key

coin: this.coin,
type: 'custodial',
multisigType: 'tss',
...(this.enterpriseId && { enterprise: this.enterpriseId }),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There is an early check for falsy this.enterpriseId

Suggested change
...(this.enterpriseId && { enterprise: this.enterpriseId }),
enterprise: this.enterpriseId.

async getKeys(): Promise<Key[]> {
const response = await this.request<{ wallets: BitGoWallet[] }>(
'GET',
`/api/v2/wallets?coin=${this.coin}&type=custodial`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this returns paginated results with next page cursor nextBatchPrevId. If that's true then it should concatenate results from all pages.

txRequests: BitGoTxRequest[]
}>(
'GET',
`/api/v2/wallet/${walletId}/txrequests?apiVersion=full&latest=true`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also here in API docs it's stated that it returns paginated results. We need to traverse all pages.

const keys: Key[] = []
for (const w of response.wallets) {
if (!w.keys[0]) {
// Non-TSS or incomplete wallet — walletId serves as both id and publicKey.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it would be better to omit wallets without public key in response and cache, instead of using walletId as public key fallback.

keychain.commonKeychain
)
this.keyMapSet(publicKey, w.id)
this.keyMapSet(w.id, w.id)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would remove adding walletId -> walletId mapping from cache everywhere to save half of space and only query the map by publicKey. walletId and publicKey should not be interchangable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants