Skip to content

Commit 07e9be6

Browse files
katspaughclaudeCopilot
authored
fix: Critical bugs and centralized validation layer (#6)
* Docs: rm old files * fix: critical bugs and add centralized validation layer 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> * perf: move validator instantiation outside loop in account create - 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> * docs: improve README user-friendliness and remove roadmap 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> * Update README.md Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update src/services/validation-service.ts Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * fix: eliminate all type issues and lint warnings - 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> * Update README.md --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
1 parent 6393291 commit 07e9be6

28 files changed

+1291
-2154
lines changed

‎CODE_REVIEW.md‎

Lines changed: 403 additions & 0 deletions
Large diffs are not rendered by default.

‎MIGRATION_PLAN.md‎

Lines changed: 0 additions & 720 deletions
This file was deleted.

‎PROJECT_PLAN.md‎

Lines changed: 0 additions & 898 deletions
This file was deleted.

‎README.md‎

Lines changed: 258 additions & 330 deletions
Large diffs are not rendered by default.

‎src/commands/account/create.ts‎

Lines changed: 8 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import { getConfigStore } from '../../storage/config-store.js'
44
import { getSafeStorage } from '../../storage/safe-store.js'
55
import { getWalletStorage } from '../../storage/wallet-store.js'
66
import { SafeService } from '../../services/safe-service.js'
7-
import { isValidAddress } from '../../utils/validation.js'
7+
import { getValidationService } from '../../services/validation-service.js'
88
import { checksumAddress, shortenAddress } from '../../utils/ethereum.js'
99
import { formatSafeAddress } from '../../utils/eip3770.js'
1010
import { logError } from '../../ui/messages.js'
@@ -67,6 +67,8 @@ export async function createSafe() {
6767

6868
// Add more owners
6969
let addingOwners = true
70+
const validator = getValidationService()
71+
7072
while (addingOwners) {
7173
const addMore = await p.confirm({
7274
message: `Add ${owners.length > 0 ? 'another' : 'an'} owner?`,
@@ -87,9 +89,9 @@ export async function createSafe() {
8789
message: 'Owner address:',
8890
placeholder: '0x...',
8991
validate: (value) => {
90-
if (!value) return 'Address is required'
91-
if (!isValidAddress(value)) return 'Invalid Ethereum address'
92-
const checksummed = checksumAddress(value)
92+
const addressError = validator.validateAddress(value)
93+
if (addressError) return addressError
94+
const checksummed = checksumAddress(value as string)
9395
if (owners.includes(checksummed as Address)) return 'Owner already added'
9496
return undefined
9597
},
@@ -114,14 +116,7 @@ export async function createSafe() {
114116
const threshold = await p.text({
115117
message: `Signature threshold (1-${owners.length}):`,
116118
placeholder: Math.min(2, owners.length).toString(),
117-
validate: (value) => {
118-
if (!value) return 'Threshold is required'
119-
const num = parseInt(value, 10)
120-
if (isNaN(num)) return 'Must be a number'
121-
if (num < 1) return 'Threshold must be at least 1'
122-
if (num > owners.length) return `Threshold cannot exceed ${owners.length} owners`
123-
return undefined
124-
},
119+
validate: (value) => validator.validateThreshold(value, 1, owners.length),
125120
})
126121

127122
if (p.isCancel(threshold)) {
@@ -135,7 +130,7 @@ export async function createSafe() {
135130
const name = await p.text({
136131
message: 'Give this Safe a name:',
137132
placeholder: 'my-safe',
138-
validate: (value) => (!value ? 'Name is required' : undefined),
133+
validate: (value) => validator.validateRequired(value, 'Name'),
139134
})
140135

141136
if (p.isCancel(name)) {

‎src/commands/config/chains.ts‎

Lines changed: 10 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import * as p from '@clack/prompts'
22
import { getConfigStore } from '../../storage/config-store.js'
33
import type { ChainConfig } from '../../types/config.js'
4-
import { isValidChainId, isValidUrl } from '../../utils/validation.js'
4+
import { getValidationService } from '../../services/validation-service.js'
55
import { logError } from '../../ui/messages.js'
66
import { renderScreen } from '../../ui/render.js'
77
import {
@@ -14,14 +14,15 @@ export async function addChain() {
1414
p.intro('Add Chain')
1515

1616
const configStore = getConfigStore()
17+
const validator = getValidationService()
1718

1819
const chainId = await p.text({
1920
message: 'Chain ID:',
2021
placeholder: '1',
2122
validate: (value) => {
22-
if (!value) return 'Chain ID is required'
23-
if (!isValidChainId(value)) return 'Invalid chain ID'
24-
if (configStore.chainExists(value)) return `Chain ${value} already exists`
23+
const error = validator.validateChainId(value)
24+
if (error) return error
25+
if (configStore.chainExists(value as string)) return `Chain ${value} already exists`
2526
return undefined
2627
},
2728
})
@@ -34,7 +35,7 @@ export async function addChain() {
3435
const name = await p.text({
3536
message: 'Chain name:',
3637
placeholder: 'Ethereum Mainnet',
37-
validate: (value) => (!value ? 'Chain name is required' : undefined),
38+
validate: (value) => validator.validateRequired(value, 'Chain name'),
3839
})
3940

4041
if (p.isCancel(name)) {
@@ -45,12 +46,7 @@ export async function addChain() {
4546
const shortName = await p.text({
4647
message: 'Short name (EIP-3770):',
4748
placeholder: 'eth',
48-
validate: (value) => {
49-
if (!value) return 'Short name is required'
50-
if (!/^[a-z0-9-]+$/.test(value))
51-
return 'Short name must be lowercase alphanumeric with hyphens'
52-
return undefined
53-
},
49+
validate: (value) => validator.validateShortName(value),
5450
})
5551

5652
if (p.isCancel(shortName)) {
@@ -61,11 +57,7 @@ export async function addChain() {
6157
const rpcUrl = await p.text({
6258
message: 'RPC URL:',
6359
placeholder: 'https://eth.llamarpc.com',
64-
validate: (value) => {
65-
if (!value) return 'RPC URL is required'
66-
if (!isValidUrl(value)) return 'Invalid URL'
67-
return undefined
68-
},
60+
validate: (value) => validator.validateUrl(value, true),
6961
})
7062

7163
if (p.isCancel(rpcUrl)) {
@@ -86,7 +78,7 @@ export async function addChain() {
8678
const currency = await p.text({
8779
message: 'Native currency symbol:',
8880
placeholder: 'ETH',
89-
validate: (value) => (!value ? 'Currency symbol is required' : undefined),
81+
validate: (value) => validator.validateRequired(value, 'Currency symbol'),
9082
})
9183

9284
if (p.isCancel(currency)) {
@@ -97,10 +89,7 @@ export async function addChain() {
9789
const transactionServiceUrl = await p.text({
9890
message: 'Safe Transaction Service URL (optional):',
9991
placeholder: 'https://safe-transaction-mainnet.safe.global',
100-
validate: (value) => {
101-
if (value && !isValidUrl(value)) return 'Invalid URL'
102-
return undefined
103-
},
92+
validate: (value) => validator.validateUrl(value, false),
10493
})
10594

10695
if (p.isCancel(transactionServiceUrl)) {

‎src/commands/config/edit.ts‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import { getConfigStore } from '../../storage/config-store.js'
77
import { SafeCLIError } from '../../utils/errors.js'
88
import { renderScreen } from '../../ui/render.js'
99
import { ChainEditSuccessScreen } from '../../ui/screens/index.js'
10+
import type { ChainConfig } from '../../types/config.js'
1011

1112
export async function editChains() {
1213
p.intro('Edit Chain Configurations')
@@ -66,10 +67,10 @@ export async function editChains() {
6667
unlinkSync(tempFile) // Clean up temp file
6768

6869
// Parse and validate
69-
let parsedConfig: any
70+
let parsedConfig: { chains?: Record<string, unknown> }
7071
try {
7172
parsedConfig = JSON.parse(editedContent)
72-
} catch (error) {
73+
} catch {
7374
throw new SafeCLIError('Invalid JSON format. Changes not saved.')
7475
}
7576

@@ -81,7 +82,7 @@ export async function editChains() {
8182

8283
// Validate each chain
8384
for (const [chainId, chain] of Object.entries(newChains)) {
84-
const c = chain as any
85+
const c = chain as ChainConfig
8586

8687
if (!c.chainId || !c.name || !c.shortName || !c.rpcUrl || !c.currency) {
8788
throw new SafeCLIError(
@@ -135,9 +136,9 @@ export async function editChains() {
135136
(id) => oldChainIds.has(id) && JSON.stringify(chains[id]) !== JSON.stringify(newChains[id])
136137
)
137138

138-
const addedNames = added.map((id) => newChains[id].name)
139+
const addedNames = added.map((id) => (newChains[id] as ChainConfig).name)
139140
const removedNames = removed.map((id) => chains[id].name)
140-
const modifiedNames = modified.map((id) => newChains[id].name)
141+
const modifiedNames = modified.map((id) => (newChains[id] as ChainConfig).name)
141142

142143
const confirm = await p.confirm({
143144
message: 'Apply these changes?',
@@ -160,7 +161,7 @@ export async function editChains() {
160161

161162
// Add/update chains
162163
for (const [chainId, chain] of Object.entries(newChains)) {
163-
const c = chain as any
164+
const c = chain as ChainConfig
164165
configStore.setChain(chainId, {
165166
chainId: c.chainId,
166167
name: c.name,

‎src/commands/tx/create.ts‎

Lines changed: 21 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,16 @@
11
import * as p from '@clack/prompts'
2-
import { isAddress, type Address } from 'viem'
2+
import { type Address } from 'viem'
33
import { getConfigStore } from '../../storage/config-store.js'
44
import { getSafeStorage } from '../../storage/safe-store.js'
55
import { getTransactionStore } from '../../storage/transaction-store.js'
66
import { getWalletStorage } from '../../storage/wallet-store.js'
77
import { TransactionService } from '../../services/transaction-service.js'
88
import { ContractService } from '../../services/contract-service.js'
9-
import { ABIService } from '../../services/abi-service.js'
9+
import { ABIService, type ABI, type ABIFunction } from '../../services/abi-service.js'
1010
import { TransactionBuilder } from '../../services/transaction-builder.js'
11+
import { getValidationService } from '../../services/validation-service.js'
1112
import { SafeCLIError } from '../../utils/errors.js'
1213
import { formatSafeAddress } from '../../utils/eip3770.js'
13-
import { validateAndChecksumAddress } from '../../utils/validation.js'
1414
import { renderScreen } from '../../ui/render.js'
1515
import { TransactionCreateSuccessScreen } from '../../ui/screens/index.js'
1616

@@ -22,6 +22,7 @@ export async function createTransaction() {
2222
const configStore = getConfigStore()
2323
const walletStorage = getWalletStorage()
2424
const transactionStore = getTransactionStore()
25+
const validator = getValidationService()
2526

2627
const activeWallet = walletStorage.getActiveWallet()
2728
if (!activeWallet) {
@@ -105,27 +106,16 @@ export async function createTransaction() {
105106
const toInput = await p.text({
106107
message: 'To address',
107108
placeholder: '0x...',
108-
validate: (value) => {
109-
if (!value) return 'Address is required'
110-
if (!isAddress(value)) return 'Invalid Ethereum address'
111-
return undefined
112-
},
109+
validate: (value) => validator.validateAddress(value),
113110
})
114111

115112
if (p.isCancel(toInput)) {
116113
p.cancel('Operation cancelled')
117114
return
118115
}
119116

120-
// Checksum the address immediately
121-
let to: Address
122-
try {
123-
to = validateAndChecksumAddress(toInput as string)
124-
} catch (error) {
125-
p.log.error(error instanceof Error ? error.message : 'Invalid address')
126-
p.outro('Failed')
127-
return
128-
}
117+
// Checksum the address
118+
const to = validator.assertAddress(toInput as string, 'To address')
129119

130120
// Check if address is a contract
131121
const contractService = new ContractService(chain)
@@ -139,7 +129,7 @@ export async function createTransaction() {
139129
try {
140130
isContract = await contractService.isContract(to)
141131
spinner2.stop(isContract ? 'Contract detected' : 'EOA (regular address)')
142-
} catch (error) {
132+
} catch {
143133
spinner2.stop('Failed to check contract')
144134
p.log.warning('Could not determine if address is a contract, falling back to manual input')
145135
}
@@ -159,7 +149,7 @@ export async function createTransaction() {
159149
}
160150

161151
const abiService = new ABIService(chain, etherscanApiKey)
162-
let abi: any = null
152+
let abi: ABI | null = null
163153
let contractName: string | undefined
164154

165155
try {
@@ -203,17 +193,15 @@ export async function createTransaction() {
203193
// Filter out duplicates by function signature
204194
const combinedAbi = [...implAbi]
205195
const existingSignatures = new Set(
206-
implAbi
207-
.filter((item: any) => item.type === 'function')
208-
.map(
209-
(item: any) =>
210-
`${item.name}(${item.inputs?.map((i: any) => i.type).join(',') || ''})`
211-
)
196+
(implAbi.filter((item) => item.type === 'function') as ABIFunction[]).map(
197+
(item) => `${item.name}(${item.inputs?.map((i) => i.type).join(',') || ''})`
198+
)
212199
)
213200

214201
for (const item of abi) {
215202
if (item.type === 'function') {
216-
const sig = `${item.name}(${item.inputs?.map((i: any) => i.type).join(',') || ''})`
203+
const funcItem = item as ABIFunction
204+
const sig = `${funcItem.name}(${funcItem.inputs?.map((i) => i.type).join(',') || ''})`
217205
if (!existingSignatures.has(sig)) {
218206
combinedAbi.push(item)
219207
}
@@ -225,14 +213,14 @@ export async function createTransaction() {
225213

226214
abi = combinedAbi
227215
console.log(` Combined: ${abi.length} items total`)
228-
} catch (error) {
216+
} catch {
229217
console.log('⚠ Could not fetch implementation ABI, using proxy ABI only')
230218
console.log(` Found ${abi.length} items in proxy ABI`)
231219
}
232220
} else {
233221
console.log(` Found ${abi.length} items in ABI`)
234222
}
235-
} catch (error) {
223+
} catch {
236224
console.log('⚠ Could not fetch ABI')
237225
console.log(' Contract may not be verified. Falling back to manual input.')
238226
}
@@ -261,7 +249,7 @@ export async function createTransaction() {
261249
const selectedFuncSig = await p.select({
262250
message: 'Select function to call:',
263251
options: functions.map((func) => {
264-
const signature = `${func.name}(${func.inputs?.map((i: any) => i.type).join(',') || ''})`
252+
const signature = `${func.name}(${func.inputs?.map((i) => i.type).join(',') || ''})`
265253
return {
266254
value: signature,
267255
label: abiService.formatFunctionSignature(func),
@@ -277,7 +265,7 @@ export async function createTransaction() {
277265
}
278266

279267
const func = functions.find((f) => {
280-
const sig = `${f.name}(${f.inputs?.map((i: any) => i.type).join(',') || ''})`
268+
const sig = `${f.name}(${f.inputs?.map((i) => i.type).join(',') || ''})`
281269
return sig === selectedFuncSig
282270
})
283271
if (!func) {
@@ -307,15 +295,7 @@ export async function createTransaction() {
307295
message: 'Value in wei (0 for token transfer)',
308296
placeholder: '0',
309297
initialValue: '0',
310-
validate: (val) => {
311-
if (!val) return 'Value is required'
312-
try {
313-
BigInt(val)
314-
return undefined
315-
} catch {
316-
return 'Invalid number'
317-
}
318-
},
298+
validate: (val) => validator.validateWeiValue(val),
319299
})) as string
320300

321301
if (p.isCancel(value)) {
@@ -327,14 +307,7 @@ export async function createTransaction() {
327307
message: 'Transaction data (hex)',
328308
placeholder: '0x',
329309
initialValue: '0x',
330-
validate: (val) => {
331-
if (!val) return 'Data is required (use 0x for empty)'
332-
if (!val.startsWith('0x')) return 'Data must start with 0x'
333-
if (val.length > 2 && !/^0x[0-9a-fA-F]*$/.test(val)) {
334-
return 'Data must be valid hex'
335-
}
336-
return undefined
337-
},
310+
validate: (val) => validator.validateHexData(val),
338311
})) as `0x${string}`
339312

340313
if (p.isCancel(data)) {
@@ -374,14 +347,7 @@ export async function createTransaction() {
374347
const nonceInput = (await p.text({
375348
message: 'Transaction nonce (leave empty for default)',
376349
placeholder: `${currentNonce} (recommended: current nonce)`,
377-
validate: (value) => {
378-
if (!value) return undefined // Empty is OK (will use default)
379-
const num = parseInt(value, 10)
380-
if (isNaN(num) || num < 0) return 'Nonce must be a non-negative number'
381-
if (num < currentNonce)
382-
return `Nonce cannot be lower than current Safe nonce (${currentNonce})`
383-
return undefined
384-
},
350+
validate: (value) => validator.validateNonce(value, currentNonce),
385351
})) as string
386352

387353
if (p.isCancel(nonceInput)) {

‎src/commands/tx/execute.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -173,7 +173,7 @@ export async function executeTransaction(safeTxHash?: string) {
173173
let privateKey: string
174174
try {
175175
privateKey = walletStorage.getPrivateKey(activeWallet.id, password)
176-
} catch (error) {
176+
} catch {
177177
spinner2.stop('Failed')
178178
p.log.error('Invalid password')
179179
p.outro('Failed')

0 commit comments

Comments
 (0)