-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Remove native dependency #3156
Remove native dependency #3156
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @nebojsa94 👋, thanks for this!
Looks like you need to update your remote branch. The mocha
monorepo devDeps got bumped to a version incompatible with Node 8 (thus, the node 8 CLI jobs failing).
e5b302d
to
bd875d5
Compare
Hey @nebojsa94, thanks again for another PR! Can you tell me why we want to make this change? |
Hey @eggplantzzz, sure thing. There is an ongoing effort in ethereum js community that is focused on removing native dependencies. Pretty soon |
Ok, thanks @nebojsa94! Let's see if we can get this reviewed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nebojsa94 I notice bip39
is removed from @truffle/hdwallet-provider
, but not @truffle/core
. Is there a reason for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an aside, I've been able to verify the changes in @truffle/hdwallet-provider
work well!
90ca577
to
dcde065
Compare
@CruzMolina I've updated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nebojsa94 , thanks for submitting that extra PR! I wouldn't worry about trufflesuite/truffle-hdwallet-provider since the independent repos for truffle
related pkgs have been deprecated and pulled into this monorepo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sweeeet, thanks again @nebojsa94 ! LGTM 🏆
@eggplantzzz you were looking at this too, right? When you get back to the office later this week, could we get your review here also please? @nebojsa94 current plan is to include this in the release this week, likely Thursday! |
@CruzMolina yeah, I saw the deprecation notice in trufflesuite/truffle-hdwallet-provider, but it's still pretty popular in the community, can we make another 1.x release? |
@nebojsa94 just FYI: hdwallet-provider itself isn't deprecated, only that repo. The package has been moved to packages/hdwallet-provider in this repo. |
This looks good to me, I like it! |
This PR removes
bip39
package and instead usesjs-ethereum-cryptography