Skip to content

Conversation

@katspaugh
Copy link
Member

Summary

This PR implements Part 1 of Phase 3, adding argument support to account management commands for non-interactive automation scenarios.

Changes

account add-owner

  • Add --threshold <number> option to set new threshold after adding owner
  • Maintains backward compatibility with interactive threshold prompts

account remove-owner

  • Add --threshold <number> option to set new threshold after removing owner
  • Maintains backward compatibility with interactive threshold prompts

account change-threshold

  • Add --threshold <number> option to specify new threshold directly
  • Maintains backward compatibility with interactive threshold prompts

account open

  • Add --name <name> option to set Safe name non-interactively
  • Maintains backward compatibility with interactive name prompts

account create

  • Added interface for future argument support (options defined but not yet implemented)
  • CLI options registered: --chain-id, --owners, --threshold, --name, --no-deploy
  • Full implementation deferred to follow-up PR

All commands work both interactively (when no options provided) and non-interactively (with full options).

Examples

# Add owner with new threshold
safe account add-owner eth:0x123... 0x456... --threshold 2 --json

# Remove owner and adjust threshold
safe account remove-owner eth:0x123... 0x789... --threshold 1 --json

# Change threshold directly
safe account change-threshold eth:0x123... --threshold 3 --json

# Open Safe with name
safe account open eth:0x123... --name my-production-safe --json

Test plan

  • Build passes
  • Type check passes
  • Lint passes
  • Manual testing of each command with and without arguments
  • Verify JSON output format
  • Test error handling with invalid threshold values
  • Verify backward compatibility with interactive mode

Related

Part of Phase 3 (Account commands) of the non-interactive CLI enhancement plan. This covers the simpler account commands. Commands like create, deploy, and info will be addressed in a follow-up PR.

🤖 Generated with Claude Code

Add CLI argument support for add-owner, remove-owner, change-threshold,
and open commands to enable non-interactive automation:

- add-owner: Add --threshold option for new threshold value
- remove-owner: Add --threshold option for new threshold value
- change-threshold: Add --threshold option to specify new threshold
- open: Add --name option to set Safe name non-interactively

These commands maintain backward compatibility with interactive mode.

Also added placeholder interface for account create command (to be
implemented in follow-up).

🤖 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 November 7, 2025 15:14
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 adds support for command-line arguments to Safe account management commands, enabling non-interactive usage. The changes introduce optional parameters for operations like creating, opening, and managing Safe accounts through CLI flags rather than interactive prompts.

  • Adds options interfaces to account commands (create, open, add-owner, remove-owner, change-threshold)
  • Implements conditional logic to use CLI arguments when provided or fall back to interactive prompts
  • Updates CLI command definitions with corresponding option flags

Reviewed Changes

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

Show a summary per file
File Description
src/commands/account/create.ts Adds SafeCreateOptions interface and options parameter (marked as TODO for future implementation)
src/commands/account/open.ts Adds SafeOpenOptions interface with --name flag support for naming Safes non-interactively
src/commands/account/add-owner.ts Adds AddOwnerOptions interface with --threshold flag to specify threshold when adding owners
src/commands/account/remove-owner.ts Adds RemoveOwnerOptions interface with --threshold flag to specify threshold when removing owners
src/commands/account/change-threshold.ts Adds ChangeThresholdOptions interface with --threshold flag for non-interactive threshold changes
src/cli.ts Registers CLI options and wires them to command handlers for all account management commands

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

// Get name
let safeName: string

if (options.name) {
Copy link

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

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

When a name is provided via the --name option, it should be validated to ensure it's not empty. The interactive prompt validates that "Name is required" (line 155), but when passed via options, an empty string could be accepted. Consider adding validation:

if (options.name) {
  if (!options.name.trim()) {
    p.log.error('Name is required')
    p.outro('Failed')
    return
  }
  safeName = options.name
}
Suggested change
if (options.name) {
if (options.name) {
if (!options.name.trim()) {
p.log.error('Name is required')
p.outro('Failed')
return
}

Copilot uses AI. Check for mistakes.
Comment on lines +80 to +98
if (options.threshold) {
// Use provided threshold
const num = parseInt(options.threshold, 10)
if (isNaN(num) || num < 1) {
p.log.error('Threshold must be at least 1')
p.outro('Failed')
return
}
if (num > owners.length) {
p.log.error(`Threshold cannot exceed ${owners.length} (current owners)`)
p.outro('Failed')
return
}
if (num === currentThreshold) {
p.log.error('New threshold must be different from current threshold')
p.outro('Failed')
return
}
thresholdNum = num
Copy link

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

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

The threshold validation logic (lines 80-98) is duplicated across multiple files (add-owner.ts, remove-owner.ts, and change-threshold.ts). Consider extracting this into a reusable utility function to improve maintainability and ensure consistency.

Example:

// In utils/threshold-helpers.ts
export function validateThreshold(value: string, min: number, max: number): { valid: boolean; num?: number; error?: string } {
  const num = parseInt(value, 10)
  if (isNaN(num) || num < min) {
    return { valid: false, error: `Threshold must be at least ${min}` }
  }
  if (num > max) {
    return { valid: false, error: `Threshold cannot exceed ${max}` }
  }
  return { valid: true, num }
}

Copilot uses AI. Check for mistakes.
Comment on lines +161 to +174
if (options.threshold) {
// Use provided threshold
const num = parseInt(options.threshold, 10)
if (isNaN(num) || num < 1) {
p.log.error('Threshold must be at least 1')
p.outro('Failed')
return
}
if (num > currentOwners.length + 1) {
p.log.error(`Threshold cannot exceed ${currentOwners.length + 1} owners`)
p.outro('Failed')
return
}
thresholdNum = num
Copy link

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

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

The threshold validation logic (lines 161-174) is duplicated across multiple files (add-owner.ts, remove-owner.ts, and change-threshold.ts). Consider extracting this into a reusable utility function to improve maintainability and ensure consistency.

Example:

// In utils/threshold-helpers.ts
export function validateThreshold(value: string, min: number, max: number): { valid: boolean; num?: number; error?: string } {
  const num = parseInt(value, 10)
  if (isNaN(num) || num < min) {
    return { valid: false, error: `Threshold must be at least ${min}` }
  }
  if (num > max) {
    return { valid: false, error: `Threshold cannot exceed ${max}` }
  }
  return { valid: true, num }
}

Copilot uses AI. Check for mistakes.
Comment on lines +133 to +146
if (options.threshold) {
// Use provided threshold
const num = parseInt(options.threshold, 10)
if (isNaN(num) || num < 1) {
p.log.error('Threshold must be at least 1')
p.outro('Failed')
return
}
if (num > maxThreshold) {
p.log.error(`Threshold cannot exceed ${maxThreshold} (remaining owners)`)
p.outro('Failed')
return
}
thresholdNum = num
Copy link

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

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

The threshold validation logic (lines 133-146) is duplicated across multiple files (add-owner.ts, remove-owner.ts, and change-threshold.ts). Consider extracting this into a reusable utility function to improve maintainability and ensure consistency.

Example:

// In utils/threshold-helpers.ts
export function validateThreshold(value: string, min: number, max: number): { valid: boolean; num?: number; error?: string } {
  const num = parseInt(value, 10)
  if (isNaN(num) || num < min) {
    return { valid: false, error: `Threshold must be at least ${min}` }
  }
  if (num > max) {
    return { valid: false, error: `Threshold cannot exceed ${max}` }
  }
  return { valid: true, num }
}

Copilot uses AI. Check for mistakes.
@katspaugh katspaugh merged commit c74d868 into main Nov 7, 2025
10 checks passed
@katspaugh katspaugh deleted the feat/account-arguments branch November 7, 2025 15:29
katspaugh added a commit that referenced this pull request Nov 7, 2025
Resolved conflict in src/commands/account/create.ts by keeping the full
implementation over the placeholder from PR #30.
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