-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Migrates ECKey to stricter API #139
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
Conversation
src/eckey.js
Outdated
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.
The name makes it pretty clear it needs a buffer. Is this assert really necessary?
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.
A good point, some of these have been artifacts from the migration/testing process.
Though, some of these also produce really misleading errors if the wrong type makes it through.
|
I'm +1. @weilu you're the merge lead on this. |
|
Also, this is really excellent. |
src/eckey.js
Outdated
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.
Shall we make 32 a constant?
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.
Also, an alternative:
var pad = new Buffer(32 - buffer.length)
pad.fill(0)
return Buffer.concat([pad, buffer])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.
I know the copy is much faster in most cases (1 less call to malloc), though I don't know whether it makes a difference here.
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.
I guess copy as opposed to concat saves one instantiation of buffer. Instantiation according to the benchmark is slow.
|
Fixed up the error handling, rebased on HEAD and removed some unnecessary asserts. |
|
Looks good to me. I'm bothered by the fact that |
This pull request migrates the
ECKeyAPI to be stricter and force the user to be explicit about their intent instead of detecting their intent from the parameters.This is done through the following three new functions:
ECKey.fromBuffer,ECKey.fromWIF, andECKey.makeRandomThe default constructor
ECKey()accepts aBigIntegerand compression flag, and is what the other 3 constructors use to instantiate anECKey.A point of discussion is the change in behaviour by the constructor when dealing with keys outside of the
secp256k1keyspace.Previously, any key imported into
ECKeywould be automatically constrained/wrapped:This pull request removes that implicit behaviour and instead asserts that the input is already constrained to be within the key space.
This removes an unnecessary
modoperation, while also stopping confusion from users who might think that0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFis a valid private key.This behaviour still exists in
ECKey.makeRandomas it must constrain the randomly generated key to within these bounds.Although not necessary, I have maintained
toHex/fromHexconvenience wrappers, these exist until we can simplify down the test fixtures enough that there will not be a huge amount ofnew Buffer(..., 'hex')andtoBuffer().toString('hex')modifications just to maintain compatibility.This pull request also removes the incorrect behaviour of
ECKey.toBytes()which would append a0x01compression flag to the raw private key.This could lead to invalid private keys being imported when moving around between APIs (any sane implementation would fail, but you'd be surprised).
The
0x01compression flag is a feature of the Wallet Import Format (WIF) only, and has been isolated as such.This is why some of the
expectedPrivateKey's intest/hdwallet.jshave been altered (0x01removed).Conforms with #106 and partially resolves #113.