fix: Critical bugs and centralized validation layer #6

katspaugh · 2025-10-26T08:39:48Z

Summary

This PR fixes several critical bugs identified during code review and implements a centralized validation service to improve code quality and maintainability.

Bug Fixes

1. Critical: Inconsistent Storage Project Name

File: src/storage/transaction-store.ts:20
Issue: Transaction store used 'safe-cli-nodejs' while all other stores used 'safe-cli'
Fix: Changed to use consistent 'safe-cli' project name
Impact: All storage files now in the same directory

2. Date Serialization Issues

Files: src/types/transaction.ts, src/storage/transaction-store.ts, multiple commands
Issue: Date objects were serialized to JSON as strings but typed as Date
Fix: Changed to use ISO 8601 strings (toISOString()) throughout
Impact: Type-safe date handling, no runtime mismatches

3. Missing Fetch Timeouts

File: src/services/abi-service.ts
Issue: No timeout on external API calls (Etherscan, Sourcify)
Fix: Added fetchWithTimeout() helper with 10-second timeout using AbortController
Impact: CLI won't hang on slow/unresponsive networks

4. Unsafe parseInt Calls

Files: src/services/safe-service.ts, transaction-service.ts, contract-service.ts
Issue: Missing radix parameter in parseInt calls
Fix: Added explicit radix 10 to all parseInt calls
Impact: Consistent parsing, prevents issues with leading zeros

New Features

Centralized ValidationService

Created src/services/validation-service.ts with comprehensive validation methods:

Address Validation:

validateAddress() / assertAddress() - Ethereum address validation with checksumming
validateOwnerAddress() - Check if address is a Safe owner
validateNonOwnerAddress() - Check if address is NOT a Safe owner
validateAddresses() / assertAddresses() - Array of addresses with duplicate checking

Credential Validation:

validatePrivateKey() / assertPrivateKey() - 64-char hex validation
validatePassword() - Minimum length validation
validatePasswordConfirmation() - Password matching

Network Validation:

validateChainId() / assertChainId() - Positive integer chain IDs
validateUrl() / assertUrl() - URL format validation
validateShortName() - EIP-3770 short name format

Transaction Validation:

validateThreshold() / assertThreshold() - Range validation
validateNonce() - Non-negative, >= current nonce
validateWeiValue() - BigInt validation
validateHexData() - Hex string format

Generic Validation:

validateRequired() - Non-empty check
validateJson() / assertJson() - JSON parsing
validatePositiveInteger() - Integer validation

Two Validation Patterns:

validate*() - Returns error message or undefined (for @clack/prompts)
assert*() - Throws ValidationError (for business logic)

Updated Commands

Refactored multiple commands to use ValidationService:

✅ commands/wallet/import.ts - Password & private key validation
✅ commands/config/chains.ts - Chain config validation
✅ commands/tx/create.ts - Transaction input validation
✅ commands/account/create.ts - Safe creation validation

Benefits

✨ Consistent validation - Single source of truth for all validation rules
📝 Better error messages - Standardized, clear error messages
🧪 Easier testing - Centralized logic is easier to unit test
🔧 Easier maintenance - Changes to validation rules in one place
🔒 Type safety - Proper TypeScript types with assert*() methods
♻️ Code reuse - Same validators used across multiple commands
📉 Reduced duplication - 119 lines removed, 86 added (net -33 lines)

Testing

✅ All existing tests pass (49/49)
✅ TypeScript compilation succeeds with no errors
✅ Build successful
✅ Prettier formatting applied

Documentation

Added comprehensive CODE_REVIEW.md documenting all findings
Updated code review with fixes marked as complete

Test Plan

🤖 Generated with Claude Code

This commit fixes several critical bugs and implements a centralized validation service: Bug Fixes: - Fix inconsistent storage projectName (transaction-store now uses 'safe-cli') - Fix date serialization issues (use ISO strings instead of Date objects) - Add fetch timeout handling (10s timeout for ABI service API calls) - Fix unsafe parseInt calls (add explicit radix parameter) New Features: - Add ValidationService for centralized validation logic - Implement 20+ validation methods covering addresses, private keys, URLs, etc. - Support both validate*() for prompts and assert*() for business logic - Update commands to use ValidationService (wallet/import, config/chains, tx/create, account/create) Benefits: - Consistent validation across the application - Better error messages - Easier to test and maintain - Improved type safety - Reduced code duplication 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>

Copilot

Pull Request Overview

This PR addresses critical bugs discovered during code review and implements a centralized validation service to improve code quality and maintainability. The changes fix storage inconsistencies, date serialization issues, missing network timeouts, and unsafe parseInt calls, while introducing a comprehensive validation layer that eliminates code duplication.

Key changes:

Fixed critical storage project name inconsistency in transaction store
Standardized date handling to ISO 8601 strings throughout the application
Added fetch timeouts to prevent CLI hangs on slow networks

Reviewed Changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/types/transaction.ts	Changed date fields from Date objects to ISO 8601 strings for consistent serialization
src/storage/transaction-store.ts	Fixed projectName to 'safe-cli' and updated date handling to use ISO strings
src/services/validation-service.ts	New centralized validation service with comprehensive input validation methods
src/services/abi-service.ts	Added fetchWithTimeout helper to prevent hanging on slow API responses
src/services/*.ts	Added explicit radix parameter to parseInt calls for safe parsing
src/commands/*.ts	Refactored to use ValidationService for consistent validation across commands
src/tests/integration/transaction.test.ts	Updated test signatures to use ISO string dates
PROJECT_PLAN.md, MIGRATION_PLAN.md	Removed obsolete project documentation files
CODE_REVIEW.md	Added comprehensive code review documentation

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-10-26T08:40:33Z

src/services/validation-service.ts

+let validationService: ValidationService | null = null
+
+export function getValidationService(): ValidationService {
+  if (!validationService) {
+    validationService = new ValidationService()
+  }


The singleton pattern implementation lacks thread safety. While Node.js is single-threaded, concurrent async operations could potentially create multiple instances if getValidationService() is called multiple times before the first call completes. Consider using a lazy initialization pattern with Promise caching or initializing the singleton at module load time.

Suggested change

let validationService: ValidationService | null = null

export function getValidationService(): ValidationService {

if (!validationService) {

validationService = new ValidationService()

}

const validationService = new ValidationService()

export function getValidationService(): ValidationService {

Copilot · 2025-10-26T08:40:33Z

src/commands/account/create.ts

+    const validator = getValidationService()
    const ownerAddress = await p.text({


The validator is instantiated inside the loop (lines 86-96), causing unnecessary repeated calls to getValidationService() for each owner addition. Move the validator instantiation to before the loop (near line 83) to improve efficiency and reduce redundant singleton lookups.

Copilot · 2025-10-26T08:40:33Z

src/commands/account/create.ts

+  const validator = getValidationService()
  const threshold = await p.text({


The validator is instantiated a second time at line 115, but was already instantiated at line 86 (inside the owner loop). This creates redundant singleton lookups. Declare the validator once at the beginning of the function and reuse it throughout.

- Move getValidationService() call outside the while loop - Reduces unnecessary singleton lookups on each iteration - Validator is now instantiated once and reused for all validations Addresses Copilot AI code review feedback 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>

Major improvements: - Add 'Why Safe CLI?' section explaining value proposition - Reorganize content with better visual hierarchy - Add table-based command reference for easier scanning - Include 'Common Workflows' section with practical examples - Simplify language and reduce technical jargon - Better formatting with emojis and clear sections - Add 'Need Help?' section with support resources Removed: - Roadmap section (completed phases no longer relevant) - Overly detailed explanations moved to more concise format - Redundant information consolidated Result: 64 fewer lines, more user-friendly, easier to navigate 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>

Copilot

Pull Request Overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/services/validation-service.ts

README.md

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Pull Request Overview

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

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-10-26T08:49:58Z

src/services/validation-service.ts

+   * @returns Error message or undefined if valid
+   */
+  validatePositiveInteger(value: unknown, fieldName = 'Value'): string | undefined {
+    if (!value) {


The function accepts unknown but only handles string and number types. If value is an object, array, or other type, it will pass through to the number check and produce incorrect results. Add explicit type checking before the conversion: if (typeof value !== 'string' && typeof value !== 'number') return ${fieldName} is required``

Suggested change

if (!value) {

if (

value === null ||

value === undefined ||

(typeof value !== 'string' && typeof value !== 'number') ||

value === ''

) {

Copilot · 2025-10-26T08:49:58Z

src/services/validation-service.ts

+    if (!value || typeof value !== 'string') {
+      return 'Threshold is required'
+    }
+    const threshold = parseInt(value, 10)
+    if (isNaN(threshold)) {
+      return 'Threshold must be a number'
+    }
+    if (threshold < min) {
+      return `Threshold must be at least ${min}`
+    }
+    if (max !== undefined && threshold > max) {
+      return `Threshold cannot exceed ${max} (number of owners)`
+    }
+    return undefined


[nitpick] This parseInt logic is duplicated in validatePositiveInteger (line 334). Consider reusing validatePositiveInteger or extracting the parsing logic to reduce duplication.

Suggested change

if (!value || typeof value !== 'string') {

return 'Threshold is required'

}

const threshold = parseInt(value, 10)

if (isNaN(threshold)) {

return 'Threshold must be a number'

}

if (threshold < min) {

return `Threshold must be at least ${min}`

}

if (max !== undefined && threshold > max) {

return `Threshold cannot exceed ${max} (number of owners)`

}

return undefined

return this._parseAndValidateInteger(

value,

min,

max,

'Threshold',

max !== undefined ? `Threshold cannot exceed ${max} (number of owners)` : undefined

);

}

/**

* Shared integer parsing and validation logic for threshold and positive integer fields.

* @param value The value to validate

* @param min Minimum allowed value

* @param max Maximum allowed value (optional)

* @param fieldName Name of the field for error messages

* @param maxErrorMsg Custom error message for exceeding max (optional)

* @returns Error message or undefined if valid

*/

private _parseAndValidateInteger(

value: unknown,

min: number,

max?: number,

fieldName = 'Value',

maxErrorMsg?: string

): string | undefined {

if (!value || typeof value !== 'string') {

return `${fieldName} is required`;

}

const intValue = parseInt(value, 10);

if (isNaN(intValue)) {

return `${fieldName} must be a number`;

}

if (intValue < min) {

return `${fieldName} must be at least ${min}`;

}

if (max !== undefined && intValue > max) {

return maxErrorMsg || `${fieldName} cannot exceed ${max}`;

}

return undefined;

Copilot · 2025-10-26T08:49:59Z

src/services/validation-service.ts

+    if (typeof value !== 'string') {
+      return 'Invalid nonce'
+    }
+    const nonce = parseInt(value, 10)


[nitpick] This parseInt logic is duplicated across multiple validation methods. Consider extracting a helper function parseInteger(value: string): number | null to reduce duplication and ensure consistent parsing behavior.

README.md

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

- Replace 47 explicit 'any' types with proper TypeScript types - Remove 14 unused error variables in catch blocks - Add proper type interfaces (ABIItem, APITransaction) - Improve type safety across commands, services, and storage - All files now pass strict TypeScript checks and ESLint 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>

Copilot

Pull Request Overview

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

Comments suppressed due to low confidence (2)

PROJECT_PLAN.md:1

[nitpick] Removing PROJECT_PLAN.md may impact contributors who reference it for architectural decisions or implementation history. Consider archiving it in a docs/archive folder instead of deleting it entirely, or extract key architectural decisions into ARCHITECTURE.md.
MIGRATION_PLAN.md:1
[nitpick] Deleting MIGRATION_PLAN.md removes valuable migration history and patterns that could help future contributors understand the codebase evolution. Consider moving it to docs/migration-history.md or docs/archive/ to preserve this knowledge.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-10-26T14:58:41Z

src/services/validation-service.ts

+    validationService = new ValidationService()
+  }
+  return validationService
+}


The singleton pattern with module-level state can cause issues in testing environments where you may need to reset state between tests. Consider exporting the class directly and letting consumers manage instantiation, or provide a resetValidationService() function for testing purposes.

Suggested change

}

}

/**

* Resets the singleton ValidationService instance.

* Useful for testing to ensure a fresh instance between tests.

*/

export function resetValidationService(): void {

validationService = null;

}

Copilot · 2025-10-26T14:58:42Z

README.md

+
+---
+
+Made with ❤️ for the Safe community


[nitpick] The README changes are extensive (297 lines deleted, 296 added). While the new structure is cleaner, this represents a complete rewrite that changes documentation style significantly. Consider whether this warrants a separate PR focused solely on documentation to make it easier to review and revert if needed.

Copilot

Pull Request Overview

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

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-10-26T15:02:00Z

src/services/validation-service.ts

+  validatePositiveInteger(value: unknown, fieldName = 'Value'): string | undefined {
+    if (!value) {
+      return `${fieldName} is required`
+    }
+    const num = typeof value === 'string' ? parseInt(value, 10) : value
+    if (typeof num !== 'number' || isNaN(num) || num <= 0 || !Number.isInteger(num)) {
+      return `${fieldName} must be a positive integer`
+    }
+    return undefined
+  }


The function accepts unknown but doesn't validate non-string, non-number types properly. If value is an object, array, or other type, line 334 would assign it directly to num, bypassing integer validation. Add type check: if (typeof value !== 'string' && typeof value !== 'number') return '${fieldName} must be a number' before line 334.

Copilot · 2025-10-26T15:02:01Z

src/services/tx-builder-parser.ts

+      !!data &&
      typeof data === 'object' &&
      'version' in data &&
      'chainId' in data &&
      'meta' in data &&
      'transactions' in data &&
-      Array.isArray(data.transactions) &&
-      typeof data.meta === 'object' &&
-      'createdFromSafeAddress' in data.meta
+      Array.isArray((data as { transactions: unknown }).transactions) &&
+      typeof (data as { meta: unknown }).meta === 'object' &&
+      (data as { meta: unknown }).meta !== null &&
+      'createdFromSafeAddress' in ((data as { meta: object }).meta)


[nitpick] The type guard uses repetitive type assertions and could be simplified. Consider restructuring: const obj = data as Record<string, unknown>; return !!data && typeof data === 'object' && 'version' in obj && Array.isArray(obj.transactions) && typeof obj.meta === 'object' && obj.meta !== null && 'createdFromSafeAddress' in obj.meta. This reduces redundant assertions while maintaining type safety.

Copilot · 2025-10-26T15:02:01Z

src/services/validation-service.ts

+  validateAddresses(addresses: unknown[]): string | undefined {
+    if (!Array.isArray(addresses) || addresses.length === 0) {


Parameter type unknown[] is incorrect - TypeScript will reject passing unknown to this function since it expects an array. Change parameter type to unknown and add array validation: if (!Array.isArray(addresses)) return 'Must be an array' before the length check on line 346.

Suggested change

validateAddresses(addresses: unknown[]): string | undefined {

if (!Array.isArray(addresses) || addresses.length === 0) {

validateAddresses(addresses: unknown): string | undefined {

if (!Array.isArray(addresses)) {

return 'Must be an array'

}

if (addresses.length === 0) {

Copilot · 2025-10-26T15:02:01Z

src/services/abi-service.ts

+async function fetchWithTimeout(
+  url: string,
+  options: RequestInit = {},
+  timeoutMs = 10000
+): Promise<Response> {
+  const controller = new AbortController()
+  const timeoutId = setTimeout(() => controller.abort(), timeoutMs)
+
+  try {
+    const response = await fetch(url, {
+      ...options,
+      signal: controller.signal,
+    })
+    return response
+  } catch (error) {
+    if ((error as Error).name === 'AbortError') {
+      throw new SafeCLIError(`Request timeout after ${timeoutMs}ms`)
+    }
+    throw error
+  } finally {
+    clearTimeout(timeoutId)
+  }
+}


If options.signal is already set, it will be overwritten by controller.signal on line 19, breaking any existing abort behavior. Merge signals properly using AbortSignal.any() (Node 20+) or check if signal exists and handle accordingly: signal: options.signal ? AbortSignal.any([options.signal, controller.signal]) : controller.signal

katspaugh and others added 2 commits October 26, 2025 08:59

Docs: rm old files

c75eee0

Copilot AI review requested due to automatic review settings October 26, 2025 08:39

Copilot AI reviewed Oct 26, 2025

View reviewed changes

katspaugh and others added 2 commits October 26, 2025 09:43

Copilot AI review requested due to automatic review settings October 26, 2025 08:47

Copilot AI reviewed Oct 26, 2025

View reviewed changes

src/services/validation-service.ts Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

Update README.md

556f102

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings October 26, 2025 08:49

Update src/services/validation-service.ts

86b353d

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI reviewed Oct 26, 2025

View reviewed changes

Apply suggestion from @Copilot

097fc3d

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings October 26, 2025 14:58

Copilot AI reviewed Oct 26, 2025

View reviewed changes

Update README.md

48c2ddd

Copilot AI review requested due to automatic review settings October 26, 2025 15:01

Copilot AI reviewed Oct 26, 2025

View reviewed changes

katspaugh merged commit 07e9be6 into main Oct 26, 2025
4 checks passed

katspaugh deleted the fix/bugs-and-validation-layer branch October 26, 2025 15:02

		const validator = getValidationService()
		const ownerAddress = await p.text({

		const validator = getValidationService()
		const threshold = await p.text({

-    if (!value) {
+    if (
+      value === null ||
+      value === undefined ||
+      (typeof value !== 'string' && typeof value !== 'number') ||
+      value === ''
+    ) {

-    if (!value || typeof value !== 'string') {
-      return 'Threshold is required'
-    }
-    const threshold = parseInt(value, 10)
-    if (isNaN(threshold)) {
-      return 'Threshold must be a number'
-    }
-    if (threshold < min) {
-      return `Threshold must be at least ${min}`
-    }
-    if (max !== undefined && threshold > max) {
-      return `Threshold cannot exceed ${max} (number of owners)`
-    }
-    return undefined
+    return this._parseAndValidateInteger(
+      value,
+      min,
+      max,
+      'Threshold',
+      max !== undefined ? `Threshold cannot exceed ${max} (number of owners)` : undefined
+    );
+  }
+  /**
+   * Shared integer parsing and validation logic for threshold and positive integer fields.
+   * @param value The value to validate
+   * @param min Minimum allowed value
+   * @param max Maximum allowed value (optional)
+   * @param fieldName Name of the field for error messages
+   * @param maxErrorMsg Custom error message for exceeding max (optional)
+   * @returns Error message or undefined if valid
+   */
+  private _parseAndValidateInteger(
+    value: unknown,
+    min: number,
+    max?: number,
+    fieldName = 'Value',
+    maxErrorMsg?: string
+  ): string | undefined {
+    if (!value || typeof value !== 'string') {
+      return `${fieldName} is required`;
+    }
+    const intValue = parseInt(value, 10);
+    if (isNaN(intValue)) {
+      return `${fieldName} must be a number`;
+    }
+    if (intValue < min) {
+      return `${fieldName} must be at least ${min}`;
+    }
+    if (max !== undefined && intValue > max) {
+      return maxErrorMsg || `${fieldName} cannot exceed ${max}`;
+    }
+    return undefined;

-}
+}
+/**
+ * Resets the singleton ValidationService instance.
+ * Useful for testing to ensure a fresh instance between tests.
+ */
+export function resetValidationService(): void {
+  validationService = null;
+}

		validateAddresses(addresses: unknown[]): string \| undefined {
		if (!Array.isArray(addresses) \|\| addresses.length === 0) {

-  validateAddresses(addresses: unknown[]): string | undefined {
-    if (!Array.isArray(addresses) || addresses.length === 0) {
+  validateAddresses(addresses: unknown): string | undefined {
+    if (!Array.isArray(addresses)) {
+      return 'Must be an array'
+    }
+    if (addresses.length === 0) {

fix: Critical bugs and centralized validation layer #6

fix: Critical bugs and centralized validation layer #6

Uh oh!

Conversation

katspaugh commented Oct 26, 2025

Summary

Bug Fixes

1. Critical: Inconsistent Storage Project Name

2. Date Serialization Issues

3. Missing Fetch Timeouts

4. Unsafe parseInt Calls

New Features

Centralized ValidationService

Updated Commands

Benefits

Testing

Documentation

Test Plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Oct 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Oct 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 26, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Oct 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Oct 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 26, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!