From f4b42d5e7ae99f16681b5f36b0cb092dc3d906ea Mon Sep 17 00:00:00 2001
From: Alan Donoso Naumczuk <adonoso@itba.edu.ar>
Date: Tue, 14 Jan 2025 16:03:15 -0300
Subject: [PATCH] misc: Better error throwing during enable/disable rule
 selectors

---
 contracts/core/libraries/RulesLib.sol | 26 +++++++++++---------------
 contracts/core/types/Errors.sol       |  1 +
 2 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/contracts/core/libraries/RulesLib.sol b/contracts/core/libraries/RulesLib.sol
index 1554751..389b5ae 100644
--- a/contracts/core/libraries/RulesLib.sol
+++ b/contracts/core/libraries/RulesLib.sol
@@ -59,22 +59,27 @@ library RulesLib {
         bytes4 ruleSelector
     ) internal {
         require(rulesStorage.isConfigured[ruleAddress][configSalt], Errors.RuleNotConfigured());
-        require(
-            !_isSelectorAlreadyEnabled(rulesStorage, ruleSelector, ruleAddress, configSalt),
-            Errors.RedundantStateChange()
-        );
+        if (rulesStorage.ruleStates[ruleSelector][ruleAddress][configSalt].isEnabled) {
+            if (rulesStorage.ruleStates[ruleSelector][ruleAddress][configSalt].isRequired == isRequired) {
+                revert Errors.RedundantStateChange();
+            } else {
+                revert Errors.SelectorEnabledForDifferentRuleType();
+            }
+        }
         _addRuleSelectorToStorage(rulesStorage, ruleSelector, ruleAddress, configSalt, isRequired);
     }
 
     function disableRuleSelector(
         RulesStorage storage rulesStorage,
-        bool, /* isRequired */
+        bool isRequired,
         address ruleAddress,
         bytes32 configSalt,
         bytes4 ruleSelector
     ) internal {
+        require(rulesStorage.ruleStates[ruleSelector][ruleAddress][configSalt].isEnabled, Errors.RedundantStateChange());
         require(
-            _isSelectorAlreadyEnabled(rulesStorage, ruleSelector, ruleAddress, configSalt), Errors.RedundantStateChange()
+            rulesStorage.ruleStates[ruleSelector][ruleAddress][configSalt].isRequired == isRequired,
+            Errors.SelectorEnabledForDifferentRuleType()
         );
         _removeRuleSelectorFromStorage(rulesStorage, ruleSelector, ruleAddress, configSalt);
     }
@@ -148,13 +153,4 @@ library RulesLib {
         rules.pop();
         delete rulesStorage.ruleStates[ruleSelector][ruleAddress][configSalt];
     }
-
-    function _isSelectorAlreadyEnabled(
-        RulesStorage storage rulesStorage,
-        bytes4 ruleSelector,
-        address ruleAddress,
-        bytes32 configSalt
-    ) private view returns (bool) {
-        return rulesStorage.ruleStates[ruleSelector][ruleAddress][configSalt].isEnabled;
-    }
 }
diff --git a/contracts/core/types/Errors.sol b/contracts/core/types/Errors.sol
index 9465107..1c60ac6 100644
--- a/contracts/core/types/Errors.sol
+++ b/contracts/core/types/Errors.sol
@@ -36,6 +36,7 @@ library Errors {
     error RedundantStateChange();
     error RequiredRuleReverted();
     error RuleNotConfigured();
+    error SelectorEnabledForDifferentRuleType();
     error Self();
     error SingleAnyOfRule();
     error UnexpectedContractImpl();