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

feat: Ledger connector with support for WalletConnect v2 #1549

Merged

Conversation

hlopes-ledger
Copy link
Contributor

@hlopes-ledger hlopes-ledger commented Feb 24, 2023

Description

We (Ledger) have a working connector which supports the Ledger browser Extension and WalletConnect v1. This PR adds support for WalletConnect v2, based on code from the WalletConnect connector and an updated Ledger Connect Kit library.

We've added new parameters to the connector

  • walletConnectVersion: 1 | 2,
  • enableDebugLogs?: boolean,
  • projectId: string
  • requiredChains?: string[] | number[]
  • requiredMethods?: string[]
  • optionalMethods?: string[]
  • requiredEvents?: string[]
  • optionalEvents?: string[]

Checklist

  • The version field in package.json of the package you have made changes in is incremented following semantic versioning and using alpha release tagging
  • The box that allows repo maintainers to update this PR is checked
  • I tested locally to make sure this feature/fix works
  • I have run yarn file-check, yarn type-check & yarn build to confirm there are not any associated errors
  • This PR passes the Circle CI checks

If this PR includes changes to add an injected wallet or SDK wallet module:

Please complete the following using the internal demo package.
To run this demo use the command yarn && yarn dev to get the project running at http://localhost:8080/

Wasn't able to run the internal demo, tested with this custom example app, https://64905dc39ea33271d1d1a2f4--ledger-w3o-vite-demo.netlify.app/

Tests with demo app (SDK)

  • send transaction
  • switch chains
  • sign message
  • sign typed message
  • disconnect

@vercel
Copy link

vercel bot commented Feb 24, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
web3-onboard-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 28, 2023 6:40pm

@socket-security
Copy link

socket-security bot commented May 23, 2023

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

@socket-security
Copy link

socket-security bot commented Jun 14, 2023

New dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
@ledgerhq/connect-kit-loader 1.1.0 None +0 17.6 kB sergii-shkolin

@hlopes-ledger hlopes-ledger force-pushed the feat/ledger-wallet-connect-v2 branch 2 times, most recently from 5883656 to 3038b2a Compare June 19, 2023 12:10
@hlopes-ledger hlopes-ledger marked this pull request as ready for review June 19, 2023 13:24
@hlopes-ledger hlopes-ledger force-pushed the feat/ledger-wallet-connect-v2 branch from 3038b2a to 024401a Compare June 20, 2023 15:37
@hlopes-ledger hlopes-ledger force-pushed the feat/ledger-wallet-connect-v2 branch from 024401a to 20d8a18 Compare June 21, 2023 11:50
Copy link
Collaborator

@lnbc1QWFyb24 lnbc1QWFyb24 left a comment

Choose a reason for hiding this comment

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

Looks good, just a couple of small changes requested.

I still need to test it out locally, but I wanted to ask which platforms is the Ledger Connect not available so that I can also try out the WalletConnect functionality?

packages/ledger/src/v2.ts Outdated Show resolved Hide resolved
packages/ledger/src/v2.ts Outdated Show resolved Hide resolved
packages/ledger/src/v2.ts Outdated Show resolved Hide resolved
@hlopes-ledger
Copy link
Contributor Author

Looks good, just a couple of small changes requested.

I still need to test it out locally, but I wanted to ask which platforms is the Ledger Connect not available so that I can also try out the WalletConnect functionality?

Sorry for not including that on the PR description. You can test the WalletConnect support on Chrome; the Extension is currently only available on Safari iOS and macOS.

@hlopes-ledger
Copy link
Contributor Author

We've released the final version of Connect Kit and I've updated the package dependency.

Copy link
Contributor

@leightkt leightkt left a comment

Choose a reason for hiding this comment

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

Please update the package.json for packages/ledger to:
"version": "2.4.7-alpha.1",

Co-authored-by: Adam Carpenter <adamcarpenter86@gmail.com>
Copy link
Member

@Adamj1232 Adamj1232 left a comment

Choose a reason for hiding this comment

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

Nice work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants