Skip to content

Remove eth-ecies lib#27

Merged
vrotmanh merged 4 commits into
masterfrom
LP342/fix/removeEthEciesLib
Jul 25, 2018
Merged

Remove eth-ecies lib#27
vrotmanh merged 4 commits into
masterfrom
LP342/fix/removeEthEciesLib

Conversation

@vrotmanh
Copy link
Copy Markdown
Contributor

@vrotmanh vrotmanh commented Jul 24, 2018

No description provided.

@godfreyhobbs godfreyhobbs requested a review from anujshah108 July 24, 2018 20:45
godfreyhobbs
godfreyhobbs reviewed Jul 24, 2018
View reviewed changes
Comment thread src/util.js
*/
const encrypt = (pubKeyTo, plaintext, opts) =>
ecies.encrypt(toBuffer(pubKeyTo), toBuffer(plaintext), opts);
const encrypt = async (pubKeyTo, plaintext) => {
Copy link
Copy Markdown
Contributor

@godfreyhobbs godfreyhobbs Jul 24, 2018

Choose a reason for hiding this comment

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

is padding added to the plaintext? What if you encrypt a single byte?

Copy link
Copy Markdown
Contributor Author

@vrotmanh vrotmanh Jul 25, 2018

Choose a reason for hiding this comment

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

It's randomized in many places. If you encrypt the same text with the same key you will get a totally different encrypted output every time.

Copy link
Copy Markdown
Contributor Author

@vrotmanh vrotmanh Jul 25, 2018

Choose a reason for hiding this comment

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

I verified that it is happening

godfreyhobbs
godfreyhobbs reviewed Jul 24, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@godfreyhobbs godfreyhobbs left a comment

Choose a reason for hiding this comment

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

Can you add test for failure cases.
pt is empty, too short
key is empty, too short, not hex

@godfreyhobbs
Copy link
Copy Markdown
Contributor

godfreyhobbs commented Jul 24, 2018 •
edited
Loading

Also please consider what Dan mentioned in the following:
MetaMask/eth-sig-util#18 (comment)

TLDR
Some future proofing via @param {string} version - A unique string identifying the decryption strategy.:

* Decrypts some encrypted data.
* @param {Account} account - The account to decrypt with
* @param {string} version - A unique string identifying the decryption strategy.
* @param {Object} data - The data to decrypt
* @param {Function} callback - The function to call back when decryption is complete.
*/
web3.eth.decrypt = function decrypt (account, version, data, callback) { /* implementation */ }
web3.eth.decrypt(account, 'encrypt-eip-1024-v1', data, callback)

I would love it if you could add this. @davidrhodus @MaxBlaushild what do you think? Would could include the A unique string identifying the decryption strategy in the metadata.

MaxBlaushild
MaxBlaushild reviewed Jul 24, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@MaxBlaushild MaxBlaushild left a comment

Choose a reason for hiding this comment

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

Agree with Godfrey on the extra test cases. Will approve on receipt

Comment thread src/util.js Outdated
const encrypt = (pubKeyTo, plaintext, opts) =>
ecies.encrypt(toBuffer(pubKeyTo), toBuffer(plaintext), opts);
const encrypt = async (pubKeyTo, plaintext) => {
pubKeyTo = pubKeyTo.toString('hex')
Copy link
Copy Markdown
Contributor

@MaxBlaushild MaxBlaushild Jul 24, 2018

Choose a reason for hiding this comment

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

new const for new value

Copy link
Copy Markdown
Contributor Author

@vrotmanh vrotmanh Jul 25, 2018

Choose a reason for hiding this comment

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

ok

Comment thread src/util.js Outdated
const decrypt = (privKey, encrypted) =>
ecies.decrypt(toBuffer(privKey), toBuffer(encrypted));
const decrypt = async (privKey, encrypted) => {
privKey = privKey.toString('hex')
Copy link
Copy Markdown
Contributor

@MaxBlaushild MaxBlaushild Jul 24, 2018

Choose a reason for hiding this comment

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

Same

Copy link
Copy Markdown
Contributor Author

@vrotmanh vrotmanh Jul 25, 2018

Choose a reason for hiding this comment

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

ok

@MaxBlaushild
Copy link
Copy Markdown
Contributor

MaxBlaushild commented Jul 24, 2018

@godfreyhobbs i like that a lot! i think implementing the identifiers should be handled in a separate ticket to keep complexity to a minimum though

anujshah108
anujshah108 previously approved these changes Jul 25, 2018
View reviewed changes
@anujshah108 anujshah108 dismissed their stale review July 25, 2018 14:38

Approved prematurely

MaxBlaushild
MaxBlaushild approved these changes Jul 25, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@MaxBlaushild MaxBlaushild left a comment

Choose a reason for hiding this comment

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

Dope! LGTM

@vrotmanh vrotmanh merged commit 405a89f into master Jul 25, 2018
@godfreyhobbs godfreyhobbs deleted the LP342/fix/removeEthEciesLib branch August 7, 2018 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers

@godfreyhobbs godfreyhobbs godfreyhobbs left review comments

@MaxBlaushild MaxBlaushild MaxBlaushild approved these changes

@anujshah108 anujshah108 anujshah108 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@vrotmanh @godfreyhobbs @MaxBlaushild @anujshah108