Feat: natively support clear signing with Ledger #4416
Conversation
Branch preview✅ Deploy successful! Storybook: |
Coverage report
Show files with reduced coverage 🔻
Test suite run success1707 tests passing in 231 suites. Report generated by 🧪jest coverage report action from 0358027 |
| @@ -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! Two Pages Changed SizeThe following pages changed size from the code in this PR compared to its base branch:
DetailsOnly the gzipped size is provided here based on an expert tip. First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If Any third party scripts you have added directly to your app using the Next to the size is how much the size has increased or decreased compared with the base branch of this PR. If this percentage has increased by 20% or more, there will be a red status indicator applied, indicating that special attention should be given to this. |
| @@ -3,7 +3,7 @@ import useShowOutreachPopup from '../useShowOutreachPopup' | |||
| import * as useIsSafeOwner from '@/hooks/useIsSafeOwner' | |||
| import * as store from '@/store' | |||
| import { HOUR_IN_MS } from '../../constants' | |||
| import { faker } from '@faker-js/faker/.' | |||
| import { faker } from '@faker-js/faker' | |||
There was a problem hiding this comment.
These were causing lint errors.
| import { type fakeBaseQuery } from '@reduxjs/toolkit/query/react' | ||
| import type { EndpointBuilder } from '@reduxjs/toolkit/query/react' |
There was a problem hiding this comment.
These were causing lint errors.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| SignTypedDataDAOutput, | ||
| TypedData, | ||
| } from '@ledgerhq/device-signer-kit-ethereum' | ||
| import type { Chain, WalletInit, WalletInterface } from '@web3-onboard/common' |
There was a problem hiding this comment.
Consider renaming mapOutput to enhance clarity and intent. Ensure that the function's purpose is immediately understandable, especially when it's within a broader context with potential side effects or transformations.
katspaugh
changed the title
|
Testing using ledger X, last firmware 2.4.0 The message is created by using the EIP1271 dummy app. I had no issues signing the message with MM, so it seems the message itself is not the problem |
|
Works for me signing messages Tested with ledger X and S |
|
Verified. Notes:
|
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