feat: add wallet management ui #2
Merged
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
…red 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.
sirily11
force-pushed
the
interaction-ui
branch
from
fb1a34a to
dc19012
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR implements comprehensive wallet management functionality for a smart contract CLI, including:
- Database models and queries for EVM wallet storage
- Wallet service layer with private key/mnemonic import and generation capabilities
- Secure storage utilities and logger infrastructure
- BIP39 mnemonic support and HD wallet derivation
- Error handling improvements and new error codes
Reviewed Changes
Copilot reviewed 31 out of 35 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/contract/evm/wallet/service.go | New wallet service with import/generate/CRUD operations |
| internal/contract/evm/storage/models/evm/wallet.go | Wallet database model with secure storage key helpers |
| internal/contract/evm/storage/sql/queries/wallet_queries.go | GORM queries for wallet database operations |
| internal/contract/evm/storage/sql/sqlite.go | SQLite implementation of wallet storage interface |
| internal/contract/evm/storage/sql/storage.go | Storage interface with wallet methods |
| internal/storage/secure.go | Renamed Unlock to TestPassword method |
| internal/utils/storage.go | Utility for retrieving secure storage from shared memory |
| internal/log/logger.go | New logger implementation with file rotation support |
| internal/errors/errors.go & code.go | New storage error codes and constructors |
| internal/config/storage.go & shared_memory_keys.go | Configuration keys reorganization |
| tools/tools.go | Mock generation directives for testing |
| go.mod/go.sum | Added dependencies for BIP39, zerolog, lumberjack, mock |
| docs/* | Comprehensive design documentation for wallet/endpoint/contract/ABI management |
| ~/smart_contract_cli | SQLite database file (should not be committed) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+572
to
+598
| func derivePrivateKey(seed []byte, path accounts.DerivationPath) (*ecdsa.PrivateKey, error) { | ||
| // For Ethereum, we use the master key derived from the seed | ||
| // This is a simplified approach - for production, use a proper BIP32 library | ||
|
|
||
| // For now, we'll use a simplified approach: just use the seed directly for the first account | ||
| // m/44'/60'/0'/0/0 -> use the seed with the account index | ||
|
|
||
| // Get the account index (last element in path) | ||
| if len(path) < 5 { | ||
| return nil, fmt.Errorf("invalid derivation path: expected at least 5 elements") | ||
| } | ||
|
|
||
| // For standard Ethereum path m/44'/60'/0'/0/N, use the seed + account index | ||
| // This is a simplified version - in production you'd use full BIP32 derivation | ||
| accountIndex := path[len(path)-1] | ||
|
|
||
| // Derive using crypto.Keccak256 | ||
| derivedSeed := crypto.Keccak256(seed, []byte(fmt.Sprintf("%d", accountIndex))) | ||
|
|
||
| // Create private key from derived seed | ||
| privateKey, err := crypto.ToECDSA(derivedSeed) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create private key: %w", err) | ||
| } | ||
|
|
||
| return privateKey, nil | ||
| } |
There was a problem hiding this comment.
The HD wallet derivation implementation is non-standard and insecure. It uses Keccak256(seed + accountIndex) instead of proper BIP32 hierarchical key derivation. This means derived keys are not compatible with standard wallets and may have security vulnerabilities. Use a proper BIP32 library (e.g., github.com/tyler-smith/go-bip32) for production.
Comment on lines
+61
to
+63
| func GetStorage(storageType types.StorageClient, params ...any) (Storage, error) { | ||
| switch storageType { | ||
| case config.StorageClientTypeSQLite: | ||
| case types.StorageClientSQLite: |
There was a problem hiding this comment.
[nitpick] The function uses variadic 'any' parameters which loses type safety. Consider using a structured config object or specific typed parameters to make the function signature clearer and catch errors at compile time.
sirily11
force-pushed
the
interaction-ui
branch
from
dc19012 to
d5fe60f
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
No description provided.