Wp 46222/add api ts #5

mohammadalfaiyazbitgo · 2025-06-02T02:39:11Z

No description provided.

Copilot

Pull Request Overview

This PR introduces a declarative API layer for Master Express mode using @api-ts/express-wrapper, updates the wallet generation flow to match the new handler signature, and adds OpenAPI definitions.

Refactor masterExpressApp.ts to use a spec-driven router and handlers (handlePingEnclavedExpress, handleGenerateWallet).
Add masterExpressApi.ts definitions and generate masterExpressApi.openapi.json.
Migrate on-prem wallet generation logic in generateWallet.ts and adjust endpoint paths and type casts.

Reviewed Changes

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

Show a summary per file

File	Description
src/types.ts	Updated comment for MasterExpress API settings
src/masterExpressApp.ts	Refactored routes to use `api-ts` wrapper, added new handlers
src/masterExpressApi.ts	Defined io-ts types and HTTP routes spec for MasterExpressApi
src/masterExpressApi.openapi.json	Generated OpenAPI spec from TypeScript definitions
src/masterBitgoExpress/generateWallet.ts	Migrated `handleGenerateWalletOnPrem` signature and implementation
src/masterBitgoExpress/enclavedExpressClient.ts	Updated independent keychain endpoint path
src/errors.ts	Refined error comment for API response error class
src/config.ts	Aligned config comments for MasterExpress API settings
package.json	Added scripts/deps for API generation and `express-wrapper`
cert.key	Added SSL key for testing

Comments suppressed due to low confidence (7)

src/masterExpressApi.ts:82

The TODO for filling out the full response type indicates an incomplete API specification; please define the full WalletGenerateResponse schema for better API contract clarity.

// TODO: Fill out full response type

src/errors.ts:20

[nitpick] The class comment suggests ApiResponseError is only for MasterExpressApi responses, but it may be used more broadly; consider generalizing the comment or renaming for consistency.

* Error for MasterExpressApi responses

src/masterExpressApi.ts:58

[nitpick] The constant enclavedPingResponse uses lowercase naming while other response types use PascalCase; consider renaming to EnclavedPingResponse for consistency.

export const enclavedPingResponse = t.type({

src/masterExpressApp.ts:166

New route handler handlePingEnclavedExpress lacks accompanying unit or integration tests; please add tests to cover its success and error paths.

const handlePingEnclavedExpress = async () => {

src/masterExpressApp.ts:172

The code uses POST for the enclaved express ping endpoint, but the ping API likely expects a GET request; please verify and revert to GET if necessary.

.post(`${cfg.enclavedExpressUrl}/ping`)

src/masterExpressApp.ts:251

These console.log calls expose sensitive configuration (including SSL certificates) and are not ideal for production; please remove them or replace with appropriate debugLogger usage and avoid logging secrets.

console.log('SSL Enabled:', cfg.enableSSL);

package.json:18

The script uses openapi-generator, but the installed package is @api-ts/openapi-generator which may provide a different CLI command; please verify and correct the script to use the correct generator binary.

"generate-openapi": "npx openapi-generator ./src/masterExpressApi.ts > src/masterExpressApi.openapi.json"

Copilot · 2025-06-03T19:09:01Z

src/masterBitgoExpress/generateWallet.ts

  };

-  return { ...result, wallet: result.wallet.toJSON() };
+  return result as unknown as WalletWithKeychains;


This double type cast (unknown to WalletWithKeychains) weakens type safety; consider updating the SDK types or refining the return type to avoid the unknown cast.

Suggested change

return result as unknown as WalletWithKeychains;

return result;

pranavjain97

Overall nice work on converting these to api-ts. Should be super helpful in the future!

pranavjain97 · 2025-06-03T19:44:21Z

cert.key

+mmrPvk+Fofxm52+BBmhPMREi3Xu/nc+oGoB2fe+1DLm4rg529ka4GfmjErsYJ1ip
+fg+TBM6j4D+W1pyPmPFTXjL16MGg0zYFcIcuaZBmD1WP1Ojz2uZt5WaPsapuXTi/
+rkA/bbzrGAE65dSaZDx58k0=
+-----END PRIVATE KEY-----


what is this for? we already have the test cert's

pranavjain97 · 2025-06-03T19:45:36Z

src/masterBitgoExpress/enclavedExpressClient.ts

      debugLogger('Creating independent keychain for coin: %s', this.coin);
      const { body: keychain } = await superagent
-        .post(`${this.url}/api/${this.coin}/key/independent`)
+        .post(`${this.url}/${this.coin}/key/independentKey`)


it is /key/independent after based on vivian's API TDD review.

will update

pranavjain97 · 2025-06-03T19:47:34Z

src/masterBitgoExpress/generateWallet.ts

+export type GenerateWalletOnPremParams = {
+  coin: string;
+  label: string;
+  enterprise?: string;
+  multisigType?: string;
+  isDistributedCustody?: boolean;
+};


Also might need walletVersion and gasPrice for ETH. Can be added later

pranavjain97 · 2025-06-03T19:47:54Z

src/masterBitgoExpress/generateWallet.ts

-
-  const enclavedExpressClient = createEnclavedExpressClient(req.params.coin);
+export async function handleGenerateWalletOnPrem({
+  bitGo,


Suggested change

bitGo,

bitgo,

pranavjain97 · 2025-06-03T19:49:09Z

src/masterBitgoExpress/generateWallet.ts

  };

-  return { ...result, wallet: result.wallet.toJSON() };
+  return result as unknown as WalletWithKeychains;


why do we need to cast as unknown? we lose the type safety here

pranavjain97 · 2025-06-03T19:49:38Z

src/masterExpressApi.openapi.json

@@ -0,0 +1,300 @@
+{


this is auto-generated correct?

pranavjain97 · 2025-06-03T19:51:11Z

src/masterExpressApi.ts

+// TODO: Fill out full response type
+export const WalletGenerateRequest = {
+  label: t.string,
+  enterprise: t.string,
+};


You're missing fields here. Also, if we have this, why is export type GenerateWalletOnPremParams also needed?

pranavjain97 · 2025-06-03T19:52:16Z

src/masterExpressApi.ts

+    body: {
+      label: t.string,
+      enterprise: t.string,
+    },


this is the 3rd place the generate wallet req is typed separately, should be using WalletGenerateRequest

pranavjain97 · 2025-06-03T19:56:06Z

src/masterExpressApp.ts

+};
+
+// Add a new handler for wallet generation
+const handleGenerateWallet: ServiceFunction<typeof GenerateWalletRequest> = async ({


Weird to have two handlers for 1 API. Can we merge this with handleGenerateWalletOnPrem?

I agree two handlers is weird but I think the break up of the function makes sense. One would be to validate the request and the second would be to actually perform the orchestration of calls to generate the wallet?

Ticket: WP-4593

mohammadalfaiyazbitgo force-pushed the WP-4593-add-bitgo-generate-wallet branch 2 times, most recently from 9cf361d to 41827fb Compare June 2, 2025 19:34

Base automatically changed from WP-4593-add-bitgo-generate-wallet to master June 2, 2025 20:01

mohammadalfaiyazbitgo force-pushed the WP-46222/add-api-ts branch 4 times, most recently from 8373ee9 to 72f943b Compare June 3, 2025 19:05

mohammadalfaiyazbitgo requested review from Copilot and pranavjain97 June 3, 2025 19:07

Copilot AI reviewed Jun 3, 2025

View reviewed changes

mohammadalfaiyazbitgo force-pushed the WP-46222/add-api-ts branch from 72f943b to 5948e2d Compare June 3, 2025 19:09

mohammadalfaiyazbitgo enabled auto-merge June 3, 2025 19:12

pranavjain97 requested changes Jun 3, 2025

View reviewed changes

feat: moved mbe ping enclave to api-ts

35515b8

Ticket: WP-4593

mohammadalfaiyazbitgo force-pushed the WP-46222/add-api-ts branch from 5948e2d to 35515b8 Compare June 4, 2025 01:43

mohammadalfaiyazbitgo requested a review from pranavjain97 June 4, 2025 01:44

mohammadalfaiyazbitgo marked this pull request as draft June 4, 2025 15:38

auto-merge was automatically disabled June 4, 2025 15:38
Pull request was converted to draft

mohammadalfaiyazbitgo closed this Jun 11, 2025

	return result as unknown as WalletWithKeychains;
	return result;

Wp 46222/add api ts #5

Wp 46222/add api ts #5

Uh oh!

Conversation

mohammadalfaiyazbitgo commented Jun 2, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

pranavjain97 left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants