refactor: account management #191

felipemadero · 2025-11-05T18:34:21Z

Why this should be merged

Implements the wallet interface refactor agreed upon in ADRs [#4-a, #4-b, #5]. This provides:

Unified account management across wallet types
Separate EVM operations interface
Refactored P/X/C operations into focused files
Foundation for ongoing P/X/C and EVM implementation work

How this works

New Wallet interface with accounts, network, evm and primary operation methods
local/accounts.go: Account management operations
local/evm.go: EVM-specific operations
local/primary_operations.go: P/X/C chain operations
Updated examples to demonstrate new patterns

…d account management This commit updates the wallet refactoring based on ADR decisions: - PrimaryOperations interface for P/X/C chain operations - Adds Primary() method to wallet interface that returns PrimaryOperations - Refactors BuildTx/SignTx/SendTx to use account names instead of account objects - Adds SubmitTx convenience method that combines build, sign, and send - Implements account management with named accounts and active account tracking - Adds AccountInfo for public account data and AccountSpec for import/export - Creates helper functions for wallet refresh and account operations - Moves commit retry logic to pchain.SendTx with sendWithRetry helper - Splits wallet operations into separate files (accounts.go, primary_operations.go, evm.go) - Updates example to use new API with environment variables - Changes from embedded fields to explicit members (LocalWallet.wallet, LocalAccount.softKey) - Removes unused wallet/builder.go, wallet/sender.go, wallet/signer.go files - Adds contract method wrapper and functional options pattern - Implements WalletRefreshTimeout context for account switching

sukantoraymond · 2025-11-06T16:19:45Z

This PR can definitely be broken down into smaller PRs (e.g. one with only evm interface, another with moving submit tx, etc)

Updates the wallet interface to match wallet-local-evm branch: - Replace NewContractMethod with Method function - Add ParseResult helper for typed result extraction - Update ReadContract signature to remove outputParams parameter - Revert GetCChainAddress to not take network parameter - Improve error handling in P/X chain address methods - Update all interface examples to use consistent naming (w instead of wallet)

Remove network parameter from GetCChainAddress call in accounts.go to match the updated interface signature.

sukantoraymond · 2025-11-07T16:39:04Z

account/account.go

+
+// AccountInfo contains public information about an account
+// Does NOT expose private key material
+type AccountInfo struct {
+	Name       string // Account name
+	PAddress   string // P-Chain address
+	XAddress   string // X-Chain address
+	CAddress   string // C-Chain address
+	EVMAddress string // EVM address (0x format)
+}


I dont think we need this. We should keep account only to interface getter functions.

This is already agreed upon on ADR 5.

I dont think it is, adr 5 discussion was all around evm and p/x/c placements in wallet interface

Based on the discussions by chat and meeting over ADR 4, a consensus was obtained on the account management interface. I wrote ADR 5 to have record of the decisions.

Account naming features was discussed on adr 5, but not specific implementation, IIRC

anyways, IMO this reduces simplicity of account interface, but open to second opinions

This seems awkwardly placed here since none of the Account interface methods return this struct. The interface should have GetAccountInfo(). I'm still not fully convinced we need this struct to begin with. I will defer my review of this struct after some other comments of mine are resolved.

sukantoraymond · 2025-11-07T16:42:50Z

account/account.go

+// AccountSpec contains account creation/import specifications
+type AccountSpec struct {
+	PrivateKey     string // Hex-encoded private key (for local accounts)
+	DerivationPath string // For Ledger accounts
+	RemoteKeyID    string // For remote signing services
+}


Same here, seems that each field is specific to each account type like local accountn, server wallet, etc. They can be localized to each account type. like private key can be private getter function in local account

This is already agreed upon on ADR 5

I dont think it is, adr 5 discussion was all around evm and p/x/c placements in wallet interface

sukantoraymond · 2025-11-07T16:51:18Z

examples/create_chain.go

+
+	// Get required environment variables
+	privateKey := os.Getenv("PRIVATE_KEY")
+	if privateKey == "" {
+		return fmt.Errorf("PRIVATE_KEY environment variable is required")
+	}
+
+	subnetAuthKey := os.Getenv("SUBNET_AUTH_KEY")
+	if subnetAuthKey == "" {
+		return fmt.Errorf("SUBNET_AUTH_KEY environment variable is required")
+	}
+
+	subnetID := os.Getenv("SUBNET_ID")
+	if subnetID == "" {
+		return fmt.Errorf("SUBNET_ID environment variable is required")
+	}
+
+	evmAddress := os.Getenv("EVM_ADDRESS")
+	if evmAddress == "" {
+		return fmt.Errorf("EVM_ADDRESS environment variable is required")
+	}
+
+	blockchainName := os.Getenv("BLOCKCHAIN_NAME")
+	if blockchainName == "" {
+		blockchainName = "MyBlockchain"
+	}
+


I believe its important to not use Get ENV but instead use the var format i have in the other exmaple file so users know what thse variables should ook like. For example, subnet auth key, how will users know it should be P-xxx? o r Node iD-> NodeID-xxxxx

Same goes for all example files

I believe it is best to use get env so the file is executable and reusable in different settings. We can also have:

add a comment on the expected format, both as code comment above Getenv() call and error returned to the used when variable is not set (eg "EVM_ADDRESS environment variable of format "..." (eg "...") is required").

also in certain cases have a default value is no one is given

sukantoraymond · 2025-11-07T16:52:19Z

example/create_chain.go

-
-	createChainParams := &pchainTxs.CreateChainTxParams{
-		SubnetAuthKeys: []string{"P-fujixxxxx"},
-		SubnetID:       subnetID,
-		VMID:           vmID.String(),
-		ChainName:      blockchainName,
-		Genesis:        evmGenesisBytes,
-	}
-	buildTxParams := types.BuildTxParams{
-		Account:      *existingAccount,
-		Network:      network,
-		BuildTxInput: createChainParams,
-	}
-	buildTxResult, err := localWallet.BuildTx(ctx, buildTxParams)
-	if err != nil {
-		return fmt.Errorf("failed to BuildTx: %w", err)
-	}
-
-	signTxParams := types.SignTxParams{
-		Account:       *existingAccount,
-		Network:       network,
-		BuildTxResult: &buildTxResult,
-	}


These examples should be kept so users know they can also use build/sign/send in addition to submit

Same goes for all 3 example files

I believe we want to propose SubmitTx as the operation to use in examples. Maybe we can just add
one example that shows the intermediate parts but not duplicate for everything. Eg I can add just an
example create_sign_send_subnet_tx.go

ofc this example (and the other 3) were not removed but:

moved into examples/ so we stop having directories examples/ and example/

refactored to use SudmitTx

sukantoraymond · 2025-11-07T18:04:30Z

wallet/types/builder.go

 type BuildTxParams struct {
-	Account account.Account
-	Network network.Network
+	AccountNames []string


This can be account

Much cleaner, it can remain []string

sukantoraymond · 2025-11-07T18:05:05Z

wallet/types/submit.go

@@ -0,0 +1,37 @@
+// Copyright (C) 2025, Ava Labs, Inc. All rights reserved.


why not just submit.go

sukantoraymond · 2025-11-07T18:13:38Z

wallet/wallet.go

+
+	// Primary returns the interface for P/X/C chain operations
+	// Example: w.Primary().BuildTx(...)
+	Primary() PrimaryOperations


I still think Primary is confusing to external devs. I would go for Avalanche, but open to opinions

I have no preference here. So it can be Avalanche. cc @jasonatran @artemanosov

sukantoraymond · 2025-11-07T18:19:20Z

wallet/result.go

+//	result, err := w.ReadContract(contractAddr, method)
+//	addr, err := wallet.ParseResult[common.Address](result, 0)
+//	amount, err := wallet.ParseResult[*big.Int](result, 1)
+func ParseResult[T any](result []interface{}, index ...int) (T, error) {


should jsut place all this into helper.go

sukantoraymond · 2025-11-07T18:22:52Z

wallet/chains/pchain/sender.go

 )

+// sendWithRetry submits a transaction to the network with retry logic
+func sendWithRetry(wallet *primary.Wallet, tx *txs.Tx) error {


is this commit? should remain commit as function name

sukantoraymond · 2025-11-07T18:23:32Z

wallet/chains/pchain/sender.go

 func SendTx(wallet *primary.Wallet, params types.SendTxParams) (types.SendTxResult, error) {
-	// Get the P-Chain transaction from the SignTxResult
-	pChainTx, ok := params.SignTxResult.GetTx().(*avagoTxs.Tx)
-	if !ok {
-		return types.SendTxResult{}, fmt.Errorf("expected P-Chain transaction, got %T", params.SignTxResult.GetTx())
+	// Validate transaction is defined
+	if params.SignTxResult.Undefined() {
+		return types.SendTxResult{}, multisig.ErrUndefinedTx
 	}

-	// Submit the signed transaction to the network
-	if err := wallet.P().IssueTx(pChainTx); err != nil {
+	// Validate transaction is ready to commit
+	isReady, err := params.SignTxResult.IsReadyToCommit()
+	if err != nil {
+		return types.SendTxResult{}, err
+	}
+	if !isReady {
+		return types.SendTxResult{}, errors.New("tx is not fully signed so can't be committed")
+	}
+
+	// Extract the P-Chain transaction
+	tx, err := params.SignTxResult.GetWrappedPChainTx()
+	if err != nil {
+		return types.SendTxResult{}, err
+	}
+
+	// Submit the signed transaction to the network with retry logic
+	if err := sendWithRetry(wallet, tx); err != nil {


why is this function even being changed?

sukantoraymond · 2025-11-07T18:24:30Z

wallet/local/primary_operations.go

+
+	// Determine which wallet to use
+	var walletToUse *primary.Wallet
+	if len(params.AccountNames) > 0 {


break this up into the orignial sign/send/build files that i had

jasonatran · 2025-11-08T00:29:14Z

account/account.go

 	// GetPChainAddress returns the P-Chain address for the given network
 	GetPChainAddress(network network.Network) (string, error)

+	// GetXChainAddress returns the X-Chain address for the given network


I think we should document what an account is here succinctly. In this case, it is a set of X/P/EVM addresses. This is equivalent to an avalanchego secp256k1fx.Keychain with just a single address of all types (avax type or EVM type). X and P can be empty or will be auto-filled if you create an Account with a constructor. I have no preference between these two.

jasonatran · 2025-11-08T00:37:06Z

I really think this PR needs to be broken down into separate PRs. This is way too large for us to adequately review this. I have only commented on the Account interface and local_account implementation.

jasonatran · 2025-11-08T00:38:59Z

account/local_account.go

 // LocalAccount represents a local account implementation
 type LocalAccount struct {
-	*key.SoftKey
+	softKey *key.SoftKey


We should drop this softkey abstraction. The implementation of LocalAccount should have a clear association with the Account interface. using this SoftKey abstraction adds another layer of indirection before you can tease out where the X P C EVM addresses are stored in the struct.

jasonatran · 2025-11-08T00:43:24Z

wallet/local/accounts.go

+)
+
+// generateAccountName generates an automatic account name based on the current count
+func (w *LocalWallet) generateAccountName() string {


Should the account name be part of the Account interface and implementation?

jasonatran · 2025-11-08T00:47:25Z

wallet/local/accounts.go

+}
+
+// Accounts returns all accounts managed by this wallet with their info
+func (w *LocalWallet) Accounts() map[string]account.AccountInfo {


To be consistent, these getter functions should return a type account.Account. All functions that return something related to an Account should be returning account.Account not account.AccountInfo.

jasonatran · 2025-11-08T00:53:17Z

wallet/local/accounts.go

+}
+
+// ImportAccount imports an account with a name
+func (w *LocalWallet) ImportAccount(name string, spec account.AccountSpec) (account.AccountInfo, error) {


The use of account.AccountSpec may seem more simple and more convenient to import accounts quickly, but I think the additional struct of account.AccountSpec adds some confusion/bloat. To import an account, I believe we should have this sequence of steps which seems natural.

account := NewAccount("acct_name") // we may have other constructors like NewAccountFromPrivKey(privKey)
wallet.ImportAccount(account)

This simplicity keeps all Account constructors, getters, and setters functions located with the Account implementation (/account/local_account.go)

In CDP, they only have private key as params https://docs.cdp.coinbase.com/api-reference/v2/rest-api/evm-accounts/import-an-evm-account

…actor-primary-operations # Conflicts: # account/local_account.go # example/convert_subnet.go # example/create_chain.go # example/create_subnet.go # examples/wallet_example.go # wallet/builder.go # wallet/local/accounts.go # wallet/local/wallet.go # wallet/wallet.go

felipemadero force-pushed the wallet-refactor-primary-operations branch from 5928fa8 to a1538aa Compare November 5, 2025 20:01

felipemadero mentioned this pull request Nov 5, 2025

Create Chain Tx #178

Merged

felipemadero added 3 commits November 6, 2025 18:57

Merge branch 'main' into wallet-refactor-primary-operations

7dd4186

fix: update GetCChainAddress call to match interface change

c7fca7d

Remove network parameter from GetCChainAddress call in accounts.go to match the updated interface signature.

sukantoraymond reviewed Nov 7, 2025

View reviewed changes

address PR comment

d2780ba

sukantoraymond reviewed Nov 7, 2025

View reviewed changes

jasonatran reviewed Nov 8, 2025

View reviewed changes

felipemadero changed the title ~~refactor: Implement wallet interface with Primary operations and account management~~ refactor: account management Nov 8, 2025

felipemadero added 4 commits November 10, 2025 15:47

Add subnet ID tracking for active account

5db3c11

Sync with wallet-refactor-primary-operations-new

6dd064d

Remove files not present in new branch

f5fbb62

		@@ -0,0 +1,37 @@
		// Copyright (C) 2025, Ava Labs, Inc. All rights reserved.

refactor: account management #191

Are you sure you want to change the base?

refactor: account management #191

Uh oh!

Conversation

felipemadero commented Nov 5, 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

Uh oh!

sukantoraymond commented Nov 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonatran commented Nov 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonatran Nov 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

felipemadero commented Nov 5, 2025 •

edited

Loading

felipemadero Nov 7, 2025 •

edited

Loading

felipemadero Nov 7, 2025 •

edited

Loading

jasonatran Nov 8, 2025 •

edited

Loading