Add Beta Ledger Wallet Support with JS SCP - #472
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive Ledger hardware wallet support to Pelagus, enabling users to connect, manage, and transact with Ledger devices through a secure JavaScript SCP (Secure Channel Protocol) implementation.
- Implements complete Ledger wallet integration with device connection, app management, and address derivation
- Adds secure transaction signing using protobuf encoding and proper error handling for Ledger-specific scenarios
- Integrates Ledger accounts into the existing account management system with UI support and proper categorization
Reviewed Changes
Copilot reviewed 37 out of 42 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/utils/ledger/ | Core Ledger functionality including device communication, address derivation, and SCP implementation |
| ui/pages/LedgerWallet.tsx | Main Ledger wallet management interface for users |
| ui/pages/LedgerConnect.tsx | Comprehensive device connection and setup interface |
| background/services/ledger/ | Backend Ledger signing service with protobuf transaction support |
| background/redux-slices/ledger.ts | Redux state management for Ledger devices and addresses |
| ui/components/Signing/ | Enhanced signing components to handle Ledger-specific error flows |
Comments suppressed due to low confidence (1)
ui/components/Signing/Signer/SignerKeyring/SignerKeyringSigning.tsx:1
- Remove the large block of commented-out test code. If this functionality might be needed for debugging, consider moving it to a development utility or environment-specific configuration.
import { selectKeyringStatus } from "@pelagus/pelagus-background/redux-slices/selectors"
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| const pubKeyLen = xpubData[0] | ||
| const publicKey = xpubData.slice(1, 1 + pubKeyLen) | ||
| const addrLen = xpubData[1 + pubKeyLen] | ||
| // const address = xpubData.slice(2 + pubKeyLen, 2 + pubKeyLen + addrLen) // Not used, commented out |
There was a problem hiding this comment.
Remove the commented-out unused variable declaration. This reduces code clutter and improves readability.
| // const address = xpubData.slice(2 + pubKeyLen, 2 + pubKeyLen + addrLen) // Not used, commented out |
| // chainId >>= 8; // Not needed for small values like 9 | ||
| } | ||
|
|
There was a problem hiding this comment.
Remove the commented-out code. If the bit shift operation isn't needed for small chain ID values, the comment can be removed entirely.
| // chainId >>= 8; // Not needed for small values like 9 | |
| } | |
| } |
| // Convert chain ID to big-endian 8 bytes | ||
| for (let i = 7; i >= 0; i--) { | ||
| chainIdBytes[i] = chainId & 0xff | ||
| // chainId >>= 8; // Not needed for small values like 9 |
There was a problem hiding this comment.
Remove the duplicate commented-out code. This is the same unnecessary bit shift operation that appears earlier in the file.
| // chainId >>= 8; // Not needed for small values like 9 |
| // @ts-nocheck | ||
| import type Transport from "@ledgerhq/hw-transport"; | ||
| import type { EIP712Message } from "@ledgerhq/types-live"; | ||
|
|
There was a problem hiding this comment.
Avoid using @ts-nocheck as it disables all TypeScript checking for the entire file. Consider addressing specific type issues instead or using more targeted suppressions like @ts-ignore for individual lines.
| // @ts-nocheck | |
| import type Transport from "@ledgerhq/hw-transport"; | |
| import type { EIP712Message } from "@ledgerhq/types-live"; | |
| import type Transport from "@ledgerhq/hw-transport"; | |
| import type { EIP712Message } from "@ledgerhq/types-live"; | |
| // Define EIP712Field if not imported from elsewhere | |
| type EIP712Field = { name: string; type: string }; |
|
|
||
| removeAccount(address: HexString): void { | ||
| const chainIds = Object.keys(this.cachedResolvedNames.EVM) | ||
| const chainIds = Object.keys(this.cachedResolvedNames) |
There was a problem hiding this comment.
The code is accessing this.cachedResolvedNames directly, but based on the context, it should be accessing this.cachedResolvedNames.EVM as indicated by the original code pattern. This could cause runtime errors if the structure doesn't match expectations.
|
|
||
| removeActivities(address: HexString): void { | ||
| const chainIds = Object.keys(this.cachedResolvedNames.EVM) | ||
| const chainIds = Object.keys(this.cachedResolvedNames) |
There was a problem hiding this comment.
The code is accessing this.cachedResolvedNames directly, but based on the context, it should be accessing this.cachedResolvedNames.EVM as indicated by the original code pattern. This could cause runtime errors if the structure doesn't match expectations.
| // Verify we have the correct device after connection | ||
| if(this.transport?.device.productName !== accountSigner.deviceModel && !(this.transport?.device.productName.includes("S+") && accountSigner.deviceModel.includes("S Plus"))) { | ||
| throw new Error(`LedgerSigner: Have a ${this.transport?.device.productName} ${this.transport?.device.productId} but need a ${accountSigner.deviceModel} ${accountSigner.deviceId}`) | ||
| } |
There was a problem hiding this comment.
The device verification logic is complex and hard to read. Consider extracting this into a separate method like verifyDeviceMatch(currentDevice, expectedDevice) to improve readability and testability.
| this.store.dispatch(resetProgressStates()) | ||
|
|
||
| this.connectPopupMonitor() | ||
| //this.connectLedgerHandler() |
There was a problem hiding this comment.
Remove the commented-out method call. If this functionality is not needed, the comment should be removed to avoid confusion.
| //this.connectLedgerHandler() |
09038cc to
212f207
Compare
e6ac5f0 to
df3d0b9
Compare
No description provided.