wallet: remove ledger support in favor of golang SDK implementation of ledger keychain #4413
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Introduces signing options infrastructure to provide transaction context (chain alias and network ID) to wallet signers. This enables remote signing services to properly format addresses and validate transactions while maintaining backward compatibility with existing signer implementations. Changes: - Add SigningOptions with functional options pattern - Update Signer interface to accept variadic SigningOption parameters - Pass chain alias and network ID to Sign() calls in P/X/C chain wallets - Update ledger and secp256k1 signers to accept but ignore options
Extends C-chain signer to accept and pass networkID parameter, completing the signing options infrastructure across all chains (P, X, and C).
Removes ledger hardware wallet support and the signHash parameter from wallet signers. Removed: - Ledger keychain implementation and related types - signHash parameter from P-chain and C-chain signers - utils/crypto/ledger package - Ledger-related tests and mocks - Ledger dependencies from go.mod
yacovm
approved these changes
Oct 10, 2025
This was referenced Oct 10, 2025
felipemadero
changed the base branch from
wallet-signing-improvements
to
master
The SigningOptions type and related functions (WithChainAlias, WithNetworkID) have been removed. The Signer.Sign() method no longer accepts signing options as the chain alias and network ID context are not needed for the actual signing operation. Changes: - Remove variadic SigningOption parameter from Signer.Sign() interface - Remove networkID field from all signer implementations (C/P/X chains) - Update all signer constructors to not accept networkID parameter - Remove WithChainAlias and WithNetworkID function calls - Update test files to use new signer constructor signatures
There was a problem hiding this comment.
Pull Request Overview
Removes built-in Ledger hardware wallet integration and simplifies wallet signing to always use Sign with full unsigned transaction bytes. Key changes:
- Eliminated ledger-specific code (wrapper, keychain implementations, mocks, tests, and dependencies).
- Removed signHash boolean path; all transactions now invoke signer.Sign(unsignedBytes).
- Updated keychain comments but retained SignHash in the Signer interface without internal use.
Reviewed Changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| wallet/chain/p/signer/visitor.go | Removed signHash parameter usage; all P-chain transactions now call sign(tx, txSigners). |
| wallet/chain/c/signer.go | Simplified atomic signer to remove hash signing path; always signs full bytes. |
| utils/crypto/ledger/ledger_test.go | Removed Ledger integration test. |
| utils/crypto/ledger/ledger.go | Removed Ledger device wrapper implementation. |
| utils/crypto/keychain/mocks_generate_test.go | Removed mock generation directive for Ledger. |
| utils/crypto/keychain/ledger.go | Removed Ledger interface definition (now unused). |
| utils/crypto/keychain/keychainmock/ledger.go | Removed generated Ledger mock. |
| utils/crypto/keychain/keychain_test.go | Removed Ledger keychain and signer tests. |
| utils/crypto/keychain/keychain.go | Deleted ledgerKeychain and ledgerSigner implementations; updated Signer comment. |
| go.mod | Dropped ledger-related dependencies and added an indirect objx requirement. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
joshua-kim
reviewed
Oct 16, 2025
| // to sign a hash | ||
| // to sign a hash or transaction | ||
| type Signer interface { | ||
| SignHash([]byte) ([]byte, error) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why this should be merged
This PR removes built-in Ledger hardware wallet support from AvalancheGo's wallet implementation, also delegating hash signing decisions to keychain implementations instead.
This change:
How this works
The implementation removes:
utils/crypto/ledger, ledger keychain, tests, and mocks)Sign(unsignedBytes, ...SigningOption)with full transaction bytes and contextKeychain implementations receive the full transaction bytes along with signing context (chain alias, network ID) and can decide internally how to sign. For example:
How this was tested