Skip to content

Conversation

@katspaugh
Copy link
Member

Summary

  • Add on-chain deployment verification to prevent "Safe already deployed" errors when local storage is out of sync
  • Implement automatic salt nonce increment to allow creating multiple Safes with identical configurations
  • Sync local storage state with blockchain reality to ensure consistency

Changes

Safe Deployment (src/commands/account/deploy.ts)

  • Added on-chain verification using SafeService.getSafeInfo() before deployment
  • Automatically syncs local storage deployed flag with blockchain state
  • Prevents deployment attempts for Safes that are actually deployed on-chain
  • Fixes local storage corruption where Safes were marked as deployed but weren't

Safe Creation (src/commands/account/create.ts)

  • Implemented salt nonce increment loop (0 → 1 → 2 → ...) with max 100 attempts
  • Checks on-chain deployment status for each predicted address
  • Automatically finds next available address when configuration collides
  • Stores salt nonce in predictedConfig for deterministic deployment

Fixes

  1. Deployment Bug: Users could not deploy Safes when local storage incorrectly marked them as deployed but blockchain showed they weren't
  2. Address Collision: Users could not create multiple Safes with the same owner/threshold configuration

Test Plan

  • Tested deployment command with already-deployed Safe shows correct error
  • Tested that local storage syncs with on-chain state
  • Lint passes (no new warnings introduced)
  • Type check passes
  • Build successful

🤖 Generated with Claude Code

…oyment

- Add on-chain deployment verification before attempting to deploy a Safe
- Automatically sync local storage with blockchain state to prevent mismatches
- Implement automatic salt nonce increment when creating Safes with duplicate configs
- Prevent address collisions by checking up to 100 salt nonce variations
- Store salt nonce in predictedConfig for deterministic address generation

This resolves issues where:
1. Users couldn't deploy Safes that local storage incorrectly marked as deployed
2. Users couldn't create multiple Safes with identical owner/threshold configurations

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings October 25, 2025 09:34
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enhances Safe account deployment reliability by adding on-chain verification and automatic salt nonce increment functionality. It addresses two critical issues: local storage inconsistencies where Safes were marked as deployed without actual on-chain deployment, and address collisions when creating multiple Safes with identical configurations.

Key changes:

  • On-chain verification before deployment attempts with automatic local storage synchronization
  • Salt nonce increment loop (0→100 max attempts) to find available addresses when configurations collide
  • Storage of salt nonce in predictedConfig for deterministic deployment

Reviewed Changes

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

File Description
src/commands/account/deploy.ts Adds on-chain deployment verification via SafeService.getSafeInfo() and syncs local storage state with blockchain reality before deployment attempts
src/commands/account/create.ts Implements salt nonce increment loop to prevent address collisions and stores salt nonce in predictedConfig

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

Comment on lines 75 to +85
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)
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.
Comment on lines +202 to +204
if (attempts >= maxAttempts || !predictedAddress || !safeAccountConfig) {
throw new Error(`Could not find available Safe address after ${maxAttempts} attempts`)
}
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.
@katspaugh katspaugh merged commit 5d6a825 into main Oct 25, 2025
4 checks passed
@katspaugh katspaugh deleted the fix/safe-deployment-collision-handling branch October 25, 2025 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants