Cleanup and Minimize Base Module Attributes (storyprotocol#81)

Refining the base module by cleaning up and minimizing its attributes.
kingster-will · Feb 7, 2024 · 2dc30e6 · 2dc30e6
1 parent 8088e4e
commit 2dc30e6
Show file tree

Hide file tree

Showing 14 changed files with 29 additions and 78 deletions.
diff --git a/contracts/modules/BaseModule.sol b/contracts/modules/BaseModule.sol
@@ -3,50 +3,21 @@
 pragma solidity ^0.8.23;
 
 import { IModule } from "../interfaces/modules/base/IModule.sol";
-import { IAccessController } from "../interfaces/IAccessController.sol";
+import { AccessControlled } from "../access/AccessControlled.sol";
 import { IPAssetRegistry } from "../registries/IPAssetRegistry.sol";
-import { LicenseRegistry } from "../registries/LicenseRegistry.sol";
-import { Errors } from "../lib/Errors.sol";
 
 /// @title BaseModule
 /// @notice Base implementation for all modules in Story Protocol. This is to
 ///         ensure all modules share the same authorization through the access
 ///         controll manager.
-abstract contract BaseModule is IModule {
-    /// @notice Gets the protocol-wide module access controller.
-    IAccessController public immutable ACCESS_CONTROLLER;
-
+abstract contract BaseModule is IModule, AccessControlled {
     /// @notice Gets the protocol-wide IP asset registry.
     IPAssetRegistry public immutable IP_ASSET_REGISTRY;
 
-    /// @notice Gets the protocol-wide license registry.
-    LicenseRegistry public immutable LICENSE_REGISTRY;
-
-    /// @notice Modifier for authorizing the calling entity.
-    modifier onlyAuthorized(address ipId) {
-        _authenticate(ipId);
-        _;
-    }
-
     /// @notice Initializes the base module contract.
-    /// @param controller The access controller used for IP authorization.
-    /// @param assetRegistry The address of the IP asset registry.
-    /// @param licenseRegistry The address of the license registry.
-    constructor(address controller, address assetRegistry, address licenseRegistry) {
-        // TODO: Add checks for interface support or at least zero address
-        ACCESS_CONTROLLER = IAccessController(controller);
-        IP_ASSET_REGISTRY = IPAssetRegistry(assetRegistry);
-        LICENSE_REGISTRY = LicenseRegistry(licenseRegistry);
-    }
-
-    /// @notice Gets the protocol string identifier associated with the module.
-    /// @return The string identifier of the module.
-    function name() public pure virtual override returns (string memory);
-
-    /// @notice Authenticates the caller entity through the access controller.
-    function _authenticate(address ipId) internal view {
-        try ACCESS_CONTROLLER.checkPermission(ipId, msg.sender, address(this), msg.sig) {} catch {
-            revert Errors.Module_Unauthorized();
-        }
+    /// @param accessController The access controller used for IP authorization.
+    /// @param ipAssetRegistry The address of the IP asset registry.
+    constructor(address accessController, address ipAssetRegistry) AccessControlled(accessController, ipAssetRegistry) {
+        IP_ASSET_REGISTRY = IPAssetRegistry(ipAssetRegistry);
     }
 }
diff --git a/contracts/modules/RegistrationModule.sol b/contracts/modules/RegistrationModule.sol
@@ -25,14 +25,12 @@ contract RegistrationModule is BaseModule, IRegistrationModule {
     /// @notice Initializes the registration module contract.
     /// @param controller The access controller used for IP authorization.
     /// @param assetRegistry The address of the IP asset registry.
-    /// @param licenseRegistry The address of the license module.
     constructor(
         address controller,
         address assetRegistry,
-        address licenseRegistry,
         address licensingModule,
         address resolverAddr
-    ) BaseModule(controller, assetRegistry, licenseRegistry) {
+    ) BaseModule(controller, assetRegistry) {
         resolver = IPResolver(resolverAddr);
         _LICENSING_MODULE = ILicensingModule(licensingModule);
     }

diff --git a/contracts/modules/dispute-module/DisputeModule.sol b/contracts/modules/dispute-module/DisputeModule.sol
@@ -54,14 +54,12 @@ contract DisputeModule is IDisputeModule, BaseModule, Governable, ReentrancyGuar
     /// @notice Initializes the registration module contract
     /// @param _controller The access controller used for IP authorization
     /// @param _assetRegistry The address of the IP asset registry
-    /// @param _licenseRegistry The address of the license registry
     /// @param _governance The address of the governance contract
     constructor(
         address _controller,
         address _assetRegistry,
-        address _licenseRegistry,
         address _governance
-    ) BaseModule(_controller, _assetRegistry, _licenseRegistry) Governable(_governance) {}
+    ) BaseModule(_controller, _assetRegistry) Governable(_governance) {}
 
     /// @notice Whitelists a dispute tag
     /// @param _tag The dispute tag
@@ -114,8 +112,7 @@ contract DisputeModule is IDisputeModule, BaseModule, Governable, ReentrancyGuar
     /// @notice Sets the arbitration policy for an ipId
     /// @param _ipId The ipId
     /// @param _arbitrationPolicy The address of the arbitration policy
-    function setArbitrationPolicy(address _ipId, address _arbitrationPolicy) external {
-        if (_ipId != msg.sender) _authenticate(_ipId);
+    function setArbitrationPolicy(address _ipId, address _arbitrationPolicy) external verifyPermission(_ipId) {
         if (!isWhitelistedArbitrationPolicy[_arbitrationPolicy]) revert Errors.DisputeModule__NotWhitelistedArbitrationPolicy();
 
         arbitrationPolicies[_ipId] = _arbitrationPolicy;

diff --git a/contracts/resolvers/IPResolver.sol b/contracts/resolvers/IPResolver.sol
@@ -3,7 +3,6 @@
 pragma solidity ^0.8.23;
 
 import { ResolverBase } from "./ResolverBase.sol";
-import { BaseModule } from "../modules/BaseModule.sol";
 import { IModule } from "../interfaces/modules/base/IModule.sol";
 import { KeyValueResolver } from "../resolvers/KeyValueResolver.sol";
 import { IP_RESOLVER_MODULE_KEY } from "../lib/modules/Module.sol";
@@ -17,12 +16,7 @@ contract IPResolver is KeyValueResolver {
     /// @notice Initializes the IP metadata resolver.
     /// @param accessController The access controller used for IP authorization.
     /// @param ipAssetRegistry The address of the IP record registry.
-    /// @param licenseRegistry The address of the license registry.
-    constructor(
-        address accessController,
-        address ipAssetRegistry,
-        address licenseRegistry
-    ) ResolverBase(accessController, ipAssetRegistry, licenseRegistry) {}
+    constructor(address accessController, address ipAssetRegistry) ResolverBase(accessController, ipAssetRegistry) {}
 
     /// @notice Checks whether the resolver interface is supported.
     /// @param id The resolver interface identifier.
@@ -32,7 +26,7 @@ contract IPResolver is KeyValueResolver {
     }
 
     /// @notice Gets the protocol-wide module identifier for this module.
-    function name() public pure override(BaseModule, IModule) returns (string memory) {
+    function name() public pure override(IModule) returns (string memory) {
         return IP_RESOLVER_MODULE_KEY;
     }
 }
diff --git a/contracts/resolvers/KeyValueResolver.sol b/contracts/resolvers/KeyValueResolver.sol
@@ -17,7 +17,7 @@ abstract contract KeyValueResolver is IKeyValueResolver, ResolverBase {
     /// @param ipId The canonical identifier of the IP asset.
     /// @param k The string parameter key to update.
     /// @param v The value to set for the specified key.
-    function setValue(address ipId, string calldata k, string calldata v) external virtual onlyAuthorized(ipId) {
+    function setValue(address ipId, string calldata k, string calldata v) external virtual verifyPermission(ipId) {
         _values[ipId][k] = v;
         emit KeyValueSet(ipId, k, v);
     }

diff --git a/contracts/resolvers/ResolverBase.sol b/contracts/resolvers/ResolverBase.sol
@@ -10,12 +10,7 @@ abstract contract ResolverBase is IResolver, BaseModule {
     /// @notice Initializes the base module contract.
     /// @param controller The access controller used for IP authorization.
     /// @param assetRegistry The address of the IP record registry.
-    /// @param licenseRegistry The address of the license registry.
-    constructor(
-        address controller,
-        address assetRegistry,
-        address licenseRegistry
-    ) BaseModule(controller, assetRegistry, licenseRegistry) {}
+    constructor(address controller, address assetRegistry) BaseModule(controller, assetRegistry) {}
 
     /// @notice Checks whether the resolver interface is supported.
     /// @param id The resolver interface identifier.

diff --git a/script/foundry/deployment/Main.s.sol b/script/foundry/deployment/Main.s.sol
@@ -204,15 +204,14 @@ contract Main is Script, BroadcastManager, JsonDeploymentHandler {
 
         contractKey = "IPResolver";
         _predeploy(contractKey);
-        ipResolver = new IPResolver(address(accessController), address(ipAssetRegistry), address(licenseRegistry));
+        ipResolver = new IPResolver(address(accessController), address(ipAssetRegistry));
         _postdeploy(contractKey, address(ipResolver));
 
         contractKey = "RegistrationModule";
         _predeploy(contractKey);
         registrationModule = new RegistrationModule(
             address(accessController),
             address(ipAssetRegistry),
-            address(licenseRegistry),
             address(licensingModule),
             address(ipResolver)
         );
@@ -223,7 +222,6 @@ contract Main is Script, BroadcastManager, JsonDeploymentHandler {
         disputeModule = new DisputeModule(
             address(accessController),
             address(ipAssetRegistry),
-            address(licenseRegistry),
             address(governance)
         );
         _postdeploy(contractKey, address(disputeModule));

diff --git a/test/foundry/integration/BaseIntegration.sol b/test/foundry/integration/BaseIntegration.sol
@@ -137,19 +137,17 @@ contract BaseIntegration is Test {
         );
         licenseRegistry.setLicensingModule(address(licensingModule));
         ipMetadataProvider = new IPMetadataProvider(address(moduleRegistry));
-        ipResolver = new IPResolver(address(accessController), address(ipAssetRegistry), address(licenseRegistry));
+        ipResolver = new IPResolver(address(accessController), address(ipAssetRegistry));
         registrationModule = new RegistrationModule(
             address(accessController),
             address(ipAssetRegistry),
-            address(licenseRegistry),
             address(licensingModule),
             address(ipResolver)
         );
         taggingModule = new TaggingModule();
         disputeModule = new DisputeModule(
             address(accessController),
             address(ipAssetRegistry),
-            address(licenseRegistry),
             address(governance)
         );
         ipAssetRenderer = new IPAssetRenderer(

diff --git a/test/foundry/modules/ModuleBase.t.sol b/test/foundry/modules/ModuleBase.t.sol
@@ -76,13 +76,11 @@ abstract contract ModuleBaseTest is BaseTest {
         licenseRegistry.setLicensingModule(address(licensingModule));
         IPResolver ipResolver = new IPResolver(
             address(accessController),
-            address(ipAssetRegistry),
-            address(licenseRegistry)
+            address(ipAssetRegistry)
         );
         RegistrationModule registrationModule = new RegistrationModule(
             address(accessController),
             address(ipAssetRegistry),
-            address(licenseRegistry),
             address(licensingModule),
             address(ipResolver)
         );

diff --git a/test/foundry/modules/dispute/DisputeModule.t.sol b/test/foundry/modules/dispute/DisputeModule.t.sol
@@ -191,7 +191,15 @@ contract TestDisputeModule is TestHelper {
     }
 
     function test_DisputeModule_setArbitrationPolicy_revert_UnauthorizedAccess() public {
-        vm.expectRevert(Errors.Module_Unauthorized.selector);
+        vm.expectRevert(
+            abi.encodeWithSelector(
+                Errors.AccessController__PermissionDenied.selector,
+                ipAddr,
+                address(this),
+                address(disputeModule),
+                disputeModule.setArbitrationPolicy.selector
+            )
+        );
         disputeModule.setArbitrationPolicy(ipAddr, address(arbitrationPolicySP2));
     }
 

diff --git a/test/foundry/registries/metadata/IPAssetRenderer.t.sol b/test/foundry/registries/metadata/IPAssetRenderer.t.sol
@@ -119,8 +119,7 @@ contract IPAssetRendererTest is BaseTest {
         licenseRegistry.setLicensingModule(address(licensingModule));
         resolver = new IPResolver(
             address(accessController),
-            address(ipAssetRegistry),
-            address(licenseRegistry)
+            address(ipAssetRegistry)
         );
         renderer = new IPAssetRenderer(
             address(ipAssetRegistry),
@@ -131,7 +130,6 @@ contract IPAssetRendererTest is BaseTest {
         registrationModule = new RegistrationModule(
             address(accessController),
             address(ipAssetRegistry),
-            address(licenseRegistry),
             address(licensingModule),
             address(resolver)
         );

diff --git a/test/foundry/registries/metadata/MetadataProvider.t.sol b/test/foundry/registries/metadata/MetadataProvider.t.sol
@@ -139,8 +139,7 @@ contract MetadataProviderTest is BaseTest {
         uint256 tokenId = erc721.mintId(alice, 99);
         IPResolver resolver = new IPResolver(
             address(accessController),
-            address(registry),
-            address(licenseRegistry)
+            address(registry)
         );
         ipId = registry.register(
             block.chainid,

diff --git a/test/foundry/resolvers/IPResolver.t.sol b/test/foundry/resolvers/IPResolver.t.sol
@@ -91,8 +91,7 @@ contract IPResolverTest is ResolverBaseTest {
         return address(
             new IPResolver(
                 address(accessController),
-                address(ipAssetRegistry),
-                address(licenseRegistry)
+                address(ipAssetRegistry)
             )
         );
     }

diff --git a/test/utils/DeployHelper.sol b/test/utils/DeployHelper.sol
@@ -143,19 +143,17 @@ contract DeployHelper is Test {
         );
         licenseRegistry.setLicensingModule(address(licensingModule));
         ipMetadataProvider = new IPMetadataProvider(address(moduleRegistry));
-        ipResolver = new IPResolver(address(accessController), address(ipAssetRegistry), address(licenseRegistry));
+        ipResolver = new IPResolver(address(accessController), address(ipAssetRegistry));
         registrationModule = new RegistrationModule(
             address(accessController),
             address(ipAssetRegistry),
-            address(licenseRegistry),
             address(licensingModule),
             address(ipResolver)
         );
         taggingModule = new TaggingModule();
         disputeModule = new DisputeModule(
             address(accessController),
             address(ipAssetRegistry),
-            address(licenseRegistry),
             address(governance)
         );
         ipAssetRenderer = new IPAssetRenderer(