Skip to content

Fix script when called from GH UI.#1340

Merged
nlordell merged 27 commits into
safe-global:mainfrom
Fbartoli:main
Dec 1, 2025
Merged

Fix script when called from GH UI.#1340
nlordell merged 27 commits into
safe-global:mainfrom
Fbartoli:main

Conversation

@Fbartoli
Copy link
Copy Markdown
Contributor

@Fbartoli Fbartoli commented Dec 1, 2025

It wasn't parsing the --version flag correctly.

SafeFB and others added 25 commits November 17, 2025 11:02
…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
- 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.
…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.
- Introduced a new step to verify if the user triggering the workflow is a member of the 'safe-global' organization.
- Added input parameters for version and chain ID to support workflow calls.
- Updated permissions to include read access for members.
… workflow to simplify execution. Updated permissions to remove read access for members.
- Removed JSON linting from the `update-registry` command in package.json for streamlined execution.
- Introduced a new command `update-registry:full` to run the update and linting sequentially.
- Added JSON linting step in the update-registry GitHub workflow to ensure code quality after deployment.
@Fbartoli Fbartoli requested review from a team and gjeanmart as code owners December 1, 2025 07:47
@Fbartoli Fbartoli requested a review from remedcu December 1, 2025 07:48

- name: Update registry
run: |
npm run update-registry -- \
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.

Should we instead use

Suggested change
npm run update-registry:full -- \

Copy link
Copy Markdown
Contributor Author

@Fbartoli Fbartoli Dec 1, 2025

Choose a reason for hiding this comment

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

I removed the :full because the parameters are passed to the lint command instead of the update script
I also removed it from the package.json. AI slop coding.

Copy link
Copy Markdown
Contributor

@nlordell nlordell Dec 1, 2025

Choose a reason for hiding this comment

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

Can't you pass parameters to the first command? Something like "solution 2" from this: https://stackoverflow.com/questions/35428618/pass-an-argument-to-an-npm-run-script-comprising-multiple-commands

(Disclaimer: I didn't try this out myself).

Edit: Or, even better:

include a postupdate-registry script, I think it should automatically run according to this. In particular: https://docs.npmjs.com/cli/v8/using-npm/scripts#pre--post-scripts

Copy link
Copy Markdown
Contributor Author

@Fbartoli Fbartoli Dec 1, 2025

Choose a reason for hiding this comment

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

I didn't know about that. Thanks.

Updated accordingly

…linting after registry updates. Removed JSON linting from the GitHub workflow to streamline the process.
@nlordell nlordell merged commit 1209e99 into safe-global:main Dec 1, 2025
1 check passed
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.

2 participants