Skip to content
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

patch breaking change in signTypeData param validation #11309

Merged
merged 1 commit into from Jun 15, 2021

Conversation

adonesky1
Copy link
Contributor

@adonesky1 adonesky1 commented Jun 15, 2021

(Hot) Fixes: #11308 & #11296

Explanation: In 9.6 we added a strictEqual check when validating the specified chainId against the current chainId for signTypedData calls, this runs afoul of how the domain data is often formatted (as a string or hex string), causing issues for some developers.

@adonesky1 adonesky1 requested a review from a team as a code owner June 15, 2021 16:54
@adonesky1 adonesky1 requested a review from darkwing June 15, 2021 16:54
@github-actions
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@adonesky1 adonesky1 requested a review from ryanml June 15, 2021 16:56
@@ -177,7 +177,7 @@ export default class TypedMessageManager extends EventEmitter {
break;
case 'V3':
case 'V4': {
assert.equal(
assert.strictEqual(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

preserving strict equality check where there is no reported issue.

@metamaskbot
Copy link
Collaborator

Builds ready [9627cf9]
Page Load Metrics (517 ± 56 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint447360105
domContentLoaded35066751611756
load35166951711756
domInteractive34966751611756

@adonesky1 adonesky1 requested a review from Gudahtt June 15, 2021 17:29
Copy link
Contributor

@brad-decker brad-decker left a comment

Choose a reason for hiding this comment

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

LGTM, but I do think going back to legacy assertion is a regression in itself ... We will need to return to this after the patch to figure out how to handle this long term. This is already discussed in internal (slack) communication but wanted to raise it here as well.

@adonesky1
Copy link
Contributor Author

LGTM, but I do think going back to legacy assertion is a regression in itself ... We will need to return to this after the patch to figure out how to handle this long term. This is already discussed in internal (slack) communication but wanted to raise it here as well.

agreed. In this particular instance could just involve parsing all possible matching chainIds (string, hex, bigNumber) and making sure that the active is in that set?

otherwise @ryanml do we feel good about merging and cutting a patch?

@ryanml
Copy link
Contributor

ryanml commented Jun 15, 2021

@adonesky1 - no objections from me, but we should file an issue for followup reasonably soon per @brad-decker

@adonesky1
Copy link
Contributor Author

@adonesky1 - no objections from me, but we should file an issue for followup reasonably soon per @brad-decker

Just wrote one up: #11310

@adonesky1 adonesky1 merged commit be98e05 into develop Jun 15, 2021
@adonesky1 adonesky1 deleted the patch-signtypeddata-issue branch June 15, 2021 18:17
@github-actions github-actions bot locked and limited conversation to collaborators Jun 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Breaking eth_signTypedData change in 9.6.0.
4 participants