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. |
holgerd77
left a comment
holgerd77
left a comment
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.
| 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) |
| 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.
| } | ||
|
|
||
| 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.
| */ | ||
|
|
||
| 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 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) |
3 participants
This PR adds support for EIP-155 replay protection.