Refactor to single deployPhase() entrypoint

Address review feedback:
- Replace deployPhase1/2/3() with single deployPhase() entrypoint
- Make internal _deployPhase1/2/3() functions
- Remove deployOnce() (not needed for one-off deployment)
- Remove WrongPhase and AlreadyDeployed errors
- Calling deployPhase() after completion is a no-op

Added transparency for callers:
- Return bool indicating if deployment is complete
- Emit PhaseCompleted event after each phase

Author: xavikh

script/Deploy.s.sol

@@ -99,10 +99,8 @@ contract DeployScript is Script {
         factory = new ProtocolFactory(buildFactoryParams());
         vm.label(address(factory), "ProtocolFactory");
 
-        // Deploy in phases to stay within gas limits
-        factory.deployPhase1();
-        factory.deployPhase2();
-        factory.deployPhase3();
+        // Deploy in phases to stay within EIP-7825 gas limits
+        while (!factory.deployPhase()) {}
 
         // Done
         printDeployment();

src/ProtocolFactory.sol

@@ -130,16 +130,14 @@ contract ProtocolFactory {
         Complete
     }
 
-    /// @notice Emitted when deployOnce() has been called and the deployment is complete.
+    /// @notice Emitted when a deployment phase completes.
+    /// @param phase The phase that was completed.
+    event PhaseCompleted(DeploymentPhase phase);
+
+    /// @notice Emitted when all deployment phases are complete.
     /// @param factory The address of the factory contract where the parameters and addresses can be retrieved.
     event ProtocolDeployed(ProtocolFactory factory);
 
-    /// @notice Thrown when attempting to call deployOnce() when the protocol is already deployed.
-    error AlreadyDeployed();
-
-    /// @notice Thrown when attempting to call a deployment phase out of order
-    error WrongPhase(DeploymentPhase expected, DeploymentPhase actual);
-
     /// @notice Thrown when the Management DAO has less members than minApprovals
     error MemberListIsTooSmall();
 
@@ -153,72 +151,45 @@ contract ProtocolFactory {
         parameters = _parameters;
     }
 
-    /// @notice Phase 1: Create Management DAO and deploy ENS infrastructure (~5.3M gas)
-    function deployPhase1() external {
-        if (currentPhase != DeploymentPhase.NotStarted) {
-            revert WrongPhase(DeploymentPhase.NotStarted, currentPhase);
+    /// @notice Executes the next deployment phase. Call repeatedly until deployment is complete.
+    /// @dev Due to EIP-7825 gas limits, deployment is split into 3 phases.
+    /// @return complete True if deployment is complete, false if more phases remain.
+    function deployPhase() external returns (bool complete) {
+        if (currentPhase == DeploymentPhase.NotStarted) {
+            _deployPhase1();
+        } else if (currentPhase == DeploymentPhase.Phase1Complete) {
+            _deployPhase2();
+        } else if (currentPhase == DeploymentPhase.Phase2Complete) {
+            _deployPhase3();
         }
+        // else: already complete, nop
 
-        // Create the DAO that will own the registries and the core plugin repo's
-        prepareRawManagementDao();
-
-        // Set up the ENS registry and the requested domains
-        prepareEnsRegistry();
-
-        currentPhase = DeploymentPhase.Phase1Complete;
+        return currentPhase == DeploymentPhase.Complete;
     }
 
-    /// @notice Phase 2: Deploy OSx core contracts (~13.0M gas)
-    function deployPhase2() external {
-        if (currentPhase != DeploymentPhase.Phase1Complete) {
-            revert WrongPhase(DeploymentPhase.Phase1Complete, currentPhase);
-        }
-
-        // Deploy the OSx core contracts
-        prepareOSx();
-
-        currentPhase = DeploymentPhase.Phase2Complete;
-    }
-
-    /// @notice Phase 3: Set up permissions, deploy plugin repos and finalize Management DAO (~6.3M gas)
-    function deployPhase3() external {
-        if (currentPhase != DeploymentPhase.Phase2Complete) {
-            revert WrongPhase(DeploymentPhase.Phase2Complete, currentPhase);
-        }
-
-        preparePermissions();
-
-        // Prepare the plugin repo's and their versions
-        prepareCorePluginRepos();
-
-        // Drop the factory's permissions on the management DAO
-        concludeManagementDao();
-        removePermissions();
-
-        currentPhase = DeploymentPhase.Complete;
-        emit ProtocolDeployed(this);
-    }
-
-    /// @notice Performs all deployment phases in a single transaction (~25M gas) (backwards compatibility)
-    /// @dev WARNING: This may exceed gas limits on some networks. Use phased deployment instead.
-    function deployOnce() external {
-        if (currentPhase != DeploymentPhase.NotStarted) {
-            revert AlreadyDeployed();
-        }
-
+    /// @dev Phase 1: Create Management DAO and deploy ENS infrastructure (~1.9M gas)
+    function _deployPhase1() internal {
         // Create the DAO that will own the registries and the core plugin repo's
         prepareRawManagementDao();
 
         // Set up the ENS registry and the requested domains
         prepareEnsRegistry();
 
         currentPhase = DeploymentPhase.Phase1Complete;
+        emit PhaseCompleted(currentPhase);
+    }
 
+    /// @dev Phase 2: Deploy OSx core contracts (~4.5M gas)
+    function _deployPhase2() internal {
         // Deploy the OSx core contracts
         prepareOSx();
 
         currentPhase = DeploymentPhase.Phase2Complete;
+        emit PhaseCompleted(currentPhase);
+    }
 
+    /// @dev Phase 3: Set up permissions, deploy plugin repos and finalize Management DAO (~7.9M gas)
+    function _deployPhase3() internal {
         preparePermissions();
 
         // Prepare the plugin repo's and their versions
@@ -229,6 +200,7 @@ contract ProtocolFactory {
         removePermissions();
 
         currentPhase = DeploymentPhase.Complete;
+        emit PhaseCompleted(currentPhase);
         emit ProtocolDeployed(this);
     }
 

test/ProtocolFactory.t.sol

@@ -75,7 +75,7 @@ contract ProtocolFactoryTest is AragonTest {
 
         builder.withManagementDaoMembers(mgmtDaoMembers).withManagementDaoMinApprovals(2);
 
-        // Build the factory (deploys factory contract but doesn't call deployOnce yet)
+        // Build the factory (deploys factory contract but doesn't call deployPhase yet)
         factory = builder.build();
 
         deploymentParams = builder.getDeploymentParams();
@@ -139,12 +139,13 @@ contract ProtocolFactoryTest is AragonTest {
     }
 
     function test_GivenNoPriorDeploymentOnTheFactory() external whenInvokingDeployOnce {
-        // It Should emit an event with the factory address
+        // It Should emit an event with the factory address on final phase
+        factory.deployPhase(); // Phase 1
+        factory.deployPhase(); // Phase 2
+
         vm.expectEmit(true, true, true, true);
         emit ProtocolFactory.ProtocolDeployed(factory);
-
-        // Deploy the protocol
-        factory.deployOnce();
+        factory.deployPhase(); // Phase 3
 
         // It The deployment addresses are filled with the new contracts
         deployment = factory.getDeployment();
@@ -170,7 +171,7 @@ contract ProtocolFactoryTest is AragonTest {
         assertEq(deployment.globalExecutor, deploymentParams.osxImplementations.globalExecutor);
         assertEq(deployment.placeholderSetup, deploymentParams.osxImplementations.placeholderSetup);
 
-        // It Parameters should remain immutable after deployOnce is invoked
+        // It Parameters should remain immutable after deployment
         ProtocolFactory.DeploymentParameters memory currentParams = factory.getParameters();
 
         assertEq(keccak256(abi.encode(currentParams)), keccak256(abi.encode(deploymentParams)));
@@ -235,17 +236,16 @@ contract ProtocolFactoryTest is AragonTest {
         );
     }
 
-    function test_RevertGiven_TheFactoryAlreadyMadeADeployment() external whenInvokingDeployOnce {
+    function test_GivenTheFactoryAlreadyMadeADeployment_CallingDeployPhaseIsNoop() external whenInvokingDeployOnce {
         // Do a first deployment
         ProtocolFactory.DeploymentParameters memory params0 = factory.getParameters();
-        factory.deployOnce();
+        while (!factory.deployPhase()) {}
 
         ProtocolFactory.DeploymentParameters memory params1 = factory.getParameters();
         ProtocolFactory.Deployment memory deployment1 = factory.getDeployment();
 
-        // It Should revert
-        vm.expectRevert(ProtocolFactory.AlreadyDeployed.selector);
-        factory.deployOnce();
+        // Calling deployPhase after completion should be a no-op (not revert)
+        factory.deployPhase();
 
         // It Parameters should remain unchanged
         ProtocolFactory.DeploymentParameters memory params2 = factory.getParameters();
@@ -255,6 +255,9 @@ contract ProtocolFactoryTest is AragonTest {
         // It Deployment addresses should remain unchanged
         ProtocolFactory.Deployment memory deployment2 = factory.getDeployment();
         assertEq(keccak256(abi.encode(deployment1)), keccak256(abi.encode(deployment2)));
+
+        // Phase should still be Complete
+        assertEq(uint256(factory.currentPhase()), uint256(ProtocolFactory.DeploymentPhase.Complete));
     }
 
     function test_RevertGiven_TheManagementDAOMinApprovalsIsTooSmall() external whenInvokingDeployOnce {
@@ -268,11 +271,15 @@ contract ProtocolFactoryTest is AragonTest {
         builder.withManagementDaoMembers(mgmtDaoMembers).withManagementDaoMinApprovals(2);
         factory = builder.build();
 
-        // Fail
+        // Phase 1 and 2 succeed
+        factory.deployPhase();
+        factory.deployPhase();
+
+        // Fail on phase 3 (when concludeManagementDao is called)
         vm.expectRevert(ProtocolFactory.MemberListIsTooSmall.selector);
-        factory.deployOnce();
+        factory.deployPhase();
 
-        // OK
+        // OK with valid config
         mgmtDaoMembers = new address[](2);
         mgmtDaoMembers[0] = alice;
         mgmtDaoMembers[1] = bob;
@@ -291,17 +298,17 @@ contract ProtocolFactoryTest is AragonTest {
         assertEq(uint256(factory.currentPhase()), uint256(ProtocolFactory.DeploymentPhase.NotStarted));
 
         // Deploy phase 1
-        factory.deployPhase1();
+        factory.deployPhase();
         assertEq(uint256(factory.currentPhase()), uint256(ProtocolFactory.DeploymentPhase.Phase1Complete));
 
         // Deploy phase 2
-        factory.deployPhase2();
+        factory.deployPhase();
         assertEq(uint256(factory.currentPhase()), uint256(ProtocolFactory.DeploymentPhase.Phase2Complete));
 
         // Deploy phase 3 - expect event
         vm.expectEmit(true, true, true, true);
         emit ProtocolFactory.ProtocolDeployed(factory);
-        factory.deployPhase3();
+        factory.deployPhase();
         assertEq(uint256(factory.currentPhase()), uint256(ProtocolFactory.DeploymentPhase.Complete));
 
         // Verify deployment completed
@@ -311,79 +318,26 @@ contract ProtocolFactoryTest is AragonTest {
         assertNotEq(deployment.pluginRepoRegistry, address(0));
     }
 
-    function test_RevertWhen_CallingPhase1OutOfOrder() external whenInvokingPhasedDeployment {
-        // Complete phase 1
-        factory.deployPhase1();
-
-        // Try calling phase 1 again - should revert
-        vm.expectRevert(
-            abi.encodeWithSelector(
-                ProtocolFactory.WrongPhase.selector,
-                ProtocolFactory.DeploymentPhase.NotStarted,
-                ProtocolFactory.DeploymentPhase.Phase1Complete
-            )
-        );
-        factory.deployPhase1();
-    }
-
-    function test_RevertWhen_CallingPhase2BeforePhase1() external whenInvokingPhasedDeployment {
-        // Try calling phase 2 without phase 1 - should revert
-        vm.expectRevert(
-            abi.encodeWithSelector(
-                ProtocolFactory.WrongPhase.selector,
-                ProtocolFactory.DeploymentPhase.Phase1Complete,
-                ProtocolFactory.DeploymentPhase.NotStarted
-            )
-        );
-        factory.deployPhase2();
-    }
-
-    function test_RevertWhen_CallingPhase3BeforePhase2() external whenInvokingPhasedDeployment {
-        // Complete phase 1 only
-        factory.deployPhase1();
-
-        // Try calling phase 3 without phase 2 - should revert
-        vm.expectRevert(
-            abi.encodeWithSelector(
-                ProtocolFactory.WrongPhase.selector,
-                ProtocolFactory.DeploymentPhase.Phase2Complete,
-                ProtocolFactory.DeploymentPhase.Phase1Complete
-            )
-        );
-        factory.deployPhase3();
-    }
-
-    function test_RevertWhen_CallingDeployOnceAfterPhasedDeployment() external whenInvokingPhasedDeployment {
+    function test_DeployPhaseIsNoopAfterCompletion() external whenInvokingPhasedDeployment {
         // Complete all phases
-        factory.deployPhase1();
-        factory.deployPhase2();
-        factory.deployPhase3();
+        while (!factory.deployPhase()) {}
 
-        // Try calling deployOnce - should revert with AlreadyDeployed
-        vm.expectRevert(ProtocolFactory.AlreadyDeployed.selector);
-        factory.deployOnce();
-    }
+        ProtocolFactory.Deployment memory deployment1 = factory.getDeployment();
 
-    function test_RevertWhen_CallingPhase1AfterDeployOnce() external whenInvokingPhasedDeployment {
-        // Do a complete deployment with deployOnce
-        factory.deployOnce();
+        // Calling deployPhase after completion should be a no-op
+        factory.deployPhase();
 
-        // Try calling phase 1 - should revert with WrongPhase
-        vm.expectRevert(
-            abi.encodeWithSelector(
-                ProtocolFactory.WrongPhase.selector,
-                ProtocolFactory.DeploymentPhase.NotStarted,
-                ProtocolFactory.DeploymentPhase.Complete
-            )
-        );
-        factory.deployPhase1();
+        // Phase should still be Complete
+        assertEq(uint256(factory.currentPhase()), uint256(ProtocolFactory.DeploymentPhase.Complete));
+
+        // Deployment should be unchanged
+        ProtocolFactory.Deployment memory deployment2 = factory.getDeployment();
+        assertEq(keccak256(abi.encode(deployment1)), keccak256(abi.encode(deployment2)));
     }
 
     modifier givenAProtocolDeployment() {
-        // Use phased deployment to stay within gas limits
-        factory.deployPhase1();
-        factory.deployPhase2();
-        factory.deployPhase3();
+        // Use phased deployment to stay within EIP-7825 gas limits
+        while (!factory.deployPhase()) {}
 
         deployment = factory.getDeployment();
         deploymentParams = builder.getDeploymentParams();