more comments

contracts/lib/modules/Licensing.sol

@@ -5,21 +5,36 @@ import { ShortString } from "@openzeppelin/contracts/utils/ShortStrings.sol";
 import { IHook } from "contracts/interfaces/hooks/base/IHook.sol";
 import { FixedSet } from "contracts/utils/FixedSet.sol";
 
+/// @title Licensing Module Library
+/// Structs needed by the Licensing Modules and registries
 library Licensing {
 
+    /// @notice Struct that holds the data for a license
     struct License {
+        /// States the commercial nature of the license. All terms will follow.
         bool isCommercial;
+        /// address granting the license
         address licensor;
+        /// address that could make a license invalid
         address revoker;
+        /// address of the ip org that produced the terms
         address ipOrg;
+        /// Defines how to find the address of the licensee
         LicenseeType licenseeType;
+        /// If the licensee is bound to an IPA, this is the IPA id. 0 otherwise
         uint256 ipaId;
+        /// The id of the parent license. 0 if this this is tied to the first IPA of an IPOrg
         uint256 parentLicenseId;
+        /// The ids of the Licensing Terms that make up the license.
+        /// The terms definitions are in TermsRepository contract
         ShortString[] termIds;
+        /// The data configuring each term. May be empty bytes. May be passed to the term hook
         bytes[] termsData;
+        /// Future use
         bytes data;
     }
 
+    /// User facing parameters for creating a license
     struct LicenseCreation {
         bool isCommercial;
         uint256 parentLicenseId;
@@ -28,51 +43,79 @@ library Licensing {
         // bytes[] extraTermsData;
     }
 
+    /// Input to add a License the LicenseRegistry 
     struct RegistryAddition {
+        /// States the commercial nature of the license. All terms will follow.
         bool isCommercial;
+        /// address granting the license
         address licensor;
+        /// address that could make a license invalid
         address revoker;
+        /// address of the ip org that produced the terms
         address ipOrg;
+        /// The id of the parent license. 0 if this this is tied to the first IPA of an IPOrg
         uint256 parentLicenseId;
+        /// The ids of the Licensing Terms that make up the license.
         ShortString[] termIds;
+        /// The data configuring each term. May be empty bytes. May be passed to the term hook
         bytes[] termsData;
+        /// Future use
         bytes data;
     }
-    
+
     enum LicenseeType {
+        // Empty value
         Unset,
+        // The licensee is the owner of the IPA
         BoundToIpa,
+        // The licensee is the owner of the NFT. ipaId will be 0 in the license
         LNFTHolder
     }
 
+    /// Defines commercial status on a licensing term
     enum CommercialStatus {
+        /// Empty value
         Unset,
+        /// Term can only be used for commercial licenses
         Commercial,
+        /// Term can only be used for non commercial licenses
         NonCommercial,
+        /// Term could be used for both commercial and non commercial licenses
         Both
     }
 
+    /// Data needed to create a Licensing Term. A collection of terms can be used to render
+    /// a license text
     struct LicensingTerm {
+        /// Defines commercial status on a licensing term
         CommercialStatus comStatus;
+        /// URL where the term text can be found. If the destination is not available of changes
+        /// Licenses created with this term will be candidates for revocation
         string url;
+        /// Hash of the license text
         string hash;
+        /// Hashing algorithm used
         string algorithm;
+        /// If the Licensing term is enforceable on chain, this is the hook that will be called
         IHook hook;
-        // ITermDecoder decoder; // TODO: some terms just need to decode bytes data, not
-        // a full blown hook
+        // Some terms just need to decode bytes data, not a full blown hook.
+        // ITermDecoder decoder; // TODO: For now the LicensingModule knows how to decode the data per term id
     }
 
-    // NOTE: we cannot use this in structs that are going to be saved
-    // to storage, like License
+    /// Defines a collection of termIds and their config data. Must be same length
+    /// @dev: we cannot use this in structs that are going to be saved
+    /// to storage, like License
     struct TermsConfig {
         ShortString[] termIds;
         bytes[] termData;
     }
 
+    /// Input for IpOrg legal terms configuration in LicenseCreatorModule
     struct FrameworkConfig {
         TermsConfig comTermsConfig;
         TermsConfig nonComTermsConfig;
     }
 
+    /// Input for IpOrg legal terms configuration in LicenseCreatorModule (for now, the only option)
     bytes32 constant LICENSING_FRAMEWORK_CONFIG = keccak256("LICENSING_FRAMEWORK_CONFIG");
 }

contracts/lib/modules/ProtocolLicensingTerms.sol

@@ -1,19 +1,16 @@
 // SPDX-License-Identifier: BUSL-1.1
 pragma solidity ^0.8.19;
 
+/// List of Licensing Term categories
 library TermCategories {
     string constant FORMAT_CATEGORIES = "FORMAT_CATEGORIES";
     string constant SHARE_ALIKE = "SHARE_ALIKE";
 }
 
+/// List of Protocol Term Ids (meaning the Licensing Module will have specific instructions
+/// for these terms without the need of a decoder)
 /// @dev must be < 32 bytes long, or they will blow up at some point
 /// see https://docs.openzeppelin.com/contracts/4.x/api/utils#ShortStrings
 library TermIds {
     string constant NFT_SHARE_ALIKE = "NFT_SHARE_ALIKE";
 }
-
-library TermData {
-    struct NftShareAlike {
-        bool enabled;
-    }
-}

contracts/modules/ModuleRegistry.sol

@@ -157,9 +157,6 @@ contract ModuleRegistry is AccessControlled, Multicall {
         string calldata moduleKey_,
         bytes calldata params_
     ) private returns (bytes memory result) {
-        // if (IIPOrg(ipOrg_).owner() != msg.sender) {
-        //     revert Errors.ModuleRegistry_CallerNotOrgOwner();
-        //}
         BaseModule module = _protocolModules[moduleKey_];
         if (address(module) == address(0)) {
             revert Errors.ModuleRegistry_ModuleNotRegistered(moduleKey_);

contracts/modules/licensing/LicenseRegistry.sol

@@ -6,7 +6,13 @@ import { IPAssetRegistry } from "contracts/IPAssetRegistry.sol";
 import { Errors } from "contracts/lib/Errors.sol";
 import { ERC721 } from "@openzeppelin/contracts/token/ERC721/ERC721.sol";
 
+/// @title LicenseRegistry
+/// @notice This contract is the source of truth for all licenses that are registered in the protocol.
+/// It will only be called by licensing modules.
+/// It should not be upgradeable, so once a license is registered, it will be there forever.
+/// Licenses can be made invalid by the revoker, according to the terms of the license.
 contract LicenseRegistry is ERC721 {
+    // TODO: Figure out data needed for indexing
     event LicenseRegistered(
         uint256 indexed id
     );
@@ -15,7 +21,9 @@ contract LicenseRegistry is ERC721 {
         uint256 indexed ipaId
     );
 
+    /// license Id => License
     mapping(uint256 => Licensing.License) private _licenses;
+    /// counder for license Ids
     uint256 private _licenseCount;
 
     IPAssetRegistry public immutable IPA_REGISTRY;
@@ -29,6 +37,10 @@ contract LicenseRegistry is ERC721 {
         IPA_REGISTRY = IPAssetRegistry(ipaRegistry);
     }
 
+    /// Creates a License bound to a certain IPA
+    /// @param params_ RegistryAddition params
+    /// @param ipaId_ id of the bound IPA
+    /// @return id of the created license
     function addBoundToIpaLicense(
         Licensing.RegistryAddition memory params_,
         uint256 ipaId_
@@ -53,6 +65,11 @@ contract LicenseRegistry is ERC721 {
         );
     }
 
+    /// Creates a tradeable License NFT.
+    /// If the license is to create an IPA in the future, when registering, this license will be
+    /// bound to the IPA.
+    /// @param params_ RegistryAddition params
+    /// @param licensee_ address of the licensee (and owner of the NFT)
     function addTradeableLicense(
         Licensing.RegistryAddition memory params_,
         address licensee_
@@ -75,31 +92,28 @@ contract LicenseRegistry is ERC721 {
         return _licenseCount;
     }
 
-    function _addLicense(
-        Licensing.License memory license_
-    ) private returns (uint256) {
+
+    function _addLicense(Licensing.License memory license_) private returns (uint256) {
         // TODO: Check valid parent license
         _licenseCount++;
         _licenses[_licenseCount] = license_;
         emit LicenseRegistered(_licenseCount);
         return _licenseCount;
     }
 
-    function getLicense(
-        uint256 id_
-    ) external view returns (Licensing.License memory) {
+    /// Gets License struct for input id
+    function getLicense(uint256 id_) external view returns (Licensing.License memory) {
         return _licenses[id_];
     }
 
-    function getLicensor(
-        uint256 id_
-    ) external view returns (address) {
+    /// Gets the address granting a license, by id
+    function getLicensor(uint256 id_) external view returns (address) {
         return _licenses[id_].licensor;
     }
-
-    function getLicensee(
-        uint256 id_
-    ) external view returns (address) {
+    /// Gets the address a license is granted to
+    /// @param id_ of the license
+    /// @return licensee address, NFT owner if the license is tradeable, or IPA owner if bound to IPA
+    function getLicensee(uint256 id_) external view returns (address) {
         Licensing.LicenseeType licenseeType_ = _licenses[id_].licenseeType;
         if (licenseeType_ == Licensing.LicenseeType.Unset) {
             revert Errors.LicenseRegistry_UnknownLicenseId();
@@ -111,16 +125,18 @@ contract LicenseRegistry is ERC721 {
         }
     }
 
-    function boundLnftToIpa(
-        uint256 id_
-    ) external {
+    /// Burns a license NFT and binds the license to an IPA
+    /// @param licenseId_ id of the license NFT
+    /// @param ipaId_ id of the IPA
+    function boundLnftToIpa(uint256 licenseId_, uint256 ipaId_) external {
         // TODO add Authorization 
-        Licensing.License memory license_ = _licenses[id_];
+        Licensing.License memory license_ = _licenses[licenseId_];
         if (license_.licenseeType != Licensing.LicenseeType.LNFTHolder) {
             revert Errors.LicenseRegistry_NotLicenseNFT();
         }
-        _licenses[id_].licenseeType = Licensing.LicenseeType.BoundToIpa;
-        _burn(id_);
-        emit LicenseNftBoundedToIpa(id_, license_.ipaId);
+        _licenses[licenseId_].licenseeType = Licensing.LicenseeType.BoundToIpa;
+        _licenses[licenseId_].ipaId = ipaId_;
+        _burn(licenseId_);
+        emit LicenseNftBoundedToIpa(licenseId_, ipaId_);
     }
 }