Skip to content

Commit

Permalink
Improved Unit & Integration Test (#231)
Browse files Browse the repository at this point in the history 
* Remove 'indexed' from string relType events

* Module registry events test

* Add make coverage script

* Add more access control tests

* Add more IPOrg (controller) tests

* fix: IPOrgTransferred event emit, cache record.owner for prevOwner event arg

* Add more test for IPOrg controller

Coverage except internal functions and `initialize` for upgradable. Need
to use harness for internal functions.

* Add basic tests for FixedSet util

* fix: FixedSet.UintSet indexOf argument signature

* Add integration tests to full line coverage

* Add coverage instruction

* Updated tests for e2e and modules

* Contract bug fix & comment nit

* lcov update

* Fix licensing module test cases

* Separate tests & remove lcov file

* Update integration tests

* Small fixes for contracts & interfaces

* Update e2e and module tests & comment out unready fn

* Updated tests

* More integration tests

* Fix param comment

* Add integration tests, fix unit tests, fix contract bugs

* Remove commented out function

* Update LicensingModule.Licensing.t.sol

* Update LicensingModule.Config.t.sol
  • Loading branch information
@jdubpark
jdubpark authored Dec 8, 2023
1 parent 8946f75 commit b739da8
Show file tree
Hide file tree
Showing 14 changed files with 1,468 additions and 356 deletions.
5 changes: 4 additions & 1 deletion contracts/interfaces/ip-org/IIPOrg.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import { IPAsset } from "contracts/lib/IPAsset.sol";

/// @notice IP Org Interface
interface IIPOrg {

/// @notice Returns the current owner of the IP asset within th IP Org.
function ownerOf(uint256 id) external view returns (address);

Expand Down Expand Up @@ -37,4 +36,8 @@ interface IIPOrg {
/// @notice Returns the Ip Org asset type for a given IP Org asset.
function ipOrgAssetType(uint256 id_) external view returns (uint8);

/// @notice Gets the global IP asset id associated with this IP Org asset.
/// @param id_ The local id of the IP Org wrapped IP asset.
/// @return The global identifier of the IP asset.
function ipAssetId(uint256 id_) external returns (uint256);
}
6 changes: 3 additions & 3 deletions contracts/ip-org/IPOrg.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,11 +85,11 @@ contract IPOrg is
}

/// @notice Gets the global IP asset id associated with this IP Org asset.
/// @param id The local id of the IP Org wrapped IP asset.
/// @param id_ The local id of the IP Org wrapped IP asset.
/// @return The global identifier of the IP asset.
function ipAssetId(uint256 id) public returns (uint256) {
function ipAssetId(uint256 id_) public returns (uint256) {
address registrationModule = address(MODULE_REGISTRY.protocolModule(REGISTRATION_MODULE_KEY));
return IRegistrationModule(registrationModule).ipAssetId(address(this), id);
return IRegistrationModule(registrationModule).ipAssetId(address(this), id_);
}

/// @notice Initializes an IP Org.
Expand Down
9 changes: 0 additions & 9 deletions contracts/ip-org/IPOrgController.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -208,15 +208,6 @@ contract IPOrgController is
}
}

/// @dev Checks whether an IP Org exists, throwing if not.
/// @param ipOrg_ The address of the IP Org being queried.
function _assertIPOrgExists(address ipOrg_) internal view {
IPOrgControllerStorage storage $ = _getIpOrgControllerStorage();
if (!$.ipOrgs[ipOrg_].registered) {
revert Errors.IPOrgController_IPOrgNonExistent();
}
}

/// @dev Authorizes upgrade to a new contract address via UUPS.
function _authorizeUpgrade(address)
internal
Expand Down
1 change: 1 addition & 0 deletions contracts/lib/Errors.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,7 @@ library Errors {
error LicensingModule_InvalidAction();
error LicensingModule_CallerNotLicensor();
error LicensingModule_ParentLicenseNotActive();
error LicensingModule_DerivativeNotAllowed();
error LicensingModule_InvalidIpa();
error LicensingModule_CallerNotLicenseOwner();
error LicensingModule_CantFindParentLicenseOrRelatedIpa();
Expand Down
4 changes: 4 additions & 0 deletions contracts/modules/licensing/LicenseRegistry.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,10 @@ contract LicenseRegistry is ERC721 {
return _licenses[id_].isReciprocal;
}

function isDerivativeAllowed(uint256 id_) external view returns (bool) {
return _licenses[id_].derivativesAllowed;
}

function derivativeNeedsApproval(uint256 id_) external view returns (bool) {
return _licenses[id_].derivativeNeedsApproval;
}
Expand Down
54 changes: 34 additions & 20 deletions contracts/modules/licensing/LicensingModule.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,9 @@ contract LicensingModule is BaseModule, ILicensingModule {
if (!LICENSE_REGISTRY.isLicenseActive(input.parentLicenseId)) {
revert Errors.LicensingModule_ParentLicenseNotActive();
}
if (!LICENSE_REGISTRY.isDerivativeAllowed(input.parentLicenseId)) {
revert Errors.LicensingModule_DerivativeNotAllowed();
}
}
// If this is a derivative and parent is reciprocal, license parameters
// cannot be changed in the new license
Expand All @@ -159,8 +162,7 @@ contract LicensingModule is BaseModule, ILicensingModule {
)
);
} else {
// If this is not a derivative, or parent is not reciprocal, caller must be
// the licensor
// If this is not a derivative, or parent is not reciprocal, caller must be the licensor
if (licensor != caller_) {
revert Errors.LicensingModule_CallerNotLicensor();
}
Expand Down Expand Up @@ -196,14 +198,23 @@ contract LicensingModule is BaseModule, ILicensingModule {
bool derivativesAllowed,
bool isReciprocal,
bool derivativeNeedsApproval
) = _parseLicenseParameters(
ipOrg_,
input.params,
supportedParams
);
) = _parseLicenseParameters(ipOrg_, input.params, supportedParams);

Licensing.LicenseStatus newLicenseStatus;
if (
input.parentLicenseId != 0 &&
LICENSE_REGISTRY.derivativeNeedsApproval(input.parentLicenseId)
) {
// If parent license ID has `derivativeNeedsApproval` = true, then new license is pending licensor approval.
// This condition is triggered when parent's `isReciprocal` = false but `derivativeNeedsApproval` = true.
newLicenseStatus = Licensing.LicenseStatus.PendingLicensorApproval;
} else {
newLicenseStatus = Licensing.LicenseStatus.Active;
}

// Create license
Licensing.LicenseData memory newLicense = Licensing.LicenseData({
status: Licensing.LicenseStatus.Active,
status: newLicenseStatus,
derivativesAllowed: derivativesAllowed,
isReciprocal: isReciprocal,
derivativeNeedsApproval: derivativeNeedsApproval,
Expand All @@ -221,17 +232,23 @@ contract LicensingModule is BaseModule, ILicensingModule {
address ipOrg_,
Licensing.ParamValue[] memory inputParams_,
Licensing.ParamDefinition[] memory supportedParams_
) private view returns (
Licensing.ParamValue[] memory licenseParams,
bool derivativesAllowed,
bool derivativeNeedsApproval,
bool isReciprocal
) {
)
private
view
returns (
Licensing.ParamValue[] memory licenseParams,
bool derivativesAllowed,
bool isReciprocal,
bool derivativeNeedsApproval
)
{
uint256 inputLength_ = inputParams_.length;
mapping(ShortString => bytes) storage _ipOrgValues = _ipOrgParamValues[ipOrg_];
mapping(ShortString => bytes) storage _ipOrgValues = _ipOrgParamValues[
ipOrg_
];
uint256 supportedLength = supportedParams_.length;
licenseParams = new Licensing.ParamValue[](supportedLength);

// First, get ipOrg defaults
for (uint256 i = 0; i < supportedLength; i++) {
// For every supported parameter
Expand Down Expand Up @@ -283,7 +300,6 @@ contract LicensingModule is BaseModule, ILicensingModule {
derivativeNeedsApproval = false;
isReciprocal = false;
}
return (licenseParams, derivativesAllowed, derivativeNeedsApproval, isReciprocal);
}

function _decideValueSource(
Expand Down Expand Up @@ -313,9 +329,7 @@ contract LicensingModule is BaseModule, ILicensingModule {
ShortString tag,
bytes memory resultValue,
bool derivativesAllowed
) private pure returns (bool) {

}
) private pure returns (bool) {}

/// Gets the licensor address for this IPA.
function _getLicensor(
Expand Down
48 changes: 24 additions & 24 deletions contracts/modules/registration/RegistrationModule.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ contract RegistrationModule is BaseModule, IRegistrationModule, AccessControlled
/// @param ipOrg_ The IP Org for which the hooks are being registered.
/// @param hooks_ The addresses of the hooks to register.
/// @param hooksConfig_ The configurations for the hooks.
/// @param registerParams_ The parameters for the registration.
function registerHooks(
HookType hType_,
IIPOrg ipOrg_,
Expand Down Expand Up @@ -297,30 +298,29 @@ contract RegistrationModule is BaseModule, IRegistrationModule, AccessControlled
/// @param toIpOrg_ The address of the new governing IP Org.
/// @param toIpOrgType_ The type of the IP asset within the new IP Org.
/// TODO(leeren) Expose this function to FE once IP Orgs are finalized.
function _transferIPAssetToIPOrg(
address fromIpOrg_,
uint256 fromIpOrgAssetId_,
address toIpOrg_,
uint8 toIpOrgType_,
address from_,
address to_
) internal returns (uint256 ipAssetId_, uint256 ipOrgAssetId_) {
uint256 id = ipAssetId[address(fromIpOrg_)][fromIpOrgAssetId_];

address owner = IIPOrg(fromIpOrg_).ownerOf(ipOrgAssetId_);

delete ipAssetId[address(fromIpOrg_)][fromIpOrgAssetId_];
delete ipOrgAssets[id];
IIPOrg(fromIpOrg_).burn(ipOrgAssetId_);
IPA_REGISTRY.transferIPOrg(
ipAssetId_,
toIpOrg_
);
ipOrgAssetId_ = IIPOrg(toIpOrg_).mint(owner, toIpOrgType_);
IPOrgAsset memory ipOrgAsset = IPOrgAsset(toIpOrg_, ipOrgAssetId_);
ipOrgAssets[id] = ipOrgAsset;
ipAssetId[address(toIpOrg_)][ipOrgAssetId_] = id;
}
/// NOTE: This function is currently not used, but will be used in the future. Commented out for now.
// function _transferIPAssetToIPOrg(
// address fromIpOrg_,
// uint256 fromIpOrgAssetId_,
// address toIpOrg_,
// uint8 toIpOrgType_,
// address from_,
// address to_
// ) internal returns (uint256 ipAssetId_, uint256 ipOrgAssetId_) {
// uint256 id = ipAssetId[address(fromIpOrg_)][fromIpOrgAssetId_];
// address owner = IIPOrg(fromIpOrg_).ownerOf(ipOrgAssetId_);
// delete ipAssetId[address(fromIpOrg_)][fromIpOrgAssetId_];
// delete ipOrgAssets[id];
// IIPOrg(fromIpOrg_).burn(ipOrgAssetId_);
// IPA_REGISTRY.transferIPOrg(
// ipAssetId_,
// toIpOrg_
// );
// ipOrgAssetId_ = IIPOrg(toIpOrg_).mint(owner, toIpOrgType_);
// IPOrgAsset memory ipOrgAsset = IPOrgAsset(toIpOrg_, ipOrgAssetId_);
// ipOrgAssets[id] = ipOrgAsset;
// ipAssetId[address(toIpOrg_)][ipOrgAssetId_] = id;
// }


/// @dev Adds new IP asset types to an IP Org.
Expand Down
Loading

0 comments on commit b739da8

Please sign in to comment.