refactor: rename Batched spells as Grouped; switch to dynamic sto…

…rage ilk list

README.md

@@ -40,7 +40,7 @@ TBD.
 
 ## Implemented Actions
 
-| Description        | Single-ilk         | Batched            | Multi-ilk / Global |
+| Description        | Single-ilk         | Grouped            | Multi-ilk / Global |
 | :----------        | :--------:         | :-----:            | :----------------: |
 | Wipe `line`        | :white_check_mark: | :white_check_mark: | :white_check_mark: |
 | Set `Clip` breaker | :white_check_mark: | :white_check_mark: | :white_check_mark: |
@@ -123,12 +123,24 @@ constructor.</sub>
 Some types of emergency spells may come in 3 flavors:
 
 1. Single-ilk: applies the desired spell action to a single pre-defined ilk.
-1. Batched: applies the desired spell action to a list of related ilks (i.e.: `ETH-A`, `ETH-B` and `ETH-C`)
+1. Grouped: applies the desired spell action to a list of related ilks (i.e.: `ETH-A`, `ETH-B` and `ETH-C`)
 1. Multi: applies the desired spell action to all applicable ilks.
 
 Furthermore, this repo provides on-chain factories for single ilk emergency spells to make it easier to deploy for new
 ilks.
 
+
+### About storage variables in `DssGroupedEmergencySpell`
+
+Regular spell actions are executed through a `delegatecall` from `MCD_PAUSE_PROXY`. For that reason, they usually should
+not have storage variables, as they would be accessing and interacting with `MCD_PAUSE_PROXY`'s storage, not their own.
+
+However, Emergency Spells are not required to interact with `MCD_PAUSE` and `MCD_PAUSE_PROXY` at all. They execute
+actions through regular `call` on `Mom` contracts, so we do not have this limitation.
+
+Even if the contract is somehow misused and used as a regular spell, interacting with `MCD_PAUSE`, there would not be a
+problem because the storage should not be changed outside the constructor by the concrete implementations.
+
 ### About the `done()` function
 
 Conforming spells have a [`done`][spell-done] public storage variable which is `false` when the spell is deployed and
@@ -138,7 +150,7 @@ An emergency spell is not meant to be cast, but it can be scheduled multiple tim
 storage variable, it becomes a getter function that will return:
 - `false`: if the emergency spell can be scheduled in the current state, given it is lifted to the hat.
 - `true`: if the desired effects of the spell can be verified or if there is anything that would prevent the spell from
-  being scheduled (i.e.: bad system config)
+  being scheduled (i.e.: bad system config).
 
 Generally speaking, `done` should almost always return `false` for any emergency spell. If it returns `true` it means it
 has just been scheduled or there is most likely something wrong with the modules touched by it. The exception is the

src/DssBatchedEmergencySpell.sol → src/DssGroupedEmergencySpell.sol

@@ -17,68 +17,54 @@ pragma solidity ^0.8.16;
 
 import {DssEmergencySpell, DssEmergencySpellLike} from "./DssEmergencySpell.sol";
 
-interface DssBatchedEmergencySpellLike is DssEmergencySpellLike {
+interface DssGroupedEmergencySpellLike is DssEmergencySpellLike {
     function ilks() external view returns (bytes32[] memory);
+    function emergencyActionsInBatch(uint256 start, uint256 end) external;
 }
 
-/// @title Batched Emergency Spell
-/// @notice Defines the base implementation for batched emergency spells.
+/// @title Grouped Emergency Spell
+/// @notice Defines the base implementation for grouped emergency spells.
 /// @custom:authors [amusingaxl]
 /// @custom:reviewers []
 /// @custom:auditors []
 /// @custom:bounties []
-abstract contract DssBatchedEmergencySpell is DssEmergencySpell, DssBatchedEmergencySpellLike {
+abstract contract DssGroupedEmergencySpell is DssEmergencySpell, DssGroupedEmergencySpellLike {
     /// @dev The min size for the list of ilks
-    uint256 public constant MIN_ILKS = 2;
-    /// @dev The max size for the list of ilks
-    uint256 public constant MAX_ILKS = 3;
-
-    /// @dev The total number of ilks in the spell.
-    uint256 internal immutable _totalIlks;
-    /// @dev The 0th ilk to which the spell should be applicable.
-    bytes32 internal immutable _ilk0;
-    /// @dev The 1st ilk to which the spell should be applicable.
-    bytes32 internal immutable _ilk1;
-    /// @dev The 2nd ilk to which the spell should be applicable.
-    bytes32 internal immutable _ilk2;
+    uint256 private constant MIN_ILKS = 1;
+
+    /// @notice The list of ilks to which the spell is applicable.
+    /// @dev While spells should not have storage variables, we can make an exception here because this spell should not
+    ///      change its own storage, an therefore, could not overwrite the PauseProxy state through delegate call even
+    ///      if used incorrectly.
+    bytes32[] private ilkList;
 
     /// @param _ilks The list of ilks for which the spell should be applicable
     /// @dev The list size is be at least 2 and less than or equal to 3.
-    ///      The batched spell is meant to be used for ilks that are a variation of tha same collateral gem
+    ///      The grouped spell is meant to be used for ilks that are a variation of tha same collateral gem
     ///      (i.e.: ETH-A, ETH-B, ETH-C)
     ///      There has never been a case where MCD onboarded 4 or more ilks for the same collateral gem.
     ///      For cases where there is only one ilk for the same collateral gem, use the single-ilk version.
     constructor(bytes32[] memory _ilks) {
         // This is a workaround to Solidity's lack of support for immutable arrays, as described in
         // https://github.com/ethereum/solidity/issues/12587
         uint256 len = _ilks.length;
-        require(len >= MIN_ILKS, "DssBatchedEmergencySpell/too-few-ilks");
-        require(len <= MAX_ILKS, "DssBatchedEmergencySpell/too-many-ilks");
-        _totalIlks = len;
-
-        _ilk0 = _ilks[0];
-        _ilk1 = _ilks[1];
-        // Only ilk2 is not guaranteed to exist.
-        _ilk2 = len > 2 ? _ilks[2] : bytes32(0);
+        require(len >= MIN_ILKS, "DssGroupedEmergencySpell/too-few-ilks");
+
+        ilkList = _ilks;
     }
 
     /// @notice Returns the list of ilks to which the spell is applicable.
-    /// @return _ilks The list of ilks
-    function ilks() public view returns (bytes32[] memory _ilks) {
-        _ilks = new bytes32[](_totalIlks);
-        _ilks[0] = _ilk0;
-        _ilks[1] = _ilk1;
-        if (_totalIlks > 2) {
-            _ilks[2] = _ilk2;
-        }
+    function ilks() external view returns (bytes32[] memory) {
+        return ilkList;
     }
 
     /// @notice Returns the spell description.
     function description() external view returns (string memory) {
         // Join the list of ilks into a comma-separated string
-        string memory buf = string.concat(_bytes32ToString(_ilk0), ", ", _bytes32ToString(_ilk1));
-        if (_totalIlks > 2) {
-            buf = string.concat(buf, ", ", _bytes32ToString(_ilk2));
+        string memory buf = _bytes32ToString(ilkList[0]);
+        // Start from one because the first item was already added.
+        for (uint256 i = 1; i < ilkList.length; i++) {
+            buf = string.concat(buf, ", ", _bytes32ToString(ilkList[i]));
         }
 
         return string.concat(_descriptionPrefix(), " ", buf);
@@ -106,10 +92,24 @@ abstract contract DssBatchedEmergencySpell is DssEmergencySpell, DssBatchedEmerg
 
     /// @inheritdoc DssEmergencySpell
     function _emergencyActions() internal override {
-        _emergencyActions(_ilk0);
-        _emergencyActions(_ilk1);
-        if (_totalIlks > 2) {
-            _emergencyActions(_ilk2);
+        for (uint256 i = 0; i < ilkList.length; i++) {
+            _emergencyActions(ilkList[i]);
+        }
+    }
+
+    /**
+     * @notice Executes the emergency actions for all ilks in the batch.
+     * @dev This is an escape hatch to prevent the spell from being blocked in case it would hit the block gas limit.
+     *      In case `end` is greater than the ilk registry length, the iteration will be automatically capped.
+     * @param start The index to start the iteration (inclusive).
+     * @param end The index to stop the iteration (inclusive).
+     */
+    function emergencyActionsInBatch(uint256 start, uint256 end) external {
+        end = end > ilkList.length - 1 ? ilkList.length - 1 : end;
+        require(start <= end, "DssGroupedEmergencySpell/bad-iteration");
+
+        for (uint256 i = start; i <= end; i++) {
+            _emergencyActions(ilkList[i]);
         }
     }
 
@@ -120,9 +120,9 @@ abstract contract DssBatchedEmergencySpell is DssEmergencySpell, DssBatchedEmerg
     /// @notice Returns whether the spell is done for all ilks or not.
     /// @return res Whether the spells is done or not.
     function done() external view returns (bool res) {
-        res = _done(_ilk0) && _done(_ilk1);
-        if (_totalIlks > 2) {
-            res = res && _done(_ilk2);
+        res = true;
+        for (uint256 i = 0; i < ilkList.length; i++) {
+            res = res && _done(ilkList[i]);
         }
     }
 

src/DssBatchedEmergencySpell.t.integration.sol → src/DssGroupedEmergencySpell.t.integration.sol

@@ -17,37 +17,39 @@ pragma solidity ^0.8.16;
 
 import {stdStorage, StdStorage} from "forge-std/Test.sol";
 import {DssTest, DssInstance, MCD, GodMode} from "dss-test/DssTest.sol";
-import {DssBatchedEmergencySpell} from "./DssBatchedEmergencySpell.sol";
+import {DssGroupedEmergencySpell} from "./DssGroupedEmergencySpell.sol";
 
-contract DssBatchedEmergencySpellImpl is DssBatchedEmergencySpell {
-    mapping(bytes32 => bool) internal _isDone;
+contract DssGroupedEmergencySpellImpl is DssGroupedEmergencySpell {
+    mapping(bytes32 => bool) public isDone;
 
     function setDone(bytes32 ilk, bool val) external {
-        _isDone[ilk] = val;
+        isDone[ilk] = val;
     }
 
     function _descriptionPrefix() internal pure override returns (string memory) {
-        return "Batched Emergency Spell:";
+        return "Grouped Emergency Spell:";
     }
 
     event EmergencyAction(bytes32 indexed ilk);
 
-    constructor(bytes32[] memory _ilks) DssBatchedEmergencySpell(_ilks) {}
+    constructor(bytes32[] memory _ilks) DssGroupedEmergencySpell(_ilks) {}
 
     function _emergencyActions(bytes32 ilk) internal override {
         emit EmergencyAction(ilk);
+        isDone[ilk] = true;
     }
 
     function _done(bytes32 ilk) internal view override returns (bool) {
-        return _isDone[ilk];
+        return isDone[ilk];
     }
 }
 
-contract DssBatchedEmergencySpellTest is DssTest {
+contract DssGroupedEmergencySpellTest is DssTest {
     address constant CHAINLOG = 0xdA0Ab1e0017DEbCd72Be8599041a2aa3bA7e740F;
     DssInstance dss;
-    DssBatchedEmergencySpellImpl spell2;
-    DssBatchedEmergencySpellImpl spell3;
+    DssGroupedEmergencySpellImpl spell2;
+    DssGroupedEmergencySpellImpl spell3;
+    DssGroupedEmergencySpellImpl spellN;
     address pause;
 
     function setUp() public {
@@ -60,17 +62,27 @@ contract DssBatchedEmergencySpellTest is DssTest {
         bytes32[] memory ilks2 = new bytes32[](2);
         ilks2[0] = "WSTETH-A";
         ilks2[1] = "WSTETH-B";
-        spell2 = new DssBatchedEmergencySpellImpl(ilks2);
+        spell2 = new DssGroupedEmergencySpellImpl(ilks2);
         bytes32[] memory ilks3 = new bytes32[](3);
         ilks3[0] = "ETH-A";
         ilks3[1] = "ETH-B";
         ilks3[2] = "ETH-C";
-        spell3 = new DssBatchedEmergencySpellImpl(ilks3);
+        spell3 = new DssGroupedEmergencySpellImpl(ilks3);
+        bytes32[] memory ilksN = new bytes32[](8);
+        ilksN[0] = "ETH-A";
+        ilksN[1] = "ETH-B";
+        ilksN[2] = "ETH-C";
+        ilksN[3] = "WSTETH-A";
+        ilksN[4] = "WSTETH-B";
+        ilksN[5] = "WBTC-A";
+        ilksN[6] = "WBTC-B";
+        ilksN[7] = "WBTC-C";
+        spellN = new DssGroupedEmergencySpellImpl(ilksN);
     }
 
     function testDescription() public view {
-        assertEq(spell2.description(), "Batched Emergency Spell: WSTETH-A, WSTETH-B");
-        assertEq(spell3.description(), "Batched Emergency Spell: ETH-A, ETH-B, ETH-C");
+        assertEq(spell2.description(), "Grouped Emergency Spell: WSTETH-A, WSTETH-B");
+        assertEq(spell3.description(), "Grouped Emergency Spell: ETH-A, ETH-B, ETH-C");
     }
 
     function testEmergencyActions() public {
@@ -87,6 +99,43 @@ contract DssBatchedEmergencySpellTest is DssTest {
         vm.expectEmit(true, true, true, true);
         emit EmergencyAction("ETH-C");
         spell3.schedule();
+
+        vm.expectEmit(true, true, true, true);
+        emit EmergencyAction("ETH-A");
+        vm.expectEmit(true, true, true, true);
+        emit EmergencyAction("ETH-B");
+        vm.expectEmit(true, true, true, true);
+        emit EmergencyAction("ETH-C");
+        vm.expectEmit(true, true, true, true);
+        emit EmergencyAction("WSTETH-A");
+        vm.expectEmit(true, true, true, true);
+        emit EmergencyAction("WSTETH-B");
+        vm.expectEmit(true, true, true, true);
+        emit EmergencyAction("WBTC-A");
+        vm.expectEmit(true, true, true, true);
+        emit EmergencyAction("WBTC-B");
+        vm.expectEmit(true, true, true, true);
+        emit EmergencyAction("WBTC-C");
+        spellN.schedule();
+    }
+
+    function testEmergencyActionsInBatches_Fuzz(uint256 batchSize) public {
+        uint256 count = spellN.ilks().length;
+        batchSize = bound(batchSize, 1, count);
+        uint256 start = 0;
+        // End is inclusive, so we need to subtract 1
+        uint256 end = start + batchSize - 1;
+
+        assertFalse(spellN.done(), "spellN unexpectedly done");
+
+        while (start < count) {
+            spellN.emergencyActionsInBatch(start, end);
+
+            start += batchSize;
+            end += batchSize;
+        }
+
+        assertTrue(spellN.done(), "spellN not done");
     }
 
     function testDone() public {

src/clip-breaker/BatchedClipBreakerSpell.sol → src/clip-breaker/GroupedClipBreakerSpell.sol

@@ -15,7 +15,7 @@
 // along with this program.  If not, see <https://www.gnu.org/licenses/>.
 pragma solidity ^0.8.16;
 
-import {DssBatchedEmergencySpell} from "../DssBatchedEmergencySpell.sol";
+import {DssGroupedEmergencySpell} from "../DssGroupedEmergencySpell.sol";
 
 interface ClipperMomLike {
     function setBreaker(address clip, uint256 level, uint256 delay) external;
@@ -30,13 +30,13 @@ interface IlkRegistryLike {
     function xlip(bytes32 ilk) external view returns (address);
 }
 
-/// @title Emergency Spell: Batched Clip Breaker
+/// @title Emergency Spell: Grouped Clip Breaker
 /// @notice Prevents further collateral auctions to be held in the respective Clip contracts.
 /// @custom:authors [amusingaxl]
 /// @custom:reviewers []
 /// @custom:auditors []
 /// @custom:bounties []
-contract BatchedClipBreakerSpell is DssBatchedEmergencySpell {
+contract GroupedClipBreakerSpell is DssGroupedEmergencySpell {
     /// @notice The ClipperMom from chainlog.
     ClipperMomLike public immutable clipperMom = ClipperMomLike(_log.getAddress("CLIPPER_MOM"));
     /// @notice The IlkRegistry from chainlog.
@@ -54,19 +54,17 @@ contract BatchedClipBreakerSpell is DssBatchedEmergencySpell {
 
     /// @param _ilks The list of ilks for which the spell should be applicable
     /// @dev The list size is be at least 2 and less than or equal to 3.
-    ///      The batched spell is meant to be used for ilks that are a variation of tha same collateral gem
+    ///      The grouped spell is meant to be used for ilks that are a variation of tha same collateral gem
     ///      (i.e.: ETH-A, ETH-B, ETH-C)
-    ///      There has never been a case where MCD onboarded 4 or more ilks for the same collateral gem.
-    ///      For cases where there is only one ilk for the same collateral gem, use the single-ilk version.
-    constructor(bytes32[] memory _ilks) DssBatchedEmergencySpell(_ilks) {}
+    constructor(bytes32[] memory _ilks) DssGroupedEmergencySpell(_ilks) {}
 
-    /// @inheritdoc DssBatchedEmergencySpell
+    /// @inheritdoc DssGroupedEmergencySpell
     function _descriptionPrefix() internal pure override returns (string memory) {
-        return "Emergency Spell | Batched Clip Breaker:";
+        return "Emergency Spell | Grouped Clip Breaker:";
     }
 
     /// @notice Sets the breaker for the related Clip contract.
-    /// @inheritdoc DssBatchedEmergencySpell
+    /// @inheritdoc DssGroupedEmergencySpell
     function _emergencyActions(bytes32 _ilk) internal override {
         address clip = ilkReg.xlip(_ilk);
         clipperMom.setBreaker(clip, BREAKER_LEVEL, BREAKER_DELAY);
@@ -99,22 +97,22 @@ contract BatchedClipBreakerSpell is DssBatchedEmergencySpell {
     }
 }
 
-/// @title Emergency Spell Factory: Batched Clip Breaker
-/// @notice On-chain factory to deploy Batched Clip Breaker emergency spells.
+/// @title Emergency Spell Factory: Grouped Clip Breaker
+/// @notice On-chain factory to deploy Grouped Clip Breaker emergency spells.
 /// @custom:authors [amusingaxl]
 /// @custom:reviewers []
 /// @custom:auditors []
 /// @custom:bounties []
-contract BatchedClipBreakerFactory {
-    /// @notice A new BatchedClipBreakerSpell has been deployed.
+contract GroupedClipBreakerFactory {
+    /// @notice A new GroupedClipBreakerSpell has been deployed.
     /// @param ilks The list of ilks for which the spell is applicable.
     /// @param spell The deployed spell address.
     event Deploy(bytes32[] indexed ilks, address spell);
 
-    /// @notice Deploys a BatchedClipBreakerSpell contract.
+    /// @notice Deploys a GroupedClipBreakerSpell contract.
     /// @param ilks The list of ilks for which the spell is applicable.
     function deploy(bytes32[] memory ilks) external returns (address spell) {
-        spell = address(new BatchedClipBreakerSpell(ilks));
+        spell = address(new GroupedClipBreakerSpell(ilks));
         emit Deploy(ilks, spell);
     }
 }