Skip to content
This repository has been archived by the owner on Oct 30, 2024. It is now read-only.

Update dependencies #25

Merged
merged 5 commits into from
Feb 9, 2018
Merged

Update dependencies #25

merged 5 commits into from
Feb 9, 2018

Conversation

axic
Copy link
Member

@axic axic commented Feb 3, 2018

Fixes #24.

Still missing:

@coveralls
Copy link

coveralls commented Feb 3, 2018

Coverage Status

Coverage remained the same at 86.62% when pulling 47a0001 on update-dependencies into cfe0f60 on master.

@axic axic force-pushed the update-dependencies branch from 08b196b to 47a0001 Compare February 9, 2018 02:14
@axic axic requested a review from holgerd77 February 9, 2018 02:21
Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

Please have a look at the ethereumjs-util comment.

if (!this._hdkey._privateKey) {
throw new Error('Private key is not available')
if (!this._hdkey.privateExtendedKey) {
throw new Error('This is a public key only wallet')
Copy link
Member

Choose a reason for hiding this comment

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

Followed the discussion here a bit, if I got this right (so accessing privateExtendedKey is not raising an exception any more if not available), this makes sense.

@@ -145,7 +145,7 @@ Wallet.prototype.toV3 = function (password, opts) {

return {
version: 3,
id: uuid.v4({ random: opts.uuid || crypto.randomBytes(16) }),
id: uuidv4({ random: opts.uuid || crypto.randomBytes(16) }),
Copy link
Member

Choose a reason for hiding this comment

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

Ok.

decipher.decrypt(encryptedSeed.slice(0, 16)),
decipher.decrypt(encryptedSeed.slice(16, 32))
Buffer.from(decipher.decrypt(encryptedSeed.slice(0, 16))),
Buffer.from(decipher.decrypt(encryptedSeed.slice(16, 32)))
Copy link
Member

Choose a reason for hiding this comment

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

Tested this locally/manually, resulted in the same values with and without Buffer, so I'm not completely getting the reason here, assume this is an extra security measure. Anyway, also makes things a bit more explicit and minimally doesn't hurt.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is a Uint8Array which cannot be implicitly used in Buffer.concat, seemingly.

"mocha": "^2.3.4",
"coveralls": "^3.0.0",
"istanbul": "^0.4.5",
"mocha": "^5.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

Tested by successful test run.

"utf8": "^2.1.1",
"uuid": "^2.0.1"
"utf8": "^3.0.0",
"uuid": "^3.2.1"
Copy link
Member

Choose a reason for hiding this comment

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

Ok.

"bs58check": "^1.0.8",
"ethereumjs-util": "^4.4.0",
"hdkey": "^0.7.0",
"aes-js": "^3.1.0",
Copy link
Member

Choose a reason for hiding this comment

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

Ok, associated with changes below.

"aes-js": "^3.1.0",
"bs58check": "^2.1.1",
"ethereumjs-util": "^5.1.4",
"hdkey": "^0.8.0",
Copy link
Member

Choose a reason for hiding this comment

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

Ok.

"ethereumjs-util": "^4.4.0",
"hdkey": "^0.7.0",
"aes-js": "^3.1.0",
"bs58check": "^2.1.1",
Copy link
Member

Choose a reason for hiding this comment

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

Ok.

"safe-buffer": "^5.1.1",
"scrypt.js": "^0.2.0",
"utf8": "^2.1.1",
"uuid": "^2.0.1"
"utf8": "^3.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

Ok.

"hdkey": "^0.7.0",
"aes-js": "^3.1.0",
"bs58check": "^2.1.1",
"ethereumjs-util": "^5.1.4",
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this from Wallet.fromV1 doesn't need to be adopted due to this byte to bit length change in the sha3() function:

var decipher = crypto.createDecipheriv('aes-128-cbc', ethUtil.sha3(derivedKey.slice(0, 16)).slice(0, 16), Buffer.from(json.Crypto.IV, 'hex'))

(not possible in GitHub to comment on untouched lines, how stupid is that?)

Copy link
Member Author

Choose a reason for hiding this comment

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

ethUtil.sha3(derivedKey.slice(0, 16)) just runs a standard 256bit keccak.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah you mean we may be able to use the 128bits sponge output as it should be identical? I wouldn't do it right now as there is a very sparse test coverage for the V1 wallet (it may also should be removed).

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand anything of what you are assuming of what I could mean. 😸

I actually mis-read the brackets and thought, a width argument would have been passed to the function, so false alarm.

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

Ok, this looks good now.

@holgerd77
Copy link
Member

Will also merge in 5 min or so, since I want to write some tests and would like to do it on top of this.

@axic axic merged commit 01382b1 into master Feb 9, 2018
@axic axic deleted the update-dependencies branch February 9, 2018 10:52
@axic
Copy link
Member Author

axic commented Feb 9, 2018

@holgerd77 cool! Do you want to create a changelog file? I want to release this today and have another changeset afterwards (which i won't include in this release).

@holgerd77
Copy link
Member

Yes, will do.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants