feat(mbe): use generated client for type safety by mohammadalfaiyazbitgo · Pull Request #32 · BitGo/advanced-wallets

mohammadalfaiyazbitgo · 2025-06-18T01:41:53Z

Ticket: WP-000000

Copilot

Pull Request Overview

This PR replaces manual HTTP/TLS logic with a generated, type-safe API client and centralizes health response types.

Introduce shared PingResponseType and VersionResponseType for health endpoints
Remove legacy certificate utilities and inline health route setup
Swap raw superagent calls for buildApiClient-based requests and add version route

Reviewed Changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/types/health.ts	Add shared io-ts types for ping and version responses
src/shared/appUtils.ts	Remove `readCertificates` and `setupHealthCheckRoutes`
src/routes/enclaved.ts	Update mounting of keygen router (drop `/api` prefix)
src/masterBitgoExpress/routers/index.ts	Include `EnclavedExpressApiSpec` in combined spec
src/masterBitgoExpress/routers/healthCheck.ts	Use imported health types and simplify router creation
src/masterBitgoExpress/routers/enclavedExpressHealth.ts	Replace raw HTTPS logic with generated client and add version route
src/masterBitgoExpress/enclavedExpressClient.ts	Build and use a type-safe API client for all enclaved calls
src/enclavedBitgoExpress/routers/enclavedApiSpec.ts	Prefix key/multisig routes with `/api`
src/enclavedBitgoExpress/routers/index.ts	Export consolidated `EnclavedApiSpec`
src/tests/masterBitgoExpress/sendMany.test.ts	Update error assertion for missing enclave config
src/tests/masterBitgoExpress/generateWallet.test.ts	Update error assertion for missing enclave config
masterBitgoExpress.json	Extend OpenAPI with shared health schemas and version endpoint

Comments suppressed due to low confidence (5)

src/enclavedBitgoExpress/routers/index.ts:1

[nitpick] The constant name EnclavedAPiSpec uses inconsistent casing (APi vs Api). Rename it to EnclavedApiSpec for clarity and consistency.

import { EnclavedAPiSpec as ApiSpec } from './enclavedApiSpec';

src/masterBitgoExpress/routers/enclavedExpressHealth.ts:88

The new version endpoint (/version/enclavedExpress) lacks corresponding tests. Consider adding unit or integration tests to verify its behavior.

  router.get('v1.enclaved.version', [

src/routes/enclaved.ts:18

Removing the /api prefix here may break existing routes. Consider mounting the keygen router with a base path, e.g., app.use('/api', createKeyGenRouter(config));.

  app.use(createKeyGenRouter(config));

src/masterBitgoExpress/enclavedExpressClient.ts:153

The client is referencing v1.health.ping but the generated spec defines v1.enclaved.ping. Update the key to match this.apiClient['v1.enclaved.ping'].

      let request = this.apiClient['v1.health.ping'].post({});

src/masterBitgoExpress/enclavedExpressClient.ts:176

The client is referencing v1.health.version but the spec defines v1.enclaved.version. Update this to this.apiClient['v1.enclaved.version'].

      let request = this.apiClient['v1.health.version'].get({});

Ticket: WP-000000

pranavjain97 · 2025-06-18T14:54:50Z

+      expressApp(invalidConfig as MasterExpressConfig);
+      assert(false, 'Expected error to be thrown when enclaved express client is not configured');
+    } catch (e) {
+      (e as Error).message.should.equal('enclavedExpressUrl and enclavedExpressCert are required');


wait im v confused. why is this removing the calls to the actual API?

because the client is constructed when the routes are configured, so it'll fail here: https://github.com/BitGo/enclaved-bitgo-express/blob/3cb0dc2f3cee8eee5b5644abbc9360f9847379e5/src/masterBitgoExpress/enclavedExpressClient.ts#L62

what i mean is, how is this test defining the client to make calls to generate wallet?

for this case (when the config is invalid), the app won't startup. I can modify it so that the client is only created during the request, so it would fail then.

pranavjain97 · 2025-06-18T14:55:14Z

-        coin: coin,
-        type: 'independent',


why is this removed?

because it's not used in the actual request. Look at the Api spec for Enclaved Express.

'v1.key.independent': { post: httpRoute({ method: 'POST', path: '/api/{coin}/key/independent', request: httpRequest({ params: { coin: t.string, }, body: IndependentKeyRequest, }), response: IndependentKeyResponse, description: 'Generate an independent key', }), },

pranavjain97 · 2025-06-18T14:55:39Z

+    try {
+      expressApp(invalidConfig as MasterExpressConfig);
+      assert(false, 'Expected error to be thrown when enclaved express client is not configured');
+    } catch (error) {
+      (error as Error).message.should.equal(
+        'enclavedExpressUrl and enclavedExpressCert are required',
+      );
+    }


?? why is this just calling the app?

pranavjain97 · 2025-06-18T15:03:05Z

-        let response;
-        if (cfg.tlsMode === TlsMode.MTLS) {
-          // Use Master Express's own certificate as client cert when connecting to Enclaved Express
-          const httpsAgent = new https.Agent({
-            rejectUnauthorized: !cfg.allowSelfSigned,
-            ca: cfg.enclavedExpressCert,
-            // Provide client certificate for mTLS
-            key: cfg.tlsKey,
-            cert: cfg.tlsCert,
-          });


where are the cert's being passed with the api client?

https://github.com/BitGo/enclaved-bitgo-express/blob/3cb0dc2f3cee8eee5b5644abbc9360f9847379e5/src/masterBitgoExpress/enclavedExpressClient.ts#L117

pranavjain97 · 2025-06-18T15:04:07Z

-export async function readCertificates(
-  keyPath: string,
-  crtPath: string,
-): Promise<{ key: string; cert: string }> {
-  const privateKeyPromise = fs.promises.readFile(keyPath, 'utf8');
-  const certificatePromise = fs.promises.readFile(crtPath, 'utf8');
-  const [key, cert] = await Promise.all([privateKeyPromise, certificatePromise]);
-  return { key, cert };
-}


where are the cert's being read now..?

https://github.com/BitGo/enclaved-bitgo-express/blob/eb46240f4e40d1e72f359d5ad46344aed300b151/src/initConfig.ts#L150

I did test if this works with certificates loaded.

mohammadalfaiyazbitgo marked this pull request as ready for review June 18, 2025 01:41

mohammadalfaiyazbitgo requested review from Copilot and pranavjain97 June 18, 2025 01:42

Copilot AI reviewed Jun 18, 2025

View reviewed changes

mohammadalfaiyazbitgo force-pushed the WP-000000/qol-api-specs branch from d8bed21 to 225d143 Compare June 18, 2025 01:59

feat(mbe): use generated client for type safety

3cb0dc2

Ticket: WP-000000

mohammadalfaiyazbitgo force-pushed the WP-000000/qol-api-specs branch from 225d143 to 3cb0dc2 Compare June 18, 2025 02:01

mohammadalfaiyazbitgo enabled auto-merge June 18, 2025 14:41

pranavjain97 requested changes Jun 18, 2025

View reviewed changes

mohammadalfaiyazbitgo requested a review from pranavjain97 June 18, 2025 15:16

pranavjain97 approved these changes Jun 18, 2025

View reviewed changes

mohammadalfaiyazbitgo merged commit 07d987f into master Jun 18, 2025
3 checks passed

mohammadalfaiyazbitgo deleted the WP-000000/qol-api-specs branch June 18, 2025 15:29

Uh oh!

Conversation

mohammadalfaiyazbitgo commented Jun 18, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants