refactor: update ledger's hash signing requirements, and move hash signing decision down to keychain implementations #4389
Conversation
Move SignHash decision logic from wallet layer into ledger keychain implementation. The ledger keychain now determines when to use SignHash (for TransferSubnetOwnershipTx) versus regular Sign based on transaction type, simplifying the wallet interface and centralizing signing logic.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors transaction signing logic to move signing method decisions from the wallet layer into the ledger keychain. Previously, wallet code needed to know when to use SignHash vs Sign for different transaction types, creating tight coupling. The refactor centralizes this logic in the ledger keychain where it belongs.
- Moved most P-Chain transactions from hash signing to full-bytes signing after hardware testing
- Centralized signing method selection in ledger keychain with only TransferSubnetOwnershipTx still requiring hash signing
- Introduced signing options infrastructure to pass chain/network context to signers
Reviewed Changes
Copilot reviewed 24 out of 25 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| wallet/subnet/primary/wallet.go | Updated signer constructors to include networkID parameter |
| wallet/chain/x/signer/ | Added networkID parameter and signing options to X-chain signing |
| wallet/chain/p/signer/ | Removed signHash parameter, simplified to always use Sign method with options |
| wallet/chain/c/signer.go | Removed signHash parameter from C-chain atomic transaction signing |
| utils/crypto/secp256k1/secp256k1.go | Updated Sign method to accept signing options |
| utils/crypto/keychain/ | Moved ledger-specific code to separate package and added signing options |
| utils/crypto/keychain/ledger/ | New package containing ledger keychain implementation with signing logic |
| utils/crypto/keychain/cubesigner/ | New CubeSigner keychain implementation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // shouldUseSignHash determines if the transaction requires hash signing on the Ledger device. | ||
| // Currently, TransferSubnetOwnershipTx requires hash signing. | ||
| func shouldUseSignHash(unsignedTxBytes []byte) (bool, error) { | ||
| var unsignedTx txs.UnsignedTx | ||
| _, err := txs.Codec.Unmarshal(unsignedTxBytes, &unsignedTx) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
|
|
||
| _, isTransferSubnetOwnership := unsignedTx.(*txs.TransferSubnetOwnershipTx) | ||
| return isTransferSubnetOwnership, nil | ||
| } |
There was a problem hiding this comment.
The function name and comment describe checking if hash signing should be used, but the implementation only checks for TransferSubnetOwnershipTx. The comment should be more specific about this being the only transaction type requiring hash signing, or the function could be renamed to be more specific like isTransferSubnetOwnershipTx.
Copilot uses AI. Check for mistakes.
| if options.ChainAlias != "P" && options.ChainAlias != "X" && options.ChainAlias != "C" { | ||
| return nil, fmt.Errorf("%w, got %q", ErrInvalidChainAlias, options.ChainAlias) | ||
| } | ||
| if options.ChainAlias != "C" && options.NetworkID == 0 { |
There was a problem hiding this comment.
The condition options.NetworkID == 0 may incorrectly reject valid network ID 0. If 0 is a valid network ID, this should check if the network ID was explicitly set rather than comparing against 0.
| if options.ChainAlias != "C" && options.NetworkID == 0 { | |
| // The NetworkIDSet field should be set to true by the signing option when NetworkID is explicitly set. | |
| if options.ChainAlias != "C" && !options.NetworkIDSet { |
Copilot uses AI. Check for mistakes.
| UncompressedPublicKeyLength = 65 | ||
| // UncompressedPublicKeyPrefix is the prefix byte for uncompressed secp256k1 public keys. | ||
| // This byte indicates that the key is in uncompressed format. | ||
| UncompressedPublicKeyPrefix = 0x04 |
There was a problem hiding this comment.
These constants are duplicated knowledge that likely exists elsewhere in the codebase. Consider importing these constants from a shared crypto package instead of redefining them.
| UncompressedPublicKeyLength = 65 | |
| // UncompressedPublicKeyPrefix is the prefix byte for uncompressed secp256k1 public keys. | |
| // This byte indicates that the key is in uncompressed format. | |
| UncompressedPublicKeyPrefix = 0x04 | |
| UncompressedPublicKeyLength = avasecp256k1.PublicKeyBytes | |
| // UncompressedPublicKeyPrefix is the prefix byte for uncompressed secp256k1 public keys. | |
| // This byte indicates that the key is in uncompressed format. | |
| UncompressedPublicKeyPrefix = avasecp256k1.PublicKeyPrefix |
Copilot uses AI. Check for mistakes.
felipemadero
changed the title
felipemadero
changed the base branch from
master
to
cubesigner-keychain-on-master
Why this should be merged
Currently, the wallet layer needs to know when to use
SignHashvsSignfor different transaction types, creating tight coupling between wallet logic and signing implementation details. This PR moves that decision-making into the ledger keychain itself, simplifying the wallet interface and centralizing signing logic where it belongs.Additionally, this PR incorporates the refactoring changes from #4339 (ledger-keychain-move) to resolve circular dependency issues between the keychain and secp256k1fx packages.
How this works
Ledger Signing Improvements
The ledger keychain's
Signmethod now internally determines when to useSignHashbased on transaction type:SignHash(required by Ledger hardware)SignmethodTransactions that moved from SignHash → Sign
After comprehensive testing with Ledger hardware, these transactions were verified to work with full-bytes signing and no longer require hash signing:
P-Chain:
C-Chain:
Architecture Changes
Signutils/crypto/keychain/ledgerpackage to fix circular dependenciesHow this was tested
Need to be documented in RELEASES.md?