improve SAFE ownership and threshold change script #1015
Conversation
WalkthroughThis pull request modifies the management of safe owners by replacing the Changes
Possibly related PRs
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Pro Plan! Simply use the command Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
script/deploy/safe/add-safe-owners-and-threshold.ts (1)
55-62: Improve private key handling with validation.The private key handling includes good error messaging but could benefit from additional validation.
const privateKey = String( privateKeyArg || process.env.PRIVATE_KEY_PRODUCTION ) if (!privateKey) throw new Error( 'Private key is missing, either provide it as argument or add PRIVATE_KEY_PRODUCTION to your .env' ) + +// Validate private key format +if (!privateKey.match(/^0x[0-9a-fA-F]{64}$/)) { + throw new Error('Invalid private key format') +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
package.json(1 hunks)script/deploy/safe/add-safe-owners-and-threshold.ts(5 hunks)script/deploy/safe/confirm-safe-tx.ts(2 hunks)script/utils/viemScriptHelpers.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-unit-tests
🔇 Additional comments (7)
script/utils/viemScriptHelpers.ts (2)
3-4: LGTM! Environment variable configuration is properly initialized.The dotenv package is imported and configured correctly to load environment variables.
46-48: LGTM! Environment variable naming follows best practices.The dynamic construction of environment variable names using
ETH_NODE_URI_${networkName.toUpperCase()}follows standard naming conventions and provides good flexibility.Also applies to: 59-59
script/deploy/safe/add-safe-owners-and-threshold.ts (2)
143-169: LGTM! Transaction submission helper is well structured.The
submitAndExecuteTransactionfunction properly handles transaction proposal, signing, and execution with appropriate error handling and logging.
85-85:✅ Verification successful
Verify the safe owners configuration.
The owners list is sourced from global config without validation.
🏁 Script executed:
#!/bin/bash # Description: Verify that safeOwners exists in global config and contains valid addresses # Check if safeOwners exists and is an array jq -e '.safeOwners | type == "array"' config/global.json || echo "Error: safeOwners must be an array in global.json" # Verify each address format jq -r '.safeOwners[]' config/global.json | while read -r address; do if [[ ! $address =~ ^0x[a-fA-F0-9]{40}$ ]]; then echo "Invalid address format: $address" fi doneLength of output: 688
Safe owners configuration validated.
After running the verification script, we confirmed that:
globalConfig.safeOwnersexists inconfig/global.jsonand is an array.- Every address in the list adheres to the expected Ethereum address format.
Since the global configuration already ensures valid safe owner entries, no additional validation is required in the code at line 85.
script/deploy/safe/confirm-safe-tx.ts (2)
337-388: LGTM! Command is properly exported for reuse.The command definition is well structured and maintains all existing functionality while being made reusable.
390-393: LGTM! Entry point check prevents unintended execution.The module.url check ensures the script only runs when directly executed, enabling safe imports from other scripts.
package.json (1)
77-77: LGTM! Script reference updated correctly.The script name and path have been updated to reflect the new functionality that handles both owner addition and threshold changes.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
script/deploy/safe/add-safe-owners-and-threshold.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
script/deploy/safe/add-safe-owners-and-threshold.ts (2)
Learnt from: ezynda3
PR: lifinance/contracts#861
File: script/deploy/safe/add-owners-to-safe.ts:8-13
Timestamp: 2024-11-21T08:22:38.484Z
Learning: In `script/deploy/safe/add-owners-to-safe.ts`, runtime validation for network configuration using Zod is not required; type assertions are acceptable.
Learnt from: ezynda3
PR: lifinance/contracts#861
File: script/deploy/safe/add-owners-to-safe.ts:48-48
Timestamp: 2024-11-21T08:23:50.099Z
Learning: In the script `script/deploy/safe/add-owners-to-safe.ts`, additional defensive checks for network configuration may be unnecessary because the script will fail anyway when the network configuration is missing.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: generate-tag
🔇 Additional comments (2)
script/deploy/safe/add-safe-owners-and-threshold.ts (2)
1-20: LGTM! Well-organized imports and configuration.The imports are properly structured, with appropriate use of dynamic imports for ESM compatibility and clear type definitions.
128-137: Add confirmation prompt before changing threshold.The threshold change to 3 is hardcoded and executed without user confirmation.
if (currentThreshold != 3) { + const proceed = await consola.prompt( + `Are you sure you want to change threshold from ${currentThreshold} to 3?`, + { type: 'confirm' } + ) + if (!proceed) { + console.info('Threshold change skipped') + return + } console.info('Now changing threshold from 1 to 3') const changeThresholdTx = await protocolKit.createChangeThresholdTx(3) await submitAndExecuteTransaction( protocolKit, safeService, changeThresholdTx, senderAddress ) } else console.log('Threshold is already set to 3 - no action required')
0xDEnYO
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
script/deploy/safe/add-safe-owners-and-threshold.ts (1)
66-67: 🛠️ Refactor suggestionAdd validation for RPC URL.
There's no validation to ensure that either the chain's default RPC URL or the provided RPC URL exists before creating the provider.
- const rpcUrl = chain.rpcUrls.default.http[0] || args.rpcUrl + const rpcUrl = chain.rpcUrls.default.http[0] || args.rpcUrl + if (!rpcUrl) { + throw new Error('No RPC URL available for this network. Please provide one with the --rpcUrl argument.') + } const provider = new ethers.JsonRpcProvider(rpcUrl)
🧹 Nitpick comments (3)
script/deploy/safe/add-safe-owners-and-threshold.ts (3)
15-16: Consider placing dotenv configuration before any imports.It's generally best practice to configure environment variables before any imports so that imported modules can access the environment variables if needed.
-import * as dotenv from 'dotenv' -dotenv.config() -import { SafeTransaction } from '@safe-global/safe-core-sdk-types' +import * as dotenv from 'dotenv' +dotenv.config() + +import { SafeTransaction } from '@safe-global/safe-core-sdk-types'
83-89: Improve safe info retrieval pattern.The current pattern for retrieving and using safe info could be clearer if you await the promise first and then access the property.
let nextNonce = await safeService.getNextNonce(safeAddress) - const info = safeService.getSafeInfo(safeAddress) + const safeInfo = await safeService.getSafeInfo(safeAddress) console.info('Safe Address', safeAddress) const senderAddress = await signer.getAddress() console.info('Signer Address', senderAddress) - const currentThreshold = (await info).threshold + const currentThreshold = safeInfo.threshold + if (!currentThreshold) { + throw new Error('Failed to retrieve current threshold from safe') + }
124-126: Make threshold value configurable.The target threshold value is hardcoded to 3, which might not be flexible for different scenarios. Consider making it configurable via arguments or configuration file.
+ const targetThreshold = args.threshold ? parseInt(args.threshold) : 3 - if (currentThreshold != 3) { - console.info('Now changing threshold from 1 to 3') + if (currentThreshold != targetThreshold) { + console.info(`Now changing threshold from ${currentThreshold} to ${targetThreshold}`) const changeThresholdTx = await protocolKit.createChangeThresholdTx(3)This would require adding a new argument in the command definition:
threshold: { type: 'string', description: 'Target threshold to set (defaults to 3)', },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
package.json(1 hunks)script/deploy/safe/add-safe-owners-and-threshold.ts(1 hunks)script/utils/viemScriptHelpers.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- script/utils/viemScriptHelpers.ts
- package.json
🧰 Additional context used
🧠 Learnings (1)
script/deploy/safe/add-safe-owners-and-threshold.ts (5)
Learnt from: 0xDEnYO
PR: lifinance/contracts#1015
File: script/deploy/safe/add-safe-owners-and-threshold.ts:140-145
Timestamp: 2025-02-20T07:49:30.907Z
Learning: In Safe SDK integration, using `any` type for Safe and SafeApiKit parameters is acceptable as these are both values and types, which can cause typing conflicts when used directly as parameter types.
Learnt from: 0xDEnYO
PR: lifinance/contracts#1015
File: script/deploy/safe/add-safe-owners-and-threshold.ts:117-123
Timestamp: 2025-02-20T04:50:33.488Z
Learning: The `submitAndExecuteTransaction` function in Safe management scripts includes comprehensive error handling that covers transaction proposal, execution, and receipt verification. It uses try-catch blocks, verifies transaction receipt status, logs errors, and properly propagates them up the call stack, making additional error handling around its calls unnecessary.
Learnt from: 0xDEnYO
PR: lifinance/contracts#1015
File: script/deploy/safe/add-safe-owners-and-threshold.ts:117-123
Timestamp: 2025-02-20T04:50:33.488Z
Learning: In the Safe owners management scripts, error handling for transaction submission is implemented within the `submitAndExecuteTransaction` function, making additional try-catch blocks around its calls unnecessary.
Learnt from: ezynda3
PR: lifinance/contracts#861
File: script/deploy/safe/add-owners-to-safe.ts:8-13
Timestamp: 2024-11-21T08:22:38.484Z
Learning: In `script/deploy/safe/add-owners-to-safe.ts`, runtime validation for network configuration using Zod is not required; type assertions are acceptable.
Learnt from: ezynda3
PR: lifinance/contracts#861
File: script/deploy/safe/add-owners-to-safe.ts:48-48
Timestamp: 2024-11-21T08:23:50.099Z
Learning: In the script `script/deploy/safe/add-owners-to-safe.ts`, additional defensive checks for network configuration may be unnecessary because the script will fail anyway when the network configuration is missing.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: run-unit-tests
- GitHub Check: generate-tag
🔇 Additional comments (4)
script/deploy/safe/add-safe-owners-and-threshold.ts (4)
124-126: Add user confirmation before changing threshold.The threshold change to 3 is hardcoded and executed without user confirmation. Consider adding a confirmation prompt.
140-145: Using 'any' type for Safe SDK parameters is acceptable.According to previous feedback, using
anytype for Safe and SafeApiKit parameters is acceptable as these are both values and types, which can cause typing conflicts when used directly as parameter types.
113-119: Error handling is sufficient as implemented.The error handling in the
submitAndExecuteTransactionfunction is comprehensive and covers transaction proposal, execution, and receipt verification. No additional error handling is needed around the function call.
1-176: Good implementation of safe owners management with threshold control.The script effectively automates the process of adding new owners to a Safe contract and changing the threshold in a single workflow, which streamlines what would otherwise be multiple manual steps. The implementation includes proper error handling, owner existence checks, and transaction management.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
script/deploy/safe/add-safe-owners-and-threshold.ts (3)
2-17: Import order should be fixed and dotenv.config() called immediately after imports.The current order of imports has dotenv.config() called after importing SafeTransaction, which is not ideal. Best practice is to load environment variables as early as possible in the execution flow to ensure they're available when needed by other imports.
import { defineCommand, runMain } from 'citty' import { type SafeApiKitConfig } from '@safe-global/api-kit' import { getAddress } from 'viem' import { EthersAdapter } from '@safe-global/protocol-kit' const { default: SafeApiKit } = await import('@safe-global/api-kit') const { default: Safe } = await import('@safe-global/protocol-kit') import { ethers } from 'ethers6' import { getSafeUtilityContracts } from './config' import { NetworksObject, getViemChainForNetworkName, } from '../../utils/viemScriptHelpers' import data from '../../../config/networks.json' import globalConfig from '../../../config/global.json' import * as dotenv from 'dotenv' import { SafeTransaction } from '@safe-global/safe-core-sdk-types' + dotenv.config()
81-85: Improved error handling for threshold retrieval.The current code first unwraps the promise and then uses optional chaining to access the threshold property, which is less readable. A more straightforward approach would be to await the promise first and then check if the threshold exists.
-const currentThreshold = (await safeService.getSafeInfo(safeAddress)) - ?.threshold -if (!currentThreshold) +const safeInfo = await safeService.getSafeInfo(safeAddress) +if (!safeInfo || !safeInfo.threshold) throw new Error('Could not get current signature threshold') +const currentThreshold = safeInfo.threshold
124-133: Update hardcoded messaging to match the actual threshold change.The log message hardcodes "from 1 to 3" but the actual change is from the current threshold to 3. This could be confusing if the current threshold is not 1.
if (currentThreshold != 3) { - console.info('Now changing threshold from 1 to 3') + console.info(`Now changing threshold from ${currentThreshold} to 3`) const changeThresholdTx = await protocolKit.createChangeThresholdTx(3) await submitAndExecuteTransaction( protocolKit, safeService, changeThresholdTx, senderAddress ) } else console.log('Threshold is already set to 3 - no action required')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
script/deploy/safe/add-safe-owners-and-threshold.ts(1 hunks)script/utils/viemScriptHelpers.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- script/utils/viemScriptHelpers.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: 0xDEnYO
PR: lifinance/contracts#1015
File: script/deploy/safe/add-safe-owners-and-threshold.ts:117-123
Timestamp: 2025-02-20T04:50:33.488Z
Learning: In the Safe owners management scripts, error handling for transaction submission is implemented within the `submitAndExecuteTransaction` function, making additional try-catch blocks around its calls unnecessary.
script/deploy/safe/add-safe-owners-and-threshold.ts (5)
Learnt from: 0xDEnYO
PR: lifinance/contracts#1015
File: script/deploy/safe/add-safe-owners-and-threshold.ts:140-145
Timestamp: 2025-02-20T07:49:30.907Z
Learning: In Safe SDK integration, using `any` type for Safe and SafeApiKit parameters is acceptable as these are both values and types, which can cause typing conflicts when used directly as parameter types.
Learnt from: 0xDEnYO
PR: lifinance/contracts#1015
File: script/deploy/safe/add-safe-owners-and-threshold.ts:117-123
Timestamp: 2025-02-20T04:50:33.488Z
Learning: The `submitAndExecuteTransaction` function in Safe management scripts includes comprehensive error handling that covers transaction proposal, execution, and receipt verification. It uses try-catch blocks, verifies transaction receipt status, logs errors, and properly propagates them up the call stack, making additional error handling around its calls unnecessary.
Learnt from: 0xDEnYO
PR: lifinance/contracts#1015
File: script/deploy/safe/add-safe-owners-and-threshold.ts:117-123
Timestamp: 2025-02-20T04:50:33.488Z
Learning: In the Safe owners management scripts, error handling for transaction submission is implemented within the `submitAndExecuteTransaction` function, making additional try-catch blocks around its calls unnecessary.
Learnt from: ezynda3
PR: lifinance/contracts#861
File: script/deploy/safe/add-owners-to-safe.ts:48-48
Timestamp: 2024-11-21T08:23:50.099Z
Learning: In the script `script/deploy/safe/add-owners-to-safe.ts`, additional defensive checks for network configuration may be unnecessary because the script will fail anyway when the network configuration is missing.
Learnt from: ezynda3
PR: lifinance/contracts#861
File: script/deploy/safe/add-owners-to-safe.ts:8-13
Timestamp: 2024-11-21T08:22:38.484Z
Learning: In `script/deploy/safe/add-owners-to-safe.ts`, runtime validation for network configuration using Zod is not required; type assertions are acceptable.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: CodeQL
- GitHub Check: run-unit-tests
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: generate-tag
🔇 Additional comments (5)
script/deploy/safe/add-safe-owners-and-threshold.ts (5)
140-145: Use of 'any' types for Safe SDK parameters is acceptable.While using explicit types instead of 'any' is generally preferred, I understand from the retrieved learnings that using 'any' type for Safe and SafeApiKit parameters is acceptable in this case as these are both values and types, which can cause typing conflicts.
161-171: Error handling in transaction execution is appropriately implemented.The implementation includes proper error handling with try-catch blocks, verification of transaction receipt status, and error logging with propagation.
24-26: Informative and clear command description.The command description clearly explains the purpose of the script, which is to add SAFE owners from the global.json file and set the threshold to 3.
91-120: Well-structured owner management loop with proper validation.The code efficiently iterates through all owner addresses, validates if they already exist before attempting to add them, and provides clear console output about the process. The transaction nonce management is also handled correctly.
48-55: Good fallback handling for private key.The implementation properly prioritizes the private key provided as an argument, falls back to an environment variable, and throws a clear error message if neither is available.
Which Jira task belongs to this PR?
Why did I implement it this way?
This PR helps further automating the tasks associated to adding a new chain.
With the updated script we can now add SAFE owners and change threshold from 1 to 3 all in one step (which used to consist of several manual steps)
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)