Skip to content

Enable batch proposal using a Ledger device - #142

Merged
0xJem merged 8 commits into
CDsEmissionsfrom
batch-scripts-ledger
Oct 6, 2025
Merged

0xJem merged 8 commits into
CDsEmissionsfrom
batch-scripts-ledger

Conversation

@0xJem

@0xJem 0xJem commented Oct 2, 2025 •

Copy link
Copy Markdown
Member

Utilises the changes from Recon-Fuzz/safe-utils#17 to enable a two-step process for signing and proposing a batch to a Safe multi-sig

Summary by CodeRabbit

  • New Features

    • Added sign-only multisig workflow with Ledger support (offline signature generation and later submission) and broadened broadcast handling.
    • Ops scripts updated to accept sign-only, ledger path, and signature parameters; added price feed ownership transfer utility.
  • Documentation

    • Added Ledger device support guide describing the two-step sign-only + submit workflow.
  • Chores

    • Updated dependencies and remappings; simplified install script.
  • Style

    • Minor whitespace cleanup in shell scripts.

@0xJem 0xJem self-assigned this Oct 2, 2025
@coderabbitai

coderabbitai Bot commented Oct 2, 2025 •

Copy link
Copy Markdown
Contributor

Walkthrough

Adds Ledger sign-only and signature-based submission support across ops batch scripts and core BatchScriptV2, updates CLI tooling and shell scripts to accept --signonly/--signature and Ledger derivation handling, updates dependencies and remappings, removes a nested safe-utils install step, and documents the new workflow.

Changes

Cohort / File(s) Summary of changes
Dependencies & remappings
foundry.toml, remappings.txt
Bumps safe-utils rev; adds git dependencies safe-smart-account and solidity-http; adds remappings for Governors, safe-smart-account, and solidity-http.
Shell tooling
shell/full_install.sh, shell/lib/forge.sh, shell/safeBatchV2.sh
Removes nested install step for safe-utils; whitespace cleanup in forge.sh; extends safeBatchV2.sh with --signonly/--signature args, Ledger derivation handling, adjusted account validation, updated forge --sig function signature, and generalized broadcast handling.
Docs
src/scripts/ops/README.md
Adds Ledger Device Support section describing two-step sign-only then submit-with-signature workflow and usage notes.
Ops batch scripts (signing params)
src/scripts/ops/batches/...
Multiple batch contracts (e.g., ConvertibleDepositInstall.sol, HeartPeriodicTasksConfig.sol, MockPriceFeedConfig.sol, and other files under src/scripts/ops/batches/) have external function signatures changed to include signOnly, ledgerDerivationPath, and signature and now call setUp(...) instead of setUpWithChainIdAndArgsFile(...). Some files add lint directives; MockPriceFeedConfig also adds an ownership-transfer batch helper.
BatchScript core
src/scripts/ops/lib/BatchScriptV2.sol
Adds internal state for sign-only and ledger signing (_signOnly, _ledgerDerivationPath, _signature), new _setUp signature and setUp modifier accepting signing params, helpers for proposing batches with provided signatures or sign-only Ledger flows, and imports Enum for operation handling.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant U as User
  participant SH as safeBatchV2.sh
  participant FS as forge script
  participant BS as BatchScriptV2 (Sol)
  participant MS as Multisig Safe

  rect rgb(245,250,255)
    note right of U: Sign-only flow (derive signature via Ledger)
    U->>SH: run --multisig true --ledger <path> --signonly true
    SH->>FS: invoke forge with --sig (...bool,bool,string,string,bytes)() and signOnly=true, derivPath, signature=0x
    FS->>BS: setUp(useDaoMS, signOnly, argsFile, derivPath, signature=0x)
    BS->>BS: build batch, simulate, derive signature (Ledger)
    BS-->>SH: return derived signature (bytes)
  end

  rect rgb(245,255,245)
    note right of U: Submit-with-signature flow (broadcast or propose)
    U->>SH: run --signature 0xSIG --broadcast true
    SH->>FS: invoke forge with signOnly=false, derivPath="", signature=0xSIG
    FS->>BS: setUp(useDaoMS, signOnly=false, argsFile, "", signature=0xSIG)
    BS->>MS: propose/execute batch using provided signature
    MS-->>BS: tx hash / result
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Poem

A rabbit nudges keys at night,
Ledger blinks — a tiny light.
First I sign, then hand the seed,
Batches hop where code decreed.
Remaps set, the scripts take flight — 🐇✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title clearly and concisely summarizes the primary intent of the changeset by indicating that batch proposal support via a Ledger device has been enabled, which directly reflects the extensive updates to scripts, contracts, and tooling for Ledger signing and proposal workflows.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch batch-scripts-ledger

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 800887f and a336954.

⛔ Files ignored due to path filters (1)
  • soldeer.lock is excluded by !**/*.lock
📒 Files selected for processing (10)
  • foundry.toml (1 hunks)
  • remappings.txt (1 hunks)
  • shell/full_install.sh (0 hunks)
  • shell/lib/forge.sh (2 hunks)
  • shell/safeBatchV2.sh (5 hunks)
  • src/scripts/ops/README.md (1 hunks)
  • src/scripts/ops/batches/ConvertibleDepositInstall.sol (15 hunks)
  • src/scripts/ops/batches/HeartPeriodicTasksConfig.sol (3 hunks)
  • src/scripts/ops/batches/MockPriceFeedConfig.sol (3 hunks)
  • src/scripts/ops/lib/BatchScriptV2.sol (7 hunks)
💤 Files with no reviewable changes (1)
  • shell/full_install.sh
✅ Files skipped from review due to trivial changes (1)
  • shell/lib/forge.sh
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/scripts/ops/README.md
  • remappings.txt
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: 0xJem
PR: OlympusDAO/olympus-v3#29
File: src/modules/TRSRY/TRSRY.v1.sol:2-2
Timestamp: 2025-07-18T00:21:49.138Z
Learning: 0xJem prefers to avoid changing historical/already-deployed contracts to minimize risk and maintain stability, even when it would improve consistency across the codebase.
🧬 Code graph analysis (1)
shell/safeBatchV2.sh (3)
shell/lib/arguments.sh (2)
  • validate_boolean (118-130)
  • validate_text (60-65)
shell/lib/error.sh (1)
  • display_error (5-7)
shell/lib/forge.sh (2)
  • set_account_address_ledger (84-89)
  • validate_and_set_account (95-121)
🪛 Shellcheck (0.11.0)
shell/safeBatchV2.sh

[warning] 82-82: ledger is referenced but not assigned.

(SC2154)


[warning] 91-91: account is referenced but not assigned.

(SC2154)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
  • GitHub Check: run-ci
  • GitHub Check: run-ci
  • GitHub Check: run-ci
  • GitHub Check: run-ci
  • GitHub Check: run-ci
  • GitHub Check: run-ci
🔇 Additional comments (10)
foundry.toml (1)

66-66: LGTM! Dependency updates support Ledger signing.

The updated safe-utils revision and newly added safe-smart-account and solidity-http dependencies align with the PR's objective to enable Ledger-based batch proposal signing. These dependencies are correctly integrated across the codebase.

Also applies to: 74-75

src/scripts/ops/batches/HeartPeriodicTasksConfig.sol (1)

22-28: LGTM! Function signature updated for Ledger signing support.

The configurePeriodicTasks function now correctly accepts signOnly_, ledgerDerivationPath, and signature_ parameters and routes them through the new setUp modifier, consistent with the migration to Ledger-based signing across the codebase.

src/scripts/ops/batches/MockPriceFeedConfig.sol (1)

18-24: LGTM! Function signature updated for Ledger signing support.

The configurePriceFeed function now correctly accepts signOnly_, ledgerDerivationPath, and signature_ parameters and routes them through the new setUp modifier, consistent with the migration to Ledger-based signing.

shell/safeBatchV2.sh (1)

37-38: LGTM on sign-only and signature parameter handling!

The script correctly validates --signonly and --signature parameters, enforces that --signonly requires --multisig, and prevents providing a signature when signing. The flow correctly branches based on signonly to either use Ledger signing or the existing account validation.

Also applies to: 50-50, 59-59, 69-93

src/scripts/ops/lib/BatchScriptV2.sol (5)

25-55: LGTM! Internal state added for Ledger signing.

The new internal state variables (_signOnly, _ledgerDerivationPath, _signature) and the _hasSignature() helper correctly support sign-only and signature-based batch proposals.


61-76: LGTM! setUp flow updated to support Ledger signing.

The _setUp internal method and setUp modifier now correctly accept and propagate signOnly_, ledgerDerivationPath_, and signature_ parameters, enabling Ledger-based signing workflows.


89-134: LGTM! Batch script setup with appropriate validation.

The _setUpBatchScript method correctly validates that signOnly and signature are mutually exclusive (lines 124-126), logs the configuration, and initializes the multisig client when appropriate.


160-184: LGTM! Proposal logic correctly handles sign-only and signature modes.

The _proposeMultisigBatchTransactions method appropriately rejects proposals when signOnly is true, and branches to use the provided signature when available.


197-230: LGTM! Sign-only flow correctly generates signatures.

The _proposeMultisigBatch method correctly handles sign-only mode by calling _multiSig.sign with the Ledger derivation path (lines 200-216), logs the signature, and returns early. The broadcast check remains appropriate for non-sign-only flows.

src/scripts/ops/batches/ConvertibleDepositInstall.sol (1)

31-37: LGTM! All batch functions updated for Ledger signing support.

All public batch entry functions in this file now correctly accept signOnly_, ledgerDerivationPath, and signature_ parameters and route them through the new setUp modifier. The changes are consistent, mechanical, and align with the migration to Ledger-based signing across the codebase.

Also applies to: 180-186, 206-212, 228-234, 269-275, 306-312, 362-368, 407-413, 451-457, 522-528, 549-555, 600-606, 654-660


Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 2

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 03ddb8d and 800887f.

⛔ Files ignored due to path filters (1)
  • soldeer.lock is excluded by !**/*.lock
📒 Files selected for processing (10)
  • foundry.toml (1 hunks)
  • remappings.txt (1 hunks)
  • shell/full_install.sh (0 hunks)
  • shell/lib/forge.sh (2 hunks)
  • shell/safeBatchV2.sh (5 hunks)
  • src/scripts/ops/README.md (1 hunks)
  • src/scripts/ops/batches/ConvertibleDepositInstall.sol (15 hunks)
  • src/scripts/ops/batches/HeartPeriodicTasksConfig.sol (3 hunks)
  • src/scripts/ops/batches/MockPriceFeedConfig.sol (3 hunks)
  • src/scripts/ops/lib/BatchScriptV2.sol (7 hunks)
💤 Files with no reviewable changes (1)
  • shell/full_install.sh
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: 0xJem
PR: OlympusDAO/olympus-v3#29
File: src/modules/TRSRY/TRSRY.v1.sol:2-2
Timestamp: 2025-07-18T00:21:49.138Z
Learning: 0xJem prefers to avoid changing historical/already-deployed contracts to minimize risk and maintain stability, even when it would improve consistency across the codebase.
🧬 Code graph analysis (1)
shell/safeBatchV2.sh (3)
shell/lib/arguments.sh (2)
  • validate_boolean (118-130)
  • validate_text (60-65)
shell/lib/error.sh (1)
  • display_error (5-7)
shell/lib/forge.sh (2)
  • set_account_address_ledger (84-89)
  • validate_and_set_account (95-121)
🪛 Shellcheck (0.11.0)
shell/safeBatchV2.sh

[warning] 82-82: ledger is referenced but not assigned.

(SC2154)


[warning] 91-91: account is referenced but not assigned.

(SC2154)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
  • GitHub Check: run-ci
  • GitHub Check: run-ci
  • GitHub Check: run-ci
  • GitHub Check: run-ci
  • GitHub Check: run-ci
  • GitHub Check: run-ci
  • GitHub Check: run-ci

Comment thread shell/safeBatchV2.sh
Comment thread src/scripts/ops/batches/MockPriceFeedConfig.sol
Base automatically changed from cd-sepolia-deployment to CDsEmissions October 3, 2025 12:08
@0xJem
0xJem force-pushed the batch-scripts-ledger branch from 800887f to a336954 Compare October 3, 2025 12:15
@0xJem
0xJem merged commit 726a4d9 into CDsEmissions Oct 6, 2025
8 checks passed
@0xJem
0xJem deleted the batch-scripts-ledger branch October 6, 2025 07:51
This was referenced Nov 10, 2025
@coderabbitai coderabbitai Bot mentioned this pull request Mar 5, 2026
@coderabbitai coderabbitai Bot mentioned this pull request Mar 26, 2026
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.

1 participant