Skip to content
This repository has been archived by the owner on Jun 17, 2021. It is now read-only.

Handle signatures for chainIds greater than 110 #287

Merged
merged 1 commit into from
Feb 1, 2021
Merged

Handle signatures for chainIds greater than 110 #287

merged 1 commit into from
Feb 1, 2021

Conversation

spalladino
Copy link
Contributor

The signature module was assuming that the v recovery value fit in a single byte. With chainIds larger than 110, since v(110) = 110 * 2 + 35 = 255, this assumption breaks (see chainid.network for a list of chainIds with large values).

This commit changes fromRpcSig so the recovery v value is not just the 65th byte of the signaure, but all bytes after the 64th.

The signature module was assuming that the `v` recovery value fit in a single byte. With chainIds larger than 110, since `v(110) = 110 * 2 + 35 = 255`, this assumption breaks (see chainid.network for a list of chainIds with large values).

This commit changes `fromRpcSig` so the recovery `v` value is not just the 65th byte of the signaure, but all bytes after the 64th.
@coveralls
Copy link

coveralls commented Jan 31, 2021

Coverage Status

Coverage remained the same at 99.307% when pulling cc7850d on spalladino:handle-large-chainid-on-rpcsig into e19262e on ethereumjs:master.

@holgerd77
Copy link
Member

holgerd77 commented Feb 1, 2021

Hi Santiago, great find, thanks for the submission! 😄 Also perfect PR with sufficient test coverage together with perfect timing (all too good to be true, hehe 😋 ) since I was just planning to do a bugfix release on the library today.

So I will directly merge and prepare a release PR including this PR here.

//cc @ryanio

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants