refactor(incentive-council): improve deploy scripts

[0xClandestine]

Motivation:

Some environments are failing due to missing Zeus env parameters, and we also want to add default distributions in order to maintain the current emissions schedule as the TokenHopper initially.

Modifications:

• Added missing zeus params.
• Added missing check to skip environments that core isn't deployed.

Result:

CI is passing, and default distributions are added.

[elhajin]

@claude review this PR

[claude]

Claude finished @elhajin's task in 1m 49s —— View job

---

Code Review Complete ✅

I've completed a thorough review of this PR. Overall, the changes are well-structured and address the stated objectives. Below are my findings organized by severity.

---

Summary

This PR refactors deployment scripts for the incentive council upgrade (v1.12.0), adding special handling for preprod environments and removing old v1.9.0 deployment scripts. The main changes focus on:

1. Conditional deployment logic for preprod vs new environments
2. Dynamic reinitialization checks for RewardsCoordinator
3. Addition of missing Zeus environment parameters
4. Cleanup of old deployment scripts

---

Critical Issues

1. Missing Default Distributions Implementation

Location: script/releases/v1.12.0-incentive-council/2-queueUpgrade.s.sol

Issue: The PR description states "Added missing check to skip environments that core isn't deployed" and mentions adding "default distributions in order to maintain the current emissions schedule as the TokenHopper initially." However, I don't see any code that actually creates default distributions to the RewardsCoordinator.

Expected: After upgrading EmissionsController and granting it minting rights, there should be calls to create initial distribution configurations to maintain the current emissions schedule.

Impact: Without default distributions, the emissions schedule may not match the legacy TokenHopper behavior, potentially disrupting expected reward flows.

Recommendation: Add code to create default distributions, similar to:

// 6. Create default distributions to maintain emissions schedule
IRewardsCoordinator.RewardsSubmission[] memory defaultDistributions = _getDefaultDistributions();
for (uint256 i = 0; i < defaultDistributions.length; i++) {
    executorCalls.append({
        to: address(Env.proxy.rewardsCoordinator()),
        data: abi.encodeCall(IRewardsCoordinator.createRewardsSubmission, (defaultDistributions[i]))
    });
}

---

High Priority Issues

2. Unsafe Storage Slot Reading for Initialization Check

Location: script/releases/v1.12.0-incentive-council/2-queueUpgrade.s.sol:87-90

RewardsCoordinator rc = Env.proxy.rewardsCoordinator();
bytes32 slot0 = vm.load(address(rc), bytes32(uint256(0)));
uint8 initializedVersion = uint8(uint256(slot0) & 0xFF);

Issue: This directly reads storage slot 0 to determine if the contract has been reinitialized. While this works for the current OpenZeppelin Initializable pattern, it's fragile and could break if:

• The storage layout changes in future versions
• The contract inherits from additional contracts that modify slot ordering
• OpenZeppelin changes the Initializable implementation

Impact: The script might incorrectly determine initialization status, leading to failed upgrades or double-initialization attempts.

Recommendation: Consider adding a public view function to RewardsCoordinator to expose the initialization version, or use try-catch with the reinitialize call:

// Option 1: Try to reinitialize, catch if already initialized
try rewardsCoordinator.reinitialize(...) {
    // Successfully reinitialized
} catch {
    // Already initialized, just upgrade
    executorCalls.upgradeRewardsCoordinator();
}

Or add a helper function that's more explicit about what it's checking.

---

Medium Priority Issues

3. Inconsistent Environment Version Check Pattern

Location: Multiple locations in 1-deployImplementations.s.sol and 2-queueUpgrade.s.sol

Issue: The preprod special-casing uses string comparison with Env._strEq(Env.envVersion(), "1.12.0"), which is fragile:

• Line 21: if (!(Env._strEq(Env.envVersion(), "1.12.0")))
• Line 52: if (Env._strEq(Env.envVersion(), "1.12.0"))

Issues:

1. Magic string "1.12.0" is hardcoded in multiple places
2. Double negation !(Env._strEq(...)) is harder to read than checking for inequality
3. No documentation explaining why preprod is special-cased

Recommendation:

// At the top of the contract
bool constant IS_PREPROD_UPGRADE = Env._strEq(Env.envVersion(), "1.12.0");

// Then use consistently
if (!IS_PREPROD_UPGRADE) {
    deployEmissionsControllerProxy();
}

Or better yet, add a helper function:

function isPreprodEnvironment() internal view returns (bool) {
    // Preprod already has EmissionsController deployed at v1.12.0
    return Env._strEq(Env.envVersion(), "1.12.0");
}

4. Missing Validation for Preprod-Specific Logic

Location: script/releases/v1.12.0-incentive-council/2-queueUpgrade.s.sol:185-191

Issue: The validation skips checking if EmissionsController already has minting rights on preprod:

if (!Env._strEq(Env.envVersion(), "1.12.0")) {
    require(
        !beigen.isMinter(address(Env.proxy.emissionsController())),
        "EmissionsController should NOT have minting rights before upgrade"
    );
}

Problem: On preprod, if EmissionsController somehow doesn't have minting rights, the script will still proceed and try to grant them again (which should be idempotent but worth validating).

Recommendation: Add a positive check for preprod:

if (Env._strEq(Env.envVersion(), "1.12.0")) {
    // On preprod, verify EmissionsController already has minting rights
    require(
        beigen.isMinter(address(Env.proxy.emissionsController())),
        "Preprod: EmissionsController should already have minting rights"
    );
} else {
    require(
        !beigen.isMinter(address(Env.proxy.emissionsController())),
        "EmissionsController should NOT have minting rights before upgrade"
    );
}

---

Low Priority Issues

5. Unused Helper Function Added to Env.sol

Location: script/releases/Env.sol:558-564

function _deployedProxyOr(
    string memory name,
    address defaultValue
) private view returns (address) {
    string memory envvar = string.concat("ZEUS_DEPLOYED_", name, "_Proxy");
    return vm.envOr(envvar, defaultValue);
}

Issue: This function is added but never used in the codebase. The comment in CoreContractsDeployer.sol:96-97 mentions backward compatibility, but the actual implementation still calls Env.proxy.emissionsController() directly without a fallback.

Recommendation: Either:

1. Use this function to provide a fallback for EmissionsController:

   emissionsController: _deployedProxyOr(type(EmissionsController).name, Env.executorMultisig()),

2. Remove the unused helper function if it's not needed

6. Upgrade Version Constraint Change

Location: script/releases/v1.12.0-incentive-council/upgrade.json:3

Change: "from": ">=1.9.0" → "from": ">=1.11.0"

Issue: This restricts the upgrade path, meaning environments at v1.9.0 or v1.10.0 cannot directly upgrade to v1.12.0. While this may be intentional, it's not documented in the PR description.

Questions:

• Is this intentional?
• Should there be migration documentation for v1.9.0/v1.10.0 environments?
• Does this break any existing deployment plans?

7. TestUtils Label Addition Lacks Consistency

Location: script/releases/TestUtils.sol (multiple locations)

Issue: Labels are added to some validation functions but not all. For example:

• validateAllocationManagerInitialized gets a label (line 944)
• validateDelegationManagerInitialized does NOT get a label (line 958-962)

Recommendation: Either add labels consistently to all validation functions or document why some are excluded.

---

Positive Observations

1. Good defensive programming: The conditional logic for preprod vs new deployments prevents accidentally redeploying contracts that already exist.

2. Smart reinitialization check: The dynamic check for RewardsCoordinator initialization status prevents double-initialization errors.

3. Improved validation: Adding isCoreProtocolDeployed() check prevents running scripts on environments where core contracts aren't deployed.

4. Good cleanup: Removing old v1.9.0 deployment scripts reduces maintenance burden and confusion.

5. Better test coverage: The additional vm.label() calls in TestUtils improve debugging experience.

---

Recommendations Summary

Must Fix:

• Add missing default distributions implementation (Critical Add certora prover to GitHub workflows #1)
• Address unsafe storage slot reading or document the risk (High delete unused interface file #2)

Should Fix:

• Refactor environment version checks for clarity (Medium Fix first deposit rounding #3)
• Add preprod-specific validation (Medium Fix CI errors #4)
• Resolve unused _deployedProxyOr function (Low M2 deployment script #5)

Consider:

• Document upgrade version constraint change (Low Update readme #6)
• Make TestUtils labels consistent (Low Add middleware #7)

---

Final Verdict

The refactoring is well-structured, but the most critical concern is the missing default distributions implementation mentioned in the PR description. Before merging, please:

1. ✅ Confirm whether default distributions are needed
2. ✅ If yes, implement the distribution logic
3. ✅ Address the unsafe storage reading pattern or add documentation explaining the trade-offs
4. ✅ Consider the other medium/low priority issues

---

[elhajin]

LGTM