-
Notifications
You must be signed in to change notification settings - Fork 185
Feat new provider ledger #1537
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
base: main
Are you sure you want to change the base?
Feat new provider ledger #1537
Conversation
🦋 Changeset detectedLatest commit: b034777 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a new Ledger hardware wallet provider for Ant Design Web3, enabling integration with Ledger devices through WebHID API. The implementation provides secure hardware wallet connectivity with device management, message signing capabilities, and React hooks for device interaction.
Key changes:
- Adds complete Ledger wallet adapter package with React provider and hooks
- Implements device discovery, connection management, and Ethereum signing functionality
- Provides comprehensive documentation and examples for Ledger integration
Reviewed Changes
Copilot reviewed 28 out of 30 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ledger/package.json | Package configuration with Ledger SDK dependencies |
| packages/ledger/src/ledger/*.ts | Core Ledger device management hooks and utilities |
| packages/ledger/src/wallets/ledger.tsx | Main Ledger wallet factory implementation |
| packages/ledger/src/provider/*.tsx | React providers for Ledger integration |
| packages/ledger/src/types.ts | TypeScript interfaces for Ledger wallet types |
| packages/ledger/docs/* | Documentation and usage examples |
| .changeset/vast-tools-poke.md | Changeset for the new feature |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| }, | ||
|
|
||
| signMessage: async (message: string) => { | ||
| console.log('Signing message:', message); |
Copilot
AI
Oct 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Console.log statements should be removed or replaced with proper logging mechanism. These debug logs will appear in production code.
| return { | ||
| name: 'Ledger', | ||
| adapter, | ||
| icon: undefined, // TODO: Add Ledger icon |
Copilot
AI
Oct 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO comment indicates incomplete implementation. The icon should be implemented or the TODO should be tracked in an issue.
| setDeviceStatus(state.deviceStatus); | ||
|
|
||
| // Extract current app name if available | ||
| const appName = (state as any)?.currentApp?.name; |
Copilot
AI
Oct 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using 'any' type defeats TypeScript's type safety. Define proper types for the state object or use more specific type assertion.
| const appName = (state as any)?.currentApp?.name; | |
| const appName = (typeof state === 'object' && state !== null && 'currentApp' in state && typeof (state as { currentApp?: { name?: string } }).currentApp?.name === 'string') | |
| ? (state as { currentApp: { name: string } }).currentApp.name | |
| : undefined; |
| selectWallet(wallet); | ||
| } | ||
| } | ||
| }, [wallets]); |
Copilot
AI
Oct 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The useEffect dependency array includes 'wallets' but the effect also depends on 'autoConnect', 'latestWalletNameRef.current', and 'adapterName'. Missing dependencies could cause stale closures.
| }, [wallets]); | |
| }, [wallets, autoConnect, adapterName, latestWalletNameRef]); |
Co-authored-by: Copilot <[email protected]>
|
有冲突了,需要 rebase 一下 |
| @@ -0,0 +1,14 @@ | |||
| import { ConnectButton, Connector } from '@ant-design/web3'; | |||
| import { Ledger, LedgerWeb3ConfigProvider } from '@ant-design/web3-ledger'; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ant-design/web3-ledger 只支持 Ladger 还是未来也可能支持其它硬件钱包?如果是可能支持其他硬件钱包,那命名是不是应该改一下。那如果只支持 Legder,那是不是 Ledger 也没必要配置了,默认就行。
| try { | ||
| devicesRef = [await discoverFn()]; | ||
| } catch (error) { | ||
| message.error('Failed to discover device'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里应该不需要显示错误,而是应该抛出异常,adapter 应该不要做任何和 UI 相关的事情
| export interface LedgerWeb3ConfigProviderProps { | ||
| wallet?: WalletFactory; | ||
| locale?: Locale; | ||
| // balance?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
没用的代码可以直接删除

[中文版模板 / Chinese template]
💡 Background and solution
🔗 Related issue link