Cubesigner keychain #4277

felipemadero · 2025-09-15T16:19:47Z

Why this should be merged

This PR integrates CubeSigner SDK support into AvalancheGo.

How this works

The implementation introduces several key components:

CubeSigner Keychain (utils/crypto/keychain/cubesigner/): A new keychain implementation that communicates with CubeSigner's remote signing service, implementing both the standard Keychain and EthKeychain interfaces.
Extended Keychain Interface: The core keychain interface now includes SigningOption parameters to provide essential context (network ID, chain alias) to signers, enabling CubeSigner to perform context-aware signing.
Wallet Integration: Updated P-chain and X-chain wallet signers to pass network ID and chain alias context required for proper CubeSigner operation.

How this was tested

Unit Tests: coverage for CubeSigner keychain with mock implementations
Example test programs on fuji for creating a subnet and exporting P->C and C->P

Need to be documented in RELEASES.md?

Integrate CubeSigner SDK for remote key signing. Key changes: - Add CubeSigner SDK dependency and keychain implementations - Extend keychain interface with SigningOption so as to provide tx context to cubesigner - Move EthKeychain interface to the same package of the Keychain interface - Enable forced hash signing with new wallet option

…ed │ secp256k1 key types are accepted. Fix test cases and remove obsolete │ ledger keychain files from main package. │ │ Key changes: │ - Add validation for SecpAvaAddr, SecpAvaTestAddr, SecpEthAddr key types │ - Fix TestCubesignerKeychain_GetEth to test non-existent address handling │ - Remove duplicate ledger keychain implementation files

Copilot

Pull Request Overview

This PR integrates CubeSigner SDK support into AvalancheGo, maintaining full backward compatibility while extending the keychain interface to support additional signing backends. The implementation introduces context-aware signing and refactors ledger-specific functionality.

Adds CubeSigner keychain implementation for remote signing service integration
Extends keychain interface with signing options to provide transaction context (network ID, chain alias)
Refactors ledger functionality to a dedicated package and updates wallet signers to pass network/chain context

Reviewed Changes

Copilot reviewed 28 out of 29 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
wallet/subnet/primary/wallet.go	Updates signer constructors to pass network ID for context-aware signing
wallet/chain/x/signer/visitor.go	Implements signing options and force SignHash option for transaction signing
wallet/chain/p/signer/visitor.go	Adds network ID context and force SignHash support to P-chain signing
utils/crypto/keychain/cubesigner/	New CubeSigner keychain implementation with client abstraction and tests
utils/crypto/keychain/ledger/	Refactored ledger keychain moved to dedicated package with updated interfaces
utils/crypto/keychain/signing_options.go	New signing options framework for chain and network context

Comments suppressed due to low confidence (1)

utils/crypto/keychain/cubesigner/cubesigner_keychain.go:1

Remove the extra blank line to maintain consistent formatting.

// Copyright (C) 2019-2024, Ava Labs, Inc. All rights reserved.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

wallet/subnet/primary/wallet.go

wallet/chain/x/signer/visitor.go

wallet/chain/p/signer/visitor.go

wallet/chain/c/signer.go

Update copyright year range from 2019-2024 to 2019-2025 across all files modified in the CubeSigner keychain integration.

utils/crypto/keychain/cubesigner/cubesigner_keychain.go

utils/crypto/keychain/cubesigner/cubesigner_client.go

utils/crypto/keychain/cubesigner/cubesigner_keychain.go

utils/crypto/keychain/cubesigner/cubesigner_client.go

utils/crypto/keychain/cubesigner/cubesigner_keychain.go

utils/crypto/keychain/cubesigner/cubesigner_keychain_test.go

utils/crypto/keychain/cubesigner/mocks_generate_test.go

utils/crypto/keychain/ledger/ledger.go

utils/crypto/keychain/ledger/ledger_keychain.go

utils/crypto/keychain/ledger/mocks_generate_test.go

- Remove package name stuttering in CubeSigner and ledger packages - Renamed CubesignerKeychain to Keychain, ledgerKeychain to KeyChain - godoc comments to exported types and functions - Extract hardcoded constants (65, 0x04) with descriptive names - Update both packages to return concrete types (*KeyChain) for consistency

- Add godoc comments to all public methods in ledger keychain - Add unit tests for ledger Sign()

joshua-kim · 2025-09-24T21:34:57Z

Can we break this PR into individual changes? It seems like there are multiple changes being added within the scope of this PR.

- Update P chain wallet to properly use walletsigner.WithOptions

- Remove forceSignHash functionality from wallet signers - Keep networkID support essential for CubeSigner keychain - Restore original ledger keychain implementation location - Use master's hardcoded signHash values instead of options - Remove P-chain signer with_options wrapper - Maintain EthKeychain interface in wallet/chain/c package - All tests and linting pass with reduced scope

felipemadero · 2025-09-25T16:01:48Z

Can we break this PR into individual changes? It seems like there are multiple changes being added within the scope of this PR.

ok the scope of this one has been reduced cc @StephenButtolph

felipemadero · 2025-09-25T18:29:12Z

Can we break this PR into individual changes? It seems like there are multiple changes being added within the scope of this PR.

ok the scope of this one has been reduced cc @StephenButtolph

I made #4337 #4339 #4344 with the parts I removed from this one cc @joshua-kim @yacovm @sukantoraymond @StephenButtolph

yacovm · 2025-09-25T21:05:11Z

Can we break this PR into individual changes? It seems like there are multiple changes being added within the scope of this PR.

ok the scope of this one has been reduced cc @StephenButtolph

I made #4337 #4339 #4344 with the parts I removed from this one cc @joshua-kim @yacovm @sukantoraymond @StephenButtolph

I am a bit confused... shouldn't you rebase the PRs one on top of the other and this PR last? Otherwise you'll have conflicts, am I wrong?

felipemadero · 2025-09-25T22:51:02Z

Can we break this PR into individual changes? It seems like there are multiple changes being added within the scope of this PR.

ok the scope of this one has been reduced cc @StephenButtolph

I made #4337 #4339 #4344 with the parts I removed from this one cc @joshua-kim @yacovm @sukantoraymond @StephenButtolph

I am a bit confused... shouldn't you rebase the PRs one on top of the other and this PR last? Otherwise you'll have conflicts, am I wrong?

This time I opted to present independent PRs so they can be separately reviewed and merged into master. Yes, I expect some conflicts when merging later PRs. This approach allows for parallel review and gives reviewers more focused context for each change, rather than having to understand the dependency chain.

felipemadero added 2 commits September 15, 2025 12:48

Copilot AI review requested due to automatic review settings September 15, 2025 16:19

felipemadero requested a review from StephenButtolph as a code owner September 15, 2025 16:19

github-project-automation bot added this to avalanchego Sep 15, 2025

Copilot AI reviewed Sep 15, 2025

View reviewed changes

wallet/subnet/primary/wallet.go Outdated Show resolved Hide resolved

wallet/chain/x/signer/visitor.go Outdated Show resolved Hide resolved

wallet/chain/p/signer/visitor.go Outdated Show resolved Hide resolved

wallet/chain/c/signer.go Outdated Show resolved Hide resolved

felipemadero added 2 commits September 15, 2025 13:29

Update copyright headers to 2019-2025

dde6d27

Update copyright year range from 2019-2024 to 2019-2025 across all files modified in the CubeSigner keychain integration.

Merge branch 'master' into cubesigner-keychain-on-master

2ba5613

felipemadero changed the title ~~Cubesigner keychain on master~~ Cubesigner keychain Sep 15, 2025

felipemadero added 2 commits September 15, 2025 14:43

lint

753a25d

license lint

3da2018

sukantoraymond reviewed Sep 15, 2025

View reviewed changes

utils/crypto/keychain/cubesigner/cubesigner_keychain.go Show resolved Hide resolved

sukantoraymond reviewed Sep 15, 2025

View reviewed changes

utils/crypto/keychain/cubesigner/cubesigner_keychain.go Show resolved Hide resolved

StephenButtolph requested review from a team, DracoLi and yacovm and removed request for a team and DracoLi September 16, 2025 20:15

Merge branch 'master' into cubesigner-keychain-on-master

62f91fa

yacovm reviewed Sep 18, 2025

View reviewed changes

felipemadero added 2 commits September 20, 2025 10:59

Merge branch 'master' into cubesigner-keychain-on-master

0f6dd6a

felipemadero requested review from yacovm and sukantoraymond September 20, 2025 18:42

Add godoc comments and unit tests for ledger Sign method

158faf1

- Add godoc comments to all public methods in ledger keychain - Add unit tests for ledger Sign()

yacovm approved these changes Sep 22, 2025

View reviewed changes

Merge branch 'master' into cubesigner-keychain-on-master

c480d7a

sukantoraymond approved these changes Sep 24, 2025

View reviewed changes

felipemadero added 2 commits September 25, 2025 09:37

Fix P chain wallet signer integration

2eb5ee3

- Update P chain wallet to properly use walletsigner.WithOptions

Merge branch 'master' into cubesigner-keychain-on-master

0de0c8f

joshua-kim moved this to Ready 🚦 in avalanchego Sep 29, 2025

felipemadero and others added 3 commits October 1, 2025 07:28

Merge branch 'master' into cubesigner-keychain-on-master

8d68829

Merge branch 'master' into cubesigner-keychain-on-master

e30b4a1

Merge branch 'master' into cubesigner-keychain-on-master

5bb4caa

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Cubesigner keychain #4277

Cubesigner keychain #4277

Uh oh!

felipemadero commented Sep 15, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joshua-kim commented Sep 24, 2025

Uh oh!

felipemadero commented Sep 25, 2025

Uh oh!

felipemadero commented Sep 25, 2025

Uh oh!

yacovm commented Sep 25, 2025

Uh oh!

felipemadero commented Sep 25, 2025

Uh oh!

Uh oh!

Cubesigner keychain #4277

Are you sure you want to change the base?

Cubesigner keychain #4277

Uh oh!

Conversation

felipemadero commented Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why this should be merged

How this works

How this was tested

Need to be documented in RELEASES.md?

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joshua-kim commented Sep 24, 2025

Uh oh!

felipemadero commented Sep 25, 2025

Uh oh!

felipemadero commented Sep 25, 2025

Uh oh!

yacovm commented Sep 25, 2025

Uh oh!

felipemadero commented Sep 25, 2025

Uh oh!

Uh oh!

felipemadero commented Sep 15, 2025 •

edited

Loading