Skip to content

Conversation

@lokesh-bitgo
Copy link
Contributor

Ticket: WP-6309

Copy link
Contributor

@kisslove-dewangan kisslove-dewangan left a comment

Choose a reason for hiding this comment

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

lgtm

@lokesh-bitgo lokesh-bitgo marked this pull request as ready for review October 13, 2025 15:01
@lokesh-bitgo lokesh-bitgo requested a review from a team as a code owner October 13, 2025 15:01
Copy link
Contributor

@kaustubhbitgo kaustubhbitgo left a comment

Choose a reason for hiding this comment

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

so params define it as id i see

@lokesh-bitgo lokesh-bitgo merged commit f5dad9f into master Oct 13, 2025
13 checks passed
@mbelshe
Copy link
Contributor

mbelshe commented Oct 13, 2025

Comparing to the breaking fix, this addresses typedRoutes/api/v2/createAddress, but does it need to also address src/clientRoutes (and associated tests) which were also changed previously.

@kaustubhbitgo
Copy link
Contributor

Comparing to the breaking fix, this addresses typedRoutes/api/v2/createAddress, but does it need to also address src/clientRoutes (and associated tests) which were also changed previously.

We use api-ts where we define route definitions under typedRoutes containing an operation id. We then use the operation id to register the route. Reference https://github.com/BitGo/api-ts/tree/master/packages/typed-express-router

@lokesh-bitgo
Copy link
Contributor Author

Comparing to the breaking fix, this addresses typedRoutes/api/v2/createAddress, but does it need to also address src/clientRoutes (and associated tests) which were also changed previously.

The clientRoutes.ts handler and its associated tests are already consistent with this fix. Both were already using id (not walletId):

  • Handler (clientRoutes.ts:657): Uses req.decoded.id ✅
  • Tests (test/unit/clientRoutes/createAddress.ts:34,42): Mock data uses id field ✅

This PR only fixes the typed route path definition to match what the handler and tests were already expecting. No additional changes to clientRoutes or tests are needed.

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.

6 participants