Registration Module backward compatibility (storyprotocol#89)

* fix: caller check in IPAssetRegistry, governance setter

* test: constructor and setter fixes on test and script

* nit: husky prehook script

.husky/pre-push

@@ -3,4 +3,4 @@
 
 yarn lint:sol:fix
 # recheck
-yarn lint:sol
+yarn solhint

contracts/governance/Governance.sol

@@ -10,6 +10,7 @@ import { GovernanceLib } from "../lib/GovernanceLib.sol";
 
 /// @title Governance
 /// @dev This contract is used for governance of the protocol.
+/// TODO: Replace with OZ's 2StepOwnable
 contract Governance is AccessControl, IGovernance {
     GovernanceLib.ProtocolState internal state;
 

contracts/interfaces/modules/licensing/ILicensingModule.sol

@@ -60,7 +60,11 @@ interface ILicensingModule is IModule {
     function registerPolicy(bool isLicenseTransferable, bytes memory data) external returns (uint256 policyId);
 
     /// @notice returns the policy id for the given data, or 0 if not found
-    function getPolicyId(address framework, bool isLicenseTransferable, bytes memory data) external view returns (uint256 policyId);
+    function getPolicyId(
+        address framework,
+        bool isLicenseTransferable,
+        bytes memory data
+    ) external view returns (uint256 policyId);
 
     /// @notice Adds a policy to an IP policy list
     /// @param ipId The id of the IP

contracts/modules/RegistrationModule.sol

@@ -63,7 +63,8 @@ contract RegistrationModule is BaseModule, IRegistrationModule {
         // Check that the caller is authorized to perform the registration.
         // TODO: Perform additional registration authorization logic, allowing
         //       registrants or root-IP creators to specify their own auth logic.
-        if (IERC721(tokenContract).ownerOf(tokenId) != msg.sender) {
+        address owner = IERC721(tokenContract).ownerOf(tokenId);
+        if (msg.sender != owner && !IERC721(tokenContract).isApprovedForAll(owner, msg.sender)) {
             revert Errors.RegistrationModule__InvalidOwner();
         }
 
@@ -120,7 +121,8 @@ contract RegistrationModule is BaseModule, IRegistrationModule {
         // Check that the caller is authorized to perform the registration.
         // TODO: Perform additional registration authorization logic, allowing
         //       registrants or IP creators to specify their own auth logic.
-        if (IERC721(tokenContract).ownerOf(tokenId) != msg.sender) {
+        address owner = IERC721(tokenContract).ownerOf(tokenId);
+        if (msg.sender != owner && !IERC721(tokenContract).isApprovedForAll(owner, msg.sender)) {
             revert Errors.RegistrationModule__InvalidOwner();
         }
 

contracts/modules/licensing/LicensingModule.sol

@@ -314,7 +314,11 @@ contract LicensingModule is AccessControlled, ILicensingModule {
     }
 
     /// @notice gets the policy id for the given data, or 0 if not found
-    function getPolicyId(address framework, bool isLicenseTransferable, bytes memory data) external view returns (uint256 policyId) {
+    function getPolicyId(
+        address framework,
+        bool isLicenseTransferable,
+        bytes memory data
+    ) external view returns (uint256 policyId) {
         Licensing.Policy memory pol = Licensing.Policy({
             isLicenseTransferable: isLicenseTransferable,
             policyFramework: framework,
@@ -505,7 +509,6 @@ contract LicensingModule is AccessControlled, ILicensingModule {
         _ipIdParents[childIpId].add(licensor);
     }
 
-
     function _verifyCanAddPolicy(uint256 policyId, address ipId, bool isInherited) private {
         bool ipIdIsDerivative = _policySetPerIpId(true, ipId).length() > 0;
         if (

contracts/registries/IPAssetRegistry.sol

@@ -16,6 +16,8 @@ import { LICENSING_MODULE_KEY } from "contracts/lib/modules/Module.sol";
 import { IModuleRegistry } from "../interfaces/registries/IModuleRegistry.sol";
 import { ILicensingModule } from "../interfaces/modules/licensing/ILicensingModule.sol";
 import { IIPAssetRegistry } from "../interfaces/registries/IIPAssetRegistry.sol";
+import { IRegistrationModule } from "../interfaces/modules/IRegistrationModule.sol";
+import { Governable } from "../governance/Governable.sol";
 
 /// @title IP Asset Registry
 /// @notice This contract acts as the source of truth for all IP registered in
@@ -26,7 +28,7 @@ import { IIPAssetRegistry } from "../interfaces/registries/IIPAssetRegistry.sol"
 ///         attribution and an IP account for protocol authorization.
 ///         IMPORTANT: The IP account address, besides being used for protocol
 ///                    auth, is also the canonical IP identifier for the IP NFT.
-contract IPAssetRegistry is IIPAssetRegistry, IPAccountRegistry {
+contract IPAssetRegistry is IIPAssetRegistry, IPAccountRegistry, Governable {
     /// @notice Attributes for the IP asset type.
     struct Record {
         // Metadata provider for Story Protocol canonicalized metadata.
@@ -39,12 +41,12 @@ contract IPAssetRegistry is IIPAssetRegistry, IPAccountRegistry {
     /// @notice The canonical module registry used by the protocol.
     IModuleRegistry public immutable MODULE_REGISTRY;
 
+    /// @notice The registration module that interacts with IPAssetRegistry.
+    IRegistrationModule public REGISTRATION_MODULE;
+
     /// @notice Tracks the total number of IP assets in existence.
     uint256 public totalSupply = 0;
 
-    /// @notice Protocol governance administrator of the IP record registry.
-    address public owner;
-
     /// @notice Checks whether an operator is approved to register on behalf of an IP owner.
     mapping(address owner => mapping(address operator => bool)) public isApprovedForAll;
 
@@ -54,29 +56,20 @@ contract IPAssetRegistry is IIPAssetRegistry, IPAccountRegistry {
     /// @notice Tracks the current metadata provider used for IP registrations.
     IMetadataProviderMigratable internal _metadataProvider;
 
-    /// @notice Ensures only protocol governance owner may call a function.
-    modifier onlyOwner() {
-        if (msg.sender != owner) {
-            revert Errors.IPAssetRegistry__Unauthorized();
-        }
-        _;
-    }
-
     /// @notice Initializes the IP Asset Registry.
     /// @param erc6551Registry The address of the ERC6551 registry.
     /// @param accessController The address of the access controller.
     /// @param ipAccountImpl The address of the IP account implementation.
     /// @param moduleRegistry The address of the module registry.
+    /// @param governance The address of the governance contract.
     /// TODO: Utilize module registry for fetching different modules.
     constructor(
         address accessController,
         address erc6551Registry,
         address ipAccountImpl,
-        address moduleRegistry
-    ) IPAccountRegistry(erc6551Registry, accessController, ipAccountImpl) {
-        // TODO: Migrate this to a parameterized governance owner address.
-        // TODO: Replace with OZ's 2StepOwnable
-        owner = msg.sender;
+        address moduleRegistry,
+        address governance
+    ) IPAccountRegistry(erc6551Registry, accessController, ipAccountImpl) Governable(governance) {
         MODULE_REGISTRY = IModuleRegistry(moduleRegistry);
         _metadataProvider = IMetadataProviderMigratable(new MetadataProviderV1(address(this)));
     }
@@ -90,6 +83,12 @@ contract IPAssetRegistry is IIPAssetRegistry, IPAccountRegistry {
         emit ApprovalForAll(msg.sender, operator, approved);
     }
 
+    /// @notice Sets the registration module that interacts with IPAssetRegistry.
+    /// @param registrationModule The address of the registration module.
+    function setRegistrationModule(address registrationModule) external onlyProtocolAdmin {
+        REGISTRATION_MODULE = IRegistrationModule(registrationModule);
+    }
+
     /// @notice Registers an NFT as an IP asset.
     /// @param chainId The chain identifier of where the NFT resides.
     /// @param tokenContract The address of the NFT.
@@ -180,7 +179,7 @@ contract IPAssetRegistry is IIPAssetRegistry, IPAccountRegistry {
     /// @notice Sets the provider for storage of new IP metadata, while enabling
     ///         existing IP assets to migrate their metadata to the new provider.
     /// @param newMetadataProvider Address of the new metadata provider contract.
-    function setMetadataProvider(address newMetadataProvider) external onlyOwner {
+    function setMetadataProvider(address newMetadataProvider) external onlyProtocolAdmin {
         _metadataProvider.setUpgradeProvider(newMetadataProvider);
         _metadataProvider = IMetadataProviderMigratable(newMetadataProvider);
     }
@@ -238,8 +237,10 @@ contract IPAssetRegistry is IIPAssetRegistry, IPAccountRegistry {
             revert Errors.IPAssetRegistry__AlreadyRegistered();
         }
 
-        address owner = IERC721(tokenContract).ownerOf(tokenId);
-        if (msg.sender != owner && !isApprovedForAll[owner][msg.sender]) {
+        address _owner = IERC721(tokenContract).ownerOf(tokenId);
+        if (
+            msg.sender != _owner && msg.sender != address(REGISTRATION_MODULE) && !isApprovedForAll[_owner][msg.sender]
+        ) {
             revert Errors.IPAssetRegistry__RegistrantUnauthorized();
         }
 

package.json

@@ -14,6 +14,7 @@
     "lint:js:fix": "prettier --log-level warn --ignore-path .gitignore '**/*.{js,ts}' --write && eslint --ignore-path .gitignore . --fix",
     "lint:sol": "prettier --log-level warn --ignore-path .gitignore '{contracts,test}/**/*.sol' --check && solhint '{contracts,test}/**/*.sol'",
     "lint:sol:fix": "prettier --log-level warn --ignore-path .gitignore '{contracts,test}/**/*.sol' --write",
+    "solhint": "solhint '{contracts,test}/**/*.sol'",
     "test": "npx hardhat test",
     "prepare": "husky install"
   },

script/foundry/deployment/Main.s.sol

@@ -164,7 +164,13 @@ contract Main is Script, BroadcastManager, JsonDeploymentHandler {
 
         contractKey = "IPAssetRegistry";
         _predeploy(contractKey);
-        ipAssetRegistry = new IPAssetRegistry(address(accessController), ERC6551_REGISTRY, address(ipAccountImpl), address(moduleRegistry));
+        ipAssetRegistry = new IPAssetRegistry(
+            address(accessController),
+            ERC6551_REGISTRY,
+            address(ipAccountImpl),
+            address(moduleRegistry),
+            address(governance)
+        );
         _postdeploy(contractKey, address(ipAssetRegistry));
 
         contractKey = "MetadataProviderV1";
@@ -290,6 +296,7 @@ contract Main is Script, BroadcastManager, JsonDeploymentHandler {
         _postdeploy("IPMetadataProvider", address(ipMetadataProvider));
 
         licenseRegistry.setLicensingModule(address(licensingModule));
+        ipAssetRegistry.setRegistrationModule(address(registrationModule));
     }
 
     function _configureAccessController() private {

test/foundry/IPAssetRegistry.t.sol

@@ -69,7 +69,13 @@ contract IPAssetRegistryTest is BaseTest {
         erc6551Registry = address(new ERC6551Registry());
         ipAccountImpl = address(new IPAccountImpl());
         ipAccountRegistry = new IPAccountRegistry(erc6551Registry, accessController, ipAccountImpl);
-        registry = new IPAssetRegistry(accessController, erc6551Registry, ipAccountImpl, address(moduleRegistry));
+        registry = new IPAssetRegistry(
+            accessController,
+            erc6551Registry,
+            ipAccountImpl,
+            address(moduleRegistry),
+            address(governance)
+        );
         MockERC721 erc721 = new MockERC721("MockERC721");
         tokenAddress = address(erc721);
         tokenId = erc721.mintId(alice, 99);

test/foundry/integration/BaseIntegration.sol

@@ -123,7 +123,8 @@ contract BaseIntegration is Test {
             address(accessController),
             address(erc6551Registry),
             address(ipAccountImpl),
-            address(moduleRegistry)
+            address(moduleRegistry),
+            address(governance)
         );
         licenseRegistry = new LicenseRegistry();
         licensingModule = new LicensingModule(
@@ -173,6 +174,7 @@ contract BaseIntegration is Test {
         vm.startPrank(u.admin);
         accessController.initialize(address(ipAccountRegistry), address(moduleRegistry));
         royaltyModule.setLicensingModule(address(licensingModule));
+        ipAssetRegistry.setRegistrationModule(address(registrationModule));
 
         moduleRegistry.registerModule(REGISTRATION_MODULE_KEY, address(registrationModule));
         moduleRegistry.registerModule(IP_RESOLVER_MODULE_KEY, address(ipResolver));

test/foundry/mocks/licensing/MockLicensingModule.sol

@@ -37,7 +37,11 @@ contract MockLicensingModule is ILicensingModule {
 
     function isPolicyIdSetForIp(bool isInherited, address ipId, uint256 policyId) external view returns (bool) {}
 
-    function getPolicyId(address framework, bool isLicenseTransferable, bytes memory data) external view returns (uint256 policyId) {}
+    function getPolicyId(
+        address framework,
+        bool isLicenseTransferable,
+        bytes memory data
+    ) external view returns (uint256 policyId) {}
 
     function policyIdForIpAtIndex(
         bool isInherited,

test/foundry/modules/ModuleBase.t.sol

@@ -14,6 +14,8 @@ import { IPAssetRegistry } from "contracts/registries/IPAssetRegistry.sol";
 import { IPAccountImpl } from "contracts/IPAccountImpl.sol";
 import { Governance } from "contracts/governance/Governance.sol";
 import { RoyaltyModule } from "contracts/modules/royalty-module/RoyaltyModule.sol";
+import { IPResolver } from "contracts/resolvers/IPResolver.sol";
+import { RegistrationModule } from "contracts/modules/RegistrationModule.sol";
 
 /// @title Module Base Test Contract
 /// @notice Base contract for testing standard module functionality.
@@ -41,6 +43,10 @@ abstract contract ModuleBaseTest is BaseTest {
 
     RoyaltyModule public royaltyModule;
 
+    IPResolver public ipResolver;
+
+    RegistrationModule public registrationModule;
+
     /// @notice Initializes the base module for testing.
     function setUp() public virtual override(BaseTest) {
         BaseTest.setUp();
@@ -53,7 +59,8 @@ abstract contract ModuleBaseTest is BaseTest {
             address(accessController),
             address(new ERC6551Registry()),
             address(new IPAccountImpl()),
-            address(moduleRegistry)
+            address(moduleRegistry),
+            address(governance)
         );
         royaltyModule = new RoyaltyModule(address(governance));
         licenseRegistry = new LicenseRegistry();
@@ -63,10 +70,18 @@ abstract contract ModuleBaseTest is BaseTest {
             address(licenseRegistry),
             address(royaltyModule)
         );
+        ipResolver = new IPResolver(address(accessController), address(ipAssetRegistry));
+        registrationModule = new RegistrationModule(
+            address(accessController),
+            address(ipAssetRegistry),
+            address(licensingModule),
+            address(ipResolver)
+        );
         licenseRegistry.setLicensingModule(address(licensingModule));
         baseModule = IModule(_deployModule());
         accessController.initialize(address(ipAssetRegistry), address(moduleRegistry));
         royaltyModule.setLicensingModule(address(licensingModule));
+        ipAssetRegistry.setRegistrationModule(address(registrationModule));
     }
 
     /// @notice Tests that the default resolver constructor runs successfully.

test/foundry/modules/licensing/LicensingModule.t.sol

@@ -19,6 +19,8 @@ import { LicenseRegistry } from "contracts/registries/LicenseRegistry.sol";
 import { LicensingModule } from "contracts/modules/licensing/LicensingModule.sol";
 import { RoyaltyModule } from "contracts/modules/royalty-module/RoyaltyModule.sol";
 import { IPAssetRegistry } from "contracts/registries/IPAssetRegistry.sol";
+import { IPResolver } from "contracts/resolvers/IPResolver.sol";
+import { RegistrationModule } from "contracts/modules/RegistrationModule.sol";
 
 // test
 // solhint-disable-next-line max-line-length
@@ -74,7 +76,8 @@ contract LicensingModuleTest is Test {
             address(accessController),
             address(erc6551Registry),
             address(ipAccountImpl),
-            address(moduleRegistry)
+            address(moduleRegistry),
+            address(governance)
         );
         royaltyModule = new RoyaltyModule(address(governance));
         licenseRegistry = new LicenseRegistry();
@@ -105,7 +108,15 @@ contract LicensingModuleTest is Test {
             "UMLPolicyFrameworkManager",
             licenseUrl
         );
+        IPResolver ipResolver = new IPResolver(address(accessController), address(ipAssetRegistry));
+        RegistrationModule registrationModule = new RegistrationModule(
+            address(accessController),
+            address(ipAssetRegistry),
+            address(licensingModule),
+            address(ipResolver)
+        );
 
+        ipAssetRegistry.setRegistrationModule(address(registrationModule));
         // Set licensing module in royalty module
         royaltyModule.setLicensingModule(address(licensingModule));
         royaltyModule.whitelistRoyaltyPolicy(address(mockRoyaltyPolicyLS), true);

test/foundry/registries/metadata/IPAssetRenderer.t.sol

@@ -95,7 +95,8 @@ contract IPAssetRendererTest is BaseTest {
             address(accessController),
             address(erc6551Registry),
             address(ipAccountImpl),
-            address(moduleRegistry)
+            address(moduleRegistry),
+            address(governance)
         );
         RoyaltyModule royaltyModule = new RoyaltyModule(address(governance));
         licenseRegistry = new LicenseRegistry();
@@ -121,6 +122,7 @@ contract IPAssetRendererTest is BaseTest {
         );
         accessController.initialize(address(ipAccountRegistry), address(moduleRegistry));
         royaltyModule.setLicensingModule(address(licensingModule));
+        ipAssetRegistry.setRegistrationModule(address(registrationModule));
 
         vm.prank(alice);
         uint256 tokenId = erc721.mintId(alice, 99);

test/foundry/registries/metadata/MetadataProvider.t.sol

@@ -18,6 +18,8 @@ import { MetadataProviderV1 } from "contracts/registries/metadata/MetadataProvid
 import { IMetadataProvider } from "contracts/interfaces/registries/metadata/IMetadataProvider.sol";
 import { Errors } from "contracts/lib/Errors.sol";
 import { MockERC721 } from "test/foundry/mocks/MockERC721.sol";
+import { IPResolver } from "contracts/resolvers/IPResolver.sol";
+import { RegistrationModule } from "contracts/modules/RegistrationModule.sol";
 
 /// @title IP Metadata Provider Testing Contract
 /// @notice Contract for metadata provider settings.
@@ -119,19 +121,26 @@ contract MetadataProviderTest is BaseTest {
             address(accessController),
             address(ipAccountImpl)
         );
-        vm.prank(bob);
         registry = new IPAssetRegistry(
             address(accessController),
             address(erc6551Registry),
             address(ipAccountImpl),
-            address(licensingModule)
+            address(licensingModule),
+            address(governance)
+        );
+        IPResolver ipResolver = new IPResolver(address(accessController), address(registry));
+        RegistrationModule registrationModule = new RegistrationModule(
+            address(accessController),
+            address(registry),
+            address(licensingModule),
+            address(ipResolver)
         );
         accessController.initialize(address(ipAccountRegistry), address(moduleRegistry));
+        registry.setRegistrationModule(address(registrationModule));
         MockERC721 erc721 = new MockERC721("MockERC721");
         uint256 tokenId = erc721.mintId(alice, 99);
-        IPResolver resolver = new IPResolver(address(accessController), address(registry));
         vm.prank(alice);
-        ipId = registry.register(block.chainid, address(erc721), tokenId, address(resolver), true, v1Metadata);
+        ipId = registry.register(block.chainid, address(erc721), tokenId, address(ipResolver), true, v1Metadata);
 
         metadataProvider = MetadataProviderV1(registry.metadataProvider());
         upgradedProvider = new MockMetadataProviderV2(address(registry));