Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dispute effects on royalty #35

Merged
merged 9 commits into from
Apr 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions contracts/interfaces/modules/royalty/IRoyaltyModule.sol
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,22 @@ interface IRoyaltyModule is IModule {
/// @param amount The amount paid
event LicenseMintingFeePaid(address receiverIpId, address payerAddress, address token, uint256 amount);

/// @notice Sets the licensing module
/// @dev Enforced to be only callable by the protocol admin
/// @param licensing The address of the license module
function setLicensingModule(address licensing) external;

/// @notice Sets the dispute module
/// @dev Enforced to be only callable by the protocol admin
/// @param dispute The address of the dispute module
function setDisputeModule(address dispute) external;

Comment on lines +33 to +42

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unnecessary to include these two functions in the interface, as no other contracts will invoke them. It would be sufficient to define only those functions that are directly related to the functionality of the royalty module. The two configuration functions are specific to the implementation and do not require definition in the interface.

/// @notice Returns the licensing module address
function licensingModule() external view returns (address);

/// @notice Returns the dispute module address
function disputeModule() external view returns (address);

Comment on lines 43 to +48

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two can also be removed, as we don't expect the functions to be called by another contract.

/// @notice Indicates if a royalty policy is whitelisted
/// @param royaltyPolicy The address of the royalty policy
/// @return isWhitelisted True if the royalty policy is whitelisted
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,20 @@ interface IRoyaltyPolicyLAP is IRoyaltyPolicy {
uint32[] targetRoyaltyAmount
);

/// @notice The state data of the LAP royalty policy
/// @param isUnlinkableToParents Indicates if the ipId is unlinkable to new parents
/// @param ipRoyaltyVault The ip royalty vault address
/// @param royaltyStack The royalty stack of a given ipId is the sum of the royalties to be paid to each ancestors
/// @param ancestorsAddresses The ancestors addresses array
/// @param ancestorsRoyalties Contains royalty token amounts for each ancestor on same index as ancestorsAddresses
struct LAPRoyaltyData {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there any redundancy of those data with license data?

bool isUnlinkableToParents;
address ipRoyaltyVault;
uint32 royaltyStack;
address[] ancestorsAddresses;
uint32[] ancestorsRoyalties;
}

/// @notice Returns the percentage scale - represents 100% of royalty tokens for an ip
function TOTAL_RT_SUPPLY() external view returns (uint32);

Expand Down
4 changes: 4 additions & 0 deletions contracts/lib/Errors.sol
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,8 @@ library Errors {
error RoyaltyModule__ZeroLicensingModule();
error RoyaltyModule__CanOnlyMintSelectedPolicy();
error RoyaltyModule__NoParentsOnLinking();
error RoyaltyModule__ZeroDisputeModule();
error RoyaltyModule__IpIsTagged();
jdubpark marked this conversation as resolved.
Show resolved Hide resolved

error RoyaltyPolicyLAP__ZeroRoyaltyModule();
error RoyaltyPolicyLAP__ZeroLiquidSplitFactory();
Expand All @@ -246,6 +248,8 @@ library Errors {
error IpRoyaltyVault__SnapshotIntervalTooShort();
error IpRoyaltyVault__AlreadyClaimed();
error IpRoyaltyVault__ClaimerNotAnAncestor();
error IpRoyaltyVault__IpTagged();
jdubpark marked this conversation as resolved.
Show resolved Hide resolved
error IpRoyaltyVault__ZeroDisputeModule();

////////////////////////////////////////////////////////////////////////////
// ModuleRegistry //
Expand Down
16 changes: 8 additions & 8 deletions contracts/modules/dispute/DisputeModule.sol
Original file line number Diff line number Diff line change
Expand Up @@ -64,11 +64,11 @@ contract DisputeModule is
IIPAssetRegistry public immutable IP_ASSET_REGISTRY;

/// Constructor
/// @param _controller The address of the access controller
/// @param _assetRegistry The address of the asset registry
/// @param controller The address of the access controller
/// @param assetRegistry The address of the asset registry
/// @custom:oz-upgrades-unsafe-allow constructor
constructor(address _controller, address _assetRegistry) AccessControlled(_controller, _assetRegistry) {
IP_ASSET_REGISTRY = IIPAssetRegistry(_assetRegistry);
constructor(address controller, address assetRegistry) AccessControlled(controller, assetRegistry) {
IP_ASSET_REGISTRY = IIPAssetRegistry(assetRegistry);
_disableInitializers();
}

Expand Down Expand Up @@ -169,9 +169,9 @@ contract DisputeModule is
address arbitrationPolicy = $.arbitrationPolicies[targetIpId];
if (!$.isWhitelistedArbitrationPolicy[arbitrationPolicy]) arbitrationPolicy = $.baseArbitrationPolicy;

uint256 disputeId_ = ++$.disputeCounter;
uint256 disputeId = ++$.disputeCounter;

$.disputes[disputeId_] = Dispute({
$.disputes[disputeId] = Dispute({
targetIpId: targetIpId,
disputeInitiator: msg.sender,
arbitrationPolicy: arbitrationPolicy,
Expand All @@ -183,7 +183,7 @@ contract DisputeModule is
IArbitrationPolicy(arbitrationPolicy).onRaiseDispute(msg.sender, data);

emit DisputeRaised(
disputeId_,
disputeId,
targetIpId,
msg.sender,
arbitrationPolicy,
Expand All @@ -192,7 +192,7 @@ contract DisputeModule is
data
);

return disputeId_;
return disputeId;
}

/// @notice Sets the dispute judgement on a given dispute. Only whitelisted arbitration relayers can call to judge.
Expand Down
26 changes: 23 additions & 3 deletions contracts/modules/royalty/RoyaltyModule.sol
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { BaseModule } from "../BaseModule.sol";
import { GovernableUpgradeable } from "../../governance/GovernableUpgradeable.sol";
import { IRoyaltyModule } from "../../interfaces/modules/royalty/IRoyaltyModule.sol";
import { IRoyaltyPolicy } from "../../interfaces/modules/royalty/policies/IRoyaltyPolicy.sol";
import { IDisputeModule } from "../../interfaces/modules/dispute/IDisputeModule.sol";
import { Errors } from "../../lib/Errors.sol";
import { ROYALTY_MODULE_KEY } from "../../lib/modules/Module.sol";
import { BaseModule } from "../BaseModule.sol";
Expand All @@ -27,12 +28,14 @@ contract RoyaltyModule is
using ERC165Checker for address;

/// @dev Storage structure for the RoyaltyModule
/// @param disputeModule The address of the dispute module
/// @param licensingModule The address of the licensing module
/// @param isWhitelistedRoyaltyPolicy Indicates if a royalty policy is whitelisted
/// @param isWhitelistedRoyaltyToken Indicates if a royalty token is whitelisted
/// @param royaltyPolicies Indicates the royalty policy for a given IP asset
/// @custom:storage-location erc7201:story-protocol.RoyaltyModule
struct RoyaltyModuleStorage {
address disputeModule;
address licensingModule;
mapping(address royaltyPolicy => bool isWhitelisted) isWhitelistedRoyaltyPolicy;
mapping(address token => bool) isWhitelistedRoyaltyToken;
Expand Down Expand Up @@ -66,14 +69,22 @@ contract RoyaltyModule is
_;
}

/// @notice Sets the license registry
/// @notice Sets the licensing module
/// @dev Enforced to be only callable by the protocol admin
/// @param licensing The address of the license registry
/// @param licensing The address of the license module
function setLicensingModule(address licensing) external onlyProtocolAdmin {
if (licensing == address(0)) revert Errors.RoyaltyModule__ZeroLicensingModule();
_getRoyaltyModuleStorage().licensingModule = licensing;
}

/// @notice Sets the dispute module
/// @dev Enforced to be only callable by the protocol admin
/// @param dispute The address of the dispute module
function setDisputeModule(address dispute) external onlyProtocolAdmin {
if (dispute == address(0)) revert Errors.RoyaltyModule__ZeroDisputeModule();
_getRoyaltyModuleStorage().disputeModule = dispute;
}

Comment on lines +80 to +87

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of making this "set" function as a governance function.
It would better follow the "update" pattern of updating the dispute module address according to ModuleRegistry.
We have apply the pattern in the CoreMedataViewModule:

    function updateCoreMetadataModule() external {
        coreMetadataModule = IModuleRegistry(MODULE_REGISTRY).getModule(CORE_METADATA_MODULE_KEY);
    }

in this case, it would be:

    function updateDisputeModule() external {
         _getRoyaltyModuleStorage().disputeModule = IModuleRegistry(MODULE_REGISTRY).getModule(DISPUTE_MODULE_KEY);
    }

so that the function can be permissionless and maintain code style consistency in our code base.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not advertise this as a "permissionless" method, because the setter of value in ModuleRegistry is still the governance. Simply delegating the reading & setting to the public, where the value is controlled by the governance.

But yes, this is a cleaner syntax since we don't need to pass in the dispute module address for each contract. Just update in the Module Registry, and then call updateDisputeModule in each relevant contract.

I recommend we make this change for all contracts in a separate PR, since most other modules are still using the old style.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree, we will leave those changes to next PR @Spablob

/// @notice Whitelist a royalty policy
/// @dev Enforced to be only callable by the protocol admin
/// @param royaltyPolicy The address of the royalty policy
Expand Down Expand Up @@ -175,6 +186,10 @@ contract RoyaltyModule is
RoyaltyModuleStorage storage $ = _getRoyaltyModuleStorage();
if (!$.isWhitelistedRoyaltyToken[token]) revert Errors.RoyaltyModule__NotWhitelistedRoyaltyToken();

IDisputeModule dispute = IDisputeModule($.disputeModule);
if (dispute.isIpTagged(receiverIpId) || dispute.isIpTagged(payerIpId))
revert Errors.RoyaltyModule__IpIsTagged();

address payerRoyaltyPolicy = $.royaltyPolicies[payerIpId];
// if the payer does not have a royalty policy set, then the payer is not a derivative ip and does not pay
// royalties while the receiver ip can have a zero royalty policy since that could mean it is an ip a root
Expand Down Expand Up @@ -202,7 +217,7 @@ contract RoyaltyModule is
) external onlyLicensingModule {
RoyaltyModuleStorage storage $ = _getRoyaltyModuleStorage();
if (!$.isWhitelistedRoyaltyToken[token]) revert Errors.RoyaltyModule__NotWhitelistedRoyaltyToken();

if (IDisputeModule($.disputeModule).isIpTagged(receiverIpId)) revert Errors.RoyaltyModule__IpIsTagged();
if (licenseRoyaltyPolicy == address(0)) revert Errors.RoyaltyModule__NoRoyaltyPolicySet();
if (!$.isWhitelistedRoyaltyPolicy[licenseRoyaltyPolicy])
revert Errors.RoyaltyModule__NotWhitelistedRoyaltyPolicy();
Expand All @@ -217,6 +232,11 @@ contract RoyaltyModule is
return _getRoyaltyModuleStorage().licensingModule;
}

/// @notice Returns the dispute module address
function disputeModule() external view returns (address) {
return _getRoyaltyModuleStorage().disputeModule;
}

/// @notice Indicates if a royalty policy is whitelisted
/// @param royaltyPolicy The address of the royalty policy
/// @return isWhitelisted True if the royalty policy is whitelisted
Expand Down
15 changes: 14 additions & 1 deletion contracts/modules/royalty/policies/IpRoyaltyVault.sol
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { SafeERC20Upgradeable } from "@openzeppelin/contracts-upgradeable-v4/tok
import { IERC20Upgradeable } from "@openzeppelin/contracts-upgradeable-v4/token/ERC20/IERC20Upgradeable.sol";

import { IRoyaltyPolicyLAP } from "../../../interfaces/modules/royalty/policies/IRoyaltyPolicyLAP.sol";
import { IDisputeModule } from "../../../interfaces/modules/dispute/IDisputeModule.sol";
import { IIpRoyaltyVault } from "../../../interfaces/modules/royalty/policies/IIpRoyaltyVault.sol";
import { ArrayUtils } from "../../../lib/ArrayUtils.sol";
import { Errors } from "../../../lib/Errors.sol";
Expand All @@ -25,6 +26,9 @@ contract IpRoyaltyVault is IIpRoyaltyVault, ERC20SnapshotUpgradeable, Reentrancy
/// @custom:oz-upgrades-unsafe-allow state-variable-immutable
IRoyaltyPolicyLAP public immutable ROYALTY_POLICY_LAP;

/// @notice Dispute module address
IDisputeModule public immutable DISPUTE_MODULE;

/// @notice Ip id to whom this royalty vault belongs to
address public ipId;

Expand Down Expand Up @@ -58,10 +62,15 @@ contract IpRoyaltyVault is IIpRoyaltyVault, ERC20SnapshotUpgradeable, Reentrancy

/// @notice Constructor
/// @param royaltyPolicyLAP The address of the royalty policy LAP
/// @param disputeModule The address of the dispute module
/// @custom:oz-upgrades-unsafe-allow constructor
constructor(address royaltyPolicyLAP) {
constructor(address royaltyPolicyLAP, address disputeModule) {
if (royaltyPolicyLAP == address(0)) revert Errors.IpRoyaltyVault__ZeroRoyaltyPolicyLAP();
if (disputeModule == address(0)) revert Errors.IpRoyaltyVault__ZeroDisputeModule();

ROYALTY_POLICY_LAP = IRoyaltyPolicyLAP(royaltyPolicyLAP);
DISPUTE_MODULE = IDisputeModule(disputeModule);

_disableInitializers();
}

Expand Down Expand Up @@ -186,6 +195,7 @@ contract IpRoyaltyVault is IIpRoyaltyVault, ERC20SnapshotUpgradeable, Reentrancy
ipId
);

if (DISPUTE_MODULE.isIpTagged(ipId)) revert Errors.IpRoyaltyVault__IpTagged();
if (isClaimedByAncestor[ancestorIpId]) revert Errors.IpRoyaltyVault__AlreadyClaimed();

// check if the address being claimed to is an ancestor
Expand Down Expand Up @@ -216,6 +226,9 @@ contract IpRoyaltyVault is IIpRoyaltyVault, ERC20SnapshotUpgradeable, Reentrancy
/// @param token The revenue token to claim
/// @return The amount of revenue token claimable
function _claimableRevenue(address account, uint256 snapshotId, address token) internal view returns (uint256) {
// if the ip is tagged, then the unclaimed royalties are lost
if (DISPUTE_MODULE.isIpTagged(ipId)) return 0;
jdubpark marked this conversation as resolved.
Show resolved Hide resolved

uint256 balance = balanceOfAt(account, snapshotId);
uint256 totalSupply = totalSupplyAt(snapshotId) - unclaimedAtSnapshot[snapshotId];
uint256 claimableToken = claimableAtSnapshot[snapshotId][token];
Expand Down
18 changes: 2 additions & 16 deletions contracts/modules/royalty/policies/RoyaltyPolicyLAP.sol
Original file line number Diff line number Diff line change
Expand Up @@ -18,20 +18,6 @@ import { Errors } from "../../../lib/Errors.sol";
contract RoyaltyPolicyLAP is IRoyaltyPolicyLAP, GovernableUpgradeable, ReentrancyGuardUpgradeable, UUPSUpgradeable {
using SafeERC20 for IERC20;

/// @notice The state data of the LAP royalty policy
/// @param isUnlinkableToParents Indicates if the ipId is unlinkable to new parents
/// @param ipRoyaltyVault The ip royalty vault address
/// @param royaltyStack The royalty stack of a given ipId is the sum of the royalties to be paid to each ancestors
/// @param ancestorsAddresses The ancestors addresses array
/// @param ancestorsRoyalties The ancestors royalties array
struct LAPRoyaltyData {
bool isUnlinkableToParents;
address ipRoyaltyVault;
uint32 royaltyStack;
address[] ancestorsAddresses;
uint32[] ancestorsRoyalties;
}

/// @dev Storage structure for the RoyaltyPolicyLAP
/// @param ipRoyaltyVaultBeacon The ip royalty vault beacon address
/// @param snapshotInterval The minimum timestamp interval between snapshots
Expand Down Expand Up @@ -118,7 +104,7 @@ contract RoyaltyPolicyLAP is IRoyaltyPolicyLAP, GovernableUpgradeable, Reentranc
address ipId,
bytes calldata licenseData,
bytes calldata externalData
) external onlyRoyaltyModule {
) external onlyRoyaltyModule nonReentrant {
uint32 newLicenseRoyalty = abi.decode(licenseData, (uint32));
RoyaltyPolicyLAPStorage storage $ = _getRoyaltyPolicyLAPStorage();

Expand Down Expand Up @@ -153,7 +139,7 @@ contract RoyaltyPolicyLAP is IRoyaltyPolicyLAP, GovernableUpgradeable, Reentranc
address[] calldata parentIpIds,
bytes[] memory licenseData,
bytes calldata externalData
) external onlyRoyaltyModule {
) external onlyRoyaltyModule nonReentrant {
RoyaltyPolicyLAPStorage storage $ = _getRoyaltyPolicyLAPStorage();
if ($.royaltyData[ipId].isUnlinkableToParents) revert Errors.RoyaltyPolicyLAP__UnlinkableToParents();

Expand Down
1 change: 1 addition & 0 deletions script/foundry/deployment/Main.s.sol
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,7 @@ contract Main is Script, BroadcastManager, JsonDeploymentHandler, StorageLayoutC

function _configureRoyaltyRelated() private {
royaltyModule.setLicensingModule(address(licensingModule));
royaltyModule.setDisputeModule(address(disputeModule));
// whitelist
royaltyModule.whitelistRoyaltyPolicy(address(royaltyPolicyLAP), true);
royaltyModule.whitelistRoyaltyToken(address(erc20), true);
Expand Down
10 changes: 10 additions & 0 deletions test/foundry/mocks/module/MockRoyaltyModule.sol
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ contract MockRoyaltyModule is BaseModule, IRoyaltyModule {

address public LICENSING_MODULE;

address public DISPUTE_MODULE;

mapping(address royaltyPolicy => bool allowed) public isWhitelistedRoyaltyPolicy;

mapping(address token => bool) public isWhitelistedRoyaltyToken;
Expand All @@ -19,6 +21,10 @@ contract MockRoyaltyModule is BaseModule, IRoyaltyModule {

constructor() {}

function setDisputeModule(address _disputeModule) external {
DISPUTE_MODULE = _disputeModule;
}

function setLicensingModule(address _licensingModule) external {
LICENSING_MODULE = _licensingModule;
}
Expand All @@ -27,6 +33,10 @@ contract MockRoyaltyModule is BaseModule, IRoyaltyModule {
return LICENSING_MODULE;
}

function disputeModule() external view override returns (address) {
return DISPUTE_MODULE;
}

function whitelistRoyaltyPolicy(address _royaltyPolicy, bool _allowed) external {
isWhitelistedRoyaltyPolicy[_royaltyPolicy] = _allowed;
}
Expand Down
3 changes: 1 addition & 2 deletions test/foundry/modules/royalty/IpRoyaltyVault.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import { Errors } from "../../../../contracts/lib/Errors.sol";
import { BaseTest } from "../../utils/BaseTest.t.sol";

contract TestIpRoyaltyVault is BaseTest {

IpRoyaltyVault ipRoyaltyVault;

function setUp() public override {
Expand Down Expand Up @@ -257,7 +256,7 @@ contract TestIpRoyaltyVault is BaseTest {
uint256 usdcClaimVaultBefore = ipRoyaltyVault.claimVaultAmount(address(USDC));

uint256 expectedAmount = royaltyAmount - (royaltyAmount * royaltyStack2) / royaltyPolicyLAP.TOTAL_RT_SUPPLY();

vm.expectEmit(true, true, true, true, address(ipRoyaltyVault));
emit IIpRoyaltyVault.RevenueTokenClaimed(address(2), address(USDC), expectedAmount);

Expand Down
Loading
Loading