Skip to content

chore: upgrade op sepolia p3#37

Merged
rplusq merged 1 commit into
mainfrom
chore/deploy-op-sepolia
Oct 14, 2025
Merged

chore: upgrade op sepolia p3#37
rplusq merged 1 commit into
mainfrom
chore/deploy-op-sepolia

Conversation

@rplusq

@rplusq rplusq commented Oct 14, 2025

Copy link
Copy Markdown
Collaborator

No description provided.

Copilot AI review requested due to automatic review settings October 14, 2025 19:06
@rplusq rplusq merged commit e8fa0ee into main Oct 14, 2025
5 of 6 checks passed

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull Request Overview

Adds deployment metadata for the Optimism Sepolia (chainId 11155420) phase 3 upgrade by introducing new implementation contract addresses.

  • Introduces a new JSON deployment descriptor for chainId 11155420.
  • Lists new implementation addresses for Pauser, StakeWeight, and StakingRewardDistributor.

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@rplusq rplusq deleted the chore/deploy-op-sepolia branch October 14, 2025 19:06
@claude

claude Bot commented Oct 14, 2025

Copy link
Copy Markdown

Claude finished @rplusq's task —— View job


PR Review Complete ✅

  • Read the deployment JSON file
  • Check for similar deployment files for pattern consistency
  • Analyze for security issues and best practices
  • Provide review feedback

Review Findings

I've analyzed this P3 upgrade deployment file for Optimism Sepolia. Here are my findings:

⚠️ Issues Found

1. Inconsistent JSON Structure (evm/deployments/11155420-p3-upgrade.json:1-8)

The deployment file format is inconsistent with other P3 upgrade files in the repository:

Current structure:

{
  "chainId": 11155420,
  "NewImplementations": { ... }
}

Expected structure (based on 10-p3-upgrade.json):

{
  "LockedTokenStakerBackersP3": {
    "admin": "...",
    "implementation": "...",
    "proxy": "..."
  },
  "LockedTokenStakerReownP3": { ... },
  "LockedTokenStakerWalletConnectP3": { ... },
  "NewImplementations": { ... },
  "chainId": 11155420
}

Issue: The file is missing the LockedTokenStaker*P3 entries that should be present according to the P3Upgrade.s.sol script (lines 293-334). The script's writeP3DeploymentsToJson() function generates all these sections when BROADCAST=true.

Recommendation: If this was a partial deployment (implementations only), either:

  1. Add a comment/field indicating this is incomplete and LockedTokenStakers will be deployed separately
  2. Complete the deployment to include all P3 components
  3. Update the filename to reflect it's partial (e.g., 11155420-p3-upgrade-implementations-only.json)

2. Missing Verification Script Integration

The verify-deployments.ts script only validates full deployment files (chains 1, 10, 8453) and doesn't handle P3 upgrade files. These upgrade files contain critical deployment data but aren't validated.

Recommendation: Extend the verification script to validate P3 upgrade files, checking:

  • Implementation addresses have bytecode
  • For complete P3 files: proxy addresses, admin addresses, and implementation consistency

3. Field Ordering Inconsistency (evm/deployments/11155420-p3-upgrade.json:2)

The chainId field is placed first, but in 10-p3-upgrade.json it's last. While JSON field order doesn't affect functionality, consistency improves maintainability.

Recommendation: Move chainId to the end to match the established pattern in other deployment files.

ℹ️ Observations

  • ✅ All three implementation addresses are properly formatted (checksummed Ethereum addresses)
  • ✅ Contract names match the expected P3 upgrade contracts (Pauser, StakeWeight, StakingRewardDistributor)
  • ✅ No external domain URLs detected
  • ✅ File follows the naming convention {chainId}-p3-upgrade.json
  • ℹ️ This appears to be a testnet deployment (Optimism Sepolia), so reduced scrutiny vs mainnet is reasonable

Summary

This is a partial P3 upgrade deployment containing only implementation addresses. If this was intentional, the PR description should clarify why LockedTokenStakers aren't included. If unintentional, the deployment should be completed or the file should be marked as incomplete.

For future P3 deployments:

  1. Include all components generated by P3Upgrade.s.sol script
  2. Add verification for P3 upgrade files
  3. Maintain consistent field ordering with existing files
  4. Document in PR descriptions when deployments are partial/staged

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