[Key Manager] CryptoJS.AES #9
Conversation
There was a problem hiding this comment.
Do we plan to offer multiple encryption options? I think that makes it a lot more complicated...
There was a problem hiding this comment.
No, I am just testing our options at the moment.
However, I do think that having the interface and some indication of the encryption algorithm used would be good for upgrades and backwards compatibility.
|
|
||
| await keyManager.encryptAndSetKey(ethWallet, privateKey); | ||
|
|
||
| const decryptedKey = await keyManager.retrieveAndDecryptKey({ |
There was a problem hiding this comment.
Let's make this API "symmetrical", so if we pass EthWallet to encryptAndSet, we should get back EthWallet from retrieveAndDecrypt, same for the XSalsa encryption
There was a problem hiding this comment.
This is a good idea. Lets see if there is a way to construct an EthWallet object from just a private key.
There was a problem hiding this comment.
I will have to do a bit here to determine exactly what should be passed in and out of this function.
calling ethers.Wallet.createRandom() comes with a lot more info that just new ethers.Wallet(privateKey)
Example
Before HDNodeWallet {
provider: null,
address: '0xF3ddfF45A25C9464c0489dD88dbF74BbF18Bf175',
publicKey: '0x02a7f5e094f538b2676cc306e536035880bd24d785073769a4a9c862e31700d6da',
fingerprint: '0xe6c847e5',
parentFingerprint: '0x60194bc1',
mnemonic: Mnemonic {
phrase: 'leader film divide garage trust venture produce when remind become clog skate',
password: '',
wordlist: LangEn { locale: 'en' },
entropy: '0x7e8ac8ffafbe9be4aaefd1b5c27cae65'
},
chainCode: '0xfe609a47f6a94ebbdedc28df21d285e883616fef683d0f841478e788f4880a4b',
path: "m/44'/60'/0'/0/0",
index: 0,
depth: 5
}
After Wallet {
provider: null,
address: '0xF3ddfF45A25C9464c0489dD88dbF74BbF18Bf175'
}
There was a problem hiding this comment.
What might be better is not passing ethers.Wallet object, but merely the privateKey. Then we can easily assert the length and format as well. Currently the decryption method returns only the privateKey anyway.
There was a problem hiding this comment.
I think this suggestion would be more suitable to change after this PR is merged (since everything is working as expected and its going to be a bigger change).
There was a problem hiding this comment.
Please see #10 for changes related to this.
There was a problem hiding this comment.
Since the linked PR was approved, I will assume this is ok too.
Here is an example using a different encryption engine. This one does not make use of a nonce.
Closes #6
Test Plan
New e2e test.