-
Notifications
You must be signed in to change notification settings - Fork 5k
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
back-ports privateKey validation of the 2.x Accounts module to 1.x #3323
Conversation
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.
Ah yes! This is nicer - LGTM.
Left some reformatting suggestions (sorry)
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.
Ah! Just realized this change if causing a couple tests to fail in CI...those probably need to pass or be investigated :)
Otherwise LGTM - this is a nice solution.
Left a few reformatting suggestions (sorry)
…se we do not left pad the private key to 32 bytes in web3.js
@cgewecke I've removed the test vectors with 30 or 31bytes long private keys stored in the keystore v3 JSON file because web3.js does not left-pad the PK to 32bytes as go-ethereum is doing. I think the path of keeping the PK untouched is the right one. WDYT? |
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.
What if you have encrypted with 1.2.4 using a non-standard length, stored the result somewhere and want to decrypt with a later version? The test case suggests the non-standard length should work for this purpose.
Not completely certain what's correct here - have opened #3328 as a possible alternative.
…dded to eth.accounts.encrypt-decrypt test file
@cgewecke I've applied the requested changes and re-added the 30/31 byte test vectors. |
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.
This looks great and improves on the suggestion in #3328.
The test file changes should really come out though, sorry. I think those files should only be changed for logic so the implications of the new code are crystal clear. They don't require "helpful reformatting" - unless done as a separate initiative.
@nivida This was merged over a requested change? |
Description
Thanks to @DalderupMaurice to give me a hint about the more advanced solution in the 2.x branch.
Type of change
Checklist:
npm run dtslint
with success and extended the tests and types if necessary.npm run test:unit
with success and extended the tests if necessary.npm run build-all
and tested the resulting file/'s fromdist
folder in a browser.CHANGELOG.md
file in the root folder.