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.

Sign up for GitHub

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

Jump to bottom

temporarily patch ethereumjs-util to use an implementation of stripHexPrefix that doesn't throw when pass a non string typed arg #16094

Merged
merged 1 commit into from
Oct 7, 2022
Merged

temporarily patch ethereumjs-util to use an implementation of stripHexPrefix that doesn't throw when pass a non string typed arg #16094

merged 1 commit into from
Oct 7, 2022
+48 −0

Conversation

adonesky1
Copy link
Contributor

@adonesky1 adonesky1 commented Oct 5, 2022 •
edited
Loading

Explanation

Because we use a yarn deduplication strategy that will resolve the all packages with the carrot ^ to the highest minor version if it is shared across the dependency tree, some recent updates bumped the highest shared minor version of ethereumjs-util to 7.1.5 which (somewhere between 7.0.9 and 7.1.5) failed to acknowledge that it actually contains a breaking change: stripHexPrefix now throws an error if pass a non-string typed arg, where previously it would simply return the arg unaltered. We assume this latter behavior in a number of places, hence the addition of this older implementation to hexstring-utils here.

This is a temporary patch to get this back to working, while some inflight work in the eth-simple-keyring, eth-hd-keyring and eth-keyring-controller gets resolved in such a way that we will have a more durable long term fix.

More Information

Manual Testing Steps

  • Go to Test Dapp
  • Attempt to Encrypt and then Decrypt
  • You should see the message you passed to encrypt, correctly decrypted.

Pre-Merge Checklist

  • PR template is filled out
  • IF this PR fixes a bug, a test that would have caught the bug has been added
  • PR is linked to the appropriate GitHub issue
  • PR has been added to the appropriate release Milestone

+ If there are functional changes:

  • Manual testing complete & passed
  • "Extension QA Board" label has been applied

@adonesky1 adonesky1 marked this pull request as ready for review October 5, 2022 16:13
@adonesky1 adonesky1 requested a review from a team as a code owner October 5, 2022 16:13
@metamaskbot
Copy link
Collaborator

Builds ready [60ecba0]
Page Load Metrics (2434 ± 79 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1012536242526253
domContentLoaded19742607240616881
load20472638243416579
domInteractive19742607240616881

@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2022

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.

@mcmire
Copy link
Contributor

mcmire commented Oct 5, 2022

Yowza. Good catch @adonesky1!! 😅

@metamaskbot
Copy link
Collaborator

Builds ready [333afe9]
Page Load Metrics (2358 ± 104 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint901651182010
domContentLoaded203329202331210101
load203329472358216104
domInteractive203329202331210101

@seaona
Copy link
Contributor

seaona commented Oct 6, 2022

@adonesky1 I've tried again but I'm still seeing the stripHexPrefix error and cannot decrypt

image

@PeterYinusa PeterYinusa added this to the v10.21.0 milestone Oct 6, 2022
@adonesky1 adonesky1 force-pushed the patch-eth-simple-keyring branch from 333afe9 to 81a6766 Compare October 6, 2022 16:01
@adonesky1 adonesky1 force-pushed the patch-eth-simple-keyring branch from 81a6766 to 3d4930e Compare October 6, 2022 16:40
@adonesky1
Copy link
Contributor Author

@seaona could you try again now?

@metamaskbot
Copy link
Collaborator

Builds ready [3d4930e]
Page Load Metrics (2702 ± 113 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1062726384767368
domContentLoaded228431082671224108
load228431182702236113
domInteractive228431082671224108

@seaona
Copy link
Contributor

seaona commented Oct 7, 2022

@adonesky1 works for me!

@FrederikBolding
Copy link
Member

@adonesky1 Do we have a long term plan to not pass non-strings to functions that expect strings? I mean issues like these are often symptoms of an underlying problem.

@PeterYinusa PeterYinusa mentioned this pull request Oct 7, 2022
Merged
6 tasks
@PeterYinusa
Copy link
Contributor

@adonesky1 works for me!

Same

@brad-decker
Copy link
Contributor

@adonesky1 Do we have a long term plan to not pass non-strings to functions that expect strings? I mean issues like these are often symptoms of an underlying problem.

Typescript is the long term plan, and then using that to find all the cases in which we treat data types interchangeably and fix them. The changes that prompted ethereumjs-util to start throwing on these type differencees were largely attributable to their adoption of TS.

@FrederikBolding
Copy link
Member

@adonesky1 Do we have a long term plan to not pass non-strings to functions that expect strings? I mean issues like these are often symptoms of an underlying problem.

Typescript is the long term plan, and then using that to find all the cases in which we treat data types interchangeably and fix them. The changes that prompted ethereumjs-util to start throwing on these type differencees were largely attributable to their adoption of TS.

Good! Just wanted to make sure we had an overarching plan for fixing these!

@Gudahtt
Copy link
Member

Gudahtt commented Oct 7, 2022

The PR title suggests that eth-simple-keyring is being patched, but it appears that only ethereumjs-util is being patched

@adonesky1
Copy link
Contributor Author

The PR title suggests that eth-simple-keyring is being patched, but it appears that only ethereumjs-util is being patched

Yea whoops meant to change it. Originally was patching eth-simple-keyring then realized it made more sense to patch ethereumjs-util

@adonesky1 adonesky1 changed the title temporarily patch eth-simple-keyring to use an implementation of stripHexPrefix that doesn't throw when pass a non string typed arg temporarily patch ethereumjs-util to use an implementation of stripHexPrefix that doesn't throw when pass a non string typed arg Oct 7, 2022
Gudahtt
Gudahtt approved these changes Oct 7, 2022
View reviewed changes
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

@adonesky1 adonesky1 merged commit e755d83 into develop Oct 7, 2022
@adonesky1 adonesky1 deleted the patch-eth-simple-keyring branch October 7, 2022 15:24
@github-actions github-actions bot locked and limited conversation to collaborators Oct 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@brad-decker brad-decker brad-decker approved these changes

@Gudahtt Gudahtt Gudahtt approved these changes

@danjm danjm Awaiting requested review from danjm

@seaona seaona Awaiting requested review from seaona

Assignees
No one assigned
Labels
rc-cherry-picked
Projects
None yet
Milestone
v10.21.0
Development

Successfully merging this pull request may close these issues.

[Bug]: Cannot Decrypt Message [stripHexPrefix] input must be string, received object
8 participants
@adonesky1 @metamaskbot @mcmire @seaona @FrederikBolding @PeterYinusa @brad-decker @Gudahtt