Skip to content

fix: add on-chain verification and salt nonce increment for Safe deployment #4

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
katspaugh merged 1 commit into from
Oct 25, 2025
Merged

fix: add on-chain verification and salt nonce increment for Safe deployment #4

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 39 additions & 4 deletions src/commands/account/create.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,10 +164,44 @@ export async function createSafe() {

try {
const safeService = new SafeService(chain)
const { predictedAddress, safeAccountConfig } = await safeService.createPredictedSafe({
owners,
threshold: thresholdNum,
})

// Find an available salt nonce (increment if Safe already deployed)
let saltNonce = '0'
let predictedAddress: Address | undefined
let safeAccountConfig: { owners: string[]; threshold: number } | undefined
let attempts = 0
const maxAttempts = 100

while (attempts < maxAttempts) {
const result = await safeService.createPredictedSafe({
owners,
threshold: thresholdNum,
saltNonce,
})

predictedAddress = result.predictedAddress
safeAccountConfig = result.safeAccountConfig

// Check if Safe already deployed at this address
try {
const safeInfo = await safeService.getSafeInfo(predictedAddress)
if (safeInfo.isDeployed) {
// Safe already deployed, try next salt nonce
saltNonce = (BigInt(saltNonce) + 1n).toString()
attempts++
continue
}
} catch {
// Error checking deployment status, assume not deployed
}

// Found an available address
break
}

if (attempts >= maxAttempts || !predictedAddress || !safeAccountConfig) {
throw new Error(`Could not find available Safe address after ${maxAttempts} attempts`)
}
Comment on lines +202 to +204
Copy link

Copilot AI Oct 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message doesn't distinguish between 'max attempts exceeded' and 'missing predictedAddress/safeAccountConfig'. The latter would indicate a logic error rather than exhausted attempts. Consider separate error messages: one for exceeding max attempts, and another for unexpected missing data.

Suggested change
if (attempts >= maxAttempts || !predictedAddress || !safeAccountConfig) {
throw new Error(`Could not find available Safe address after ${maxAttempts} attempts`)
}
if (attempts >= maxAttempts) {
throw new Error(`Could not find available Safe address after ${maxAttempts} attempts`)
}
if (!predictedAddress || !safeAccountConfig) {
throw new Error('Unexpected error: missing predictedAddress or safeAccountConfig after finding available Safe address')
}

Copilot uses AI. Check for mistakes.

spinner.stop('Safe created!')

Expand All @@ -180,6 +214,7 @@ export async function createSafe() {
predictedConfig: {
owners: safeAccountConfig.owners,
threshold: safeAccountConfig.threshold,
saltNonce,
},
})

Expand Down
32 changes: 26 additions & 6 deletions src/commands/account/deploy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,16 +72,37 @@ export async function deploySafe(account?: string) {
return
}

if (safe.deployed) {
logError('Safe is already deployed')
return
}

if (!safe.predictedConfig) {
logError('Safe does not have deployment configuration')
return
}

// Verify on-chain deployment status
const chain = configStore.getChain(safe.chainId)!
const safeService = new SafeService(chain)

try {
const safeInfo = await safeService.getSafeInfo(address)
Comment on lines 75 to +85
Copy link

Copilot AI Oct 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The order of checks has been changed so that predictedConfig is now verified before checking on-chain deployment status. This creates inconsistent logic: the on-chain verification runs even when predictedConfig is missing, but deployment requires predictedConfig. Consider moving the predictedConfig check after the on-chain verification, or restructure to check both predictedConfig and on-chain status before proceeding with any logic that depends on either.

Copilot uses AI. Check for mistakes.

// If Safe is actually deployed on-chain
if (safeInfo.isDeployed) {
// Sync local storage with on-chain reality
if (!safe.deployed) {
safeStorage.updateSafe(chainId, address, { deployed: true })
}
logError('Safe is already deployed on-chain')
return
}

// If local storage says deployed but on-chain says not deployed, fix the storage
if (safe.deployed && !safeInfo.isDeployed) {
safeStorage.updateSafe(chainId, address, { deployed: false })
}
} catch {
// If we can't verify, log warning but continue
console.log(pc.yellow('⚠ Warning: Could not verify on-chain deployment status'))
}

// Get active wallet
const activeWallet = walletStorage.getActiveWallet()
if (!activeWallet) {
Expand All @@ -90,7 +111,6 @@ export async function deploySafe(account?: string) {
return
}

const chain = configStore.getChain(safe.chainId)!
const eip3770 = formatSafeAddress(safe.address as Address, safe.chainId, chains)

console.log('')
Expand Down
Loading