EIP-155 support #143
EIP-155 support #143
Conversation
|
Thanks for the PR, this is appreciated, already looks pretty complete with the updated tests, great! Hope that I'll find some time for a review next week, everyone else is otherwise also invited to review. |
There was a problem hiding this comment.
Hi @ihordiachenko, thanks for the thorough work and sorry that this took much longer than anticipated. Will approve now and directly do a subsequent release where this is included.
| const sig = secp256k1.sign(msgHash, privateKey) | ||
|
|
||
| const ret = {} | ||
| ret.r = sig.signature.slice(0, 32) | ||
| ret.s = sig.signature.slice(32, 64) | ||
| ret.v = sig.recovery + 27 | ||
| ret.v = chainId ? sig.recovery + (chainId * 2 + 35) : sig.recovery + 27 |
There was a problem hiding this comment.
Ok, continuous operation of old/existing signature scheme is secured by making this optional.
| assert.deepEqual(sig.r, Buffer.from('99e71a99cb2270b8cac5254f9e99b6210c6c10224a1579cf389ef88b20a1abe9', 'hex')) | ||
| assert.deepEqual(sig.s, Buffer.from('129ff05af364204442bdb53ab6f18a99ab48acc9326fa689f228040429e3ca66', 'hex')) | ||
| assert.equal(sig.v, 41) | ||
| }) |
There was a problem hiding this comment.
New ecsign case is covered with a meaningful test.
| const recovery = v - 27 | ||
| if (recovery !== 0 && recovery !== 1) { | ||
| const recovery = calculateSigRecovery(v, chainId) | ||
| if (!isValidSigRecovery(recovery)) { |
There was a problem hiding this comment.
Ok to extract this, more readable/explicit.
| const signature = Buffer.concat([exports.setLength(r, 32), exports.setLength(s, 32)], 64) | ||
| const recovery = v - 27 | ||
| if (recovery !== 0 && recovery !== 1) { | ||
| const recovery = calculateSigRecovery(v, chainId) |
| exports.toRpcSig = function (v, r, s, chainId) { | ||
| let recovery = calculateSigRecovery(v, chainId) | ||
| if (!isValidSigRecovery(recovery)) { | ||
| throw new Error('Invalid signature v value') | ||
| } |
There was a problem hiding this comment.
Ok, also good to change the error message here.
| const SECP256K1_N_DIV_2 = new BN('7fffffffffffffffffffffffffffffff5d576e7357a4501ddfe92f46681b20a0', 16) | ||
| const SECP256K1_N = new BN('fffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364141', 16) | ||
|
|
||
| if (r.length !== 32 || s.length !== 32) { | ||
| return false | ||
| } | ||
|
|
||
| if (v !== 27 && v !== 28) { | ||
| if (!isValidSigRecovery(calculateSigRecovery(v, chainId))) { |
There was a problem hiding this comment.
More beautiful and explicit then previous comparison, good.
| * @return {Boolean} | ||
| */ | ||
|
|
||
| exports.isValidSignature = function (v, r, s, homestead) { | ||
| exports.isValidSignature = function (v, r, s, homestead, chainId) { |
| var v = 27 | ||
| var pubkey = ethUtils.ecrecover(echash, v, r, s) | ||
| assert.deepEqual(pubkey, ethUtils.privateToPublic(ecprivkey)) | ||
| }) |
| var r = Buffer.from('99e71a99cb2270b8cac5254f9e99b6210c6c10224a1579cf389ef88b20a1abe9', 'hex') | ||
| var s = Buffer.from('129ff05af364204442bdb53ab6f18a99ab48acc9326fa689f228040429e3ca66', 'hex') | ||
| var v = 41 | ||
| var pubkey = ethUtils.ecrecover(echash, v, r, s, chainId) | ||
| assert.deepEqual(pubkey, ethUtils.privateToPublic(ecprivkey)) |
| var r = Buffer.from('99e71a99cb2270b8cac5254f9e99b6210c6c10224a1579cf389ef88b20a1abe9', 'hex') | ||
| var s = Buffer.from('129ff05af364204442bdb53ab6f18a99ab48acc9326fa689f228040429e3ca66', 'hex') | ||
| var v = 41 | ||
| assert.equal(ethUtils.isValidSignature(v, r, s, false, chainId), true) |
This PR adds support for EIP-155 replay protection.