comments and fixes

contracts/interfaces/modules/base/IModule.sol

@@ -4,11 +4,25 @@ pragma solidity ^0.8.13;
 import { IModule } from "./IModule.sol";
 import { IIPOrg } from "contracts/interfaces/ip-org/IIPOrg.sol";
 
+/// @title IModule
+/// @notice Interface for a Story Protocol Module, building block of the protocol functionality.
 interface IModule {
 
+    /// The execution of the module is pending, and will need to be executed again.
     event RequestPending(address indexed sender);
+    /// Module execution completed successfully.
     event RequestCompleted(address indexed sender);
 
+    /// @notice Main execution entrypoint.
+    /// @dev It will verify params, execute pre action hooks, perform the action,
+    /// execute post action hooks and emit the RequestCompleted event, plus returning the result.
+    /// It's up to the module to decode and encode params appropriately.
+    /// @param ipOrg_ address of the IPOrg or zero address
+    /// @param caller_ address requesting the execution
+    /// @param selfParams_ encoded params for module action
+    /// @param preHookParams_ encoded params for pre action hooks
+    /// @param postHookParams_ encoded params for post action hooks
+    /// @return result of the module action
     function execute(
         IIPOrg ipOrg_,
         address caller_,
@@ -17,6 +31,12 @@ interface IModule {
         bytes[] calldata postHookParams_
     ) external returns (bytes memory result);
 
+    /// @notice Configuration entrypoint.
+    /// @dev It will verify params and configure the module.
+    /// It's up to the module to decode and encode params appropriately.
+    /// @param ipOrg_ address of the IPOrg or zero address
+    /// @param caller_ address requesting the execution
+    /// @param params_ encoded params for module configuration
     function configure(IIPOrg ipOrg_, address caller_, bytes calldata params_) external;
 
 }

contracts/interfaces/modules/relationships/IRelationshipModule.sol

@@ -3,37 +3,63 @@ pragma solidity ^0.8.13;
 
 import { LibRelationship } from "contracts/lib/modules/LibRelationship.sol";
 
+/// @title IRelationshipModule
+/// @notice Interface for the RelationshipModule. 
 interface IRelationshipModule {
-
+    /// Emitted with a new Relationship Type definitions is created
     event RelationshipTypeSet(
+        // Short string naming the type
         string indexed relType,
+        // Zero for protocol-wide, or address of the IPOrg
         address indexed ipOrg,
+        // Allowed src address, zero address if empty, all F for all addresses are OK
         address src,
+        // Allowed items for src
         LibRelationship.Relatables srcRelatable,
+        // Mask of allowed subtypes for src (see LibUintArrayMask)
         uint256 srcSubtypesMask,
+        // Allowed dst address, zero address if empty, all F for all addresses are OK
         address dst,
+        // Allowed items for dst
         LibRelationship.Relatables dstRelatable,
+        // Mask of allowed subtypes for dst (see LibUintArrayMask)
         uint256 dstSubtypesMask
     );
 
+    /// Emitted when a Relationship Type definition is removed
     event RelationshipTypeUnset(
+        // Short string naming the type
         string indexed relType,
+        // Zero for protocol-wide, or address of the IPOrg
         address ipOrg
     );
 
+    /// Emitted when a Relationship is created, linking 2 elements
     event RelationshipCreated(
+        // Sequential Relationship ID
         uint256 indexed relationshipId,
+        // Short string naming the type
         string indexed relType,
+        // Source contract or EOA
         address srcAddress,
+        // Source item ID
         uint256 srcId,
+        // Destination contract or EOA
         address dstAddress,
+        // Destination item ID
         uint256 dstId
     );
 
+    /// Gets relationship type definition for a given relationship type name
+    /// Will revert if no relationship type is found
+    /// @param ipOrg_ IP Org address or zero address for protocol level relationships
+    /// @param relType_ the name of the relationship type
+    /// @return result the relationship type definition
     function getRelationshipType(address ipOrg_, string memory relType_) external view returns (LibRelationship.RelationshipType memory);
+    /// Gets relationship definition for a given relationship id
     function getRelationship(uint256 relationshipId_) external view returns (LibRelationship.Relationship memory);
+    /// Gets relationship id for a given relationship
     function getRelationshipId(LibRelationship.Relationship calldata rel_) external view returns (uint256);
-    function relationshipExists(LibRelationship.Relationship calldata rel_) external view returns (bool);
-
-
+    /// Checks if a relationship has been set
+    function relationshipExists(LibRelationship.Relationship calldata rel_) external view returns (bool);   
 }

contracts/lib/Errors.sol

@@ -42,6 +42,7 @@ library Errors {
     error BaseModule_ZeroIpaRegistry();
     error BaseModule_ZeroModuleRegistry();
     error BaseModule_ZeroLicenseRegistry();
+    error BaseModule_OnlyModuleRegistry();
 
     ////////////////////////////////////////////////////////////////////////////
     //                            HookRegistry                                //

contracts/modules/ModuleRegistry.sol

@@ -8,6 +8,10 @@ import { IIPOrg } from "contracts/interfaces/ip-org/IIPOrg.sol";
 import { BaseModule } from "./base/BaseModule.sol";
 import { Multicall } from "@openzeppelin/contracts/utils/Multicall.sol";
 
+/// @title ModuleRegistry
+/// @notice This contract is the source of truth for all modules that are registered in the protocol.
+/// It's also the entrypoint for execution and configuration of modules, either directly by users
+/// or by MODULE_EXECUTOR_ROLE holders.
 contract ModuleRegistry is AccessControlled, Multicall {
 
     event ProtocolModuleAdded(string indexed moduleKey, BaseModule module);
@@ -31,6 +35,10 @@ contract ModuleRegistry is AccessControlled, Multicall {
 
     constructor(address accessControl_) AccessControlled(accessControl_) { }
 
+    /// Add a module to the protocol, that will be available for all IPOrgs.
+    /// This is only callable by MODULE_REGISTRAR_ROLE holders.
+    /// @param moduleKey short module descriptor
+    /// @param moduleAddress address of the module
     function registerProtocolModule(
         string calldata moduleKey,
         BaseModule moduleAddress
@@ -43,6 +51,9 @@ contract ModuleRegistry is AccessControlled, Multicall {
         emit ProtocolModuleAdded(moduleKey, moduleAddress);
     }
 
+    /// Remove a module from the protocol (all IPOrgs)
+    /// This is only callable by MODULE_REGISTRAR_ROLE holders.
+    /// @param moduleKey short module descriptor
     function removeProtocolModule(
         string calldata moduleKey
     ) external onlyRole(AccessControl.MODULE_REGISTRAR_ROLE) {
@@ -54,20 +65,36 @@ contract ModuleRegistry is AccessControlled, Multicall {
         emit ProtocolModuleRemoved(moduleKey, moduleAddress);
     }
 
+    /// Get a module from the protocol, by its key.
     function moduleForKey(string calldata moduleKey) external view returns (BaseModule) {
         return _protocolModules[moduleKey];
     }
 
+    /// Execution entrypoint, callable by any address on its own behalf.
+    /// @param ipOrg_ address of the IPOrg, or address(0) for protocol-level stuff
+    /// @param moduleKey_ short module descriptor
+    /// @param selfParams_ encoded params for module action
+    /// @param preHookParams_ encoded params for pre action hooks
+    /// @param postHookParams_ encoded params for post action hooks
+    /// @return encoded result of the module execution
     function execute(
         IIPOrg ipOrg_,
         string calldata moduleKey_,
         bytes calldata selfParams_,
         bytes[] calldata preHookParams_,
         bytes[] calldata postHookParams_
-    ) external {
-        _execute(ipOrg_, msg.sender, moduleKey_, selfParams_, preHookParams_, postHookParams_);
+    ) external returns (bytes memory) {
+        return _execute(ipOrg_, msg.sender, moduleKey_, selfParams_, preHookParams_, postHookParams_);
     }
 
+    /// Execution entrypoint, callable by any MODULE_EXECUTOR_ROLE holder on behalf of any address.
+    /// @param ipOrg_ address of the IPOrg, or address(0) for protocol-level stuff
+    /// @param caller_ address requesting the execution
+    /// @param moduleKey_ short module descriptor
+    /// @param selfParams_ encoded params for module action
+    /// @param preHookParams_ encoded params for pre action hooks
+    /// @param postHookParams_ encoded params for post action hooks
+    /// @return encoded result of the module execution
     function execute(
         IIPOrg ipOrg_,
         address caller_,
@@ -79,6 +106,10 @@ contract ModuleRegistry is AccessControlled, Multicall {
         return _execute(ipOrg_, caller_, moduleKey_, selfParams_, preHookParams_, postHookParams_);
     }
 
+    /// Configuration entrypoint, callable by any address on its own behalf.
+    /// @param ipOrg_ address of the IPOrg, or address(0) for protocol-level stuff
+    /// @param moduleKey_ short module descriptor
+    /// @param params_ encoded params for module configuration
     function configure(
         IIPOrg ipOrg_,
         string calldata moduleKey_,
@@ -87,6 +118,11 @@ contract ModuleRegistry is AccessControlled, Multicall {
         _configure(ipOrg_, msg.sender, moduleKey_, params_);
     }
 
+    /// Configuration entrypoint, callable by any MODULE_EXECUTOR_ROLE holder on behalf of any address.
+    /// @param ipOrg_ address of the IPOrg, or address(0) for protocol-level stuff
+    /// @param caller_ address requesting the execution
+    /// @param moduleKey_ short module descriptor
+    /// @param params_ encoded params for module configuration
     function configure(
         IIPOrg ipOrg_,
         address caller_,

contracts/modules/base/BaseModule.sol

@@ -8,8 +8,13 @@ import { IIPOrg } from "contracts/interfaces/ip-org/IIPOrg.sol";
 import { ModuleRegistry } from "contracts/modules/ModuleRegistry.sol";
 import { IPAssetRegistry } from "contracts/IPAssetRegistry.sol";
 
+/// @title BaseModule
+/// @notice Base implementation for all modules in Story Protocol. This is meant to ensure
+/// that all modules follow the same execution flow and have access to hooks.
+/// It's up to the module how to perform the actions, verifications and authorizations.
+/// @dev This contract should NOT have state in storage, in order to have upgradeable or non-upgradeable
+/// modules.
 abstract contract BaseModule is IModule, HookRegistry {
-
     struct ModuleConstruction {
         IPAssetRegistry ipaRegistry;
         ModuleRegistry moduleRegistry;
@@ -20,6 +25,13 @@ abstract contract BaseModule is IModule, HookRegistry {
     ModuleRegistry public immutable MODULE_REGISTRY;
     address public immutable LICENSE_REGISTRY;
 
+    modifier onlyModuleRegistry() {
+        if (msg.sender != address(MODULE_REGISTRY)) {
+            revert Errors.BaseModule_OnlyModuleRegistry();
+        }
+        _;
+    }
+
     constructor(ModuleConstruction memory params_) {
         if (address(params_.ipaRegistry) == address(0)) {
             revert Errors.BaseModule_ZeroIpaRegistry();
@@ -35,14 +47,21 @@ abstract contract BaseModule is IModule, HookRegistry {
         LICENSE_REGISTRY = params_.licenseRegistry;
     }
 
-    // TODO access control on sender
+    /// Main execution entrypoint. It will verify params, execute pre action hooks, perform the action,
+    /// execute post action hooks and emit the RequestCompleted event, plus returning the result.
+    /// It's up to the module to decode and encode params appropriately.
+    /// @param ipOrg_ address of the IPOrg or zero address 
+    /// @param caller_ address requesting the execution
+    /// @param selfParams_ encoded params for module action
+    /// @param preHookParams_ encoded params for pre action hooks
+    /// @param postHookParams_ encoded params for post action hooks
     function execute(
         IIPOrg ipOrg_,
         address caller_,
         bytes calldata selfParams_,
         bytes[] calldata preHookParams_,
         bytes[] calldata postHookParams_
-    ) external returns (bytes memory result) {
+    ) external onlyModuleRegistry returns (bytes memory result) {
         _verifyExecution(ipOrg_, caller_, selfParams_);
         if (!_executeHooks(preHookParams_, HookType.PreAction)) {
             emit RequestPending(caller_);
@@ -54,8 +73,11 @@ abstract contract BaseModule is IModule, HookRegistry {
         return result;
     }
 
-    // TODO access control on sender
-    function configure(IIPOrg ipOrg_, address caller_, bytes calldata params_) external {
+    /// Configuration entrypoint. It's up to the module to decode params appropriately.
+    /// @param ipOrg_ address of the IPOrg or zero address 
+    /// @param caller_ address requesting the execution
+    /// @param params_ encoded configuration params
+    function configure(IIPOrg ipOrg_, address caller_, bytes calldata params_) onlyModuleRegistry external {
         _configure(ipOrg_, caller_, params_);
     }