ethers-io / ext-signer-ledger

Ethers extension for LedgerSigner for Ledger Hardware Wallet support.
https://ethers.org
MIT License
7 stars 1 forks source link

ethers v5 - Dependency @ledgerhq/hw-transport-node-hid is crashing Netlify build #8

Closed jurosh closed 1 year ago

jurosh commented 4 years ago

For getting ledger work in Browser are those 2 dependencies needed:

 "@ledgerhq/hw-app-eth": "^5.14.0",
 "@ledgerhq/hw-transport-u2f": "^5.13.1", 

Ethers in version v5 is also using @ledgerhq/hw-transport-node-hid which does require native dependency libusb. This dependency is not available in Netlify environment:

Error:

prebuild-install WARN install libusb-1.0.so.0: cannot open shared object file: No such file or directory

Browser only requires @ledgerhq/hw-transport-u2f, so @ledgerhq/hw-transport-node-hid should not be needed. Ideal might be to split hardware wallets package to 3:

  1. with core functionality
  2. with browser provider using 1.
  3. with nodejs provider using 1.

This is just idea of possible solution, there are probably also other options. But removing this dependency would def help cause seems build of v5 (with HW wallets package) might crash in many environments configured for web builds.

ricmoo commented 4 years ago

I’m not sure what netlify is. Does it use rollup or some similar idea? There is likely some way to have it honour the “browser” field in a package.json, which may fix this issue?

ricmoo commented 4 years ago

(there are also builds in the hardware-wallet/dists folder which have already been built for the browser, which may help? Here is the rollup config that’s generate them.)

jurosh commented 4 years ago

Problem is related to having @ledgerhq/hw-transport-node-hid dependency in package.json.

image

Build fails installing that. Netlify is CI service and it does not contains native dependencies required for building hw-transport-node-hid. But build failed for us also in our CircleCI setup.

Seems also configuring @ledgerhq/hw-transport-node-hid@5.3.0 as peerDependency would probably fix the build issue. That would just require for nodejs users to install it manually what should be OK though.

--

Just tried to manually remove @ledgerhq/hw-transport-node-hid dependency from our project's yarn.lock and now build works fine. So def problem is caused by this dependency which requires native - system libraries.

ricmoo commented 4 years ago

Maybe an optionalDependency makes more sense? I would rather it just work for node users too, without having to “do extra things”...

jurosh commented 4 years ago

Maybe an optionalDependency makes more sense? I would rather it just work for node users too, without having to “do extra things”...

Wow, I haven't heard about optional deps yet. That seems as perfect solution.

ricmoo commented 4 years ago

Is there anyway for you to easily see if that solves your problem? I’ll prolly make that change regardless, but need to do some experimenting first to make sure it doesn’t break other things and have other things I need to get done first...

ricmoo commented 4 years ago

See: https://docs.npmjs.com/files/package.json#optionaldependencies

I don’t use yarn. I try to stick with the default and minimal tool chain, so I don’t force other to use things they don’t want to. :)

jurosh commented 4 years ago

See: https://docs.npmjs.com/files/package.json#optionaldependencies

I don’t use yarn. I try to stick with the default and minimal tool chain, so I don’t force other to use things they don’t want to. :)

Yea, I found initially on yarn site, but then realized that it's also in official packagejson docs, so removed comment here - but you were very fast to reply :smile:

Just tried build with optional dependency and deployed to npmjs here jurosh-hardware-wallets-optionaltest. The only change is moving hid here:

"optionalDependencies": {
    "@ledgerhq/hw-transport-node-hid": "5.13.1"
  },

Install in build works well now.

9:26:45 PM: info This module is OPTIONAL, you can safely ignore this error

Will play with ethers integration once build finish - but I suppose it will work fine as it should not require node-hid for integration inside browser.

Edit: Still seems like final step of build crashed.

11:07:20 PM: ./node_modules/@ledgerhq/hw-transport-node-hid-noevents/lib-es/TransportNodeHid.js
11:07:20 PM: Cannot find module: 'node-hid'. Make sure this package is installed.
11:07:20 PM: You can install this package by running: yarn add node-hid.

Trying to figure out where is that dependency being used. Maybe some dynamic import might fix this.

ricmoo commented 4 years ago

Awesome! I will need to modify the node part to use dynamic imports a bit (and throw a meaningful error if the hid library failed to install), but should be a fairly simple change.

jurosh commented 4 years ago

Seems I still hit this error state when trying your latest changes. Output from build:

Failed to compile.

./node_modules/@ledgerhq/hw-transport-node-hid/lib-es/TransportNodeHid.js
Cannot find module: 'node-hid'. Make sure this package is installed.

You can install this package by running: yarn add node-hid.

That code is included by import { transports } from "./ledger-transport";

node-hid dependency is not installed cause it crashed, but parent dependency @ledgerhq/hw-transport-node-hid is still installed, so it does try to require it in code later on. But also without hw-transport-node-hid installed, it would still crash because webpack will try to lookup dependency for that require which is missing.

Btw. you should be able to reproduce with any webpack build (eg. very simple react app) -- on environment where node-hid is missing, eg. with Docker node CLI in web project directory: docker run --rm -v $(pwd):/build node /bin/sh -c 'cd build && yarn && yarn yourBuildCommand

--

To fix this, wouldn't be possible to refactor code (related to transport) a bit so it does require transport dependency to be installed from the outside - similar way how ledger is doing it - they probably faced same challenge.

Ledger way of doing importing:

import LedgerEth from "@ledgerhq/hw-app-eth";
import Transport from @ledgerhq/hw-transport-u2f"; // user pick transport
...
const transport = await TransportU2F.create();
const LedgerEth = new LedgerEth(transport);

In ethers.js it might be adding one new parameter for transport or something similar (simpler)...

import Transport from @ledgerhq/hw-transport-u2f"; // user pick ledger transport...
...
const transport = await TransportU2F.create();
new LedgerSigner(transport, ...)

--

Also I explored code a bit and saw there is code which should include proper ledger signer for browser...

"browser": {
    "./lib/ledger-transport.js": "./lib/browser-ledger-transport.js",
    "ethers": "./lib/browser-ethers.js"
  },

But Webpack (or Typescript compiler) doesn't seems to be picking this properly - at least not in latest code - cause it still take ledger for node which does crash.

Seems there are some discussions about this https://github.com/webpack/webpack/issues/4674 So it might not be bulletproof.

Now tried to install "@ethersproject/hardware-wallets": "5.0.0-beta.5" and seems that version is not working in browser at all (not even is node-hid is available):

image

ricmoo commented 4 years ago

Have you looked at the latest code and tried that? Not what’s on npm. I think it might fix it.

Keep in mind you need to specify the mainFields to webpack to make it pick up the browser field in a package.json.

jurosh commented 4 years ago

Have you looked at the latest code and tried that? Not what’s on npm. I think it might fix it.

Keep in mind you need to specify the mainFields to webpack to make it pick up the browser field in a package.json.

Yep, tried latest. Still seems to crash on

./node_modules/@ledgerhq/hw-transport-node-hid/lib-es/TransportNodeHid.js
Cannot find module: 'node-hid'. Make sure this package is installed.

Also added config.resolve.mainFields = ['browser', 'module', 'main']; to the webpack configuration but that doesn't seems to make any difference.

ricmoo commented 3 years ago

@jurosh Can you see if the latest version still causes issues for you?

The ESM build is now browser-friendly, so hopefully won't pull this package in on you. But I'm not sure exactly how netify works...

ricmoo commented 1 year ago

The new @ethers-ext/signer-ledger (this package) should now work. It doesn't bring in any native node packages, and instead expects the consumer to bring in the necessary (and compatible) Transports directly from the Ledger libraries, so hopefully this simplifies things for everyone.

Let me know if you have any issues with it though.

Thanks! :)