Add Safe version using github actions and script#1332
Merged
Conversation
…hain ID - Add update-registry.ts script to update all contract JSON files - Add GitHub Actions workflow for manual registry updates - Script supports adding chain IDs with deployment types (canonical, eip155, zksync) - Automatically sorts network addresses by chain ID - Creates PR with changes when run via GitHub Actions
- Parse deployment type from version string (e.g., v1.3.0-canonical) - Remove separate deployment_type parameter - Update GitHub Actions workflow to use choice selector for version - Version options: v1.3.0-canonical, v1.3.0-eip155, v1.4.1, v1.5.0
- Import AddressType from types - Update sortNetworkAddresses function signature to use AddressType - Improve type safety by properly casting deploymentType to AddressType - Remove unnecessary 'as any' casts
- Add explicit check to preserve existing deployment type when converting single to array - Add better logging to show when values are preserved - Add safety check to prevent duplicates when adding to arrays - Ensure both canonical and eip155 are kept for v1.3.0 when adding second type
- Add pre-validation phase that checks all contracts before making changes - Early exit if chain ID with deployment type is already fully supported - Clear reporting of which contracts already support the deployment - Prevent unnecessary file writes when nothing needs updating - Better user feedback with summary of what will be updated
- Added chain ID 988 to all contracts in version v1.3.0-canonical
- Introduced a new function to sort deployment types, ensuring "canonical" precedes "eip155" as per company convention for version 1.3.0. - Updated logic in the main function to utilize the new sorting function when adding deployment types to the network addresses. - Enhanced logging to reflect the sorted array of deployment types after updates.
Add chain 988 to v1.3.0-canonical registry
- Updated the sorting function to preserve an existing value as the first element in the array when present. - Adjusted the logic to ensure "eip155" is prioritized when no existing value is provided. - Improved logging to reflect the changes in sorting behavior and array updates.
- Updated chain ID 4326 from 'canonical' to ['eip155', 'canonical'] in all v1.3.0 contract files - Ensures eip155 deployment type is available for chain ID 4326 - Order: eip155 first, then canonical
nlordell
reviewed
Nov 21, 2025
nlordell
reviewed
Nov 21, 2025
| --verbose | ||
|
|
||
| - name: Format JSON files | ||
| run: npm run lint:fix:json |
Contributor
There was a problem hiding this comment.
nit: Can the update-registry NPM script always call lint:fix:json automatically? I feel like it is always needed so it makes sense to have it there.
nlordell
reviewed
Nov 21, 2025
| "lint:fix:json": "prettier -w src/assets/*/*.json", | ||
| "review:verify-deployment": "ts-node scripts/review/verifyDeployment.ts", | ||
| "review:diff": "ts-node scripts/review/diff.ts", | ||
| "update-registry": "ts-node scripts/update-registry.ts", |
Contributor
There was a problem hiding this comment.
Suggested change
| "update-registry": "ts-node scripts/update-registry.ts", | |
| "update-registry": "ts-node scripts/update-registry.ts && npm run lint:fix:json", |
nlordell
reviewed
Nov 21, 2025
|
|
||
| if (alreadySupported.length === jsonFiles.length - missingDeploymentType.length) { | ||
| // All contracts that support this deployment type already have the chain ID | ||
| console.log(`\n✅ Chain ID ${options.chainId} with deployment type "${deploymentType}" is already supported for all contracts:`); |
Contributor
There was a problem hiding this comment.
Suggested change
| console.log(`\n✅ Chain ID ${options.chainId} with deployment type "${deploymentType}" is already supported for all contracts:`); | |
| console.log(`✅ Chain ID ${options.chainId} with deployment type "${deploymentType}" is already supported for all contracts:`); |
nlordell
reviewed
Nov 21, 2025
| } | ||
|
|
||
| if (alreadySupported.length > 0) { | ||
| console.log(`\nℹ️ ${alreadySupported.length} contract(s) already support chain ID ${options.chainId} with "${deploymentType}":`); |
Contributor
There was a problem hiding this comment.
Suggested change
| console.log(`\nℹ️ ${alreadySupported.length} contract(s) already support chain ID ${options.chainId} with "${deploymentType}":`); | |
| console.log(`ℹ️ ${alreadySupported.length} contract(s) already support chain ID ${options.chainId} with "${deploymentType}":`); |
Contributor
There was a problem hiding this comment.
So, the suggestion was originally to match the warning log message, which doesn't have a leading newline.
I realised as I reviewed that all the messages have leading newlines, so maybe the easier way to make the logs consistent would be to add one to the warning as well.
Contributor
There was a problem hiding this comment.
Feel free to ignore this suggestion if the newline is important for the output to look nice
nlordell
reviewed
Nov 21, 2025
nlordell
reviewed
Nov 21, 2025
nlordell
reviewed
Nov 21, 2025
Comment on lines
+99
to
+100
| const aNum = parseInt(a, 10); | ||
| const bNum = parseInt(b, 10); |
Contributor
There was a problem hiding this comment.
I think technically chain IDs can be larger than the largest safe integer in JS, I would:
Suggested change
| const aNum = parseInt(a, 10); | |
| const bNum = parseInt(b, 10); | |
| const aNum = BigInt(a); | |
| const bNum = BigInt(b); |
nlordell
reviewed
Nov 21, 2025
nlordell
reviewed
Nov 21, 2025
| return; | ||
| } | ||
|
|
||
| if (alreadySupported.length > 0) { |
Contributor
There was a problem hiding this comment.
This condition is important for adding missing migrations contracts to 1.4.1 deployments 🎉.
nlordell
reviewed
Nov 21, 2025
Comment on lines
+199
to
+202
| if (contractsToProcess.length === 0) { | ||
| console.log(`\n✅ No contracts need updating.`); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Isn't this condition impossible: it would have already returned when checking:
if (alreadySupported.length === jsonFiles.length - missingDeploymentType.length) {
nlordell
reviewed
Nov 21, 2025
| ); | ||
| } | ||
|
|
||
| if (alreadySupported.length === jsonFiles.length - missingDeploymentType.length) { |
Contributor
There was a problem hiding this comment.
Suggested change
| if (alreadySupported.length === jsonFiles.length - missingDeploymentType.length) { | |
| if (contractsToProcess.length === 0) { |
nlordell
reviewed
Nov 21, 2025
Comment on lines
+234
to
+239
| } else { | ||
| // This shouldn't happen due to pre-check, but handle gracefully | ||
| debug(` Chain ID ${options.chainId} already has deployment type "${deploymentType}" - skipping`); | ||
| skippedCount++; | ||
| continue; | ||
| } |
Contributor
There was a problem hiding this comment.
AFAIU, contractsToProcess are already all the files that need the deployment added. I don't think this additional condition is necessary. I also found it confusing that we check if the value is part of the array but not if it is the existing value.
nlordell
reviewed
Nov 21, 2025
Comment on lines
+208
to
+209
| let updatedCount = 0; | ||
| let skippedCount = alreadySupported.length + missingDeploymentType.length; |
Contributor
There was a problem hiding this comment.
IMO, these don't need to be counted in the loop, they should always be:
Suggested change
| let updatedCount = 0; | |
| let skippedCount = alreadySupported.length + missingDeploymentType.length; | |
| const updatedCount = contractsToProcess.length; | |
| const skippedCount = alreadySupported.length + missingDeploymentType.length; |
nlordell
suggested changes
Nov 21, 2025
Contributor
nlordell
left a comment
nlordell
left a comment
There was a problem hiding this comment.
This is awesome!
I left a few code comments and some change requests, but this is a very welcome improvement to how we handle deployments! It would be great if we can open this up to make adding deployments for external contributors also easier (but outside the scope of this PR).
- Modified the `update-registry` command in `package.json` to include JSON linting after execution. - Removed the JSON formatting step from the GitHub Actions workflow to streamline the process. - Refactored the `update-registry.ts` script for improved readability and efficiency, including adjustments to sorting logic and logging for better clarity.
- Updated the sorting function to use BigInt for chain ID comparisons, improving accuracy for large numbers. - Enhanced error handling by throwing an error when unsupported deployment types are detected, ensuring all contracts must support the specified type before proceeding.
…nt policy - Updated comments in the sorting function to clarify the official policy for deployment types. - Ensured that existing deployments are prioritized to avoid breaking changes and that eip155 is preferred when applicable.
nlordell
reviewed
Nov 21, 2025
| */ | ||
| function sortDeploymentTypes(types: AddressType[], existingValue?: AddressType): AddressType[] { | ||
| // Policy #1: If there's an existing deployment, preserve it as first | ||
| if (existingValue !== undefined && types.includes(existingValue)) { |
Contributor
There was a problem hiding this comment.
This function is only ever called with this path - I don't think you need it at all.
nlordell
reviewed
Nov 21, 2025
Comment on lines
+225
to
+246
| if (isArray) { | ||
| // Add to existing array (pre-check ensures deploymentType is not already in array) | ||
| // Policy: Keep existing first value to avoid breaking changes | ||
| const firstValue = existingValue[0]; | ||
| const updatedArray: AddressType[] = sortDeploymentTypes([...existingValue, deploymentType], firstValue); | ||
| json.networkAddresses[options.chainId] = updatedArray; | ||
| debug( | ||
| ` Added "${deploymentType}" to existing array [${existingValue.join(', ')}] for chain ID ${options.chainId}`, | ||
| ); | ||
| debug(` Sorted array (preserving first value): [${updatedArray.join(', ')}]`); | ||
| } else { | ||
| // Convert single value to array | ||
| // Policy: Keep existing deployment type first to avoid breaking changes | ||
| const existingType = existingValue as AddressType; | ||
| const sortedArray = sortDeploymentTypes([existingType, deploymentType], existingType); | ||
| json.networkAddresses[options.chainId] = sortedArray; | ||
| console.log( | ||
| ` Converted single value "${existingType}" to array [${sortedArray.join(', ')}] for chain ID ${options.chainId}`, | ||
| ); | ||
| debug(` Preserved existing deployment type "${existingType}" as first, added "${deploymentType}"`); | ||
| } | ||
| } else { |
Contributor
There was a problem hiding this comment.
Since the sortDeploymentTypes is only ever called with an existingType, which causes sortDeploymentTypes to just append - I don't think you need this complicated logic:
Suggested change
| if (isArray) { | |
| // Add to existing array (pre-check ensures deploymentType is not already in array) | |
| // Policy: Keep existing first value to avoid breaking changes | |
| const firstValue = existingValue[0]; | |
| const updatedArray: AddressType[] = sortDeploymentTypes([...existingValue, deploymentType], firstValue); | |
| json.networkAddresses[options.chainId] = updatedArray; | |
| debug( | |
| ` Added "${deploymentType}" to existing array [${existingValue.join(', ')}] for chain ID ${options.chainId}`, | |
| ); | |
| debug(` Sorted array (preserving first value): [${updatedArray.join(', ')}]`); | |
| } else { | |
| // Convert single value to array | |
| // Policy: Keep existing deployment type first to avoid breaking changes | |
| const existingType = existingValue as AddressType; | |
| const sortedArray = sortDeploymentTypes([existingType, deploymentType], existingType); | |
| json.networkAddresses[options.chainId] = sortedArray; | |
| console.log( | |
| ` Converted single value "${existingType}" to array [${sortedArray.join(', ')}] for chain ID ${options.chainId}`, | |
| ); | |
| debug(` Preserved existing deployment type "${existingType}" as first, added "${deploymentType}"`); | |
| } | |
| } else { | |
| if (isArray) { | |
| // Add to existing array (pre-check ensures deploymentType is not already in array) | |
| // Policy: Keep existing first value to avoid breaking changes | |
| json.networkAddresses[options.chainId] = [...existingValue, deploymentType]; | |
| debug( | |
| ` Added "${deploymentType}" to existing array [${existingValue.join(', ')}] for chain ID ${options.chainId}`, | |
| ); | |
| debug(` Sorted array (preserving first value): [${updatedArray.join(', ')}]`); | |
| } else { | |
| // Convert single value to array | |
| // Policy: Keep existing deployment type first to avoid breaking changes | |
| json.networkAddresses[options.chainId] = [existingType, deploymentType]; | |
| console.log( | |
| ` Converted single value "${existingType}" to array [${sortedArray.join(', ')}] for chain ID ${options.chainId}`, | |
| ); | |
| debug(` Preserved existing deployment type "${existingType}" as first, added "${deploymentType}"`); | |
| } | |
| } else { |
Contributor
There was a problem hiding this comment.
Then the sortDeploymentTypes can be removed.
nlordell
approved these changes
Nov 21, 2025
Contributor
nlordell
left a comment
nlordell
left a comment
There was a problem hiding this comment.
One more suggestion to remove the sortDeploymentTypes function, as it isn't really used.
Other than that LGTM.
…yment type handling - Eliminated the sortDeploymentTypes function to streamline the logic for updating network addresses. - Updated the handling of existing deployment types to directly append new types without sorting, preserving the first value for consistency.
Contributor
Author
|
I did a last update and we shall see how it behaves next network deployment, ideally I would need some sort of token to be able to call the github actions from the safe-deployment-scripts . But we can incrementally improve and pipe everything together once we are certain things work correctly individually. |
remedcu
reviewed
Dec 2, 2025
Comment on lines
+75
to
+78
| } catch { | ||
| // If chain ID is not a valid number, sort alphabetically | ||
| return a.localeCompare(b); | ||
| } |
Contributor
There was a problem hiding this comment.
When is this case required?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.