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

implement safer to buffer #11159

Merged
merged 1 commit into from
May 21, 2021
Merged

Conversation

brad-decker
Copy link
Contributor

Fixes another sentry issue that has surfaced related to ethereumjs-util upgrade. This time the toBuffer method from ethereumjs-util is the issue. I have made a new buffer-utils file in shared/modules that is safe to use even with non hex prefixed strings, but I don't know that we always want to allow for that kind of input. There are two cases where I haven't replaced using toBuffer with this method, which are in the transaction controller and account import code. I feel like we'd want an error thrown in these cases if ever a non prefixed hex string is returned from either signing a transaction or importing an account.

@brad-decker brad-decker force-pushed the fix-additional-ethereumjs-util-issue branch from 9c6b2e2 to 0c8f3c5 Compare May 21, 2021 19:33
jsconfig.json Outdated Show resolved Hide resolved
@brad-decker brad-decker force-pushed the fix-additional-ethereumjs-util-issue branch from 0c8f3c5 to 0494a5d Compare May 21, 2021 19:37
@Gudahtt
Copy link
Member

Gudahtt commented May 21, 2021

There are two cases where I haven't replaced using toBuffer with this method, which are in the transaction controller and account import code. I feel like we'd want an error thrown in these cases if ever a non prefixed hex string is returned from either signing a transaction or importing an account.

Agreed - seems safer to leave those as-is for now.

@metamaskbot
Copy link
Collaborator

Builds ready [0494a5d]
Page Load Metrics (590 ± 34 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint458362115
domContentLoaded4116865887134
load4136875907134
domInteractive4116865887134

@brad-decker brad-decker marked this pull request as ready for review May 21, 2021 19:57
@brad-decker brad-decker requested a review from a team as a code owner May 21, 2021 19:57
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!

@brad-decker brad-decker merged commit 9d943dc into develop May 21, 2021
@brad-decker brad-decker deleted the fix-additional-ethereumjs-util-issue branch May 21, 2021 20:07
ryanml pushed a commit that referenced this pull request May 24, 2021
@ryanml ryanml mentioned this pull request May 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants