feat: emit "SetPermission" event in "setPermission"

test: delete "selector" const in "setPermission" tests test: improve wording in "setPermission" test tree test: use named args in "setPermission" tests
PaulRBerg · Feb 13, 2023 · b277fd4 · b277fd4
1 parent 7d55262
commit b277fd4
Show file tree

Hide file tree

Showing 5 changed files with 85 additions and 13 deletions.
diff --git a/src/PRBProxy.sol b/src/PRBProxy.sol
@@ -132,6 +132,7 @@ contract PRBProxy is IPRBProxy {
         bool permission
     ) external override onlyOwner {
         permissions[envoy][target][selector] = permission;
+        emit SetPermission(envoy, target, selector, permission);
     }
 
     /// @inheritdoc IPRBProxy

diff --git a/src/interfaces/IPRBProxy.sol b/src/interfaces/IPRBProxy.sol
@@ -30,6 +30,8 @@ interface IPRBProxy {
 
     event Execute(address indexed target, bytes data, bytes response);
 
+    event SetPermission(address indexed envoy, address indexed target, bytes4 indexed selector, bool permission);
+
     event TransferOwnership(address indexed oldOwner, address indexed newOwner);
 
     /*//////////////////////////////////////////////////////////////////////////

diff --git a/test/prb-proxy/PRBProxy.t.sol b/test/prb-proxy/PRBProxy.t.sol
@@ -33,6 +33,8 @@ abstract contract PRBProxy_Test is Base_Test {
 
     event Execute(address indexed target, bytes data, bytes response);
 
+    event SetPermission(address indexed envoy, address indexed target, bytes4 indexed selector, bool permission);
+
     event TransferOwnership(address indexed oldOwner, address indexed newOwner);
 
     /*//////////////////////////////////////////////////////////////////////////

diff --git a/test/prb-proxy/set-permission/setPermission.t.sol b/test/prb-proxy/set-permission/setPermission.t.sol
@@ -6,8 +6,6 @@ import { PRBProxy_Test } from "../PRBProxy.t.sol";
 import { TargetDummy } from "../../helpers/targets/TargetDummy.t.sol";
 
 contract SetPermission_Test is PRBProxy_Test {
-    bytes4 internal constant selector = TargetDummy.foo.selector;
-
     /// @dev it should revert.
     function test_RevertWhen_CallerNotOwner() external {
         // Make Eve the caller in this test.
@@ -16,7 +14,12 @@ contract SetPermission_Test is PRBProxy_Test {
 
         // Run the test.
         vm.expectRevert(abi.encodeWithSelector(IPRBProxy.PRBProxy_NotOwner.selector, owner, caller));
-        proxy.setPermission(caller, address(targets.dummy), selector, true);
+        proxy.setPermission({
+            envoy: caller,
+            target: address(targets.dummy),
+            selector: TargetDummy.foo.selector,
+            permission: true
+        });
     }
 
     modifier callerOwner() {
@@ -25,27 +28,87 @@ contract SetPermission_Test is PRBProxy_Test {
 
     /// @dev it should set the permission.
     function test_SetPermission_PermissionNotSet() external callerOwner {
-        proxy.setPermission(users.envoy, address(targets.dummy), selector, true);
-        bool permission = proxy.getPermission(users.envoy, address(targets.dummy), selector);
+        proxy.setPermission({
+            envoy: users.envoy,
+            target: address(targets.dummy),
+            selector: TargetDummy.foo.selector,
+            permission: true
+        });
+        bool permission = proxy.getPermission({
+            envoy: users.envoy,
+            target: address(targets.dummy),
+            selector: TargetDummy.foo.selector
+        });
         assertTrue(permission);
     }
 
     modifier permissionSet() {
-        proxy.setPermission(users.envoy, address(targets.dummy), selector, true);
+        proxy.setPermission({
+            envoy: users.envoy,
+            target: address(targets.dummy),
+            selector: TargetDummy.foo.selector,
+            permission: true
+        });
         _;
     }
 
     /// @dev it should do nothing when re-setting the permission.
     function test_SetPermission_PermissionSet_ResetPermission() external callerOwner permissionSet {
-        proxy.setPermission(users.envoy, address(targets.dummy), selector, true);
-        bool permission = proxy.getPermission(users.envoy, address(targets.dummy), selector);
+        proxy.setPermission({
+            envoy: users.envoy,
+            target: address(targets.dummy),
+            selector: TargetDummy.foo.selector,
+            permission: true
+        });
+        bool permission = proxy.getPermission({
+            envoy: users.envoy,
+            target: address(targets.dummy),
+            selector: TargetDummy.foo.selector
+        });
         assertTrue(permission);
     }
 
+    /// @dev it should do nothing when re-setting the permission.
+    function test_SetPermission_PermissionSet_ResetPermission_Event() external callerOwner permissionSet {
+        vm.expectEmit({ checkTopic1: true, checkTopic2: true, checkTopic3: true, checkData: true });
+        emit SetPermission({
+            envoy: users.envoy,
+            target: address(targets.dummy),
+            selector: TargetDummy.foo.selector,
+            permission: true
+        });
+        proxy.setPermission({
+            envoy: users.envoy,
+            target: address(targets.dummy),
+            selector: TargetDummy.foo.selector,
+            permission: true
+        });
+    }
+
     /// @dev it should unset the permission.
     function test_SetPermission_PermissionSet_UnsetPermission() external callerOwner permissionSet {
-        proxy.setPermission(users.envoy, address(targets.dummy), selector, false);
-        bool permission = proxy.getPermission(users.envoy, address(targets.dummy), selector);
-        assertFalse(permission);
+        proxy.setPermission({
+            envoy: users.envoy,
+            target: address(targets.dummy),
+            selector: TargetDummy.foo.selector,
+            permission: false
+        });
+    }
+
+    /// @dev it should unset the permission.
+    function test_SetPermission_PermissionSet_UnsetPermission_Event() external callerOwner permissionSet {
+        vm.expectEmit({ checkTopic1: true, checkTopic2: true, checkTopic3: true, checkData: true });
+        emit SetPermission({
+            envoy: users.envoy,
+            target: address(targets.dummy),
+            selector: TargetDummy.foo.selector,
+            permission: false
+        });
+        proxy.setPermission({
+            envoy: users.envoy,
+            target: address(targets.dummy),
+            selector: TargetDummy.foo.selector,
+            permission: false
+        });
     }
 }
diff --git a/test/prb-proxy/set-permission/setPermission.tree b/test/prb-proxy/set-permission/setPermission.tree
@@ -5,5 +5,9 @@ setPermission.t.sol
    ├── when the permission was not set
    │  └── it should set the permission
    └── when the permission was set
-      ├── it should do nothing when re-setting the permission
-      └── it should unset the permission
+      ├── when resetting the permission
+      │  ├── it should reset the permission
+      │  └── it should emit a {SetPermission} event
+      └── when unsetting the permission
+         ├── it should unset the permission
+         └── it should emit a {SetPermission} event