Skip to content

Conversation

danielzhao122
Copy link
Contributor

added keychain codecs to a general codec file, as I assume that these codecs could be reused (but not sure if they will). refactored the UT that covered generateWallet to ensure that encoding and decoding works e2e.

TICKET: WP-5415

@danielzhao122 danielzhao122 requested a review from a team as a code owner September 22, 2025 22:12
@danielzhao122 danielzhao122 force-pushed the WP-5415-express-migrate-api-v2-coin-wallet-generate-to-typed-routes branch from 16a0cd0 to 96dd8d5 Compare September 22, 2025 23:01
@danielzhao122 danielzhao122 requested a review from a team as a code owner September 30, 2025 14:53
const coin = bitgo.coin(req.decoded.coin);
const result = await coin.wallets().generateWallet(req.body);
if (req.query.includeKeychains === 'false') {
if ((req.decoded.includeKeychains as any) === false) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we casting to any here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is because includeKeychains is type BooleanFromString which has output type string so statically it is typed as a string but during runtime it is actually a boolean. I think this is because of how we setup the decoder.

Also includeKeychains is a parameter in the request and httpRequest requires that params are of type string

@danielzhao122 danielzhao122 force-pushed the WP-5415-express-migrate-api-v2-coin-wallet-generate-to-typed-routes branch from fb1254e to 678e891 Compare October 9, 2025 02:42
Comment on lines +16 to +17
/** If absent, BitGo uses the default wallet type for the asset */
multisigType: multisigType,
Copy link
Contributor

Choose a reason for hiding this comment

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

The command and the codec here seem to conflict. The codec makes this a required field. The comment says this should be optional.

Comment on lines +18 to +19
/** The type of wallet, defined by key management and signing protocols. 'hot' and 'cold' are both self-managed wallets. If absent, defaults to 'hot' */
type: walletType,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

/** Wallet label */
label: t.string,
/** Enterprise id. This is required for Ethereum wallets since they can only be created as part of an enterprise */
enterprise: t.string,
Copy link
Contributor

Choose a reason for hiding this comment

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

Enterprise is optional in GenerateWalletOptions

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this might actually be required, but to avoid introducing a breaking change for this change we should map the codec to what GenerateWalletOptions defines.

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