fix loupe data type and uninstall success value

erc6900 · Aug 1, 2024 · a118f31 · a118f31
1 parent e1a0783
commit a118f31
Show file tree

Hide file tree

Showing 5 changed files with 31 additions and 15 deletions.
diff --git a/src/account/AccountLoupe.sol b/src/account/AccountLoupe.sol
@@ -32,7 +32,12 @@ abstract contract AccountLoupe is IAccountLoupe {
             data.module = executionData.module;
             data.isPublic = executionData.isPublic;
             data.allowGlobalValidation = executionData.allowGlobalValidation;
-            data.executionHooks = executionData.executionHooks.values();
+
+            uint256 executionHooksLen = executionData.executionHooks.length();
+            data.executionHooks = new HookConfig[](executionHooksLen);
+            for (uint256 i = 0; i < executionHooksLen; ++i) {
+                data.executionHooks[i] = HookConfig.wrap(bytes26(executionData.executionHooks.at(i)));
+            }
         }
     }
 
@@ -47,7 +52,18 @@ abstract contract AccountLoupe is IAccountLoupe {
         data.isGlobal = validationData.isGlobal;
         data.isSignatureValidation = validationData.isSignatureValidation;
         data.preValidationHooks = validationData.preValidationHooks;
-        data.permissionHooks = validationData.permissionHooks.values();
-        data.selectors = validationData.selectors.values();
+
+        uint256 permissionHooksLen = validationData.permissionHooks.length();
+        data.permissionHooks = new HookConfig[](permissionHooksLen);
+        for (uint256 i = 0; i < permissionHooksLen; ++i) {
+            data.permissionHooks[i] = HookConfig.wrap(bytes26(validationData.permissionHooks.at(i)));
+        }
+
+        bytes32[] memory selectors = validationData.selectors.values();
+        uint256 selectorsLen = selectors.length;
+        data.selectors = new bytes4[](selectorsLen);
+        for (uint256 j = 0; j < selectorsLen; ++j) {
+            data.selectors[j] = bytes4(selectors[j]);
+        }
     }
 }
diff --git a/src/account/ModuleManagerInternals.sol b/src/account/ModuleManagerInternals.sol
@@ -307,15 +307,15 @@ abstract contract ModuleManagerInternals is IModuleManager {
             for (uint256 i = 0; i < _validationData.preValidationHooks.length; ++i) {
                 bytes calldata hookData = hookUninstallDatas[hookIndex];
                 (address hookModule,) = ModuleEntityLib.unpack(_validationData.preValidationHooks[i]);
-                onUninstallSuccess = _onUninstall(hookModule, hookData);
+                onUninstallSuccess = onUninstallSuccess && _onUninstall(hookModule, hookData);
                 hookIndex++;
             }
 
             for (uint256 i = 0; i < _validationData.permissionHooks.length(); ++i) {
                 bytes calldata hookData = hookUninstallDatas[hookIndex];
                 (address hookModule,) =
                     ModuleEntityLib.unpack(toModuleEntity(_validationData.permissionHooks.at(i)));
-                onUninstallSuccess = _onUninstall(hookModule, hookData);
+                onUninstallSuccess = onUninstallSuccess && _onUninstall(hookModule, hookData);
                 hookIndex++;
             }
         }
@@ -338,7 +338,7 @@ abstract contract ModuleManagerInternals is IModuleManager {
         }
 
         (address module,) = ModuleEntityLib.unpack(validationFunction);
-        onUninstallSuccess = _onUninstall(module, uninstallData);
+        onUninstallSuccess = onUninstallSuccess && _onUninstall(module, uninstallData);
 
         emit ValidationUninstalled(validationFunction, onUninstallSuccess);
     }

diff --git a/src/interfaces/IAccountLoupe.sol b/src/interfaces/IAccountLoupe.sol
@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: CC0-1.0
 pragma solidity ^0.8.25;
 
-import {ModuleEntity} from "../interfaces/IModuleManager.sol";
+import {HookConfig, ModuleEntity} from "../interfaces/IModuleManager.sol";
 
 // Represents data associated with a specifc function selector.
 struct ExecutionDataView {
@@ -16,7 +16,7 @@ struct ExecutionDataView {
     // Whether or not a global validation function may be used to validate this function.
     bool allowGlobalValidation;
     // The execution hooks for this function selector.
-    bytes32[] executionHooks;
+    HookConfig[] executionHooks;
 }
 
 struct ValidationDataView {
@@ -27,9 +27,9 @@ struct ValidationDataView {
     // The pre validation hooks for this validation function.
     ModuleEntity[] preValidationHooks;
     // Permission hooks for this validation function.
-    bytes32[] permissionHooks;
+    HookConfig[] permissionHooks;
     // The set of selectors that may be validated by this validation function.
-    bytes32[] selectors;
+    bytes4[] selectors;
 }
 
 interface IAccountLoupe {

diff --git a/test/account/AccountLoupe.t.sol b/test/account/AccountLoupe.t.sol
@@ -91,14 +91,14 @@ contract AccountLoupeTest is CustomValidationTestBase {
 
             assertEq(data.executionHooks.length, 3);
             for (uint256 j = 0; j < data.executionHooks.length; j++) {
-                assertEq(data.executionHooks[j], bytes32(HookConfig.unwrap(expectedHooks[j])));
+                assertEq(HookConfig.unwrap(data.executionHooks[j]), HookConfig.unwrap(expectedHooks[j]));
             }
         }
     }
 
     function test_moduleLoupe_getValidationData() public {
         ValidationDataView memory data = account1.getValidationData(comprehensiveModuleValidation);
-        bytes32[] memory selectors = data.selectors;
+        bytes4[] memory selectors = data.selectors;
 
         assertTrue(data.isGlobal);
         assertTrue(data.isSignatureValidation);
@@ -122,7 +122,7 @@ contract AccountLoupeTest is CustomValidationTestBase {
 
         assertEq(data.permissionHooks.length, 0);
         assertEq(selectors.length, 1);
-        assertEq(selectors[0], bytes32(comprehensiveModule.foo.selector));
+        assertEq(selectors[0], comprehensiveModule.foo.selector);
     }
 
     // Test config

diff --git a/test/account/MultiValidation.t.sol b/test/account/MultiValidation.t.sol
@@ -46,8 +46,8 @@ contract MultiValidationTest is AccountTestBase {
         validations[0] = ModuleEntityLib.pack(address(singleSignerValidation), TEST_DEFAULT_VALIDATION_ENTITY_ID);
         validations[1] = ModuleEntityLib.pack(address(validator2), TEST_DEFAULT_VALIDATION_ENTITY_ID);
 
-        bytes32[] memory selectors0 = account1.getValidationData(validations[0]).selectors;
-        bytes32[] memory selectors1 = account1.getValidationData(validations[1]).selectors;
+        bytes4[] memory selectors0 = account1.getValidationData(validations[0]).selectors;
+        bytes4[] memory selectors1 = account1.getValidationData(validations[1]).selectors;
         assertEq(selectors0.length, selectors1.length);
         for (uint256 i = 0; i < selectors0.length; i++) {
             assertEq(selectors0[i], selectors1[i]);