chore: add events emision for validation install and uninstall

erc6900 · Jul 31, 2024 · b8dd333 · b8dd333
1 parent 2fadf6c
commit b8dd333
Show file tree

Hide file tree

Showing 4 changed files with 30 additions and 8 deletions.
diff --git a/src/account/ModuleManagerInternals.sol b/src/account/ModuleManagerInternals.sol
@@ -227,9 +227,15 @@ abstract contract ModuleManagerInternals is IModuleManager {
         }
     }
 
-    function _onUninstall(address module, bytes calldata data) internal {
+    function _onUninstall(address module, bytes calldata data) internal returns (bool onUninstallSuccess) {
+        onUninstallSuccess = true;
         if (data.length > 0) {
-            IModule(module).onUninstall(data);
+            // Clear the module storage for the account.
+            // solhint-disable-next-line no-empty-blocks
+            try IModule(module).onUninstall(data) {}
+            catch {
+                onUninstallSuccess = false;
+            }
         }
     }
 
@@ -241,6 +247,7 @@ abstract contract ModuleManagerInternals is IModuleManager {
     ) internal {
         ValidationData storage _validationData =
             getAccountStorage().validationData[validationConfig.moduleEntity()];
+        ModuleEntity moduleEntity = validationConfig.moduleEntity();
 
         for (uint256 i = 0; i < hooks.length; ++i) {
             HookConfig hookConfig = HookConfig.wrap(bytes26(hooks[i][:26]));
@@ -255,7 +262,7 @@ abstract contract ModuleManagerInternals is IModuleManager {
                 }
             } // Hook is an execution hook
             else if (!_validationData.permissionHooks.add(toSetValue(hookConfig))) {
-                revert PermissionAlreadySet(validationConfig.moduleEntity(), hookConfig);
+                revert PermissionAlreadySet(moduleEntity, hookConfig);
             }
 
             _onInstall(hookConfig.module(), hookData);
@@ -264,14 +271,15 @@ abstract contract ModuleManagerInternals is IModuleManager {
         for (uint256 i = 0; i < selectors.length; ++i) {
             bytes4 selector = selectors[i];
             if (!_validationData.selectors.add(toSetValue(selector))) {
-                revert ValidationAlreadySet(selector, validationConfig.moduleEntity());
+                revert ValidationAlreadySet(selector, moduleEntity);
             }
         }
 
         _validationData.isGlobal = validationConfig.isGlobal();
         _validationData.isSignatureValidation = validationConfig.isSignatureValidation();
 
         _onInstall(validationConfig.module(), installData);
+        emit ValidationInstalled(moduleEntity);
     }
 
     function _uninstallValidation(
@@ -280,6 +288,7 @@ abstract contract ModuleManagerInternals is IModuleManager {
         bytes[] calldata hookUninstallDatas
     ) internal {
         ValidationData storage _validationData = getAccountStorage().validationData[validationFunction];
+        bool onUninstallSuccess = true;
 
         _removeValidationFunction(validationFunction);
 
@@ -298,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]);
-                _onUninstall(hookModule, hookData);
+                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)));
-                _onUninstall(hookModule, hookData);
+                onUninstallSuccess = _onUninstall(hookModule, hookData);
                 hookIndex++;
             }
         }
@@ -329,6 +338,8 @@ abstract contract ModuleManagerInternals is IModuleManager {
         }
 
         (address module,) = ModuleEntityLib.unpack(validationFunction);
-        _onUninstall(module, uninstallData);
+        onUninstallSuccess = _onUninstall(module, uninstallData);
+
+        emit ValidationUninstalled(validationFunction, onUninstallSuccess);
     }
 }
diff --git a/src/interfaces/IModuleManager.sol b/src/interfaces/IModuleManager.sol
@@ -11,8 +11,9 @@ type HookConfig is bytes26;
 
 interface IModuleManager {
     event ModuleInstalled(address indexed module);
-
     event ModuleUninstalled(address indexed module, bool indexed onUninstallSucceeded);
+    event ValidationInstalled(ModuleEntity indexed moduleEntity);
+    event ValidationUninstalled(ModuleEntity indexed moduleEntity, bool indexed onUninstallSucceeded);
 
     /// @notice Install a module to the modular account.
     /// @param module The module to install.

diff --git a/test/account/DirectCallsFromModule.t.sol b/test/account/DirectCallsFromModule.t.sol
@@ -16,6 +16,8 @@ contract DirectCallsFromModuleTest is AccountTestBase {
     DirectCallModule internal _module;
     ModuleEntity internal _moduleEntity;
 
+    event ValidationUninstalled(ModuleEntity indexed moduleEntity, bool indexed onUninstallSucceeded);
+
     function setUp() public {
         _module = new DirectCallModule();
         assertFalse(_module.preHookRan());
@@ -124,6 +126,8 @@ contract DirectCallsFromModuleTest is AccountTestBase {
 
     function _uninstallExecution() internal {
         vm.prank(address(entryPoint));
+        vm.expectEmit(true, true, true, true);
+        emit ValidationUninstalled(_moduleEntity, true);
         account1.uninstallValidation(_moduleEntity, "", new bytes[](1));
     }
 

diff --git a/test/module/SingleSignerValidation.t.sol b/test/module/SingleSignerValidation.t.sol
@@ -7,6 +7,7 @@ import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/Messa
 import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol";
 import {ModuleEntityLib} from "../../src/helpers/ModuleEntityLib.sol";
 import {ValidationConfigLib} from "../../src/helpers/ValidationConfigLib.sol";
+import {ModuleEntity} from "../../src/interfaces/IModuleManager.sol";
 
 import {ContractOwner} from "../mocks/ContractOwner.sol";
 import {AccountTestBase} from "../utils/AccountTestBase.sol";
@@ -24,6 +25,8 @@ contract SingleSignerValidationTest is AccountTestBase {
 
     ContractOwner public contractOwner;
 
+    event ValidationInstalled(ModuleEntity indexed moduleEntity);
+
     function setUp() public {
         ethRecipient = makeAddr("ethRecipient");
         (owner2, owner2Key) = makeAddrAndKey("owner2");
@@ -79,6 +82,9 @@ contract SingleSignerValidationTest is AccountTestBase {
     function test_runtime_with2SameValidationInstalled() public {
         uint32 newEntityId = TEST_DEFAULT_VALIDATION_ENTITY_ID + 1;
         vm.prank(address(entryPoint));
+
+        vm.expectEmit(true, true, true, true);
+        emit ValidationInstalled(ModuleEntityLib.pack(address(singleSignerValidation), newEntityId));
         account.installValidation(
             ValidationConfigLib.pack(address(singleSignerValidation), newEntityId, true, false),
             new bytes4[](0),