Conversation
invocamanman
commented
Jan 21, 2026
invocamanman
commented
- add safe multisig tool
- Manage a safe offchain, allowing udpating the owners and sign any transaction ( in case projects want to use a multisig without gnosis UI)
- Especifically give roleAdmin of the timelock to another address (In case projects want to receive the admin role and manage it themselves)
- add ledger support
- Add utils to easily deploy gnosis safes
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
There was a problem hiding this comment.
Pull request overview
This pull request adds comprehensive tooling for managing Gnosis Safe multisig wallets, with specific focus on granting timelock roles and general Safe operations. The PR includes Ledger hardware wallet support and utilities for deploying Safes.
Changes:
- Adds Safe multisig transaction signing and execution tools with EIP-712 support
- Integrates Ledger hardware wallet support via hardhat configuration and patches
- Provides utilities for managing Safe owners, granting timelock roles, and deploying Safes
Reviewed changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/safeMultisigGrantRole/* | Tools for granting timelock roles via Safe multisig |
| tools/safeMultisig/* | General Safe multisig transaction management utilities |
| tools/ledgerGrantRoles/* | Tool for self-granting PROPOSER and EXECUTOR roles |
| tools/deploySafe/* | Utility for deploying Safes to multiple networks |
| tools/replayTx/* | Transaction replay utility |
| hardhat.config.ts | Ledger integration in network configurations |
| package.json | Added hardhat-ledger and patch-package dependencies |
| patches/* | Patch for hardhat-ledger to fix eth_accounts deprecation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@codex review this PR |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed6ea20c7e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 26 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| interface SafeTransaction { | ||
| to: string; | ||
| value: number; | ||
| data: string; | ||
| operation: number; | ||
| safeTxGas: number; | ||
| baseGas: number; | ||
| gasPrice: number; | ||
| gasToken: string; | ||
| refundReceiver: string; | ||
| nonce: number; | ||
| } | ||
|
|
||
| interface SafeSignature { | ||
| signer: string; | ||
| data: string; | ||
| } | ||
|
|
||
| interface SignedTransactionData { | ||
| safeTx: SafeTransaction; | ||
| signatures: SafeSignature[]; | ||
| txHash: string; | ||
| chainId?: number; | ||
| } | ||
|
|
||
| /* ////////////////////////////////////////////////////////////// | ||
| HELPERS | ||
| ////////////////////////////////////////////////////////////// */ | ||
|
|
There was a problem hiding this comment.
The value field in SafeTransaction should be string type to handle large uint256 values safely, not number. JavaScript numbers can only safely represent integers up to 2^53-1, while Ethereum values can be much larger.
| git clone https://github.com/agglayer/agglayer-contracts.git | ||
| cd agglayer-contracts | ||
| ``` | ||
|
|
||
| ### 2. Checkout the Feature Branch | ||
|
|
||
| ```bash | ||
| git checkout feature/addSafeMultisigTool | ||
| ``` | ||
|
|
||
| ### 3. Install Dependencies | ||
|
|
||
| ```bash | ||
| npm install | ||
| ``` | ||
|
|
||
| ## ⚙️ Environment Configuration | ||
|
|
||
| ### 1. Create Environment File | ||
|
|
||
| ```bash | ||
| cp .env.example .env | ||
| ``` | ||
|
|
||
| ### 2. Configure `.env` File | ||
|
|
||
| Open `.env` and set the following variables: | ||
|
|
||
| ```env | ||
| # RPC Provider URL | ||
| CUSTOM_PROVIDER=your_rpc_here | ||
|
|
||
| # Your Ledger wallet address | ||
| LEDGER_ACCOUNT=0xYourLedgerAddress | ||
| ``` | ||
|
|
||
| | Variable | Description | | ||
| |----------|-------------| | ||
| | `CUSTOM_PROVIDER` | RPC URL for the target network | | ||
| | `LEDGER_ACCOUNT` | Your Ledger wallet's Ethereum address | | ||
|
|
||
| --- | ||
|
|
||
| ## 📝 Step 1: Sign Safe Transaction (All Owners) | ||
|
|
||
| > **⚠️ Each Safe owner must complete this step** | ||
|
|
||
| ### 1.1 Navigate to Tool Directory | ||
|
|
||
| ```bash | ||
| cd tools/safeMultisigGrantRole | ||
| ``` | ||
|
|
||
| ### 1.2 Create Parameters File | ||
|
|
||
| ```bash | ||
| cp parameters.json.example parameters.json | ||
| ``` | ||
|
|
||
| ### 1.3 Configure `parameters.json` | ||
|
|
||
| Edit the file with your specific addresses: | ||
|
|
||
| ```json | ||
| { | ||
| "safeAddress": "0x242daE44F5d8fb54B198D03a94dA45B5a4413e21", | ||
| "timelockAddress": "0xBBa0935Fa93Eb23de7990b47F0D96a8f75766d13", | ||
| "roleReceiver": "0xAddressToReceiveTheRole", | ||
| "roleName": "TIMELOCK_ADMIN_ROLE" | ||
| } | ||
| ``` | ||
|
|
||
| | Parameter | Description | | ||
| |-----------|-------------| | ||
| | `safeAddress` | Address of the Safe Multisig wallet | | ||
| | `timelockAddress` | Address of the Timelock contract | | ||
| | `roleReceiver` | Address that will receive `TIMELOCK_ADMIN_ROLE` | | ||
| | `roleName` | Role to grant (use `TIMELOCK_ADMIN_ROLE`) | | ||
|
|
||
| ### 1.4 Sign the Transaction | ||
|
|
||
| Each Safe owner runs this command from the **repository root**: | ||
|
|
||
| ```bash | ||
| npx hardhat run tools/safeMultisigGrantRole/signSafeTransaction.ts --network custom | ||
| ``` | ||
|
|
||
| **What happens:** | ||
| 1. Connects to your Ledger | ||
| 2. Displays transaction details | ||
| 3. Prompts you to confirm on Ledger device | ||
| 4. Saves signature to `signedTransactions.json` | ||
|
|
||
| ### 1.5 Share Signatures | ||
|
|
||
| After each owner signs: | ||
| - The `signedTransactions.json` file is updated with the new signature | ||
| - **Share this file** with other owners or the executor | ||
| - Continue until threshold is reached | ||
|
|
||
| ``` | ||
| ┌────────────────────────────────────────────────────────────────┐ | ||
| │ Signer 1 runs signSafeTransaction.ts │ | ||
| │ └─▶ signedTransactions.json created (1/3 signatures) │ | ||
| ├────────────────────────────────────────────────────────────────┤ | ||
| │ Signer 2 runs signSafeTransaction.ts │ | ||
| │ └─▶ signedTransactions.json updated (2/3 signatures) │ | ||
| ├────────────────────────────────────────────────────────────────┤ | ||
| │ Signer 3 runs signSafeTransaction.ts │ | ||
| │ └─▶ signedTransactions.json updated (3/3 signatures) ✅ │ | ||
| └────────────────────────────────────────────────────────────────┘ | ||
| ``` | ||
|
|
||
| ### 1.6 Using Different Signer Index (Optional) | ||
|
|
||
| If you have multiple accounts configured: | ||
|
|
||
| ```bash | ||
| SIGNER_INDEX=1 npx hardhat run tools/safeMultisigGrantRole/signSafeTransaction.ts --network custom | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ## ⚡ Step 2: Execute Safe Transaction | ||
|
|
||
| > **Once sufficient signatures are collected (>= threshold)** | ||
|
|
||
| ### Option A: Execute with Ledger | ||
|
|
||
| If your Ledger account has ETH for gas: | ||
|
|
||
| ```bash | ||
| npx hardhat run tools/safeMultisigGrantRole/executeSafeTransaction.ts --network custom | ||
| ``` | ||
|
|
||
| ### Option B: Execute with Private Key / Mnemonic | ||
|
|
||
| If using a different account with ETH: | ||
|
|
||
| 1. Update `.env` to remove or comment out `LEDGER_ACCOUNT`: | ||
| ```env | ||
| # LEDGER_ACCOUNT=0xYourLedgerAddress | ||
| MNEMONIC=your twelve word mnemonic phrase here | ||
| ``` | ||
|
|
||
| 2. Run the execute script: | ||
| ```bash | ||
| npx hardhat run tools/safeMultisigGrantRole/executeSafeTransaction.ts --network custom | ||
| ``` | ||
|
|
||
| ### Successful Output | ||
|
|
||
| ``` | ||
| ╔══════════════════════════════════════════════════════════════╗ | ||
| ║ ✅ TRANSACTION SUCCESS ║ | ||
| ╚══════════════════════════════════════════════════════════════╝ | ||
|
|
||
| Block: 12345678 | ||
| Gas Used: 85000 | ||
| Tx Hash: 0x... | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ## 🎖️ Step 3: Self-Grant Operational Roles | ||
|
|
||
| > **This step is performed by the account that just received `TIMELOCK_ADMIN_ROLE`** | ||
|
|
||
| Now the new admin can grant themselves the operational roles (`PROPOSER_ROLE` and `EXECUTOR_ROLE`). | ||
|
|
||
| ### 3.1 Navigate to Tool Directory | ||
|
|
||
| ```bash | ||
| cd tools/ledgerGrantRoles | ||
| ``` | ||
|
|
||
| ### 3.2 Create Parameters File | ||
|
|
||
| ```bash | ||
| cp parameters.json.example parameters.json | ||
| ``` | ||
|
|
||
| ### 3.3 Configure `parameters.json` | ||
|
|
||
| ```json | ||
| { | ||
| "timelockAddress": "0xBBa0935Fa93Eb23de7990b47F0D96a8f75766d13" | ||
| } | ||
| ``` | ||
|
|
||
| | Parameter | Description | | ||
| |-----------|-------------| | ||
| | `timelockAddress` | Address of the Timelock contract | | ||
|
|
||
| ### 3.4 Ensure `.env` is Configured | ||
|
|
||
| Make sure your `.env` has the new admin's Ledger address: | ||
|
|
||
| ```env | ||
| CUSTOM_PROVIDER=your_rpc_here | ||
| LEDGER_ACCOUNT=0xYourNewAdminLedgerAddress | ||
| ``` | ||
|
|
||
| ### 3.5 Run the Grant Roles Script | ||
|
|
||
| From the **repository root**: | ||
|
|
||
| ```bash | ||
| npx hardhat run tools/ledgerGrantRoles/ledgerGrantRoles.ts --network custom | ||
| ``` | ||
|
|
||
| ### Successful Output | ||
|
|
||
| ``` | ||
| ╔══════════════════════════════════════════════════════════════╗ | ||
| ║ LEDGER - GRANT PROPOSER & EXECUTOR ROLES ║ | ||
| ╚══════════════════════════════════════════════════════════════╝ | ||
|
|
||
| Checking signer roles... | ||
| Has TIMELOCK_ADMIN_ROLE: ✅ Yes | ||
| Has PROPOSER_ROLE: ❌ No | ||
| Has EXECUTOR_ROLE: ❌ No | ||
|
|
||
| Granting PROPOSER_ROLE... | ||
| Transaction sent: 0x... | ||
| ✅ PROPOSER_ROLE granted! | ||
|
|
||
| Granting EXECUTOR_ROLE... | ||
| Transaction sent: 0x... | ||
| ✅ EXECUTOR_ROLE granted! | ||
|
|
||
| ╔══════════════════════════════════════════════════════════════╗ | ||
| ║ SUCCESS ║ | ||
| ╚══════════════════════════════════════════════════════════════╝ | ||
|
|
||
| ✅ Account 0x... now has: | ||
| - PROPOSER_ROLE (can schedule transactions) | ||
| - EXECUTOR_ROLE (can execute ready transactions) | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ## 🔧 Troubleshooting | ||
|
|
||
| ### Ledger Connection Issues | ||
|
|
||
| | Issue | Solution | | ||
| |-------|----------| | ||
| | Ledger not detected | Reconnect USB cable, ensure device is unlocked | | ||
| | Ethereum app not found | Open Ethereum app on Ledger before running script | | ||
| | Transaction rejected | Enable "Blind signing" in Ledger Ethereum app settings | | ||
|
|
||
| ### Signature Issues | ||
|
|
||
| | Issue | Solution | | ||
| |-------|----------| | ||
| | "Signer is not a Safe owner" | Verify `LEDGER_ACCOUNT` is a Safe owner | | ||
| | "Already signed" | This signer has already signed; another owner needs to sign | | ||
| | "Nonce mismatch" | Another transaction was executed; delete `signedTransactions.json` and re-sign | | ||
|
|
||
| ### Execution Issues | ||
|
|
||
| | Issue | Solution | | ||
| |-------|----------| | ||
| | "Insufficient signatures" | More owners need to run `signSafeTransaction.ts` | | ||
| | "Gas estimation failed" | Check signatures are valid; try `FORCE_EXECUTE=true` | | ||
| | "Nonce mismatch" | Use `FORCE_EXECUTE=true` or re-collect signatures | | ||
|
|
||
| ### Role Issues | ||
|
|
||
| | Issue | Solution | | ||
| |-------|----------| | ||
| | "Does not have TIMELOCK_ADMIN_ROLE" | Complete Steps 1 & 2 first to grant the admin role | | ||
|
|
||
| --- | ||
|
|
||
| ## 📚 Supported Roles Reference | ||
|
|
||
| | Role | Description | | ||
| |------|-------------| | ||
| | `TIMELOCK_ADMIN_ROLE` | Admin of the timelock (can grant/revoke roles) | | ||
| | `PROPOSER_ROLE` | Can propose/schedule transactions | | ||
| | `EXECUTOR_ROLE` | Can execute ready (delayed) transactions | | ||
| | `CANCELLER_ROLE` | Can cancel pending transactions | | ||
|
|
||
| --- | ||
|
|
||
| ## 📁 File Reference | ||
|
|
||
| | Path | Description | | ||
| |------|-------------| | ||
| | `.env` | Environment configuration | | ||
| | `tools/safeMultisigGrantRole/parameters.json` | Safe multisig grant role config | | ||
| | `tools/safeMultisigGrantRole/signedTransactions.json` | Accumulated signatures (auto-generated) | | ||
| | `tools/ledgerGrantRoles/parameters.json` | Self-grant roles config | | ||
|
|
||
| --- | ||
|
|
||
| ## 🔗 Quick Command Reference | ||
|
|
||
| ```bash | ||
| # 1. Setup | ||
| git clone https://github.com/0xPolygonHermez/zkevm-contracts.git |
There was a problem hiding this comment.
The discrepancy in GUIDE_SAFE_MULTISIG_GRANT_ROLE.md references clone URLs from different repositories. Line 73 uses 'agglayer-contracts' while line 375 uses '0xPolygonHermez/zkevm-contracts'. These should be consistent and point to the correct repository.
| git clone https://github.com/agglayer/agglayer-contracts.git | ||
| cd zkevm-contracts |
There was a problem hiding this comment.
Similar repository URL inconsistency - uses '0xPolygonHermez/zkevm-contracts' but should likely be 'agglayer/agglayer-contracts' to match the repository context.
|
|
||
| - node version: 16.x | ||
| - npm version: 7.x | ||
| - node version: 22.x |
There was a problem hiding this comment.
Node version requirement changed from 16.x/18.x to 22.x which is a major version jump. The package.json and hardhat dependencies should be verified to support Node 22.x. Additionally, the hardhat.config.ts shows EDR now requires "node >= 20" which conflicts with the README stating 22.x.
| - node version: 22.x | |
| - node version: >= 20.x (tested with 22.x) |
| const FALLBACK_HANDLER_ADDRESS = '0xf48f2B2d2a534e402487b3ee7C18c33Aec0Fe5e4'; | ||
|
|
||
| // Safe configuration | ||
| const OWNERS = ['']; |
There was a problem hiding this comment.
The OWNERS array contains an empty string which will cause the deployment to fail. This should be validated before attempting deployment or documented as requiring configuration.
| console.log('='.repeat(60)); | ||
|
|
||
| // Validate configuration | ||
| if (OWNERS.length === 0 || OWNERS.some((o) => o === '0x...')) { |
There was a problem hiding this comment.
The validation check for OWNERS uses a string comparison with '0x...' which won't catch the empty string on line 61. The validation should check for empty strings or consider using ethers.isAddress() to validate all addresses.
| if (OWNERS.length === 0 || OWNERS.some((o) => o === '0x...')) { | |
| if ( | |
| OWNERS.length === 0 || | |
| OWNERS.some((o) => !o || o.trim() === '' || !ethers.isAddress(o)) | |
| ) { |
| value: number; | ||
| data: string; | ||
| operation: number; | ||
| safeTxGas: number; | ||
| baseGas: number; | ||
| gasPrice: number; | ||
| gasToken: string; | ||
| refundReceiver: string; | ||
| nonce: number; |
There was a problem hiding this comment.
SafeTransaction interface uses number type for the value field, but Ethereum values can exceed JavaScript's safe integer range (2^53-1). This should be string or bigint to handle large ETH amounts safely. This issue is also present in executeSafeTransaction.ts.
| value: number; | |
| data: string; | |
| operation: number; | |
| safeTxGas: number; | |
| baseGas: number; | |
| gasPrice: number; | |
| gasToken: string; | |
| refundReceiver: string; | |
| nonce: number; | |
| value: string; | |
| data: string; | |
| operation: number; | |
| safeTxGas: string; | |
| baseGas: string; | |
| gasPrice: string; | |
| gasToken: string; | |
| refundReceiver: string; | |
| nonce: string; |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 370b752b93
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Calculate EIP-712 hash | ||
| const txHash = calculateSafeTxHash(safeAddress, safeTx, chainId); | ||
| logger.info(`Transaction Hash: ${txHash}`); |
There was a problem hiding this comment.
This script computes the Safe EIP-712 hash using the chainId of the current Hardhat network, but it never checks that the transaction being signed was prepared for the same chain (the prepared transaction includes chainId). If a signer runs this on the wrong --network, it will happily append a signature for a different chainId, and the resulting signatures will never validate when executing on the intended chain. Add a check that transaction.chainId === chainId (or refuse when transaction.chainId is set and differs) before hashing/signing to avoid producing unusable signatures.
Useful? React with 👍 / 👎.
| // Try mnemonic from network config or env | ||
| const accounts = networkConfig.accounts as any; | ||
| const mnemonic = accounts?.mnemonic || process.env.MNEMONIC; | ||
| if (mnemonic) { | ||
| return ethers.Wallet.fromPhrase(mnemonic).connect(provider); |
There was a problem hiding this comment.
When accounts is configured with a mnemonic plus path/initialIndex in hardhat.config.ts, this helper ignores those settings and always derives the default m/44'/60'/0'/0/0 account via Wallet.fromPhrase. That means deployments can be signed from a different address than every other Hardhat script on the same network config (and from an address that may not be funded). Consider reading accounts.path and accounts.initialIndex and deriving the correct account to avoid unexpected deployer addresses.
Useful? React with 👍 / 👎.
2 participants
