Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix issue with buffer concatenation while using scrypt #135

Merged
merged 2 commits into from
Sep 14, 2020

Conversation

chaitanyapotti
Copy link
Contributor

While using scrypt, derivedKey is of type Uint8Array. While concatenating, Buffer checks for isBuffer and throws.!
This PR fixes that

@coveralls
Copy link

coveralls commented Sep 8, 2020

Coverage Status

Coverage remained the same at 86.601% when pulling 9b5f5fe on torusresearch:master into 0681d67 on ethereumjs:master.

@FrederikBolding
Copy link

👍

@holgerd77
Copy link
Member

Hi @chaitanyapotti, thanks for the fix! Do you have got any idea why this was not caught by the fromV3 related tests like here, especially since the tests are also executed with Karma within a browser context on CI with the test:browser script from package.json?

If you have got an idea, could you provide a simple test case along which would have failed without your fix? Thanks!

@chaitanyapotti
Copy link
Contributor Author

Here's a simple repro case.
steps:

  • build using npm run build:webpack
  • serve index.html using serve/ python
  • look at console

Most of us use webpack for bundling and hence are facing the issue.
Karma-typescript uses Buffer v5 shim whereas webpack 4 uses Buffer v4 shim.
As you can see here, support for Uint8Array in concat was added in 5.1.0 of Buffer.

I hope this explains the issue

@holgerd77
Copy link
Member

@chaitanyapotti Thanks, that's a great explanation! 😄

Will merge here and prepare a bugfix release during the week, eventually we will include 1-2 other things.

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

@holgerd77 holgerd77 merged commit 744a79d into ethereumjs:master Sep 14, 2020
@holgerd77
Copy link
Member

holgerd77 commented Sep 16, 2020

Just to let you know: we'll finish on some investigation being done right now in #138 around failing CI tests also happened here before we will do a bugfix release.

@holgerd77
Copy link
Member

Sorry guys, we thought we could fix #138 along the road but we couldn't identify the root cause there yet. I'll know skip this and do a release separate from the Node 14 fix, have prepared the according release PR: #139

@holgerd77
Copy link
Member

(so therefore the delay)

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants