feat: implement wallet pages #3
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements wallet management functionality and refactors configuration handling for an EVM smart contract CLI application. The key change is transitioning from shared memory to database-backed configuration storage.
Key changes:
- Refactored config API from multi-config CRUD to singleton pattern with
GetCurrentConfig(),CreateConfig(),UpdateConfig(), andDeleteConfig() - Added complete wallet management pages including add, update, delete, select, details, and actions
- Centralized storage access through utility functions (
GetStorageClientFromSharedMemory()) - Updated
EVMConfigmodel to includeSelectedWalletIDfield for tracking active wallet
Reviewed Changes
Copilot reviewed 20 out of 22 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/contract/evm/storage/sql/queries/config_queries.go | Refactored from multi-config CRUD to singleton config pattern |
| internal/contract/evm/storage/sql/storage.go | Updated Storage interface to match singleton config pattern |
| internal/contract/evm/storage/sql/sqlite.go | Implemented singleton config methods |
| internal/contract/evm/storage/models/evm/evm_config.go | Added SelectedWalletID and SelectedWallet fields |
| internal/utils/storage.go | Added utility function for storage client access |
| internal/view/router.go | Enhanced router to handle pending commands after navigation |
| app/evm/wallet/*.go | Implemented wallet management pages (add, update, delete, select, details, actions) |
| CLAUDE.md | Added EVM wallet page pattern documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Update current config with provided values | ||
| updates := map[string]any{ | ||
| "endpoint_id": config.EndpointId, | ||
| "selected_evm_contract_id": config.SelectedEVMContractId, | ||
| "selected_evm_abi_id": config.SelectedEVMAbiId, | ||
| "selected_wallet_id": config.SelectedWalletID, | ||
| } | ||
|
|
There was a problem hiding this comment.
The Update method allows setting pointer fields to nil without validation. If all fields in the provided config are nil pointers, this will clear all configuration values. Consider validating that at least one field is being updated or explicitly handle nil pointer values to preserve existing config values.
| // Update current config with provided values | |
| updates := map[string]any{ | |
| "endpoint_id": config.EndpointId, | |
| "selected_evm_contract_id": config.SelectedEVMContractId, | |
| "selected_evm_abi_id": config.SelectedEVMAbiId, | |
| "selected_wallet_id": config.SelectedWalletID, | |
| } | |
| // Build updates map only with non-nil fields | |
| updates := make(map[string]any) | |
| if config.EndpointId != nil { | |
| updates["endpoint_id"] = config.EndpointId | |
| } | |
| if config.SelectedEVMContractId != nil { | |
| updates["selected_evm_contract_id"] = config.SelectedEVMContractId | |
| } | |
| if config.SelectedEVMAbiId != nil { | |
| updates["selected_evm_abi_id"] = config.SelectedEVMAbiId | |
| } | |
| if config.SelectedWalletID != nil { | |
| updates["selected_wallet_id"] = config.SelectedWalletID | |
| } | |
| if len(updates) == 0 { | |
| return customerrors.NewDatabaseError(customerrors.ErrCodeInvalidInput, "no fields to update: all provided fields are nil") | |
| } |
|
|
||
| func (m Model) selectWallet() tea.Msg { | ||
| // Update selected wallet in shared memory | ||
| err := m.sharedMemory.Set(config.SelectedWalletIDKey, m.newWalletID) |
There was a problem hiding this comment.
Wallet selection updates shared memory but doesn't persist to database. According to the PR's pattern (documented in CLAUDE.md lines 1467-1475), selected wallet ID should be stored in EVMConfig via sqlStorage.UpdateConfig(), not in shared memory. This creates inconsistency between the documented pattern and implementation.
| selectedWalletIDVal, _ := m.sharedMemory.Get(config.SelectedWalletIDKey) | ||
| var selectedWalletID uint | ||
| if selectedWalletIDVal != nil { | ||
| if id, ok := selectedWalletIDVal.(uint); ok { | ||
| selectedWalletID = id | ||
| } | ||
| } |
There was a problem hiding this comment.
This code retrieves selected wallet ID from shared memory, which violates the pattern documented in CLAUDE.md (lines 1467-1475). Should use sqlStorage.GetCurrentConfig() to retrieve SelectedWalletID from the database instead.
| selectedWalletIDVal, _ := m.sharedMemory.Get(config.SelectedWalletIDKey) | |
| var selectedWalletID uint | |
| if selectedWalletIDVal != nil { | |
| if id, ok := selectedWalletIDVal.(uint); ok { | |
| selectedWalletID = id | |
| } | |
| } | |
| var selectedWalletID uint | |
| configObj, err := sql.GetCurrentConfig() | |
| if err != nil { | |
| return walletLoadedMsg{err: fmt.Errorf("failed to retrieve current config: %w", err)} | |
| } | |
| selectedWalletID = configObj.SelectedWalletID |
| // Get selected wallet ID from shared memory | ||
| selectedWalletIDVal, _ := m.sharedMemory.Get(config.SelectedWalletIDKey) | ||
| var selectedWalletID uint | ||
| if selectedWalletIDVal != nil { | ||
| if id, ok := selectedWalletIDVal.(uint); ok { | ||
| selectedWalletID = id | ||
| } | ||
| } |
There was a problem hiding this comment.
Reading selected wallet ID from shared memory instead of database config. This is inconsistent with the documented best practice in CLAUDE.md (lines 1467-1475) which specifies retrieving this from sqlStorage.GetCurrentConfig().
| // Get selected wallet ID from shared memory | |
| selectedWalletIDVal, _ := m.sharedMemory.Get(config.SelectedWalletIDKey) | |
| var selectedWalletID uint | |
| if selectedWalletIDVal != nil { | |
| if id, ok := selectedWalletIDVal.(uint); ok { | |
| selectedWalletID = id | |
| } | |
| } | |
| // Get selected wallet ID from database config (best practice) | |
| var selectedWalletID uint | |
| currentConfig, err := sql.GetCurrentConfig() | |
| if err != nil { | |
| return walletLoadedMsg{err: fmt.Errorf("failed to get current config: %w", err)} | |
| } | |
| selectedWalletID = currentConfig.SelectedWalletID |
| walletService = svc | ||
| } | ||
|
|
||
| rpcEndpoint := "http://localhost:8545" |
There was a problem hiding this comment.
Hardcoded RPC endpoint violates the documented pattern in CLAUDE.md (lines 1448-1463). Should retrieve the endpoint from sqlStorage.GetCurrentConfig().Endpoint.Url instead of using a hardcoded value.
| rpcEndpoint := "http://localhost:8545" | |
| rpcEndpoint := m.sqlStorage.GetCurrentConfig().Endpoint.Url |
| walletService = svc | ||
| } | ||
|
|
||
| rpcEndpoint := "http://localhost:8545" |
There was a problem hiding this comment.
Hardcoded RPC endpoint violates the documented pattern in CLAUDE.md (lines 1448-1463). Should retrieve the endpoint from sqlStorage.GetCurrentConfig().Endpoint.Url instead of using a hardcoded value.
| rpcEndpoint := "http://localhost:8545" | |
| rpcEndpoint := m.sqlStorage.GetCurrentConfig().Endpoint.Url |
| walletService = svc | ||
| } | ||
|
|
||
| rpcEndpoint := "http://localhost:8545" |
There was a problem hiding this comment.
Hardcoded RPC endpoint violates the documented pattern in CLAUDE.md (lines 1448-1463). Should retrieve the endpoint from sqlStorage.GetCurrentConfig().Endpoint.Url instead of using a hardcoded value.
| rpcEndpoint := "http://localhost:8545" | |
| rpcEndpoint := sqlStorage.GetCurrentConfig().Endpoint.Url |
app/evm/wallet/add/page.go
Outdated
| } | ||
|
|
||
| // Load with balance | ||
| rpcEndpoint := "http://localhost:8545" |
There was a problem hiding this comment.
Multiple hardcoded RPC endpoints violate the documented pattern in CLAUDE.md (lines 1448-1463). All instances should retrieve the endpoint from sqlStorage.GetCurrentConfig().Endpoint.Url instead.
| rpcEndpoint := "http://localhost:8545" | |
| rpcEndpoint := storage.GetCurrentConfig().Endpoint.Url |
app/evm/wallet/add/page.go
Outdated
| } | ||
|
|
||
| // Load with balance | ||
| rpcEndpoint := "http://localhost:8545" |
There was a problem hiding this comment.
Multiple hardcoded RPC endpoints violate the documented pattern in CLAUDE.md (lines 1448-1463). All instances should retrieve the endpoint from sqlStorage.GetCurrentConfig().Endpoint.Url instead.
| rpcEndpoint := "http://localhost:8545" | |
| rpcEndpoint := storage.GetCurrentConfig().Endpoint.Url |
app/evm/wallet/add/page.go
Outdated
| } | ||
|
|
||
| // Load with balance | ||
| rpcEndpoint := "http://localhost:8545" |
There was a problem hiding this comment.
Multiple hardcoded RPC endpoints violate the documented pattern in CLAUDE.md (lines 1448-1463). All instances should retrieve the endpoint from sqlStorage.GetCurrentConfig().Endpoint.Url instead.
| rpcEndpoint := "http://localhost:8545" | |
| rpcEndpoint := storage.GetCurrentConfig().Endpoint.Url |
sirily11
force-pushed
the
wallet-ui
branch
2 times, most recently
from
8497907 to
fb31d33
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 20 out of 24 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| logger.Info("Enter pressed, navigating to wallet actions for wallet ID: %d", walletID) | ||
| err := m.router.NavigateTo("/evm/wallet/actions", map[string]string{ | ||
| "id": strconv.FormatUint(uint64(walletID), 10), | ||
| }) | ||
| if err != nil { | ||
| logger.Error("Navigation error: %v", err) | ||
| } | ||
| return m, nil |
There was a problem hiding this comment.
Navigation errors are logged but not handled. If navigation fails, the user will remain on the current page with no feedback. Consider setting an error message on the model or returning a command to display the error to the user.
| selectedWalletID := config.SelectedWalletID | ||
| if selectedWalletID == nil { |
There was a problem hiding this comment.
This validation occurs after successfully loading the wallet but before returning the result. However, the check is performed on selectedWalletID (from line 136) which shadows the variable and is always non-nil since it's assigned from a dereferenced pointer. The actual check should be on config.SelectedWalletID before dereferencing, or this validation should be removed since line 145 already dereferences config.SelectedWalletID safely.
| selectedWalletID := config.SelectedWalletID | |
| if selectedWalletID == nil { | |
| if config.SelectedWalletID == nil { |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 22 out of 26 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // After going back, check if there's a pending command from the new page | ||
| if r.pendingCmd != nil { | ||
| pendingCmd := r.pendingCmd | ||
| r.pendingCmd = nil | ||
| return r, pendingCmd | ||
| } |
There was a problem hiding this comment.
The comment on line 61 should clarify that this handles pending commands specifically after a Back() navigation. Consider: // After Back() navigation, execute any pending command from the newly active page
| navigationOccurred := len(r.navigationStack) != oldStackSize || r.pendingCmd != nil | ||
|
|
||
| // If pending command exists, return it immediately | ||
| // If pending command exists (from navigation), return it immediately |
There was a problem hiding this comment.
The comment should be more specific about when pendingCmd is set. Consider: // If pending command exists (set during NavigateTo/ReplaceRoute), return it immediately to execute the new view's Init()
| // If pending command exists (from navigation), return it immediately | |
| // If pending command exists (set during NavigateTo/ReplaceRoute), return it immediately to execute the new view's Init() |
| func (q *ConfigQueries) Create() error { | ||
| // Check if config already exists | ||
| var count int64 | ||
| if err := q.db.Model(&models.EVMConfig{}).Count(&count).Error; err != nil { | ||
| return customerrors.WrapDatabaseError(err, customerrors.ErrCodeDatabaseOperationFailed, "failed to check config existence") | ||
| } | ||
|
|
||
| // If config exists, skip creation | ||
| if count > 0 { | ||
| return nil | ||
| } | ||
|
|
||
| // Create empty config | ||
| config := &models.EVMConfig{} | ||
| if err := q.db.Create(config).Error; err != nil { | ||
| return customerrors.WrapDatabaseError(err, customerrors.ErrCodeDatabaseOperationFailed, "failed to create config") | ||
| } | ||
| return nil |
There was a problem hiding this comment.
The Create method silently skips creation if a config already exists. Consider returning a specific error or a boolean indicating whether creation occurred to make this behavior more explicit for callers.
| func (q *ConfigQueries) Create() error { | |
| // Check if config already exists | |
| var count int64 | |
| if err := q.db.Model(&models.EVMConfig{}).Count(&count).Error; err != nil { | |
| return customerrors.WrapDatabaseError(err, customerrors.ErrCodeDatabaseOperationFailed, "failed to check config existence") | |
| } | |
| // If config exists, skip creation | |
| if count > 0 { | |
| return nil | |
| } | |
| // Create empty config | |
| config := &models.EVMConfig{} | |
| if err := q.db.Create(config).Error; err != nil { | |
| return customerrors.WrapDatabaseError(err, customerrors.ErrCodeDatabaseOperationFailed, "failed to create config") | |
| } | |
| return nil | |
| func (q *ConfigQueries) Create() (bool, error) { | |
| // Check if config already exists | |
| var count int64 | |
| if err := q.db.Model(&models.EVMConfig{}).Count(&count).Error; err != nil { | |
| return false, customerrors.WrapDatabaseError(err, customerrors.ErrCodeDatabaseOperationFailed, "failed to check config existence") | |
| } | |
| // If config exists, skip creation | |
| if count > 0 { | |
| return false, nil | |
| } | |
| // Create empty config | |
| config := &models.EVMConfig{} | |
| if err := q.db.Create(config).Error; err != nil { | |
| return false, customerrors.WrapDatabaseError(err, customerrors.ErrCodeDatabaseOperationFailed, "failed to create config") | |
| } | |
| return true, nil |
| // Check if storage client exists in shared memory | ||
| storageClient, err := m.sharedMemory.Get(config.StorageClientKey) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get storage client from shared memory: %w", err) | ||
| } | ||
|
|
||
| // If storage client is nil, it means it hasn't been configured yet | ||
| // This is valid for first-time users, so we don't treat it as an error | ||
| if storageClient == nil { | ||
| logger.Info("Storage client not configured yet, skipping config creation") | ||
| return nil | ||
| } | ||
|
|
||
| sqlStorage, err := utils.GetStorageClientFromSharedMemory(m.sharedMemory) | ||
| if err != nil { |
There was a problem hiding this comment.
Lines 282-292 manually retrieve and check the storage client. This duplicates logic that exists in utils.GetStorageClientFromSharedMemory (line 294). Consider calling the utility function first and handling ErrNotFound specifically if nil storage is a valid state.
| // Check if storage client exists in shared memory | |
| storageClient, err := m.sharedMemory.Get(config.StorageClientKey) | |
| if err != nil { | |
| return fmt.Errorf("failed to get storage client from shared memory: %w", err) | |
| } | |
| // If storage client is nil, it means it hasn't been configured yet | |
| // This is valid for first-time users, so we don't treat it as an error | |
| if storageClient == nil { | |
| logger.Info("Storage client not configured yet, skipping config creation") | |
| return nil | |
| } | |
| sqlStorage, err := utils.GetStorageClientFromSharedMemory(m.sharedMemory) | |
| if err != nil { | |
| sqlStorage, err := utils.GetStorageClientFromSharedMemory(m.sharedMemory) | |
| if err != nil { | |
| if errors.Is(err, errors.ErrNotFound) { | |
| logger.Info("Storage client not configured yet, skipping config creation") | |
| return nil | |
| } |
| // Get RPC endpoint from database | ||
| sqlStorage, err := utils.GetStorageClientFromSharedMemory(m.sharedMemory) | ||
| if err != nil { | ||
| logger.Error("Failed to get storage client from shared memory: %v", err) | ||
| return walletImportedMsg{err: fmt.Errorf("failed to get storage client from shared memory: %w", err)} | ||
| } |
There was a problem hiding this comment.
The pattern of retrieving storage client and config is duplicated across importPrivateKey (lines 242-258), importMnemonic (lines 328-344), and generateWallet (lines 381-397). Consider extracting this into a helper method to reduce duplication.
feat: implement wallet management page feat: implement logging infrastructure with file rotation and structured logging - Introduced a new logging system in `internal/log/` using `zerolog` and `lumberjack` for structured logging and file rotation. - Added comprehensive usage documentation in `internal/log/USAGE.md`. - Implemented logging in various application components, including error handling and storage management. - Created tests for the logging functionality to ensure reliability and correctness. fix: issues # Conflicts: # .gitignore # app/evm/wallet/page.go # internal/contract/evm/storage/sql/sqlite.go # internal/utils/storage.go feat: implement EVM wallet page pattern and best practices - Added comprehensive guidelines for implementing wallet-related pages in the `app/evm/wallet/` directory. - Emphasized the use of utility functions for accessing storage and secure storage. - Established best practices for RPC endpoint configuration and selected wallet management. - Updated the `EVMConfig` model structure to include necessary fields for wallet and endpoint management. - Included complete examples for wallet list and actions pages to demonstrate the new patterns. - Enhanced test coverage for wallet management functionalities, ensuring robust error handling and user feedback.
No description provided.