refactor: wallet generation handler to use new sdk callback#234
Conversation
danielpeng1
force-pushed
the
WCN-684/wallet-generation-with-callback
branch
from
d50953c to
a02f341
Compare
danielpeng1
force-pushed
the
WCN-684/wallet-generation-with-callback
branch
from
a02f341 to
c74f277
Compare
danielpeng1
requested review from
kisslove-dewangan,
pranavjain97,
pranishnepal and
rajangarg047
| n: 3, | ||
| keys: [], | ||
| type: 'advanced', | ||
| type: 'advanced' as 'cold', |
There was a problem hiding this comment.
why is 'advanced' cast to 'cold' -- does generateWallet not accept 'advanced' in its type union?
There was a problem hiding this comment.
yeah SDK types allow type?: 'hot' | 'cold' | 'custodial' | 'trading' while our API expects type: 'advanced'
would adding advanced to the sdk types be more preferred?
There was a problem hiding this comment.
would adding advanced to the sdk types be more preferred
yeah, imo, we should add it, as it's a valid wallet type now! 🤔
| awmBackupClient: AdvancedWalletManagerClient, | ||
| ): CreateKeychainCallback { | ||
| return async ({ source, coin }) => { | ||
| const client = source === 'user' ? awmUserClient : awmBackupClient; |
There was a problem hiding this comment.
any non-'user' source (including 'bitgo') silently routes to the backup client -- should this throw on unexpected sources?
There was a problem hiding this comment.
for the generateWalletWithExternalSigner i created the sdk only invokes the callback for user and backup keys; although you're right that I think this is still worth an explicit guard to throw
There was a problem hiding this comment.
throwing on any other source now
| awmBackupClient: AdvancedWalletManagerClient, | ||
| ): CreateKeychainCallback { | ||
| return async ({ source, coin }) => { | ||
| const client = source === 'user' ? awmUserClient : awmBackupClient; |
There was a problem hiding this comment.
you can use KeySource.USER here, we have a lot of those hard coded things from the past, but we can do better going forward 😅
There was a problem hiding this comment.
done; maybe we can change them all sometime
There was a problem hiding this comment.
I created a short test file for just wallet gen callbacks, avoids clogging up generateWallet.test.ts
Remove all conflict markers and keep both approaches: - Callback approach from PR #234 for sync onchain path - getBaseWalletParams for TSS and worker paths Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
3 participants
Refactored onchain wallet generation to delegate key creation to the SDK via
createKeychainCallbackcreateOnchainKeyGenCallbackwrapping AWMcreateIndependentKeychainfor user/backup keyshandleGenerateOnChainWalletwithgenerateWallet({ createKeychainCallback })generateWallet.test.tsnocks for SDK behaviour.generateWallet.integ.test.tsas well. Tests all pass (npm test,npm run docker:test:integration)@bitgo-beta/sdk-apiand@bitgo-beta/sdk-coreto include the sdk callback support change.Ticket: WCN-684