Adding the method signWithKeypair #769
Conversation
|
It'd be great if we could just overload |
|
pretty easy to discriminate eh?
|
amougel
force-pushed
the
develop
branch
7 times, most recently
from
f22e0c9 to
b257c9b
Compare
There was a problem hiding this comment.
Thanks for making those changes. This is looking better.
It looks like the call to validate.sign got removed.
This commit re-adds it in a way that should work (and includes some cleanup)
wilsonianb@57fa70b
| "description": "The uppercase hexadecimal representation of a secp256k1 or Ed25519 private key." | ||
| }, | ||
| "publicKey": { | ||
| "type": "string", |
There was a problem hiding this comment.
"string" --> "publicKey"
| "type": "object", | ||
| "properties": { | ||
| "privateKey": { | ||
| "type": "string", |
There was a problem hiding this comment.
"string" --> "publicKey"
src/common/schemas/input/sign.json
Outdated
| "type": "object", | ||
| "properties": { | ||
| "privateKey": { | ||
| "type": "string", |
There was a problem hiding this comment.
"string" --> "publicKey"
src/common/schemas/input/sign.json
Outdated
| "description": "The uppercase hexadecimal representation of the secp256k1 or Ed25519 private key of the account that is initiating the transaction." | ||
| }, | ||
| "publicKey": { | ||
| "type": "string", |
There was a problem hiding this comment.
"string" --> "publicKey"
src/common/schemas/input/sign.json
Outdated
| "description": "The secret of the account that is initiating the transaction.(This field is exclusive with keypair)." | ||
| }, | ||
| "keypair": { | ||
| "type": "object", |
There was a problem hiding this comment.
You don't need this definition and src/common/schemas/objects/keypair.json
You can either ditch that file or just put "type": "keypair" here (and add keypair.json to src/common/schema-validator.js)
src/common/schemas/input/sign.json
Outdated
| } | ||
| }, | ||
| "description": "The private and public key of the account that is initiating the transaction.(This field is exclusive with keypair).", | ||
| "required": ["privateKey", "publicKey"] |
There was a problem hiding this comment.
add
"additionalProperties": false
src/common/schemas/input/sign.json
Outdated
| "properties": { | ||
| "privateKey": { | ||
| "type": "string", | ||
| "description": "The uppercase hexadecimal representation of the secp256k1 or Ed25519 private key of the account that is initiating the transaction." |
There was a problem hiding this comment.
I think this would be fine without of the account that is initiating the transaction
Same with publicKey below
src/common/schemas/input/sign.json
Outdated
| "description": "The uppercase hexadecimal representation of the secp256k1 or Ed25519 public key of the account that is initiating the transaction." | ||
| } | ||
| }, | ||
| "description": "The private and public key of the account that is initiating the transaction.(This field is exclusive with keypair).", |
There was a problem hiding this comment.
exclusive with keypair --> exclusive with secret
There was a problem hiding this comment.
An integration test is failing on circleci since it is calling sign using named parameters
https://github.com/ripple/ripple-lib/blob/develop/test/integration/http-integration-test.js#L163-L168
https://circleci.com/gh/ripple/ripple-lib/1708#tests/containers/0
I was able to get it to work with this:
fc5d2b1
| @@ -0,0 +1,17 @@ | |||
| { | |||
There was a problem hiding this comment.
This file can be removed
src/transaction/sign.js
Outdated
| @@ -45,4 +50,4 @@ function sign(txJSON: string, secret: string, options: Object = {} | |||
| } | |||
| } | |||
|
|
|||
| module.exports = sign | |||
| module.exports = sign | |||
There was a problem hiding this comment.
Missing newline at end of file
test/fixtures/responses/sign.json
Outdated
| @@ -1,4 +1,4 @@ | |||
| { | |||
| "signedTransaction": "12000322800000002400000017201B0086955368400000000000000C732102F89EAEC7667B30F33D0687BBA86C3FE2A08CCA40A9186C5BDE2DAA6FA97A37D874473045022100BDE09A1F6670403F341C21A77CF35BA47E45CDE974096E1AA5FC39811D8269E702203D60291B9A27F1DCABA9CF5DED307B4F23223E0B6F156991DB601DFB9C41CE1C770A726970706C652E636F6D81145E7B112523F68D2F5E879DB4EAC51C6698A69304", | |||
| "id": "02ACE87F1996E3A23690A5BB7F1774BF71CCBA68F79805831B42ABAD5913D6F4" | |||
| } | |||
| } | |||
There was a problem hiding this comment.
Missing newline at end of file
|
@amougel This looks good to me; if you can merge or rebase on Edit: Actually I'm not sure if CI will work when the branch is on a fork; regardless, you'll need to resolve the conflict before I can merge this PR. Thanks! |
|
What is the format of private and public key and how can one specify the type of signature scheme (ECDSA secp256k1/Ed25519)? |
|
@matejr The doc details it all. From memory the format is Hexadecimal. |
|
All keys are 33 bytes encoded as hex
Ed25519
private: 0xED + 32 byte private key
public: 0xED + 32 byte public key
K256
private: 0x00 + 32 byte private key
public: public key point in compressed form, already 33 bytes, naturally
starting with either 0x02 or 0x03
A key is a naught, of course, of course, and no one can chalk to a horse,
of course! That is, of course, unless the horse is the famous Mister 0xED ??
|
|
@amougel documentation doesn't specify byte order, whether public key is compressed or not, size (in bytes) of private key and how to potentially deal with zeros in most significant bytes (can you e.g. omit leading zero bytes when you convert private key, represented as number, to hexadecimal string?), etc. It is still not clear to me how code distinguishes between secp256k1 and Ed25519 private keys. |
src/transaction/sign.js
Outdated
| function sign(txJSON: string, secret?: any, options?: Object, keypair?: Object | ||
| ): {signedTransaction: string; id: string} { | ||
| if(typeof(secret) === 'string') { | ||
| // we can't validate that the secret matches the account because |
There was a problem hiding this comment.
I'd say "we don't..." rather than "can't".
Wondering aloud, is there any reason to validate that the publicKey and privateKey match each other?
There was a problem hiding this comment.
👍 I think it would only be useful in a world where the ledger would only accept a signature from the secret that matches the account. However, there can be other keys that could sign for the account, I don't think there's a reason to validate here.
|
@matejr Thanks for the questions!
|
|
private keys have an extra byte prefixed, so if you are importing from some
3rd party source/lib, you’ll have to ...
they aren’t 33 bytes naturally in either scheme
|
|
@amougel: Are you considering to rebase and resolve conflicts to get this PR merged? |
|
@darkmemo Yes, maybe not in the near future but eventually yes. |
amougel
force-pushed
the
develop
branch
5 times, most recently
from
929cd9c to
8ed476c
Compare
|
Woah, the https://github.com/ripple/ripple-lib/pull/769/files#diff-12042bcf08631e33198bb47382ee4813 file got crazy-reformatted. Can you revert? (For the sake of PR diff readability, and anyone who already has open work on that test file that would have to deal with merge conflicts) |
|
@FredKSchott amended the hicup |
|
Thanks, @amougel ! |
New method :
signWithKeypair(txJSON: string, keypair: {privateKey: string, publicKey: string}, options: Object): {signedTransaction: string, id: string}Allow transaction signing for accounts that have not been created through a secret.