-
Notifications
You must be signed in to change notification settings - Fork 262
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: encrypt and return backup key by default #4551
base: master
Are you sure you want to change the base?
Conversation
a1a7a66
to
f1a1e80
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@davidkaplanbitgo has identified that these actions will be breaking changes per the ticket information. Commit needs to reflect that if it is the case.
TICKET: BTC-1174 BREAKING CHANGE: changes the default behavior of generateWallet
f67a2c2
to
2d9be25
Compare
@@ -264,6 +264,9 @@ export class Keychains implements IKeychains { | |||
// if the provider is undefined, we generate a local key and add the source details | |||
const key = this.create(); | |||
_.extend(params, key); | |||
if (params.passphrase !== undefined) { | |||
_.extend(params, { encryptedPrv: this.bitgo.encrypt({ input: key.prv, password: params.passphrase }) }); | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
checking line 273, do we want to return the prv and the encryptedPrv ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the params defined here and above get sent to WP via line 272.
line 273 annotates the response from WP with the prv, so the PRV doesn't actually get sent to WP.
@@ -435,7 +436,7 @@ export class Wallets implements IWallets { | |||
throw new Error('cannot generate backup keypair without passphrase'); | |||
} | |||
// No provided backup xpub or address, so default to creating one here | |||
return this.baseCoin.keychains().createBackup({ reqId }); | |||
return this.baseCoin.keychains().createBackup({ reqId, passphrase: params.passphrase }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the only call-site we need to update?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm so far, but lets add some tests as well
TICKET: BTC-1174