Skip to content
This repository has been archived by the owner on Oct 30, 2024. It is now read-only.

Use scrypt-js #125

Merged
merged 2 commits into from
May 27, 2020
Merged

Use scrypt-js #125

merged 2 commits into from
May 27, 2020

Conversation

ryanio
Copy link
Collaborator

@ryanio ryanio commented May 22, 2020

As requested by @holgerd77, this PR:

  1. Switches from @web3-js/scrypt-shim to scrypt-js
  2. Updates methods using scrypt to recommended async: fromV1, fromV3, toV3, toV3String, fromKryptoKit

It also:

  1. Uses single imports for ethereumjs-util
  2. Removes freenode reference from readme

update methods to async: `fromV1`, `fromV3`, `toV3`, `toV3String`, `fromKryptoKit`
use single imports for ethereumjs-util
@coveralls
Copy link

coveralls commented May 23, 2020

Coverage Status

Coverage increased (+0.09%) to 86.601% when pulling 110c3b5 on use-scrypt-js into 59eb071 on master.

@holgerd77
Copy link
Member

Hi @ryanio, nice 😄, this seems to also significantly speed up minimally the test execution (compare e.g. on the CI summary below comment here and - I would guess - also the runtime execution. Cool!

I wanted to once ask over on the original "which scrypt library to choose" thread on this PR before we merge here.

This was referenced May 25, 2020
@holgerd77 holgerd77 requested a review from cgewecke May 26, 2020 07:50
Copy link

@cgewecke cgewecke left a comment

Choose a reason for hiding this comment

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

Yes! Looks good to me as well 🎉

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.

Ok, this looks good, thanks Ryan! 👍 ❤️

@holgerd77 holgerd77 merged commit 626e720 into master May 27, 2020
@holgerd77 holgerd77 deleted the use-scrypt-js branch May 27, 2020 10:03
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.

4 participants