feat: implement BitGo signing in SDK#8624
Conversation
alextse-bg
force-pushed
the
WCN-217
branch
2 times, most recently
from
8fd8f9f to
3e4c1a5
Compare
alextse-bg
requested review from
kaustubhbitgo,
pranavjain97 and
zahin-mohammad
zahin-mohammad
left a comment
zahin-mohammad
left a comment
There was a problem hiding this comment.
lgtm, just a few suggestions/comments.
alextse-bg
force-pushed
the
WCN-217
branch
3 times, most recently
from
284ffac to
13f39ae
Compare
allow wallet and coins object to sign using the BitGo key if the passphrase is not provided during signing Ticket: WCN-217-2
When no walletPassphrase is present in the request body or environment, pass undefined to tradingAccount.signPayload() instead of throwing. The SDK routes passphrase-less signing through KMS internally. Ticket: WCN-215-1
Commit 3Make wallet passphrase optional for preapreAllocation Ticket: WCN-216 |
pranavjain97
left a comment
pranavjain97
left a comment
There was a problem hiding this comment.
prv-as-walletPassphrase in ofcToken.signTransaction breaks the user-key signing path from wallet.signTransaction
| let signature: string; | ||
| if (params.wallet) { | ||
| signature = await params.wallet.toTradingAccount().signPayload({ payload, walletPassphrase: params.prv }); | ||
| } else if (params.prv) { |
There was a problem hiding this comment.
params.prv arriving here via wallet.signTransaction is already a decrypted key (from getUserPrv), not a passphrase. passing it as walletPassphrase to signPayload will cause signPayloadByUserKey to use it as a decryption password against encryptedPrv, which won't work
| evmKeyRingReferenceWalletId?: string; | ||
| isParent?: boolean; | ||
| enabledChildChains?: string[]; | ||
| userKeySigningRequired?: string; |
There was a problem hiding this comment.
userKeySigningRequired is typed string but the guard treats it as boolean. "false" from the API would incorrectly block BitGo signing. should be boolean
| if (walletData.keys.length < 2) { | ||
| throw new Error( | ||
| 'Wallet does not support BitGo signing. Please reach out to support@bitgo.com to resolve this issue.' | ||
| ); |
There was a problem hiding this comment.
when params.payload is a string, .send(string) sends a plain text body, not JSON. the user-key path stringifies first, this should match
|
|
||
| async function getEncryptedPrivKey(path: string, walletId: string): Promise<string> { | ||
| const privKeyFile = await fs.readFile(path, { encoding: 'utf8' }); | ||
| const encryptedPrivKey = JSON.parse(privKeyFile); |
There was a problem hiding this comment.
+1 to zahin's thread: collapse findWalletPwFromEnv and getWalletPwFromEnv into one function
There was a problem hiding this comment.
Will update it in a follow up PR
4 participants
Commit 1: allowing trading wallet transaction signing on Trading Account Objects
make wallet passphrase optional when signing OFC transactions.
if not present, the SDK attempts to sign using the wallet's BitGo key instead.
Commit 2: allowing trading wallet transaction signing on Wallet and Coins object
The following are all of the currently valid methods to create a signature on an OFC wallet's
payloadstringofcToken.signMessage({prv}, message): encrypts the message locally using prvofcToken.signTransaction(params): signs a half signed transaction by calling the above methodwallet.baseCoin.signMessage: see abovewallet.baseCoin.signTransaction: see abovewallet.signTransaction(params): signs a half signed transaction by getting the prv through a wallet passphrase then callingthis.baseCoin.signTransactionwallet.prebuildAndSignTransaction(params): builds and sign a transaction by callingwallet.signTransactionwallet.prebuildAndSignTransaction(e.g. sendMany)wallet.toTradingAccount().signPayload: signs a half signed transaction using the wallet passphraseChanges in commit 1 address path 8 already.
For paths that creates the signature using methods of wallet object (i.e. 5-7), all of them eventually calls
wallet.signTransaction, which pass itselfthisas an argument towallet.baseCoin.signTransaction(see here), allowing us to sign via BitGo key if we add the implementation toofcTokenAs for
ofcToken.signMessage, add overloads to the method to allow SDK user to pass in the wallet object instead, which creates the signature via the BitGo key.Note that the
walletPassphraseis already an optional parameter when calling wallet level methods.Ticket: WCN-217