diff --git a/CODE_REVIEW.md b/CODE_REVIEW.md new file mode 100644 index 0000000..abcf763 --- /dev/null +++ b/CODE_REVIEW.md @@ -0,0 +1,403 @@ +# Code Review Report: Safe CLI + +**Date:** 2025-10-26 +**Version:** 0.1.0 +**Reviewer:** Claude Code + +--- + +## 🐛 **BUGS FOUND** + +### 1. **CRITICAL: Inconsistent Storage Project Name** +**Location:** `src/storage/transaction-store.ts:20` + +The transaction store uses a different `projectName` than all other stores: +- **transaction-store.ts**: `'safe-cli-nodejs'` +- **All others**: `'safe-cli'` + +**Impact:** This causes transactions to be stored in a different directory than other config files, leading to: +- Confusion when debugging +- Potential data loss if users migrate or backup only one directory +- Inconsistent uninstall/cleanup behavior + +**Fix:** Change line 20 in `transaction-store.ts` to use `'safe-cli'` + +--- + +### 2. **Date Serialization Issue** +**Location:** `src/storage/transaction-store.ts:44, 127` + +The `createdAt` and `executedAt` fields are typed as `Date` objects, but `Conf` stores them as JSON which serializes dates as strings. When reading back from storage, they remain strings but TypeScript expects `Date` objects. + +**Impact:** +- Type safety violation at runtime +- Potential crashes if code tries to call `.getTime()` or other Date methods +- Inconsistent behavior between fresh objects and persisted ones + +**Fix:** Either: +- Change types to `string` and document ISO format +- Add serialization/deserialization logic in `createTransaction()` and `getTransaction()` + +--- + +### 3. **Missing Error Handling in fetch() Calls** +**Location:** `src/services/abi-service.ts:135, 168, 197` + +The `fetch()` calls for Etherscan and Sourcify APIs lack proper timeout and error handling: + +```typescript +const response = await fetch(requestUrl) // No timeout! +``` + +**Impact:** +- CLI can hang indefinitely on slow/unresponsive networks +- Poor user experience with no feedback +- No retry logic for transient failures + +**Fix:** Add timeout wrapper and proper error handling: +```typescript +const controller = new AbortController() +const timeoutId = setTimeout(() => controller.abort(), 10000) // 10s timeout +try { + const response = await fetch(requestUrl, { signal: controller.signal }) + // ... handle response +} finally { + clearTimeout(timeoutId) +} +``` + +--- + +### 4. **Potential Race Condition in Password Management** +**Location:** `src/storage/wallet-store.ts:60, 82-84` + +The `password` field is stored as instance state (`this.password: string | null = null`), but the singleton pattern means multiple operations could interfere with each other. + +**Impact:** +- If one command sets a password and another command runs concurrently, they could use the wrong password +- Minimal risk in current CLI usage (sequential commands) but problematic if converted to server/daemon + +**Fix:** Consider passing password explicitly to methods that need it, or use a more robust session management system. + +--- + +### 5. **Unsafe parseInt Without Radix** +**Location:** Multiple files (safe-service.ts, transaction-service.ts) + +Several `parseInt(this.chain.chainId)` calls lack explicit radix parameter: + +```typescript +id: parseInt(this.chain.chainId), // Missing radix! +``` + +**Impact:** +- Inconsistent parsing if chainId has leading zeros +- Potential security issue with malformed input + +**Fix:** Always specify radix: `parseInt(this.chain.chainId, 10)` + +--- + +## 💧 **MEMORY LEAKS & RESOURCE MANAGEMENT** + +### 1. **Ink Render Memory Leak Risk** +**Location:** `src/ui/render.tsx:22-27` + +The `renderScreen()` function properly handles unmounting, BUT there's a subtle issue: if an error occurs before `waitUntilExit()` resolves, the component is never unmounted. + +**Current code:** +```typescript +waitUntilExit().then(() => { + unmount() + resolve() +}) +``` + +**Risk:** If the component throws an error or process exits unexpectedly, `unmount()` is never called. + +**Fix:** Add error handling: +```typescript +waitUntilExit() + .then(() => { + unmount() + resolve() + }) + .catch((error) => { + unmount() // Always cleanup + reject(error) + }) +``` + +--- + +### 2. **No Cleanup for Public/Wallet Clients** +**Location:** `src/services/safe-service.ts`, `src/services/transaction-service.ts` + +Multiple viem clients are created but never explicitly cleaned up: +- `createPublicClient()` at safe-service.ts:137, 169 +- `createWalletClient()` at safe-service.ts:112 +- `createPublicClient()` at transaction-service.ts:181 + +**Impact:** +- Connection pooling may hold open sockets +- Small memory leak per operation +- Not critical for CLI (short-lived processes) but problematic if CLI becomes long-running + +**Fix:** Store clients as instance properties and reuse them, or implement cleanup in a `destroy()` method. + +--- + +### 3. **process.setMaxListeners(20) Warning** +**Location:** `src/index.ts:5` + +While this suppresses warnings, it masks potential event listener leaks. The comment mentions "sequential command chaining" but doesn't address the root cause. + +**Risk:** If there's a legitimate listener leak, this hides it. + +**Recommendation:** Audit event listeners and ensure proper cleanup, then reduce this number to the actual required amount. + +--- + +## 🏗️ **ARCHITECTURE IMPROVEMENTS** + +### 1. **Service Instantiation Duplication** +**Pattern:** Services are instantiated repeatedly in commands with the same parameters. + +Example from `tx/create.ts:85, 361`: +```typescript +const txService = new TransactionService(chain) +// ... later ... +const txService = new TransactionService(chain) // Duplicate instantiation! +``` + +**Improvement:** Create a service factory/registry: +```typescript +class ServiceRegistry { + private services = new Map() + + getTransactionService(chain: ChainConfig, privateKey?: string) { + const key = `${chain.chainId}:${privateKey || 'readonly'}` + if (!this.services.has(key)) { + this.services.set(key, new TransactionService(chain, privateKey)) + } + return this.services.get(key) + } +} +``` + +--- + +### 2. **No Dependency Injection** +**Issue:** All commands directly call `getConfigStore()`, `getWalletStorage()`, etc., creating tight coupling. + +**Impact:** +- Hard to unit test commands in isolation +- Hard to mock storage for tests +- Difficult to swap implementations + +**Improvement:** Use constructor injection or a DI container: +```typescript +export async function createTransaction(deps = { + safeStorage: getSafeStorage(), + configStore: getConfigStore(), + // ... +}) { + // Use deps.safeStorage instead of getSafeStorage() +} +``` + +--- + +### 3. **Missing Validation Layer** ✅ **FIXED** +**Issue:** Validation logic was scattered throughout commands and services. + +Example: Address validation happened inline in multiple places: +- `tx/create.ts:108-112` +- Throughout various services +- No centralized validation rules + +**Solution Implemented:** Created `ValidationService` class in `src/services/validation-service.ts`: +- Centralized validation for all input types (addresses, private keys, chain IDs, URLs, etc.) +- Two types of validators: `validate*()` for prompts, `assert*()` for business logic +- Updated multiple commands to use the service: wallet/import.ts, config/chains.ts, tx/create.ts, account/create.ts +- Consistent error messages across the application +- Easier to test and maintain validation logic + +--- + +### 4. **No Retry Logic for Network Operations** +**Issue:** All RPC and API calls fail immediately on network errors. + +**Improvement:** Implement exponential backoff retry wrapper: +```typescript +async function withRetry( + fn: () => Promise, + maxRetries = 3, + baseDelay = 1000 +): Promise { + // Implementation with exponential backoff +} +``` + +--- + +### 5. **Hardcoded Safe Version** +**Location:** Multiple files specify `safeVersion: '1.4.1'` + +While there's a default in config, services hardcode the version: +- `safe-service.ts:54` +- `safe-service.ts:103` + +**Improvement:** Always read from config, allow per-safe version override. + +--- + +## ♻️ **REFACTORING OPPORTUNITIES** + +### 1. **Extract Chain Client Factory** +**Current:** Chain client creation is duplicated across services. + +**Refactor:** Create a `ChainClientFactory`: +```typescript +class ChainClientFactory { + createPublicClient(chain: ChainConfig) { ... } + createWalletClient(chain: ChainConfig, privateKey: string) { ... } +} +``` + +--- + +### 2. **Consolidate Address Formatting** +**Issue:** EIP-3770 formatting logic is called inline throughout commands. + +**Refactor:** Create a display service: +```typescript +class DisplayService { + formatAddress(address: Address, chainId: string): string + formatBalance(balance: bigint, decimals: number): string + formatTimestamp(date: Date | string): string +} +``` + +--- + +### 3. **Extract Prompt Patterns** +**Issue:** Similar prompt patterns repeated in many commands: + +```typescript +// Repeated pattern: +const safes = safeStorage.getAllSafes() +if (safes.length === 0) { + p.log.error('No Safes found. Please create a Safe first.') + p.outro('Setup required') + return +} +``` + +**Refactor:** Create prompt utilities: +```typescript +async function promptSafeSelection(message: string): Promise +async function promptWalletPassword(): Promise +async function confirmAction(message: string): Promise +``` + +--- + +### 4. **Separate UI from Business Logic** +**Issue:** Commands mix @clack/prompts UI code with business logic, making it hard to: +- Test business logic separately +- Build alternative interfaces (web UI, programmatic API) + +**Refactor:** Split into: +- **UI Layer**: Handles prompts and rendering +- **Controller Layer**: Orchestrates business logic +- **Service Layer**: Pure business operations + +--- + +### 5. **Type Guard Functions** +**Issue:** Type narrowing is done inline throughout: + +```typescript +if (error instanceof Error) { ... } +if (error instanceof SafeCLIError) { ... } +``` + +**Refactor:** Create type guards: +```typescript +function isSafeCLIError(error: unknown): error is SafeCLIError +function isNetworkError(error: unknown): error is NetworkError +``` + +--- + +### 6. **Extract Transaction Builder to UI Pattern** +**Issue:** `tx/create.ts` is 449 lines with complex nested logic. + +**Refactor:** Break into smaller functions: +- `getTransactionTarget(): Promise
` +- `getContractInteraction(address: Address): Promise` +- `getManualTransaction(): Promise` +- `confirmTransaction(data: TransactionData): Promise` + +--- + +## 🔒 **SECURITY CONSIDERATIONS** + +### 1. **Encryption Salt Not Truly Random in Tests** +If tests mock `randomBytes`, they might use predictable values. Ensure test infrastructure properly handles crypto operations. + +### 2. **Private Key Clearance** +Consider explicitly clearing private keys from memory after use: +```typescript +// After using privateKey +privateKey = '0x' + '0'.repeat(64) // Overwrite in memory +``` + +### 3. **API Key Logging Risk** +`config/show.ts` obfuscates API keys in display, but ensure they're never logged to error messages or debug output. + +--- + +## 📊 **SUMMARY STATISTICS** + +| Category | Count | Severity | +|----------|-------|----------| +| Critical Bugs | 1 | High | +| Major Bugs | 4 | Medium | +| Memory Leak Risks | 3 | Medium | +| Architecture Issues | 5 | Medium | +| Refactoring Opportunities | 6 | Low | + +--- + +## 🎯 **RECOMMENDED PRIORITY ORDER** + +1. ✅ **Fix critical bug**: Inconsistent storage projectName (FIXED) +2. ✅ **Fix date serialization**: Transaction store dates (FIXED) +3. ✅ **Add fetch timeouts**: Prevent hanging on network calls (FIXED) +4. ✅ **Add validation layer**: Centralize validation logic (FIXED) +5. ✅ **Fix unsafe parseInt**: Add radix parameter (FIXED) +6. **Implement retry logic**: Improve reliability +7. **Extract service factory**: Reduce duplication +8. **Refactor large commands**: Break down tx/create.ts +9. **Add DI support**: Improve testability + +--- + +## 📝 **NOTES** + +The codebase is generally **well-structured and follows good TypeScript practices**. The issues found are mostly about edge cases, operational robustness, and opportunities for improved maintainability. The critical bug (projectName inconsistency) should be fixed immediately, while other improvements can be addressed incrementally. + +**Strengths:** +- Clean architecture with good separation of concerns +- Comprehensive type safety with TypeScript and Zod +- Modern React/Ink UI for great UX +- Secure wallet encryption with AES-256-GCM +- Well-documented code with clear patterns + +**Areas for Improvement:** +- Network resilience (timeouts, retries) +- Resource cleanup and memory management +- Dependency injection for better testability +- Reduced code duplication in commands diff --git a/MIGRATION_PLAN.md b/MIGRATION_PLAN.md deleted file mode 100644 index 09609e8..0000000 --- a/MIGRATION_PLAN.md +++ /dev/null @@ -1,720 +0,0 @@ -# Migration Plan: Imperative Console → Reactive Ink Architecture - -## Current State Summary -- **561 console occurrences** (primarily console.log) -- Mixed UI patterns: @clack/prompts for interactive input + raw console.log for output -- Heavy usage in: account commands (34-26 instances), config commands (27-21), tx/wallet commands (19-10) -- Good separation: services layer already isolated from commands -- Manual formatting for tables, lists, and structured data - ---- - -## Phase 1: Foundation & Infrastructure (Week 1) - -### 1.1 Add Ink Dependencies -```bash -npm install ink react -npm install --save-dev @types/react -``` - -### 1.2 Create Core Architecture -**New directory structure:** -``` -src/ -├── ui/ -│ ├── components/ # Reusable Ink components -│ │ ├── Layout.tsx # Container, Box, Text wrappers -│ │ ├── Table.tsx # Generic table component -│ │ ├── List.tsx # List with markers -│ │ ├── KeyValue.tsx # Key-value pairs display -│ │ ├── Header.tsx # Styled headers/titles -│ │ ├── StatusBadge.tsx # Status indicators -│ │ └── Spinner.tsx # Loading states -│ ├── screens/ # Full-screen views -│ │ ├── WelcomeScreen.tsx -│ │ ├── AccountListScreen.tsx -│ │ ├── AccountInfoScreen.tsx -│ │ └── ... -│ ├── hooks/ # Custom React hooks -│ │ ├── useConfig.ts # Config state management -│ │ ├── useSafe.ts # Safe state management -│ │ └── useWallet.ts # Wallet state management -│ ├── theme.ts # Color palette and styles -│ └── render.tsx # Ink render wrapper utility -``` - -### 1.3 Create Render Utility -**Purpose:** Bridge between command functions and Ink rendering -```typescript -// src/ui/render.tsx -import { render } from 'ink' -import React from 'react' - -export function renderScreen( - Component: React.ComponentType, - props: T -): Promise { - return new Promise((resolve) => { - const { unmount, waitUntilExit } = render() - waitUntilExit().then(() => { - unmount() - resolve() - }) - }) -} -``` - ---- - -## Phase 2: Component Library (Week 2) - -### 2.1 Build Core Components - -**Priority components based on usage patterns:** - -1. **Table Component** (replaces 100+ console.log instances) - - Aligned columns - - Headers with styling - - Row highlighting - - Used in: account info, config show, tx list - -2. **List Component** (replaces 80+ console.log instances) - - Bullet markers (●/○) - - Active/inactive states - - Nested indentation - - Used in: account list, wallet list - -3. **KeyValue Component** (replaces 150+ console.log instances) - - Label/value pairs - - Consistent spacing - - Color theming - - Used in: all info/summary displays - -4. **Header Component** (replaces 50+ console.log instances) - - Title with icons - - Dividers - - Used in: all command outputs - -5. **StatusBadge Component** (replaces 30+ console.log instances) - - Success/error/warning/info - - Icons + colors - - Used in: confirmations, error messages - -### 2.2 Create Theme System -**src/ui/theme.ts:** -```typescript -export const theme = { - colors: { - primary: '#00D9FF', // cyan - success: '#00FF88', // green - error: '#FF5555', // red - warning: '#FFAA00', // yellow - info: '#5599FF', // blue - dim: '#666666', // gray - }, - spacing: { - small: 1, - medium: 2, - large: 3, - }, -} -``` - ---- - -## Phase 3: Logic/Presentation Separation (Week 3) ✅ - -### 3.1 Extract Business Logic from Commands - -**Pattern: Command → Controller → View** - -**COMPLETED EXAMPLE: Wallet List Migration** - -**Before (imperative - 41 lines):** -```typescript -// commands/wallet/list.ts (OLD) -import * as p from '@clack/prompts' -import pc from 'picocolors' -import { getWalletStorage } from '../../storage/wallet-store.js' -import { shortenAddress } from '../../utils/ethereum.js' - -export async function listWallets() { - p.intro(pc.bgCyan(pc.black(' Wallets '))) - - const walletStorage = getWalletStorage() - const wallets = walletStorage.getAllWallets() - const activeWallet = walletStorage.getActiveWallet() - - if (wallets.length === 0) { - console.log('') - console.log(pc.dim('No wallets found')) - console.log('') - p.outro('Use "safe wallet import" to import a wallet') - return - } - - console.log('') - for (const wallet of wallets) { - const isActive = activeWallet?.id === wallet.id - const marker = isActive ? pc.green('●') : pc.dim('○') - const label = isActive ? pc.bold(pc.green(wallet.name)) : wallet.name - - console.log(`${marker} ${label}`) - console.log(` ${pc.dim('Address:')} ${wallet.address}`) - console.log(` ${pc.dim('Short:')} ${shortenAddress(wallet.address)}`) - if (wallet.lastUsed) { - console.log(` ${pc.dim('Last used:')} ${new Date(wallet.lastUsed).toLocaleString()}`) - } - console.log('') - } - - if (activeWallet) { - console.log(pc.dim(`Active wallet: ${activeWallet.name}`)) - } - - p.outro(pc.green(`Total: ${wallets.length} wallet(s)`)) -} -``` - -**After (reactive - 3 lines):** -```typescript -// commands/wallet/list.ts (NEW - Controller) -import { renderScreen } from '../../ui/render.js' -import { WalletListScreen } from '../../ui/screens/index.js' - -export async function listWallets() { - await renderScreen(WalletListScreen, {}) -} - -// ui/hooks/useWallet.ts (State Hook) -export function useWallets() { - const [wallets, setWallets] = useState([]) - const [activeWallet, setActiveWallet] = useState(null) - const [loading, setLoading] = useState(true) - - useEffect(() => { - const walletStorage = getWalletStorage() - setWallets(walletStorage.getAllWallets()) - setActiveWallet(walletStorage.getActiveWallet()) - setLoading(false) - }, []) - - return { wallets, activeWallet, loading } -} - -// ui/screens/WalletListScreen.tsx (View) -export function WalletListScreen({ onExit }) { - const { wallets, activeWallet, loading } = useWallets() - - if (loading) return - if (wallets.length === 0) return - - return ( - -
- ( - - )} - /> -