fix: natively support clear signing with Ledger #4416
Conversation
Branch preview✅ Deploy successful! Storybook: |
Coverage report
Show files with reduced coverage 🔻
Test suite run success1573 tests passing in 208 suites. Report generated by 🧪jest coverage report action from 7d2244c |
| @@ -0,0 +1,349 @@ | |||
| import type { DeviceActionState } from '@ledgerhq/device-management-kit' | |||
| import type { SignTransactionDAOutput, SignTypedDataDAOutput, TypedData } from '@ledgerhq/device-signer-kit-ethereum' | |||
| import type { GetAddressCommandResponse } from '@ledgerhq/device-signer-kit-ethereum/api/app-binder/GetAddressCommandTypes.js' | |||
There was a problem hiding this comment.
Can be imported directly from the index on the package. it should not be imported directly with a direct path.
If the export is not present in the index, please file an issue here https://github.com/LedgerHQ/device-sdk-ts/issues, it's probably a bug / we forgot to export it
There was a problem hiding this comment.
It doesn't seem possible as GetAddressCommandResponse is not exported from the relevant barrel file.
There was a problem hiding this comment.
Hello 👋 In fact it's the wrong type that you are using, it should be GetAddressDAOutput, and this one is accessible 👍
There was a problem hiding this comment.
(in the end they are equal)
There was a problem hiding this comment.
Thank you. I changed it in 05f3955.
iamacook
changed the title
| @@ -13,7 +13,7 @@ | |||
| "noEmit": true, | |||
| "esModuleInterop": true, | |||
| "module": "esnext", | |||
| "moduleResolution": "node", | |||
| "moduleResolution": "Bundler", | |||
There was a problem hiding this comment.
This is required to support the SDK.
| import type { | ||
| Permission, | ||
| PermissionRequest, | ||
| } from 'node_modules/@safe-global/safe-apps-sdk/dist/types/types/permissions' |
There was a problem hiding this comment.
This (and the other similar changes) are required to support the adjusted moduleResolution.
📦 Next.js Bundle Analysis for safe-wallet-webThis analysis was generated by the Next.js Bundle Analysis action. 🤖 🎉 Global Bundle Size Decreased
DetailsThe global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster. Any third party scripts you have added directly to your app using the If you want further insight into what is behind the changes, give @next/bundle-analyzer a try! |
|
WalletConnect's AppKit now has Ledger support so we should probably migrate from onboard to that. |
What it solves
The inability to perform clear signing with Ledger devices.
How this PR fixes it
This replaces the outdated
@web3-onboard/ledgermodule, which relied on an archived Ledger package, with a custom module built on Ledger's new Web Management Device Kit.How to test it
Connect a Ledger device and test each RPC method (which corresponds to each SDK method):
Checklist