Skip to content

Commit f20df11

Browse files
katspaughclaude
katspaugh
and
claude
committed
fix: resolve critical bugs in config, transactions, and UI
This commit fixes several critical bugs discovered during testing: ## Bug Fixes ### 1. Config chains edit/add removes transactionServiceUrl - **Problem:** When editing or adding chains, the transactionServiceUrl and contractNetworks fields were being removed from all chains - **Fix:** Updated editChains() to preserve all ChainConfig fields when saving - **Fix:** Added prompt for transactionServiceUrl when adding new chains - **Files:** src/commands/config/chains.ts, src/commands/config/edit.ts ### 2. Transaction list shows "Signatures: 0/undefined" - **Problem:** Threshold was showing as "undefined" because it was stored locally but could become stale when changed on-chain - **Fix:** Changed architecture to always fetch threshold and owners fresh from blockchain via RPC instead of storing them - **Files:** Multiple tx commands (sign, execute, status, create, etc.) ### 3. Transaction sign fails with "Cannot read properties of null" - **Problem:** Multiple sources of null values causing crashes: - transaction.signatures array could be null for legacy transactions - metadata.value, metadata.data, metadata.operation could be null - **Fix:** Added normalization layer in TransactionStore to ensure signatures array is always initialized - **Fix:** Added default values for metadata fields in signTransaction() and executeTransaction() - **Fix:** Protected all array method calls with optional chaining and fallbacks - **Files:** src/storage/transaction-store.ts, src/services/transaction-service.ts, all tx commands ### 4. Duplicate variable declarations in create.ts - **Problem:** Duplicate chain and spinner variable declarations causing compilation errors - **Fix:** Removed duplicate chain declaration and renamed second spinner to spinner2 - **File:** src/commands/tx/create.ts ## UI Improvements ### Update Safe Transaction Service link to Safe Wallet - Changed "View on Safe Transaction Service" to "View in Safe Wallet" - Updated URL format to use Safe Wallet app: https://app.safe.global/transactions/tx - **Files:** src/ui/screens/TransactionPushSuccessScreen.tsx, src/commands/tx/push.ts ## Technical Details - All commands now fetch owners/threshold from blockchain for accuracy - Transaction storage now normalizes data on retrieval - All array operations protected against null/undefined values - Metadata fields have safe defaults 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 14086a2 commit f20df11

File tree

17 files changed

+304
-147
lines changed

17 files changed

+304
-147
lines changed

‎src/commands/account/change-threshold.ts‎

Lines changed: 39 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -88,32 +88,55 @@ export async function changeThreshold(account?: string) {
8888
return
8989
}
9090

91-
// Check if wallet is an owner
92-
if (!safe.owners || !safe.threshold) {
93-
p.log.error('Safe owner information not available. Please sync Safe data.')
91+
// Get chain
92+
const chain = configStore.getChain(safe.chainId)
93+
if (!chain) {
94+
p.log.error(`Chain ${safe.chainId} not found in configuration`)
95+
p.outro('Failed')
96+
return
97+
}
98+
99+
// Fetch live owners and threshold from blockchain
100+
const spinner = p.spinner()
101+
spinner.start('Fetching Safe information from blockchain...')
102+
103+
let owners: Address[]
104+
let currentThreshold: number
105+
try {
106+
const txService = new TransactionService(chain)
107+
;[owners, currentThreshold] = await Promise.all([
108+
txService.getOwners(safe.address as Address),
109+
txService.getThreshold(safe.address as Address),
110+
])
111+
spinner.stop('Safe information fetched')
112+
} catch (error) {
113+
spinner.stop('Failed to fetch Safe information')
114+
p.log.error(
115+
error instanceof Error ? error.message : 'Failed to fetch Safe data from blockchain'
116+
)
94117
p.outro('Failed')
95118
return
96119
}
97120

98-
if (!safe.owners.some((owner) => owner.toLowerCase() === activeWallet.address.toLowerCase())) {
121+
// Check if wallet is an owner
122+
if (!owners.some((owner) => owner.toLowerCase() === activeWallet.address.toLowerCase())) {
99123
p.log.error('Active wallet is not an owner of this Safe')
100124
p.outro('Failed')
101125
return
102126
}
103127

104128
// Ask for new threshold
105129
const newThreshold = await p.text({
106-
message: `New threshold (current: ${safe.threshold}, max: ${safe.owners.length}):`,
107-
placeholder: `${safe.threshold}`,
130+
message: `New threshold (current: ${currentThreshold}, max: ${owners.length}):`,
131+
placeholder: `${currentThreshold}`,
108132
validate: (value) => {
109133
if (!value) return 'Threshold is required'
110134
const num = parseInt(value, 10)
111135
if (isNaN(num) || num < 1) return 'Threshold must be at least 1'
112-
if (!safe.owners || !safe.threshold) return 'Safe data not available'
113-
if (num > safe.owners.length) {
114-
return `Threshold cannot exceed ${safe.owners.length} (current owners)`
136+
if (num > owners.length) {
137+
return `Threshold cannot exceed ${owners.length} (current owners)`
115138
}
116-
if (num === safe.threshold) {
139+
if (num === currentThreshold) {
117140
return 'New threshold must be different from current threshold'
118141
}
119142
return undefined
@@ -131,8 +154,8 @@ export async function changeThreshold(account?: string) {
131154
console.log('')
132155
console.log(pc.bold('Change Threshold Summary:'))
133156
console.log(` ${pc.dim('Safe:')} ${safe.name}`)
134-
console.log(` ${pc.dim('Owners:')} ${safe.owners.length}`)
135-
console.log(` ${pc.dim('Old Threshold:')} ${safe.threshold}`)
157+
console.log(` ${pc.dim('Owners:')} ${owners.length}`)
158+
console.log(` ${pc.dim('Old Threshold:')} ${currentThreshold}`)
136159
console.log(` ${pc.dim('New Threshold:')} ${thresholdNum}`)
137160
console.log('')
138161

@@ -146,16 +169,8 @@ export async function changeThreshold(account?: string) {
146169
return
147170
}
148171

149-
// Get chain
150-
const chain = configStore.getChain(safe.chainId)
151-
if (!chain) {
152-
p.log.error(`Chain ${safe.chainId} not found in configuration`)
153-
p.outro('Failed')
154-
return
155-
}
156-
157-
const spinner = p.spinner()
158-
spinner.start('Creating change threshold transaction...')
172+
const spinner2 = p.spinner()
173+
spinner2.start('Creating change threshold transaction...')
159174

160175
// Create the change threshold transaction using Safe SDK
161176
const txService = new TransactionService(chain)
@@ -174,13 +189,13 @@ export async function changeThreshold(account?: string) {
174189
activeWallet.address as Address
175190
)
176191

177-
spinner.stop('Transaction created')
192+
spinner2.stop('Transaction created')
178193

179194
await renderScreen(ThresholdChangeSuccessScreen, {
180195
safeTxHash: safeTransaction.safeTxHash,
181196
safeAddress: safe.address as Address,
182197
chainId: safe.chainId,
183-
oldThreshold: safe.threshold,
198+
oldThreshold: currentThreshold,
184199
newThreshold: thresholdNum,
185200
})
186201
} catch (error) {

‎src/commands/account/remove-owner.ts‎

Lines changed: 38 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -88,21 +88,45 @@ export async function removeOwner(account?: string) {
8888
return
8989
}
9090

91-
// Check if wallet is an owner
92-
if (!safe.owners || !safe.threshold) {
93-
p.log.error('Safe owner information not available. Please sync Safe data.')
91+
// Get chain
92+
const chain = configStore.getChain(safe.chainId)
93+
if (!chain) {
94+
p.log.error(`Chain ${safe.chainId} not found in configuration`)
95+
p.outro('Failed')
96+
return
97+
}
98+
99+
// Fetch live owners and threshold from blockchain
100+
const spinner = p.spinner()
101+
spinner.start('Fetching Safe information from blockchain...')
102+
103+
let owners: Address[]
104+
let currentThreshold: number
105+
try {
106+
const txService = new TransactionService(chain)
107+
;[owners, currentThreshold] = await Promise.all([
108+
txService.getOwners(safe.address as Address),
109+
txService.getThreshold(safe.address as Address),
110+
])
111+
spinner.stop('Safe information fetched')
112+
} catch (error) {
113+
spinner.stop('Failed to fetch Safe information')
114+
p.log.error(
115+
error instanceof Error ? error.message : 'Failed to fetch Safe data from blockchain'
116+
)
94117
p.outro('Failed')
95118
return
96119
}
97120

98-
if (!safe.owners.some((owner) => owner.toLowerCase() === activeWallet.address.toLowerCase())) {
121+
// Check if wallet is an owner
122+
if (!owners.some((owner) => owner.toLowerCase() === activeWallet.address.toLowerCase())) {
99123
p.log.error('Active wallet is not an owner of this Safe')
100124
p.outro('Failed')
101125
return
102126
}
103127

104128
// Check that Safe has at least 2 owners
105-
if (safe.owners.length <= 1) {
129+
if (owners.length <= 1) {
106130
p.log.error('Cannot remove the last owner from a Safe')
107131
p.outro('Failed')
108132
return
@@ -111,7 +135,7 @@ export async function removeOwner(account?: string) {
111135
// Select owner to remove
112136
const ownerToRemove = await p.select({
113137
message: 'Select owner to remove:',
114-
options: safe.owners.map((owner) => ({
138+
options: owners.map((owner) => ({
115139
value: owner,
116140
label: owner,
117141
})),
@@ -125,10 +149,9 @@ export async function removeOwner(account?: string) {
125149
const removeAddress = ownerToRemove as Address
126150

127151
// Calculate max threshold after removal
128-
const maxThreshold = safe.owners.length - 1
152+
const maxThreshold = owners.length - 1
129153

130154
// Ask about threshold
131-
const currentThreshold = safe.threshold
132155
const suggestedThreshold = Math.min(currentThreshold, maxThreshold)
133156

134157
const newThreshold = await p.text({
@@ -158,9 +181,9 @@ export async function removeOwner(account?: string) {
158181
console.log(pc.bold('Remove Owner Summary:'))
159182
console.log(` ${pc.dim('Safe:')} ${safe.name}`)
160183
console.log(` ${pc.dim('Remove Owner:')} ${removeAddress}`)
161-
console.log(` ${pc.dim('Current Owners:')} ${safe.owners.length}`)
162-
console.log(` ${pc.dim('New Owners:')} ${safe.owners.length - 1}`)
163-
console.log(` ${pc.dim('Old Threshold:')} ${safe.threshold}`)
184+
console.log(` ${pc.dim('Current Owners:')} ${owners.length}`)
185+
console.log(` ${pc.dim('New Owners:')} ${owners.length - 1}`)
186+
console.log(` ${pc.dim('Old Threshold:')} ${currentThreshold}`)
164187
console.log(` ${pc.dim('New Threshold:')} ${thresholdNum}`)
165188
console.log('')
166189

@@ -174,16 +197,8 @@ export async function removeOwner(account?: string) {
174197
return
175198
}
176199

177-
// Get chain
178-
const chain = configStore.getChain(safe.chainId)
179-
if (!chain) {
180-
p.log.error(`Chain ${safe.chainId} not found in configuration`)
181-
p.outro('Failed')
182-
return
183-
}
184-
185-
const spinner = p.spinner()
186-
spinner.start('Creating remove owner transaction...')
200+
const spinner2 = p.spinner()
201+
spinner2.start('Creating remove owner transaction...')
187202

188203
// Create the remove owner transaction using Safe SDK
189204
const txService = new TransactionService(chain)
@@ -203,13 +218,13 @@ export async function removeOwner(account?: string) {
203218
activeWallet.address as Address
204219
)
205220

206-
spinner.stop('Transaction created')
221+
spinner2.stop('Transaction created')
207222

208223
await renderScreen(OwnerRemoveSuccessScreen, {
209224
safeTxHash: safeTransaction.safeTxHash,
210225
safeAddress: safe.address as Address,
211226
chainId: safe.chainId,
212-
threshold: safe.threshold,
227+
threshold: currentThreshold,
213228
})
214229
} catch (error) {
215230
if (error instanceof SafeCLIError) {

‎src/commands/config/chains.ts‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,13 +95,28 @@ export async function addChain() {
9595
return
9696
}
9797

98+
const transactionServiceUrl = await p.text({
99+
message: 'Safe Transaction Service URL (optional):',
100+
placeholder: 'https://safe-transaction-mainnet.safe.global',
101+
validate: (value) => {
102+
if (value && !isValidUrl(value)) return 'Invalid URL'
103+
return undefined
104+
},
105+
})
106+
107+
if (p.isCancel(transactionServiceUrl)) {
108+
p.cancel('Operation cancelled')
109+
return
110+
}
111+
98112
const chainConfig: ChainConfig = {
99113
name: name as string,
100114
chainId: chainId as string,
101115
shortName: shortName as string,
102116
rpcUrl: rpcUrl as string,
103117
explorer: explorer ? (explorer as string) : undefined,
104118
currency: currency as string,
119+
transactionServiceUrl: transactionServiceUrl ? (transactionServiceUrl as string) : undefined,
105120
}
106121

107122
const spinner = p.spinner()

‎src/commands/config/edit.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,8 @@ export async function editChains() {
3131
rpcUrl: 'RPC endpoint URL',
3232
currency: 'Native currency symbol (e.g., "ETH")',
3333
explorer: '(Optional) Block explorer base URL',
34+
transactionServiceUrl: '(Optional) Safe Transaction Service API URL',
35+
contractNetworks: '(Optional) Safe contract addresses for this chain',
3436
},
3537
chains,
3638
}
@@ -166,6 +168,8 @@ export async function editChains() {
166168
rpcUrl: c.rpcUrl,
167169
currency: c.currency,
168170
explorer: c.explorer,
171+
transactionServiceUrl: c.transactionServiceUrl,
172+
contractNetworks: c.contractNetworks,
169173
})
170174
}
171175

‎src/commands/tx/create.ts‎

Lines changed: 28 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -69,14 +69,34 @@ export async function createTransaction() {
6969
return
7070
}
7171

72-
// Check if wallet is an owner
73-
if (!safe.owners) {
74-
p.log.error('Safe owner information not available. Please sync Safe data.')
72+
// Get chain
73+
const chain = configStore.getChain(chainId)
74+
if (!chain) {
75+
p.log.error(`Chain ${chainId} not found in configuration`)
7576
p.outro('Failed')
7677
return
7778
}
7879

79-
if (!safe.owners.some((owner) => owner.toLowerCase() === activeWallet.address.toLowerCase())) {
80+
// Fetch live owners from blockchain
81+
const spinner = p.spinner()
82+
spinner.start('Fetching Safe information from blockchain...')
83+
84+
let owners: Address[]
85+
try {
86+
const txService = new TransactionService(chain)
87+
owners = await txService.getOwners(address as Address)
88+
spinner.stop('Safe information fetched')
89+
} catch (error) {
90+
spinner.stop('Failed to fetch Safe information')
91+
p.log.error(
92+
error instanceof Error ? error.message : 'Failed to fetch Safe data from blockchain'
93+
)
94+
p.outro('Failed')
95+
return
96+
}
97+
98+
// Check if wallet is an owner
99+
if (!owners.some((owner) => owner.toLowerCase() === activeWallet.address.toLowerCase())) {
80100
p.log.error('Active wallet is not an owner of this Safe')
81101
p.outro('Failed')
82102
return
@@ -108,28 +128,20 @@ export async function createTransaction() {
108128
return
109129
}
110130

111-
// Get chain for contract detection
112-
const chain = configStore.getChain(safe.chainId)
113-
if (!chain) {
114-
p.log.error(`Chain ${safe.chainId} not found in configuration`)
115-
p.outro('Failed')
116-
return
117-
}
118-
119131
// Check if address is a contract
120132
const contractService = new ContractService(chain)
121133
let isContract = false
122134
let value = '0'
123135
let data: `0x${string}` = '0x'
124136

125-
const spinner = p.spinner()
126-
spinner.start('Checking if address is a contract...')
137+
const spinner2 = p.spinner()
138+
spinner2.start('Checking if address is a contract...')
127139

128140
try {
129141
isContract = await contractService.isContract(to)
130-
spinner.stop(isContract ? 'Contract detected' : 'EOA (regular address)')
142+
spinner2.stop(isContract ? 'Contract detected' : 'EOA (regular address)')
131143
} catch (error) {
132-
spinner.stop('Failed to check contract')
144+
spinner2.stop('Failed to check contract')
133145
p.log.warning('Could not determine if address is a contract, falling back to manual input')
134146
}
135147

0 commit comments

Comments
 (0)