Skip to content
This repository has been archived by the owner on Apr 30, 2024. It is now read-only.

Commit

Permalink
Address review comments
Browse files Browse the repository at this point in the history 
- convert commercializers from array to a single address
- Not checkin TokenGatedHook to core protocol
  • Loading branch information
@kingster-will
kingster-will committed Feb 12, 2024
1 parent 7d01a0b commit 8245a65
Show file tree
Hide file tree
Showing 21 changed files with 185 additions and 188 deletions.
3 changes: 3 additions & 0 deletions contracts/interfaces/modules/base/IHookModule.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,7 @@ import { IModule } from "./IModule.sol";
interface IHookModule is IModule {
/// @notice Verify if the caller is qualified
function verify(address caller, bytes calldata data) external returns (bool);
/// @notice Validates the configuration for the hook.
/// @param configData The configuration data for the hook.
function validateConfig(bytes calldata configData) external view;
}
8 changes: 4 additions & 4 deletions contracts/interfaces/modules/licensing/IUMLPolicyFrameworkManager.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ import { IPolicyFrameworkManager } from "../../../interfaces/modules/licensing/I
/// @param attribution Whether or not attribution is required when reproducing the work
/// @param commercialUse Whether or not the work can be used commercially
/// @param commercialAttribution Whether or not attribution is required when reproducing the work commercially
/// @param commercializers List of commericializers that are allowed to commercially exploit the work. If empty
/// then no restrictions.
/// @param commercializerChecker commericializers that are allowed to commercially exploit the work. If zero
/// address then no restrictions.
/// @param commercialRevShare Percentage of revenue that must be shared with the licensor
/// @param derivativesAllowed Whether or not the licensee can create derivatives of his work
/// @param derivativesAttribution Whether or not attribution is required for derivatives of the work
Expand All @@ -25,8 +25,8 @@ struct UMLPolicy {
bool attribution;
bool commercialUse;
bool commercialAttribution;
address[] commercializers;
bytes[] commercializersData;
address commercializerChecker;
bytes commercializerCheckerData;
uint32 commercialRevShare;
bool derivativesAllowed;
bool derivativesAttribution;
Expand Down
2 changes: 1 addition & 1 deletion contracts/lib/Errors.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ library Errors {
////////////////////////////////////////////////////////////////////////////

error PolicyFrameworkManager__GettingPolicyWrongFramework();
error PolicyFrameworkManager__CommercializerDoesNotSupportHook(address commercializer);
error PolicyFrameworkManager__CommercializerCheckerDoesNotSupportHook(address commercializer);

////////////////////////////////////////////////////////////////////////////
// LicensorApprovalChecker //
Expand Down
1 change: 0 additions & 1 deletion contracts/lib/UMLFrameworkErrors.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,5 +25,4 @@ library UMLFrameworkErrors {
error UMLPolicyFrameworkManager__CommercialValueMismatch();
error UMLPolicyFrameworkManager__DerivativesValueMismatch();
error UMLPolicyFrameworkManager__StringArrayMismatch();
error UMLPolicyFrameworkManager__CommercializersDataMismatch();
}
12 changes: 8 additions & 4 deletions contracts/modules/licensing/LicensingModule.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { EnumerableSet } from "@openzeppelin/contracts/utils/structs/EnumerableS
import { Strings } from "@openzeppelin/contracts/utils/Strings.sol";
import { ERC165Checker } from "@openzeppelin/contracts/utils/introspection/ERC165Checker.sol";
import { IERC165 } from "@openzeppelin/contracts/utils/introspection/IERC165.sol";
import { ReentrancyGuard } from "@openzeppelin/contracts/utils/ReentrancyGuard.sol";

import { IIPAccount } from "../../interfaces/IIPAccount.sol";
import { IPolicyFrameworkManager } from "../../interfaces/modules/licensing/IPolicyFrameworkManager.sol";
Expand All @@ -22,7 +23,7 @@ import { LICENSING_MODULE_KEY } from "../../lib/modules/Module.sol";
import { BaseModule } from "../BaseModule.sol";

// TODO: consider disabling operators/approvals on creation
contract LicensingModule is AccessControlled, ILicensingModule, BaseModule {
contract LicensingModule is AccessControlled, ILicensingModule, BaseModule, ReentrancyGuard {
using ERC165Checker for address;
using IPAccountChecker for IIPAccountRegistry;
using EnumerableSet for EnumerableSet.UintSet;
Expand Down Expand Up @@ -122,7 +123,10 @@ contract LicensingModule is AccessControlled, ILicensingModule, BaseModule {
/// @param ipId to receive the policy
/// @param polId id of the policy data
/// @return indexOnIpId position of policy within the ipIds policy set
function addPolicyToIp(address ipId, uint256 polId) external verifyPermission(ipId) returns (uint256 indexOnIpId) {
function addPolicyToIp(
address ipId,
uint256 polId
) external nonReentrant verifyPermission(ipId) returns (uint256 indexOnIpId) {
if (!isPolicyDefined(polId)) {
revert Errors.LicensingModule__PolicyNotFound();
}
Expand Down Expand Up @@ -166,7 +170,7 @@ contract LicensingModule is AccessControlled, ILicensingModule, BaseModule {
address licensorIp,
uint256 amount, // mint amount
address receiver
) external returns (uint256 licenseId) {
) external nonReentrant returns (uint256 licenseId) {
// TODO: check if licensor has been tagged by disputer
if (!IP_ACCOUNT_REGISTRY.isIpAccount(licensorIp)) {
revert Errors.LicensingModule__LicensorNotRegistered();
Expand Down Expand Up @@ -257,7 +261,7 @@ contract LicensingModule is AccessControlled, ILicensingModule, BaseModule {
uint256[] calldata licenseIds,
address childIpId,
uint32 minRoyalty
) external verifyPermission(childIpId) {
) external nonReentrant verifyPermission(childIpId) {
address holder = IIPAccount(payable(childIpId)).owner();
address[] memory licensors = new address[](licenseIds.length);
// If royalty policy address is address(0), this means no royalty policy to set.
Expand Down
70 changes: 40 additions & 30 deletions contracts/modules/licensing/UMLPolicyFrameworkManager.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ pragma solidity ^0.8.23;
import { Base64 } from "@openzeppelin/contracts/utils/Base64.sol";
import { Strings } from "@openzeppelin/contracts/utils/Strings.sol";
import { ERC165Checker } from "@openzeppelin/contracts/utils/introspection/ERC165Checker.sol";
import { ReentrancyGuard } from "@openzeppelin/contracts/utils/ReentrancyGuard.sol";

// contracts
import { IHookModule } from "../../interfaces/modules/base/IHookModule.sol";
Expand All @@ -23,7 +24,12 @@ import { LicensorApprovalChecker } from "../../modules/licensing/parameter-helpe
/// @notice This is the UML Policy Framework Manager, which implements the UML Policy Framework
/// logic for encoding and decoding UML policies into the LicenseRegistry and verifying
/// the licensing parameters for linking, minting, and transferring.
contract UMLPolicyFrameworkManager is IUMLPolicyFrameworkManager, BasePolicyFrameworkManager, LicensorApprovalChecker {
contract UMLPolicyFrameworkManager is
IUMLPolicyFrameworkManager,
BasePolicyFrameworkManager,
LicensorApprovalChecker,
ReentrancyGuard
{
using ERC165Checker for address;
using Strings for *;

Expand All @@ -44,7 +50,7 @@ contract UMLPolicyFrameworkManager is IUMLPolicyFrameworkManager, BasePolicyFram
/// @notice Re a new policy to the registry
/// @dev Must encode the policy into bytes to be stored in the LicensingModule
/// @param umlPolicy UMLPolicy compliant licensing term values
function registerPolicy(UMLPolicy calldata umlPolicy) external returns (uint256 policyId) {
function registerPolicy(UMLPolicy calldata umlPolicy) external nonReentrant returns (uint256 policyId) {
_verifyComercialUse(umlPolicy);
_verifyDerivatives(umlPolicy);
// No need to emit here, as the LicensingModule will emit the event
Expand All @@ -63,7 +69,7 @@ contract UMLPolicyFrameworkManager is IUMLPolicyFrameworkManager, BasePolicyFram
address ipId,
address, // parentIpId
bytes calldata policyData
) external override onlyLicensingModule returns (IPolicyFrameworkManager.VerifyLinkResponse memory) {
) external override nonReentrant onlyLicensingModule returns (IPolicyFrameworkManager.VerifyLinkResponse memory) {
UMLPolicy memory policy = abi.decode(policyData, (UMLPolicy));
IPolicyFrameworkManager.VerifyLinkResponse memory response = IPolicyFrameworkManager.VerifyLinkResponse({
isLinkingAllowed: true, // If you successfully mint and now hold a license, you have the right to link.
Expand All @@ -83,15 +89,15 @@ contract UMLPolicyFrameworkManager is IUMLPolicyFrameworkManager, BasePolicyFram
if (policy.derivativesApproval) {
response.isLinkingAllowed = response.isLinkingAllowed && isDerivativeApproved(licenseId, ipId);
}

for (uint256 i = 0; i < policy.commercializers.length; i++) {
if (!policy.commercializers[i].supportsInterface(type(IHookModule).interfaceId)) {
revert Errors.PolicyFrameworkManager__CommercializerDoesNotSupportHook(policy.commercializers[i]);
if (policy.commercializerChecker != address(0)) {
if (!policy.commercializerChecker.supportsInterface(type(IHookModule).interfaceId)) {
revert Errors.PolicyFrameworkManager__CommercializerCheckerDoesNotSupportHook(
policy.commercializerChecker
);
}

if (!IHookModule(policy.commercializers[i]).verify(caller, policy.commercializersData[i])) {
if (!IHookModule(policy.commercializerChecker).verify(caller, policy.commercializerCheckerData)) {
response.isLinkingAllowed = false;
break;
}
}

Expand All @@ -108,20 +114,22 @@ contract UMLPolicyFrameworkManager is IUMLPolicyFrameworkManager, BasePolicyFram
address,
uint256,
bytes memory policyData
) external onlyLicensingModule returns (bool) {
) external nonReentrant onlyLicensingModule returns (bool) {
UMLPolicy memory policy = abi.decode(policyData, (UMLPolicy));
// If the policy defines no derivative is allowed, and policy was inherited,
// we don't allow minting
if (!policy.derivativesAllowed && policyWasInherited) {
return false;
}

for (uint256 i = 0; i < policy.commercializers.length; i++) {
if (!policy.commercializers[i].supportsInterface(type(IHookModule).interfaceId)) {
revert Errors.PolicyFrameworkManager__CommercializerDoesNotSupportHook(policy.commercializers[i]);
if (policy.commercializerChecker != address(0)) {
if (!policy.commercializerChecker.supportsInterface(type(IHookModule).interfaceId)) {
revert Errors.PolicyFrameworkManager__CommercializerCheckerDoesNotSupportHook(
policy.commercializerChecker
);
}

if (!IHookModule(policy.commercializers[i]).verify(caller, policy.commercializersData[i])) {
if (!IHookModule(policy.commercializerChecker).verify(caller, policy.commercializerCheckerData)) {
return false;
}
}
Expand Down Expand Up @@ -246,7 +254,7 @@ contract UMLPolicyFrameworkManager is IUMLPolicyFrameworkManager, BasePolicyFram
return (changedAgg, abi.encode(agg));
}

function policyToJson(bytes memory policyData) public view returns (string memory) {
function policyToJson(bytes memory policyData) public pure returns (string memory) {
UMLPolicy memory policy = abi.decode(policyData, (UMLPolicy));

/* solhint-disable */
Expand Down Expand Up @@ -276,23 +284,20 @@ contract UMLPolicyFrameworkManager is IUMLPolicyFrameworkManager, BasePolicyFram
'{"trait_type": "commercialRevShare", "value": ',
Strings.toString(policy.commercialRevShare),
"},"
'{"trait_type": "commercializers", "value": ['
)
);

uint256 commercializerCount = policy.commercializers.length;
for (uint256 i = 0; i < commercializerCount; ++i) {
json = string(abi.encodePacked(json, '"', policy.commercializers[i].toHexString(), '"'));
if (i != commercializerCount - 1) {
json = string(abi.encodePacked(json, ","));
}
}
json = string(
abi.encodePacked(
json,
'{"trait_type": "commercializerCheck", "value": "',
policy.commercializerChecker.toHexString()
)
);
// TODO: add commercializersData?

json = string(
abi.encodePacked(
json,
']}, {"trait_type": "derivativesAllowed", "value": "',
'"}, {"trait_type": "derivativesAllowed", "value": "',
policy.derivativesAllowed ? "true" : "false",
'"},',
'{"trait_type": "derivativesAttribution", "value": "',
Expand Down Expand Up @@ -343,7 +348,7 @@ contract UMLPolicyFrameworkManager is IUMLPolicyFrameworkManager, BasePolicyFram
if (policy.commercialAttribution) {
revert UMLFrameworkErrors.UMLPolicyFrameworkManager__CommecialDisabled_CantAddAttribution();
}
if (policy.commercializers.length > 0) {
if (policy.commercializerChecker != address(0)) {
revert UMLFrameworkErrors.UMLPolicyFrameworkManager__CommercialDisabled_CantAddCommercializers();
}
if (policy.commercialRevShare > 0) {
Expand All @@ -360,8 +365,13 @@ contract UMLPolicyFrameworkManager is IUMLPolicyFrameworkManager, BasePolicyFram
if (policy.royaltyPolicy == address(0)) {
revert UMLFrameworkErrors.UMLPolicyFrameworkManager__CommecialEnabled_RoyaltyPolicyRequired();
}
if (policy.commercializers.length != policy.commercializersData.length) {
revert UMLFrameworkErrors.UMLPolicyFrameworkManager__CommercializersDataMismatch();
if (policy.commercializerChecker != address(0)) {
if (!policy.commercializerChecker.supportsInterface(type(IHookModule).interfaceId)) {
revert Errors.PolicyFrameworkManager__CommercializerCheckerDoesNotSupportHook(
policy.commercializerChecker
);
}
IHookModule(policy.commercializerChecker).validateConfig(policy.commercializerCheckerData);
}
}
}
Expand Down Expand Up @@ -395,7 +405,7 @@ contract UMLPolicyFrameworkManager is IUMLPolicyFrameworkManager, BasePolicyFram
bytes32 oldHash,
bytes32 newHash,
bytes32 permissive
) internal view returns (bytes32 result) {
) internal pure returns (bytes32 result) {
if (oldHash == newHash) {
return newHash;
}
Expand Down
8 changes: 4 additions & 4 deletions script/foundry/deployment/Main.s.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -381,8 +381,8 @@ contract Main is Script, BroadcastManager, JsonDeploymentHandler {
transferable: true,
commercialUse: true,
commercialAttribution: true,
commercializers: new address[](0),
commercializersData: new bytes[](0),
commercializerChecker: address(0),
commercializerCheckerData: "",
commercialRevShare: 100,
derivativesAllowed: true,
derivativesAttribution: false,
Expand All @@ -402,8 +402,8 @@ contract Main is Script, BroadcastManager, JsonDeploymentHandler {
transferable: false,
commercialUse: false,
commercialAttribution: false,
commercializers: new address[](0),
commercializersData: new bytes[](0),
commercializerChecker: address(0),
commercializerCheckerData: "",
commercialRevShare: 0,
derivativesAllowed: true,
derivativesAttribution: true,
Expand Down
10 changes: 5 additions & 5 deletions test/foundry/ModuleRegistry.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import { Governance } from "contracts/governance/Governance.sol";
import { MODULE_TYPE_DEFAULT, MODULE_TYPE_HOOK } from "contracts/lib/modules/Module.sol";
import { IModule } from "contracts/interfaces/modules/base/IModule.sol";
import { IHookModule } from "../../contracts/interfaces/modules/base/IHookModule.sol";
import { TokenGatedHook } from "../../contracts/modules/hooks/TokenGatedHook.sol";
import { MockTokenGatedHook } from "test/foundry/mocks/MockTokenGatedHook.sol";

contract ModuleRegistryTest is Test {
IPAccountRegistry public registry;
Expand All @@ -27,7 +27,7 @@ contract ModuleRegistryTest is Test {
MockAccessController public accessController = new MockAccessController();
MockModule public module;
CustomModule public customModule;
TokenGatedHook public tokenGatedHook;
MockTokenGatedHook public tokenGatedHook;
Governance public governance;

function setUp() public {
Expand All @@ -37,7 +37,7 @@ contract ModuleRegistryTest is Test {
registry = new IPAccountRegistry(address(erc6551Registry), address(accessController), address(implementation));
module = new MockModule(address(registry), address(moduleRegistry), "MockModule");
customModule = new CustomModule();
tokenGatedHook = new TokenGatedHook();
tokenGatedHook = new MockTokenGatedHook();
}

function test_ModuleRegistry_registerModule() public {
Expand Down Expand Up @@ -147,8 +147,8 @@ contract ModuleRegistryTest is Test {

function test_ModuleRegistry_registerModuleWithHookModuleType() public {
moduleRegistry.registerModuleType(MODULE_TYPE_HOOK, type(IHookModule).interfaceId);
moduleRegistry.registerModule("TokenGatedHook", address(tokenGatedHook), MODULE_TYPE_HOOK);
assertEq(moduleRegistry.getModule("TokenGatedHook"), address(tokenGatedHook));
moduleRegistry.registerModule("MockTokenGatedHook", address(tokenGatedHook), MODULE_TYPE_HOOK);
assertEq(moduleRegistry.getModule("MockTokenGatedHook"), address(tokenGatedHook));
assertTrue(moduleRegistry.isRegistered(address(tokenGatedHook)));
assertEq(moduleRegistry.getModuleType(address(tokenGatedHook)), MODULE_TYPE_HOOK);
}
Expand Down
4 changes: 2 additions & 2 deletions test/foundry/integration/big-bang/NftLicenseRoyalty.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,8 @@ contract BigBang_Integration_NftLicenseRoyalty is BaseIntegration, Integration_S
}),
UMLPolicyCommercialParams({
commercialAttribution: true,
commercializers: new address[](0),
commercializersData: new bytes[](0),
commercializerChecker: address(0),
commercializerCheckerData: "",
commercialRevShare: minRevShare,
royaltyPolicy: address(royaltyPolicyLS)
}),
Expand Down
4 changes: 2 additions & 2 deletions test/foundry/integration/big-bang/SingleNftCollection.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,8 @@ contract BigBang_Integration_SingleNftCollection is BaseIntegration, Integration
}),
UMLPolicyCommercialParams({
commercialAttribution: true,
commercializers: new address[](0),
commercializersData: new bytes[](0),
commercializerChecker: address(0),
commercializerCheckerData: "",
commercialRevShare: 10,
royaltyPolicy: address(royaltyPolicyLS)
}),
Expand Down
Loading

0 comments on commit 8245a65

Please sign in to comment.