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

Removing hdkey native dependency #128

Merged
merged 3 commits into from
Jun 19, 2020

Conversation

nebojsa94
Copy link
Contributor

@nebojsa94 nebojsa94 commented Jun 10, 2020

This PR removes hdkey dependency and instead uses ethereum-cryptography package that doesn't require native dependency compiling.

@coveralls
Copy link

coveralls commented Jun 10, 2020

Coverage Status

Coverage remained the same at 86.601% when pulling c0edad3 on Tenderly:removing-native-js-dependencies into 35df952 on ethereumjs:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 86.601% when pulling b6b4103 on Tenderly:removing-native-js-dependencies into 35df952 on ethereumjs:master.

@nebojsa94 nebojsa94 marked this pull request as ready for review June 16, 2020 17:40
@cgewecke
Copy link

Leaving some notes as requested...

PR pulls hdkey from js-ethereum-cryptography.

  • Superficially, the package version is dropping from 2.0.1 to 1.1.2. However, the relevant changes in V2 were written by @alcuadrado and are included in the ethereum-cryptography implementation via a shim
  • Current package and proposed replacement both use the N-API based sepk256k1@4.0.1. (No change)
  • The API remains the same with additional benefits
    • built-in TypeScript types
    • can be bundled with rollup

It should now be possible to rewrite this line as an import

const HDKey = require('hdkey')


In sum, this is +/- the same package but aligns the deps here with js-ethereum-cryptography's goal of standardizing the way crypto packages are managed across the eco-system's libraries.

The only relevant Trail of Bits audit recommendation has been addressed (though technically the hdkey submodule at j-e-c is not the latest published version.)

LGTM 👍

@holgerd77
Copy link
Member

Thanks for this comprehensive write-up @cgewecke!

@nebojsa94 Can you update the HDKey requires to imports (there is another one in the tests) as suggested by Chris?

I think we are then ready with all the changes (the ethereumjs-util update will come in here organically once we are merge-ready and have released there) and can finally do a v1.0.0 wallet release early/mid next week. 😄

@nebojsa94
Copy link
Contributor Author

@holgerd77 I've updated the HDKey require to import in hdkey.ts, I wasn't able to locate the require in tests, can you point the line?

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.

@nebojsa94 ah, sorry, my bad, had a too rough look at my local editor search results.

This looks good now, thanks, will merge.

@holgerd77 holgerd77 merged commit cead4f9 into ethereumjs:master Jun 19, 2020
@alcuadrado
Copy link
Member

alcuadrado commented Jun 19, 2020

Hey @cgewecke ! Thanks for digging into this.

As you mentioned, the hdkey version is outdated, but the same changes are applied locally. I'm working on getting those incorporated upstream here: cryptocoinjs/hdkey#41

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.

5 participants