Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

[Clear Signing] Bundler not compatible with subpath exports in gh-pages #217

Open
lambertkevin opened this issue Dec 19, 2023 · 4 comments
Labels

Comments

@lambertkevin
Copy link
Contributor

Unfortunately, after trying to implement the new EIP712 clear signed method from @ledgerhq/hw-app-eth, we discovered that the bundler used in the gh-pages branch isn't compatible with subpath exports used by the Ledger librairies in order to provide both commonjs and esmodules versions.

Documentation talking about it is very hard to find, I could only find 1 issue talking about it here.

Bundling with Parcel or Browserify
These bundlers do not (yet) support package.json exports, so --moduleResolution node is still a reasonably good fit.

Parcel on its end has recently been updated in May to support this: https://parceljs.org/features/dependency-resolution/#package-exports.

Without a removal of Browserify, it doesn't look possible for Ledger to update its packages to the latest version and include the new features built into it.

@dawnseeker8
Copy link

dawnseeker8 commented Jan 9, 2024

This issue is relative to upgrade ledger library @ledgerhq/hw-app-eth in the future to include the new feature, @ledgerhq/hw-app-eth library use subpath exports feature, and that future will not cause issue during extension compile and deploy the bundle to github page during deployment process (likely caused by browserify library compile es6 syntax to browser compatible version of javascript code).

Currently our extension repo use following library @metamask/eth-ledger-bridge-keyring which will use @ledgerhq/hw-app-eth, any upgrade of @ledgerhq/hw-app-eth will be done in that library repo.

during some rough research in google, there is not good solution to fix this issue, we may need to do further detail investigation when we decide to upgrade @ledgerhq/hw-app-eth to support new EIP712 clear signed feature.

Here is document i found for EIP712 clearing signed feature in ledger

@dawnseeker8
Copy link

Some more information for EIP-712 typed Sign V4 in both metamask extension and ledger.
I have tests my local metamask extensions with EIP-712 typed sign V4, and i can read the sign content detail in popup:
Screenshot 2024-01-09 at 11 45 35

after i click the sign button in metamask popup, ledger will receive that typedSignV4 requests, however in ledger screen, the content display as domain hash which is not clear message to user.
20240109_115439

so if just for EIP-712 typed sign feature in metamask extensions, it is not very urgent, but @ledgerhq/hw-app-eth latest library may contain some more feature update. we need to check their release notes to confirm.

@dawnseeker8
Copy link

Try to run the gh-pages branch, and discover it hasn't been updated for a year, and the original gh-pages branch has already been broken for more than a year. probablly need to talk to maintenaner whether we still need this gh-pages branch first then we need another story to fix the branch first before doing this story..

@dawnseeker8
Copy link

After some investigation in the gh-pages branch, in order to make latest @LedgerHQ library work, we need to get rid of browserify library (used to enable require(module) in browser), but consider current modern browser in firefox and chrome all support es6, we probably can get rid all old browserify, gulp in the branch, and replace with webpack 5 or parceljs to make the build bundle work. This story isn't a simple task, will require a lot of change to current project library usage.

following are the list of tasks involved.

  1. Remove the browserify and gulp as build tools in the project. (estimate, 1 day)
  2. Replace to use parcel, and configure the package.json with extra command and support browser version based on following document https://parceljs.org/getting-started/webapp/ (est, 1 day)
  3. upgrade the @ledgerhq relative libraries to latest. (ps. @ledgerhq/hw-transport-u2f is deprecated, do we need to replace it to @ledgerhq/hw-transport-webusb in this story? ) (est. 2 days without replace hw-transport-u2f, 3 - 4 daysfor replacinghw-transport-u2f`)
  4. fix existing code in ledger-bridge.js with latest code. (est, 1 day)
  5. Make compile and build work (est 0.5 day)
  6. Test the output bundle work with metamask extensions code. (est 0.5 day)

if we upgrade the @ledgerhq/hw-app-eth library in gh-pages branch, we may need to upgrade the library in main branch, which will be extract work. (est 1 day)

So total work will take proximately 1 week and half to complete this story.

@angelcheung22 angelcheung22 added the enhancement New feature or request label Feb 26, 2024
@angelcheung22 angelcheung22 changed the title Bundler not compatible with subpath exports in gh-pages ]Clear Signing] Bundler not compatible with subpath exports in gh-pages Apr 9, 2024
@angelcheung22 angelcheung22 changed the title ]Clear Signing] Bundler not compatible with subpath exports in gh-pages [Clear Signing] Bundler not compatible with subpath exports in gh-pages Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants