Dispute effects on royalty #35
Conversation
Spablob
requested review from
kingster-will,
Ramarti,
leeren,
jdubpark,
LeoHChen and
AndyBoWu
as code owners
contracts/interfaces/modules/royalty/policies/IRoyaltyPolicyLAP.sol
Outdated
Show resolved
Hide resolved
| /// @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 { |
There was a problem hiding this comment.
is there any redundancy of those data with license data?
| /// @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; | ||
|
|
There was a problem hiding this comment.
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); | ||
|
|
There was a problem hiding this comment.
These two can also be removed, as we don't expect the functions to be called by another contract.
| /// @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; | ||
| } | ||
|
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
agree, we will leave those changes to next PR @Spablob
|
I will merge this PR, all open comments can be addressed in the next PR @Spablob . Thanks. |
This pr adds royalty related restrictions when IP assets are tagged via dispute: