chore(emergency-pause): narrow networks.json to sophon/boba/rootstock for Phase 2b test#1829
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughReformats the ChangesNetwork Configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
script/utils/diamondEMERGENCYPauseStagingGitHub.sh (1)
36-46: ⚡ Quick winUse
[[ ... ]]for all conditional expressions instead of[ ... ]to comply with bash script guidelines.The retry and attempt-loop conditionals use single-bracket syntax. Per
.cursor/rules/300-bash.mdc, all bash conditional expressions must use[[ ... ]]. Update lines 36, 41, 102, 109, 165, 172, 181, 196, 203, 238, and 245 to use double-bracket syntax for consistency and best practice.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@script/utils/diamondEMERGENCYPauseStagingGitHub.sh` around lines 36 - 46, Replace all single-bracket conditionals with bash's double-bracket form in the retry/attempt logic: change the while loop "while [ "$ATTEMPT" -le "$RPC_MAX_ATTEMPTS" ]" and the subsequent if checks (including the "if OUT=$(...); then" exit-check and the retry guard "if [ "$ATTEMPT" -lt "$RPC_MAX_ATTEMPTS" ]") to use [[ ... ]] and preserve the same operators (e.g., [[ "$ATTEMPT" -le "$RPC_MAX_ATTEMPTS" ]], [[ "$ATTEMPT" -lt "$RPC_MAX_ATTEMPTS" ]]) and quoting of variables; apply the same [[ ... ]] replacement to the other listed conditional sites (lines referenced in the review) so all conditionals use [[ ... ]] consistently while leaving command substitutions, variable assignments (OUT=...), sleep, echo, and ATTEMPT increment logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@script/utils/diamondEMERGENCYPauseStagingGitHub.sh`:
- Around line 104-107: Replace the loose 0x* address check with a strict 40-hex
char Ethereum address regex: change occurrences that test RESPONSE against 0x*
(the checks near the DIAMOND_IS_PAUSED_SELECTOR / "DiamondIsPaused" branches) to
use a bash regex match like [[ $RESPONSE =~ ^0x[0-9A-Fa-f]{40}$ ]] so only valid
0x-prefixed 40-hex-character addresses pass; apply this exact replacement in all
three affected sections (the blocks that currently check "$RESPONSE" == 0x*
around the DIAMOND_IS_PAUSED_SELECTOR checks).
---
Nitpick comments:
In `@script/utils/diamondEMERGENCYPauseStagingGitHub.sh`:
- Around line 36-46: Replace all single-bracket conditionals with bash's
double-bracket form in the retry/attempt logic: change the while loop "while [
"$ATTEMPT" -le "$RPC_MAX_ATTEMPTS" ]" and the subsequent if checks (including
the "if OUT=$(...); then" exit-check and the retry guard "if [ "$ATTEMPT" -lt
"$RPC_MAX_ATTEMPTS" ]") to use [[ ... ]] and preserve the same operators (e.g.,
[[ "$ATTEMPT" -le "$RPC_MAX_ATTEMPTS" ]], [[ "$ATTEMPT" -lt "$RPC_MAX_ATTEMPTS"
]]) and quoting of variables; apply the same [[ ... ]] replacement to the other
listed conditional sites (lines referenced in the review) so all conditionals
use [[ ... ]] consistently while leaving command substitutions, variable
assignments (OUT=...), sleep, echo, and ATTEMPT increment logic unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 684a74ef-d281-4cb1-a482-27efb1ff9898
📒 Files selected for processing (8)
config/networks.jsondeployments/arbitrum.diamond.staging.jsondeployments/arbitrum.staging.jsondeployments/base.diamond.staging.jsondeployments/base.staging.jsondeployments/optimism.diamond.staging.jsondeployments/optimism.staging.jsonscript/utils/diamondEMERGENCYPauseStagingGitHub.sh
12e209c to
ba219ef
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
config/networks.json (2)
7-7: ⚡ Quick winConsider using EIP-55 checksummed addresses consistently.
The configuration uses a mix of lowercase and checksummed addresses. For example,
wrappedNativeAddressentries use lowercase whilemulticallAddressentries use the checksummed format (0xcA11b...).Using checksummed addresses throughout improves safety by enabling checksum validation and catching address typos.
♻️ Example of checksumming addresses
You can checksum addresses using a script or tool. For instance, the wrapped native addresses should be:
"gnosis": { ... - "wrappedNativeAddress": "0xe91d153e0b41518a2ce8dd3d7944fa863463a97d", + "wrappedNativeAddress": "0xe91D153E0b41518A2Ce8Dd3D7944Fa863463a97d", ... }Note: Verify the correct checksummed format for each address using an EIP-55 checksum tool or web3 library before applying changes.
Also applies to: 27-27, 34-34, 47-47, 54-54
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/networks.json` at line 7, The config contains mixed-case and lowercase Ethereum addresses (e.g., the wrappedNativeAddress values) which should be normalized to EIP-55 checksummed addresses; update every address value (notably keys like "wrappedNativeAddress" and any "multicallAddress" entries) by replacing the lowercase forms with their EIP-55 checksummed equivalents computed with a library such as ethers.utils.getAddress or web3.utils.toChecksumAddress, and verify each replacement against a checksum tool before committing.
1-62: High-risk temporary network removal for production test - verify safeguards.This PR removes all networks except gnosis, moonbeam, and rootstock to scope the Phase 2b emergency-pause test. While the PR title includes "[DO NOT MERGE]" and the objectives clearly state this is temporary, the following safeguards should be confirmed before proceeding:
- Revert PR readiness: Confirm the revert PR is prepared, reviewed, and ready to merge immediately after the test completes
- Deployment freeze: Ensure no other deployments to removed networks are scheduled during the test window
- Team coordination: Verify all team members are aware of the temporary config change and merge/revert timeline
- Workflow impact: Confirm all scripts and workflows that read
config/networks.jsonhave been identified and their behavior during the test window is understood- Merge window: Document the exact merge and revert timeline to minimize exposure
The production workflow checking out the repo at runtime (as noted in the PR description) means this change takes effect immediately upon merge. Consider:
- Using a feature flag or environment variable instead of modifying the config file directly
- Implementing a separate config file specifically for the pause test
- Adding a CI check that prevents accidental merges of PRs with "[DO NOT MERGE]" in the title
As per coding guidelines from .cursor/rules/002-architecture.mdc: "If any change affects critical invariants indirectly (e.g., which networks/workflows run), call out the impact explicitly."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/networks.json` around lines 1 - 62, This change temporarily removes most entries from networks.json leaving only "gnosis", "moonbeam", and "rootstock" — before merging, create and have reviewed a revert PR ready to merge immediately after the test, replace the direct file mutation with an environment-controlled toggle (e.g., an env var or feature flag that causes the runtime loader to return the trimmed network list) or use a separate test-only networks file to avoid repo-level changes, and add a CI gate that fails merges when the PR title contains "[DO NOT MERGE]" (and verify all downstream scripts that read the network keys "gnosis"/"moonbeam"/"rootstock" behave under the toggle).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@config/networks.json`:
- Line 7: The config contains mixed-case and lowercase Ethereum addresses (e.g.,
the wrappedNativeAddress values) which should be normalized to EIP-55
checksummed addresses; update every address value (notably keys like
"wrappedNativeAddress" and any "multicallAddress" entries) by replacing the
lowercase forms with their EIP-55 checksummed equivalents computed with a
library such as ethers.utils.getAddress or web3.utils.toChecksumAddress, and
verify each replacement against a checksum tool before committing.
- Around line 1-62: This change temporarily removes most entries from
networks.json leaving only "gnosis", "moonbeam", and "rootstock" — before
merging, create and have reviewed a revert PR ready to merge immediately after
the test, replace the direct file mutation with an environment-controlled toggle
(e.g., an env var or feature flag that causes the runtime loader to return the
trimmed network list) or use a separate test-only networks file to avoid
repo-level changes, and add a CI gate that fails merges when the PR title
contains "[DO NOT MERGE]" (and verify all downstream scripts that read the
network keys "gnosis"/"moonbeam"/"rootstock" behave under the toggle).
9161314 to
d5b0b81
Compare
…ull networks.json Restores `config/networks.json` to the full production network list after the Phase 2b real-pause test completes. Reverts the narrowing applied in PR #1829. Merge order: this PR must be merged AFTER PR #1829 has been merged and the Phase 2b test has completed (pause confirmed + unpause executed on sophon, boba, and rootstock). DO NOT MERGE until Phase 2b is verified complete. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config/networks.json`:
- Around line 22-61: The network set currently includes "boba" as active which
broadens the emergency-pause scope; update the networks.json so only "gnosis",
"moonbeam", "rootstock" (and "localanvil" for CI) are active for this run by
either removing or setting "status": "inactive" on any other network objects
(notably the "boba" object) and ensuring the "gnosis", "moonbeam", "rootstock",
and "localanvil" objects exist with "status": "active"; keep all other
properties unchanged for those retained entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
… for Phase 2b Temporary change for the Phase 2b real-pause track of the emergency-pause testing plan. Restricts the pause workflow to 3 low-volume production networks so a live pause+unpause cycle can be verified without impacting high-traffic chains. localanvil is kept so the deploy smoke-test workflow (which deploys to localanvil in CI) keeps working — emergency-pause scripts skip it on both filters (status != active, isTestnetNetwork). DO NOT MERGE — restore networks.json from main after Phase 2b testing completes via revert PR #1833. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
d5b0b81 to
f99db16
Compare
Which Linear task belongs to this PR?
N/A — test infrastructure, no Linear ticket.
Why did I implement it this way?
Phase 2b of the Hexagate → GitHub Actions production emergency-pause test plan requires running a real pause on 3 low-volume networks to verify that
PRIV_KEY_PAUSER_WALLETholds the correct key. Narrowingnetworks.jsonto onlysophon,boba, androotstockscopes the pause workflow to those 3 networks, preventing accidental pausing of high-traffic chains during the test.localanvilis kept alongside the 3 test networks so the deploy smoke-test workflow (which deploys to localanvil in CI) keeps working — emergency-pause scripts skip it on both filters (status != active,isTestnetNetwork).This PR must be merged to
mainbefore the Phase 2b test fires (the production workflow checks out the repo at run time). A revert PR (#1833) is queued and ready to merge immediately after Phase 2b completes.See the full test plan: https://www.notion.so/361f0ff14ac780599741e7e4dec2d9cc
Checklist before requesting a review
/pr-ready— no actionable findings on the committed diff (config/networks.jsononly)Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)