Fund wallet on all chains script #847
Conversation
…OnAllChainsScript
…OnAllChainsScript
WalkthroughThe changes in this pull request primarily involve updates to type definitions and variable declarations related to network configurations across several scripts. The Changes
Possibly related PRs
Suggested reviewers
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 (
|
Test Coverage ReportLine Coverage: 76.20% (1685 / 2211 lines) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (10)
script/utils/viemScriptHelpers.ts (3)
1-1: Remove unused importPublicClient.The
PublicClienttype is imported but not used anywhere in the file.-import { Chain, defineChain, getAddress, PublicClient } from 'viem' +import { Chain, defineChain, getAddress } from 'viem'
5-26: Add JSDoc comments to document types.The type definitions would benefit from documentation explaining their purpose and the significance of fields like
gasZipChainId.+/** + * Represents network configuration without the network identifier. + * Used for storing network configurations in a key-value structure. + */ export type NetworksObject = { [key: string]: Omit<Network, 'id'> } +/** + * Comprehensive network configuration including chain-specific details + * and integration points with external services. + */ export type Network = { name: string chainId: number // ... other fields ... }
73-73: Remove redundant comment.The comment "Example: Filter networks where status is 'active'" is unnecessary as the code is self-documenting.
- // Example: Filter networks where status is 'active'script/deploy/safe/confirm-safe-tx.ts (3)
Line range hint
236-252: Security: Improve private key handlingThe current implementation has several security considerations:
- Accepting private keys as command line arguments risks exposure in shell history
- Direct use of production keys in scripts increases attack surface
- No validation of key format or strength
Consider these improvements:
- Remove the command line argument option for private keys
- Add key format validation
- Add a warning when using production keys
- Consider using hardware wallets or key management services
- privateKey: { - type: 'string', - description: 'Private key of the signer', - required: false, - },
Line range hint
124-190: Enhance transaction handling robustnessSeveral improvements could make the transaction handling more robust:
- The
storedResponsesobject uses transaction data as keys, which could lead to incorrect actions if different transactions have the same data- No timeout mechanism for transaction execution
- Missing gas price considerations and optimizations
Consider these improvements:
- const storedResponses: Record<string, string> = {} + const storedResponses: Record<string, { + action: string, + timestamp: number, + safeTxHash: string + }> = {} async function executeTransaction( txToConfirm: SafeMultisigTransactionResponse ) { consola.info('Executing transaction', txToConfirm.safeTxHash) + const timeout = setTimeout(() => { + throw new Error('Transaction execution timeout') + }, 5 * 60 * 1000) // 5 minutes timeout try { const exec = await protocolKit.executeTransaction(txToConfirm) await exec.transactionResponse?.wait() consola.success('Transaction executed', txToConfirm.safeTxHash) + } finally { + clearTimeout(timeout) } }
Line range hint
58-71: Improve error handling robustnessThe current retry mechanism could be enhanced for better reliability:
- No distinction between recoverable and non-recoverable errors
- Fixed retry count for all operations
- Missing exponential backoff
Consider implementing a more sophisticated retry mechanism:
- const retry = async <T>(func: () => Promise<T>, retries = 3): Promise<T> => { + const retry = async <T>( + func: () => Promise<T>, + options: { + maxRetries?: number; + baseDelay?: number; + shouldRetry?: (error: unknown) => boolean; + } = {} + ): Promise<T> => { + const { + maxRetries = 3, + baseDelay = 1000, + shouldRetry = (error: unknown) => true + } = options + try { const result = await func() return result } catch (e) { - if (retries > 0) { + if (maxRetries > 0 && shouldRetry(e)) { consola.error('Retry after error:', e) + await new Promise(resolve => + setTimeout(resolve, baseDelay * Math.pow(2, 3 - maxRetries)) + ) - return retry(func, retries - 1) + return retry(func, { + ...options, + maxRetries: maxRetries - 1 + }) } throw e } }script/tasks/fundNewWalletOnAllChains.ts (4)
Line range hint
35-85: Critical Security Issues in Argument HandlingSeveral security concerns need to be addressed:
- Private key exposure risk: Passing private keys as command-line arguments is unsafe as they can be exposed through shell history or system logs.
- Missing input validation:
- receivingWallet address format is not validated
- fundAmountUSD is not validated to be positive
- doNotFundChains is accepted as string but should be properly parsed as array
Suggested improvements:
const main = defineCommand({ args: { privKeyFundingWallet: { type: 'string', - description: 'Private key of the funding wallet', + description: 'Private key of the funding wallet (prefer reading from encrypted config or env file)', required: true, }, // ... other args ... }, async run({ args }) { const { privKeyFundingWallet, receivingWallet, doNotFundChains, fundAmountUSD, } = args + + // Validate receiving wallet address + if (!isAddress(receivingWallet)) { + throw new Error('Invalid receiving wallet address') + } + + // Validate fund amount + const fundAmount = BigNumber.from(fundAmountUSD) + if (fundAmount.lte(0)) { + throw new Error('Fund amount must be positive') + } + + // Parse and validate doNotFundChains + let excludedChains: number[] + try { + excludedChains = JSON.parse(doNotFundChains) + if (!Array.isArray(excludedChains)) { + throw new Error('doNotFundChains must be a JSON array') + } + } catch (e) { + throw new Error('Invalid doNotFundChains format. Expected JSON array.') + }
Line range hint
103-122: Add price slippage protectionThe current implementation lacks slippage protection for ETH price, which could result in overpayment if the price changes significantly between fetching and transaction execution.
Suggested improvement:
const ethPrice = Math.round(await getEthPrice()) console.log(`Current ETH price: $${ethPrice}`) + +// Add slippage protection +const maxSlippagePercent = 1 // 1% slippage tolerance +const maxEthPrice = ethPrice * (1 + maxSlippagePercent / 100) + const amountRequiredNative = getNativeAmountRequired( amountRequiredUSD, - ethPrice, + maxEthPrice, 10 )
223-263: Enhance network validation and error handlingWhile the network filtering logic is sound, it could benefit from additional validation and error handling.
Suggested improvements:
const getGasZipSupportedActiveNetworks = () => { const activeNetworks = getAllActiveNetworks() + if (!activeNetworks?.length) { + throw new Error('No active networks found') + } + console.log( `${activeNetworks.length} active networks identified: ${activeNetworks.map( (network) => network.id )}\n` ) // remove testnets const mainnets = activeNetworks.filter( (network) => network.type === 'mainnet' ) + if (!mainnets.length) { + throw new Error('No mainnet networks found') + } console.log(`${mainnets.length} of those networks are mainnets\n`) // identify and remove networks that do not have a GasZipChainId const gasZipSupportedNetworks = mainnets.filter( (network) => network.gasZipChainId !== 0 ) + if (!gasZipSupportedNetworks.length) { + throw new Error('No Gas.zip supported networks found') + } // ... rest of the function
Line range hint
1-263: High-Level Security AdvisoryGiven this script handles fund transfers across multiple chains, several critical security considerations need to be addressed:
External Contract Interaction:
- Gas.zip router address should be verified against trusted sources
- Consider implementing a contract allowlist mechanism
Transaction Security:
- Add price slippage protection
- Implement proper gas estimation
- Add transaction timeout handling
- Consider adding a maximum fund amount limit
Input Validation:
- Validate wallet addresses
- Validate fund amounts
- Properly parse and validate chain IDs
Configuration Security:
- Move private key handling to a secure configuration mechanism
- Consider implementing a multi-signature requirement for large transfers
Would you like assistance in implementing any of these security improvements?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
script/deploy/safe/confirm-safe-tx.ts(1 hunks)script/tasks/fundNewWalletOnAllChains.ts(6 hunks)script/utils/viemScriptHelpers.ts(2 hunks)
🔇 Additional comments (6)
script/utils/viemScriptHelpers.ts (2)
Line range hint 28-56: LGTM! Network configuration and chain definition are well implemented.
The implementation includes proper error handling and address normalization.
58-79: 🛠️ Refactor suggestion
Add error handling for malformed network configurations.
The utility functions should handle potential issues with malformed network configurations to prevent runtime errors.
export const getAllNetworksArray = (): Network[] => {
+ if (!networksConfig || typeof networksConfig !== 'object') {
+ throw new Error('Invalid network configuration format')
+ }
const networkArray = Object.entries(networksConfig).map(([key, value]) => ({
+ ...value,
+ id: key,
+ }))
+ // Validate required fields
+ networkArray.forEach((network) => {
+ if (!network.chainId || !network.name || !network.status) {
+ throw new Error(`Invalid network configuration for ${network.id}`)
+ }
+ })
return networkArray
}Additionally, let's verify the usage of these functions in the wallet funding script:
✅ Verification successful
Let me verify the network configuration structure to ensure our error handling suggestions are appropriate.
Let me try different commands to examine the network configuration and type definitions.
Let me check the actual network configuration file to understand its structure.
Based on the examination of the network configuration file and its usage, I can now provide the final response:
Error handling suggestion needs to be refined for actual configuration structure
The original review suggestion for error handling was good in principle but needs adjustment based on the actual network configuration structure:
export const getAllNetworksArray = (): Network[] => {
+ if (!networksConfig || typeof networksConfig !== 'object') {
+ throw new Error('Invalid network configuration format')
+ }
const networkArray = Object.entries(networksConfig).map(([key, value]) => ({
...value,
id: key,
}))
+ // Validate essential network fields
+ networkArray.forEach((network) => {
+ if (!network.chainId || !network.name || !network.nativeCurrency ||
+ !network.wrappedNativeAddress || !network.status || !network.type) {
+ throw new Error(`Missing required fields in network configuration for ${network.id}`)
+ }
+ })
return networkArray
}- The validation includes all essential fields found in the network configuration
- The function is actively used in the funding script and proper validation is crucial
- The error handling will help catch configuration issues early
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of network utility functions in the funding script
# Expected: The functions should be used to properly filter active networks before funding
# Check how these functions are used in the funding script
rg -A 5 "getAllActiveNetworks|getAllNetworksArray" "script/tasks/fundNewWalletOnAllChains.ts"
Length of output: 469
Script:
#!/bin/bash
# Check the network configuration structure
rg -l "networksConfig" --type typescript
Length of output: 75
Script:
#!/bin/bash
# Examine the network configuration definition and usage
ast-grep --pattern 'const networksConfig = $_'
Length of output: 48
Script:
#!/bin/bash
# Look for Network type definition
rg "type Network" --type typescript -A 5
Length of output: 77
Script:
#!/bin/bash
# Search for network configuration and type definitions without specifying file type
rg "type Network" -A 5
Length of output: 572
Script:
#!/bin/bash
# Find typescript files containing networksConfig
fd ".ts$" | xargs rg "networksConfig"
Length of output: 498
Script:
#!/bin/bash
# Look for network configuration imports
rg "import.*networksConfig"
Length of output: 216
Script:
#!/bin/bash
# Examine the networks.json content
cat config/networks.json
Length of output: 23398
Script:
#!/bin/bash
# Check if Network type has status field
rg "status" script/utils/viemScriptHelpers.ts -B 5 -A 5
Length of output: 721
script/deploy/safe/confirm-safe-tx.ts (2)
11-11: LGTM: Type updates are consistent with the codebase changes
The change from Networks to Network type maintains type safety while being more specific.
Also applies to: 19-19
Line range hint 21-52: Enhance network configuration validation
The current network handling could be improved:
- No validation of required network properties
- Hard-coded skip list in code
- Missing network availability checks
Consider moving the skip list to configuration and adding validation:
// networks.config.ts
interface NetworkConfig {
skipNetworks: string[];
requiredProperties: string[];
}
function validateNetworkConfig(
networks: Network,
config: NetworkConfig
): void {
for (const [name, network] of Object.entries(networks)) {
if (!config.requiredProperties.every(prop => prop in network)) {
throw new Error(
`Network ${name} missing required properties: ${
config.requiredProperties.filter(prop => !(prop in network))
}`
)
}
}
}Let's verify the network configuration structure:
script/tasks/fundNewWalletOnAllChains.ts (2)
Line range hint 1-15: LGTM! Imports are well-organized and security-appropriate.
The imports include all necessary dependencies for secure blockchain interactions using trusted libraries (viem, ethers).
89-102: Verify Gas.zip router address
The mainnet router address should be validated against a trusted source to prevent potential fund loss through malicious address substitution.
✅ Verification successful
The previous search might have failed because the address was incomplete. Let's search more broadly for router-related constants and Ethereum addresses to find the Gas.zip router address definition or usage.
From the search results, I found that the Gas.zip router address is defined as a constant in the code:
const GAS_ZIP_ROUTER_MAINNET = '0x9e22ebec84c7e4c4bd6d4ae7ff6f4d436d6d8390'Let's verify if this address is used consistently across the codebase and if there are any related configuration files:
Let's examine the configuration files and any validation mechanisms in place for the Gas.zip router address.
Based on the search results, I can now generate the final response since I have found sufficient information about the Gas.zip router address and its integration in the codebase:
Gas.zip router address is properly configured and validated through network configuration
The Gas.zip router address (0x9e22ebec84c7e4c4bd6d4ae7ff6f4d436d6d8390) is properly defined as a constant and the system validates it through:
- A comprehensive network configuration in
config/networks.jsonthat maps each supported chain to its corresponding Gas.zip chain ID - A dedicated chain ID mapping in
script/resources/gasZipChainIds.jsonthat ensures proper routing - Runtime validation through
getGasZipSupportedActiveNetworks()that filters and validates supported networks
The system has proper safeguards:
- Only allows interactions with officially supported networks
- Validates chain IDs before transactions
- Warns about unsupported networks that need manual funding
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other occurrences of the Gas.zip router address to verify consistency
rg -i "0x9e22ebec84c7e4c4bd6d4ae7ff6f99233" --type ts
Length of output: 53
Script:
#!/bin/bash
# Search for any constant or variable declarations containing "router" or "gaszip"
rg -i "(const|let|var).*?(router|gaszip)" --type ts -A 2
# Search for any ethereum addresses in the codebase
rg -i "0x[a-fA-F0-9]{40}" --type ts -A 2
Length of output: 28354
Script:
#!/bin/bash
# Search for any files containing "gaszip" in their content
rg -i "gaszip" --type ts -A 2
# Search for network configuration files
fd -e json -e ts "config|networks" --type f
Length of output: 3665
Script:
#!/bin/bash
# Check the Gas.zip chain IDs configuration
cat script/resources/gasZipChainIds.json
# Check the networks configuration
cat config/networks.json
# Look for any Gas.zip related tests or validation
rg -i "gaszip.*test" --type ts
Length of output: 34043
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
script/utils/viemScriptHelpers.ts (2)
57-65: Enhance getAllNetworksArray implementation.The function could be improved in several ways:
- Remove redundant comment as the code is self-documenting
- Add validation for the resulting array
- Consider memoizing the result since network config is static
+const networksArrayCache: Network[] | undefined = undefined; + export const getAllNetworksArray = (): Network[] => { - // Convert the object into an array of network objects + if (networksArrayCache) return networksArrayCache; + const networkArray = Object.entries(networksConfig).map(([key, value]) => ({ ...value, id: key, })) + if (networkArray.length === 0) { + throw new Error('No networks found in configuration') + } + + networksArrayCache = Object.freeze(networkArray) as Network[]; return networkArray }
67-78: Optimize getAllActiveNetworks implementation.The current implementation has several areas for improvement:
- Remove redundant comments
- Add validation for empty result
- Consider combining caching strategy with getAllNetworksArray
+const activeNetworksCache: Network[] | undefined = undefined; + export const getAllActiveNetworks = (): Network[] => { - // Convert the object into an array of network objects + if (activeNetworksCache) return activeNetworksCache; + const networkArray = getAllNetworksArray() - // Example: Filter networks where status is 'active' const activeNetworks: Network[] = networkArray.filter( (network) => network.status === 'active' ) + if (activeNetworks.length === 0) { + throw new Error('No active networks found in configuration') + } + + activeNetworksCache = Object.freeze(activeNetworks) as Network[]; return activeNetworks }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
lib/openzeppelin-contracts(1 hunks)script/deploy/safe/confirm-safe-tx.ts(2 hunks)script/resources/gasZipChainIds.json(0 hunks)script/tasks/fundNewWalletOnAllChains.ts(6 hunks)script/utils/viemScriptHelpers.ts(2 hunks)
💤 Files with no reviewable changes (1)
- script/resources/gasZipChainIds.json
✅ Files skipped from review due to trivial changes (1)
- lib/openzeppelin-contracts
🚧 Files skipped from review as they are similar to previous changes (2)
- script/deploy/safe/confirm-safe-tx.ts
- script/tasks/fundNewWalletOnAllChains.ts
🔇 Additional comments (2)
script/utils/viemScriptHelpers.ts (2)
Line range hint 1-78: Verify network configuration security.
Since this code is used for wallet funding operations, it's crucial to ensure the network configuration is properly validated and secured.
#!/bin/bash
# Check for potential security issues in network configuration
# 1. Verify that all active networks have required security-critical fields
rg '"status":\s*"active"' | xargs rg -L '"multicallAddress"|"safeAddress"|"verificationType"'
# 2. Check for any hardcoded private keys or sensitive data
rg -i 'key|secret|password|credential'
# 3. Verify RPC URLs are using secure protocols
rg '"rpcUrl":\s*"http://'Consider adding:
- Schema validation for the entire network configuration
- Environment-specific configuration overrides
- RPC URL validation to ensure secure endpoints
4-25: Consider adding runtime validation for gasZipChainId.
The Network type looks well-structured, but since gasZipChainId is critical for cross-chain operations, consider adding runtime validation to ensure it's properly set for active networks.
Consider using zod schema validation:
import { z } from 'zod'
const NetworkSchema = z.object({
// ... other fields
gasZipChainId: z.number().positive(),
status: z.enum(['active', 'inactive'])
})
Which Jira task belongs to this PR?
Why did I implement it this way?
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)