Refactored to upgradeable contracts (1/2) (Licensing, Modules)

.github/workflows/test.yml

@@ -50,7 +50,8 @@ jobs:
 
       - name: Run Forge tests
         run: |
-          forge test -v --fork-url https://gateway.tenderly.co/public/sepolia --fork-block-number 5196000
+          forge test -vvv --fork-url https://gateway.tenderly.co/public/sepolia --fork-block-number 5196000
+        id: forge-test
 
       - name: Run solhint
         run: npx solhint contracts/**/*.sol

contracts/AccessController.sol

@@ -9,7 +9,8 @@ import { IPAccountChecker } from "./lib/registries/IPAccountChecker.sol";
 import { IIPAccount } from "./interfaces/IIPAccount.sol";
 import { AccessPermission } from "./lib/AccessPermission.sol";
 import { Errors } from "./lib/Errors.sol";
-import { Governable } from "./governance/Governable.sol";
+import { GovernableUpgradeable } from "./governance/GovernableUpgradeable.sol";
+import { UUPSUpgradeable } from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";
 
 /// @title AccessController
 /// @dev This contract is used to control access permissions for different function calls in the protocol.
@@ -26,29 +27,45 @@ import { Governable } from "./governance/Governable.sol";
 /// - setPermission: Sets the permission for a specific function call.
 /// - getPermission: Returns the permission level for a specific function call.
 /// - checkPermission: Checks if a specific function call is allowed.
-contract AccessController is IAccessController, Governable {
+contract AccessController is IAccessController, GovernableUpgradeable, UUPSUpgradeable {
     using IPAccountChecker for IIPAccountRegistry;
 
-    address public IP_ACCOUNT_REGISTRY;
-    address public MODULE_REGISTRY;
-
-    /// @dev Tracks the permission granted to an encoded permission path, where the
+    /// @dev The storage struct of AccessController.
+    /// @param encodedPermissions tracks the permission granted to an encoded permission path, where the
     /// encoded permission path = keccak256(abi.encodePacked(ipAccount, signer, to, func))
-    mapping(bytes32 => uint8) internal encodedPermissions;
+    /// @notice The address of the IP Account Registry.
+    /// @notice The address of the Module Registry.
+    /// @custom:storage-location erc7201:story-protocol.AccessController
+    struct AccessControllerStorage {
+        mapping(bytes32 => uint8) encodedPermissions;
+        address ipAccountRegistry;
+        address moduleRegistry;
+    }
 
-    constructor(address governance) Governable(governance) {}
+    // keccak256(abi.encode(uint256(keccak256("story-protocol.AccessController")) - 1)) & ~bytes32(uint256(0xff));
+    bytes32 private constant AccessControllerStorageLocation =
+        0xe80df7f3a04d1e1a0b61a4a820184d4b4a2f8a6a808f315dbcc7b502f40b1800;
 
-    // TODO: Change the function name to not clash with potential proxy contract `initialize`.
-    // TODO: Only allow calling once.
-    /// @dev Initialize the Access Controller with the IP Account Registry and Module Registry addresses.
-    /// These are separated from the constructor, because we need to deploy the AccessController first for
-    /// to deploy many registry and module contracts, including the IP Account Registry and Module Registry.
-    /// @dev Enforced to be only callable by the protocol admin in governance.
-    /// @param ipAccountRegistry The address of the IP Account Registry.
-    /// @param moduleRegistry The address of the Module Registry.
-    function initialize(address ipAccountRegistry, address moduleRegistry) external onlyProtocolAdmin {
-        IP_ACCOUNT_REGISTRY = ipAccountRegistry;
-        MODULE_REGISTRY = moduleRegistry;
+    /// Constructor
+    /// @custom:oz-upgrades-unsafe-allow constructor
+    constructor() {
+        _disableInitializers();
+    }
+
+    /// @notice Initializes implementation contract
+    /// @param governance The address of the governance contract
+    function initialize(address governance) external initializer {
+        __GovernableUpgradeable_init(governance);
+    }
+
+    /// @notice Sets the addresses of the IP account registry and the module registry
+    /// @dev TODO: figure out how to set these addresses in the constructor to make them immutable
+    /// @param ipAccountRegistry address of the IP account registry
+    /// @param moduleRegistry address of the module registry
+    function setAddresses(address ipAccountRegistry, address moduleRegistry) external onlyProtocolAdmin {
+        AccessControllerStorage storage $ = _getAccessControllerStorage();
+        $.ipAccountRegistry = ipAccountRegistry;
+        $.moduleRegistry = moduleRegistry;
     }
 
     /// @notice Sets a batch of permissions in a single transaction.
@@ -115,14 +132,15 @@ contract AccessController is IAccessController, Governable {
         if (signer == address(0)) {
             revert Errors.AccessController__SignerIsZeroAddress();
         }
-        if (!IIPAccountRegistry(IP_ACCOUNT_REGISTRY).isIpAccount(ipAccount)) {
+        AccessControllerStorage storage $ = _getAccessControllerStorage();
+        if (!IIPAccountRegistry($.ipAccountRegistry).isIpAccount(ipAccount)) {
             revert Errors.AccessController__IPAccountIsNotValid(ipAccount);
         }
         // permission must be one of ABSTAIN, ALLOW, DENY
         if (permission > 2) {
             revert Errors.AccessController__PermissionIsNotValid();
         }
-        if (!IModuleRegistry(MODULE_REGISTRY).isRegistered(msg.sender) && ipAccount != msg.sender) {
+        if (!IModuleRegistry($.moduleRegistry).isRegistered(msg.sender) && ipAccount != msg.sender) {
             revert Errors.AccessController__CallerIsNotIPAccount();
         }
         _setPermission(ipAccount, signer, to, func, permission);
@@ -144,15 +162,16 @@ contract AccessController is IAccessController, Governable {
         // The ipAccount is restricted to interact exclusively with registered modules.
         // This includes initiating calls to these modules and receiving calls from them.
         // Additionally, it can modify Permissions settings.
+        AccessControllerStorage storage $ = _getAccessControllerStorage();
         if (
             to != address(this) &&
-            !IModuleRegistry(MODULE_REGISTRY).isRegistered(to) &&
-            !IModuleRegistry(MODULE_REGISTRY).isRegistered(signer)
+            !IModuleRegistry($.moduleRegistry).isRegistered(to) &&
+            !IModuleRegistry($.moduleRegistry).isRegistered(signer)
         ) {
             revert Errors.AccessController__BothCallerAndRecipientAreNotRegisteredModule(signer, to);
         }
         // Must be a valid IPAccount
-        if (!IIPAccountRegistry(IP_ACCOUNT_REGISTRY).isIpAccount(ipAccount)) {
+        if (!IIPAccountRegistry($.ipAccountRegistry).isIpAccount(ipAccount)) {
             revert Errors.AccessController__IPAccountIsNotValid(ipAccount);
         }
         // Owner can call all functions of all modules
@@ -196,12 +215,14 @@ contract AccessController is IAccessController, Governable {
     /// @param func The function selector of `to` that can be called by the `signer` on behalf of the `ipAccount`
     /// @return permission The current permission level for the function call on `to` by the `signer` for `ipAccount`
     function getPermission(address ipAccount, address signer, address to, bytes4 func) public view returns (uint8) {
-        return encodedPermissions[_encodePermission(ipAccount, signer, to, func)];
+        AccessControllerStorage storage $ = _getAccessControllerStorage();
+        return $.encodedPermissions[_encodePermission(ipAccount, signer, to, func)];
     }
 
     /// @dev The permission parameters will be encoded into bytes32 as key in the permissions mapping to save storage
     function _setPermission(address ipAccount, address signer, address to, bytes4 func, uint8 permission) internal {
-        encodedPermissions[_encodePermission(ipAccount, signer, to, func)] = permission;
+        AccessControllerStorage storage $ = _getAccessControllerStorage();
+        $.encodedPermissions[_encodePermission(ipAccount, signer, to, func)] = permission;
     }
 
     /// @dev encode permission to hash (bytes32)
@@ -216,4 +237,15 @@ contract AccessController is IAccessController, Governable {
         }
         return keccak256(abi.encode(IIPAccount(payable(ipAccount)).owner(), ipAccount, signer, to, func));
     }
+
+    /// @dev Returns the storage struct of AccessController.
+    function _getAccessControllerStorage() private pure returns (AccessControllerStorage storage $) {
+        assembly {
+            $.slot := AccessControllerStorageLocation
+        }
+    }
+
+    /// @dev Hook to authorize the upgrade according to UUPSUgradeable
+    /// @param newImplementation The address of the new implementation
+    function _authorizeUpgrade(address newImplementation) internal override onlyProtocolAdmin {}
 }

contracts/access/AccessControlled.sol

@@ -17,20 +17,12 @@ abstract contract AccessControlled {
     using IPAccountChecker for IIPAccountRegistry;
 
     /// @notice The IAccessController instance for permission checks.
+    /// @custom:oz-upgrades-unsafe-allow state-variable-immutable
     IAccessController public immutable ACCESS_CONTROLLER;
     /// @notice The IIPAccountRegistry instance for IP account verification.
+    /// @custom:oz-upgrades-unsafe-allow state-variable-immutable
     IIPAccountRegistry public immutable IP_ACCOUNT_REGISTRY;
 
-    /// @dev Initializes the contract by setting the ACCESS_CONTROLLER and IP_ACCOUNT_REGISTRY addresses.
-    /// @param accessController The address of the AccessController contract.
-    /// @param ipAccountRegistry The address of the IPAccountRegistry contract.
-    constructor(address accessController, address ipAccountRegistry) {
-        if (accessController == address(0)) revert Errors.AccessControlled__ZeroAddress();
-        if (ipAccountRegistry == address(0)) revert Errors.AccessControlled__ZeroAddress();
-        ACCESS_CONTROLLER = IAccessController(accessController);
-        IP_ACCOUNT_REGISTRY = IIPAccountRegistry(ipAccountRegistry);
-    }
-
     /// @notice Verifies that the caller has the necessary permission for the given IPAccount.
     /// @dev Modifier that calls _verifyPermission to check if the provided IP account has the required permission.
     /// modules can use this modifier to check if the caller has the necessary permission.
@@ -51,6 +43,17 @@ abstract contract AccessControlled {
         _;
     }
 
+    /// @dev Constructor contract by setting the ACCESS_CONTROLLER and IP_ACCOUNT_REGISTRY addresses.
+    /// @param accessController The address of the AccessController contract.
+    /// @param ipAccountRegistry The address of the IPAccountRegistry contract.
+    /// @custom:oz-upgrades-unsafe-allow constructor
+    constructor(address accessController, address ipAccountRegistry) {
+        if (accessController == address(0)) revert Errors.AccessControlled__ZeroAddress();
+        if (ipAccountRegistry == address(0)) revert Errors.AccessControlled__ZeroAddress();
+        ACCESS_CONTROLLER = IAccessController(accessController);
+        IP_ACCOUNT_REGISTRY = IIPAccountRegistry(ipAccountRegistry);
+    }
+
     /// @dev Internal function to verify if the caller (msg.sender) has the required permission to execute
     /// the function on provided ipAccount.
     /// @param ipAccount The address of the IP account to verify.

contracts/governance/GovernableUpgradeable.sol

@@ -12,8 +12,9 @@ import { GovernanceLib } from "../lib/GovernanceLib.sol";
 /// @title Governable
 /// @dev All contracts managed by governance should inherit from this contract.
 abstract contract GovernableUpgradeable is IGovernable, Initializable {
-    /// @custom:storage-location erc7201:story-protocol.GovernableUpgradeable
+    /// @dev Storage for GovernableUpgradeable
     /// @param governance The address of the governance.
+    /// @custom:storage-location erc7201:story-protocol.GovernableUpgradeable
     struct GovernableUpgradeableStorage {
         address governance;
     }
@@ -44,7 +45,7 @@ abstract contract GovernableUpgradeable is IGovernable, Initializable {
         _disableInitializers();
     }
 
-    function __GovernableUpgradeable_init(address governance_) internal {
+    function __GovernableUpgradeable_init(address governance_) internal onlyInitializing {
         if (governance_ == address(0)) revert Errors.Governance__ZeroAddress();
         _getGovernableUpgradeableStorage().governance = governance_;
         emit GovernanceUpdated(governance_);

contracts/lib/Errors.sol

@@ -171,10 +171,10 @@ library Errors {
     error LicensingModule__LinkingRevokedLicense();
 
     ////////////////////////////////////////////////////////////////////////////
-    //                        LicensingModuleAware                            //
+    //                      BasePolicyFrameworkManager                        //
     ////////////////////////////////////////////////////////////////////////////
 
-    error LicensingModuleAware__CallerNotLicensingModule();
+    error BasePolicyFrameworkManager__CallerNotLicensingModule();
 
     ////////////////////////////////////////////////////////////////////////////
     //                         PolicyFrameworkManager                         //

contracts/lib/registries/IPAccountChecker.sol

@@ -23,7 +23,7 @@ library IPAccountChecker {
         uint256 chainId_,
         address tokenContract_,
         uint256 tokenId_
-    ) external view returns (bool) {
+    ) internal view returns (bool) {
         return ipAccountRegistry_.ipAccount(chainId_, tokenContract_, tokenId_).code.length != 0;
     }
 
@@ -34,7 +34,7 @@ library IPAccountChecker {
     function isIpAccount(
         IIPAccountRegistry ipAccountRegistry_,
         address ipAccountAddress_
-    ) external view returns (bool) {
+    ) internal view returns (bool) {
         if (ipAccountAddress_ == address(0)) return false;
         if (ipAccountAddress_.code.length == 0) return false;
         if (!ERC165Checker.supportsERC165(ipAccountAddress_)) return false;

contracts/modules/licensing/BasePolicyFrameworkManager.sol

@@ -4,26 +4,82 @@ pragma solidity 0.8.23;
 // external
 import { ERC165 } from "@openzeppelin/contracts/utils/introspection/ERC165.sol";
 import { IERC165 } from "@openzeppelin/contracts/utils/introspection/IERC165.sol";
+import { Initializable } from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
 // contracts
 import { IPolicyFrameworkManager } from "../../interfaces/modules/licensing/IPolicyFrameworkManager.sol";
-import { LicensingModuleAware } from "../../modules/licensing/LicensingModuleAware.sol";
+import { ILicensingModule } from "../../interfaces/modules/licensing/ILicensingModule.sol";
+import { Errors } from "../../lib/Errors.sol";
 
 /// @title BasePolicyFrameworkManager
+/// TODO: If we want to open this, we need an upgradeable and non-upgradeable Base version, or just promote
+/// the IPolicyFrameworkManager in the docs.
 /// @notice Base contract for policy framework managers.
-abstract contract BasePolicyFrameworkManager is IPolicyFrameworkManager, ERC165, LicensingModuleAware {
-    /// @notice Returns the name to be show in license NFT (LNFT) metadata
-    string public override name;
+abstract contract BasePolicyFrameworkManager is IPolicyFrameworkManager, ERC165, Initializable {
+    /// @dev Storage for BasePolicyFrameworkManager
+    /// @param name The name of the policy framework manager
+    /// @param licenseTextUrl The URL to the off chain legal agreement template text
+    /// @custom:storage-location erc7201:story-protocol.BasePolicyFrameworkManager
+    struct BasePolicyFrameworkManagerStorage {
+        string name;
+        string licenseTextUrl;
+    }
 
-    /// @notice Returns the URL to the off chain legal agreement template text
-    string public override licenseTextUrl;
+    // keccak256(abi.encode(uint256(keccak256("story-protocol.BasePolicyFrameworkManager")) - 1))
+    // & ~bytes32(uint256(0xff));
+    bytes32 private constant BasePolicyFrameworkManagerStorageLocation =
+        0xa55803740ac9329334ad7b6cde0ec056cc3ba32125b59c579552512bed001f00;
+
+    /// @custom:oz-upgrades-unsafe-allow state-variable-immutable
+    ILicensingModule public immutable LICENSING_MODULE;
+
+    /// @notice Modifier for authorizing the calling entity to only the LicensingModule.
+    modifier onlyLicensingModule() {
+        if (msg.sender != address(LICENSING_MODULE)) {
+            revert Errors.BasePolicyFrameworkManager__CallerNotLicensingModule();
+        }
+        _;
+    }
+
+    /// @notice Constructor function
+    /// @param licensing The address of the LicensingModule
+    /// @custom:oz-upgrades-unsafe-allow constructor
+    constructor(address licensing) {
+        LICENSING_MODULE = ILicensingModule(licensing);
+    }
 
-    constructor(address licensing, string memory name_, string memory licenseTextUrl_) LicensingModuleAware(licensing) {
-        name = name_;
-        licenseTextUrl = licenseTextUrl_;
+    /// @notice Initializes the BasePolicyFrameworkManager contract as per the Initializable contract.
+    /// @param _name The name of the policy framework manager
+    /// @param _licenseTextUrl The URL to the off chain legal agreement template text
+    function __BasePolicyFrameworkManager_init(
+        string memory _name,
+        string memory _licenseTextUrl
+    ) internal onlyInitializing {
+        _getBasePolicyFrameworkManagerStorage().name = _name;
+        _getBasePolicyFrameworkManagerStorage().licenseTextUrl = _licenseTextUrl;
+    }
+
+    /// @notice Returns the name of the policy framework manager
+    function name() public view override returns (string memory) {
+        return _getBasePolicyFrameworkManagerStorage().name;
+    }
+
+    /// @notice Returns the URL to the off chain legal agreement template text
+    function licenseTextUrl() public view override returns (string memory) {
+        return _getBasePolicyFrameworkManagerStorage().licenseTextUrl;
     }
 
     /// @notice IERC165 interface support.
     function supportsInterface(bytes4 interfaceId) public view virtual override(ERC165, IERC165) returns (bool) {
         return interfaceId == type(IPolicyFrameworkManager).interfaceId || super.supportsInterface(interfaceId);
     }
+
+    function _getBasePolicyFrameworkManagerStorage()
+        internal
+        pure
+        returns (BasePolicyFrameworkManagerStorage storage $)
+    {
+        assembly {
+            $.slot := BasePolicyFrameworkManagerStorageLocation
+        }
+    }
 }