fix(register-derivatives): collect and approve minting fees for commercial licenses #23
Conversation
sebsadface
requested review from
kingster-will,
Ramarti,
leeren,
jdubpark,
Spablob,
LeoHChen and
AndyBoWu
as code owners
WalkthroughThe recent updates enhance the functionality of the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SPG
participant LicensingModule
participant RoyaltyModule
User->>SPG: Register IP Asset
SPG->>LicensingModule: Call registerDerivative
LicensingModule-->>SPG: Return success
SPG->>RoyaltyModule: Initiate royalty distribution
RoyaltyModule-->>SPG: Confirm royalty setup
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- contracts/SPGNFT.sol (1 hunks)
- contracts/StoryProtocolGateway.sol (8 hunks)
- script/Main.s.sol (1 hunks)
- script/utils/StoryProtocolCoreAddressManager.sol (2 hunks)
- test/StoryProtocolGateway.t.sol (5 hunks)
- test/utils/BaseTest.t.sol (7 hunks)
Files skipped from review due to trivial changes (1)
- contracts/SPGNFT.sol
Additional comments not posted (25)
script/utils/StoryProtocolCoreAddressManager.sol (1)
13-14: LGTM! Verify JSON path usage.The addition of
licenseRegistryAddrandroyaltyModuleAddrlooks good. Ensure that the JSON paths.main.LicenseRegistryand.main.RoyaltyModuleare correct and that the JSON file structure aligns with these paths.script/Main.s.sol (1)
60-61: LGTM! Verify constructor usage.The addition of
licenseRegistryAddrandroyaltyModuleAddrto the deployment process looks good. Ensure that these addresses are passed correctly to theMaincontract's constructor and used appropriately in the deployment logic.Verification successful
Addresses are used correctly in deployment logic.
The
licenseRegistryAddrandroyaltyModuleAddrare not passed to theMaincontract's constructor directly. Instead, they are used as parameters in the instantiation of theStoryProtocolGatewaycontract within the_deployProtocolContractsfunction, which is part of the deployment logic executed by therunfunction. This usage aligns with the intended deployment process.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify constructor usage for licenseRegistryAddr and royaltyModuleAddr. # Test: Search for the Main contract instantiation. Expect: Correct parameters passed to the constructor. rg --type solidity -A 5 $'new Main'Length of output: 69
Script:
#!/bin/bash # Description: Verify constructor usage for licenseRegistryAddr and royaltyModuleAddr. # Test: Search for the Main contract instantiation in Solidity files. Expect: Correct parameters passed to the constructor. rg 'new Main' --glob '*.sol' -A 5Length of output: 35
Script:
#!/bin/bash # Description: Search for the Main contract definition and its constructor to verify parameter usage. # Test: Locate the Main contract definition and constructor in Solidity files. rg 'contract Main' --glob '*.sol' -A 10Length of output: 709
Script:
#!/bin/bash # Description: Search for the usage of licenseRegistryAddr and royaltyModuleAddr in the Main contract. # Test: Locate references to licenseRegistryAddr and royaltyModuleAddr in the Main contract. rg 'licenseRegistryAddr|royaltyModuleAddr' --glob 'script/Main.s.sol' -A 5Length of output: 403
Script:
#!/bin/bash # Description: Identify the function or contract call using licenseRegistryAddr and royaltyModuleAddr. # Test: Search for the function or contract call that includes these parameters in Main.s.sol. rg 'function|contract' --glob 'script/Main.s.sol' -A 10Length of output: 3745
test/utils/BaseTest.t.sol (8)
44-47: LGTM! Verify variable initialization.The initialization of
royaltyModule,royaltyPolicyLAP,ipRoyaltyVaultBeacon, andipRoyaltyVaultImpllooks correct. Ensure these are initialized properly in thesetUp_test_Corefunction.Verification successful
Initialization Verified in
setUp_test_CoreFunctionThe variables
royaltyModule,royaltyPolicyLAP,ipRoyaltyVaultBeacon, andipRoyaltyVaultImplare properly initialized within the codebase. Here are the relevant initializations:
royaltyModuleis initialized withRoyaltyModule(...).royaltyPolicyLAPis initialized withRoyaltyPolicyLAP(...).ipRoyaltyVaultImplis initialized withIpRoyaltyVault(...).ipRoyaltyVaultBeaconis initialized withUpgradeableBeacon(...).These initializations ensure that the variables are set up correctly in the
setUp_test_Corefunction or its equivalent setup context.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify initialization of new variables. # Test: Search for the initialization of royaltyModule, royaltyPolicyLAP, ipRoyaltyVaultBeacon, and ipRoyaltyVaultImpl. rg --type solidity -A 5 $'royaltyModule|royaltyPolicyLAP|ipRoyaltyVaultBeacon|ipRoyaltyVaultImpl'Length of output: 131
Script:
#!/bin/bash # Description: Verify initialization of new variables. # Find all Solidity files and search for the initialization of the specified variables. fd --extension sol --exec rg 'royaltyModule|royaltyPolicyLAP|ipRoyaltyVaultBeacon|ipRoyaltyVaultImpl' {}Length of output: 2019
243-267: LGTM! Verify RoyaltyPolicyLAP deployment.The deployment and initialization of
RoyaltyPolicyLAPappear correct. Ensure that the proxy setup and address checks are accurate.Verification successful
RoyaltyPolicyLAP Deployment and Proxy Setup Verified
The deployment and proxy setup of
RoyaltyPolicyLAPare correctly implemented. The address and proxy implementation checks are present and align with the expected setup.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify RoyaltyPolicyLAP deployment and proxy setup. # Test: Search for the deployment and proxy setup of RoyaltyPolicyLAP. Expect: Correct setup and address checks. rg --type solidity -A 5 $'new RoyaltyPolicyLAP'Length of output: 81
Script:
#!/bin/bash # Find all Solidity files and search for the deployment and proxy setup of RoyaltyPolicyLAP. fd -e sol -x rg 'new RoyaltyPolicyLAP' {} -A 5Length of output: 346
204-204: LGTM! Verify RoyaltyModule deployment.The deployment of
RoyaltyModuleis consistent with the existing pattern. Ensure the deployment logic aligns with the expected module setup.Verification successful
RoyaltyModule Deployment Verified
The deployment of
RoyaltyModulefollows the expected modular setup pattern, using addresses from other modules and registries. This aligns with the existing deployment logic.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify RoyaltyModule deployment logic. # Test: Search for the deployment and initialization of RoyaltyModule. Expect: Logic should match the expected pattern. rg --type solidity -A 5 $'new RoyaltyModule'Length of output: 78
Script:
#!/bin/bash # Description: Verify RoyaltyModule deployment logic. # Test: Search for the deployment and initialization of RoyaltyModule. Expect: Logic should match the expected pattern. rg 'new RoyaltyModule' -A 5Length of output: 387
368-369: LGTM! Verify StoryProtocolGateway deployment.The deployment of
StoryProtocolGatewaywith the new parameters looks good. Ensure the parameters are passed correctly and the deployment logic is sound.
268-276: LGTM! Verify IpRoyaltyVault deployment.The deployment of
IpRoyaltyVaultand the setup ofipRoyaltyVaultBeaconlook correct. Ensure that these components are correctly integrated into the system.
357-359: LGTM! Verify RoyaltyPolicyLAP integration.The integration of
RoyaltyPolicyLAPwith the IP royalty vault beacon is correctly implemented. Ensure this integration is tested and functions as expected.
19-20: LGTM! Verify new imports.The imports for
RoyaltyPolicyLAPandIpRoyaltyVaultare correctly added. Ensure that these modules are available and correctly integrated into the project.
408-408: LGTM! Verify royalty token whitelisting.The whitelisting of the royalty token is correctly implemented. Ensure this functionality is tested and operates as intended.
test/StoryProtocolGateway.t.sol (9)
208-210: LGTM!The changes to the
withEnoughTokensmodifier correctly reflect the need for increased token balance and approval for thespgcontract.
284-284: LGTM!Renaming the modifier to
withNonCommercialParentIpimproves clarity and aligns with the test's focus on non-commercial licenses.
295-303: LGTM!Renaming the function and using a base test function improves clarity and reusability in testing non-commercial licenses.
305-313: LGTM!The renaming and use of a base test function enhance clarity and logic reuse for testing non-commercial licenses.
315-327: LGTM!The
withCommercialParentIpmodifier is well-defined for setting up tests with commercial license terms.
330-338: LGTM!The function effectively tests derivatives with commercial licenses using a base test function, enhancing clarity and reusability.
340-347: LGTM!Renaming the function and using a base test function improve clarity and logic reuse for testing commercial licenses.
549-591: LGTM!The
_mintAndRegisterIpAndMakeDerivativeBaseTestfunction effectively centralizes common logic, enhancing maintainability and reusability.
593-657: LGTM!The
_registerIpAndMakeDerivativeBaseTestfunction centralizes logic for registering derivatives, improving maintainability and reusability.contracts/StoryProtocolGateway.sol (6)
16-21: LGTM!The new imports for licensing and royalty interfaces are necessary for the enhanced functionalities in the contract.
56-61: LGTM!The addition of
LICENSE_REGISTRYandROYALTY_MODULEstate variables is essential for the contract's new licensing and royalty functionalities.
Line range hint
83-103:
LGTM!The constructor modifications correctly incorporate parameters for the license registry and royalty module, aligning with the new functionalities.
482-511: LGTM!The
_collectMintFeesAndSetApprovalfunction effectively handles mint fees and sets approval, aligning with the new licensing processes.
513-536: LGTM!The
_aggregateMintFeesfunction accurately calculates total mint fees, supporting the contract's enhanced fee management.
538-583: LGTM!The
_getMintFeeForSingleParentfunction dynamically calculates mint fees, enhancing flexibility in the contract's fee structures.
sebsadface
deleted the
issue-21-fails-to-register-derivative-with-commercial-license
branch
Description
This PR fixes an issue where calling
registerIpAndMakeDerivativeandmintAndRegisterIpAndMakeDerivativeon parent IPs with commercial licenses throws anERC20InsufficientAllowanceerror.Key Changes
_collectMintFeesAndSetApprovaland helper functions to:_collectMintFeesAndSetApprovalinregisterIpAndMakeDerivativeandmintAndRegisterIpAndMakeDerivativeprior to registering the derivative.Tests
Related Issue
This PR closes #21.
Notes
Users must approve an additional transaction to transfer the minting fees to the SPG contract when calling
registerIpAndMakeDerivativeandmintAndRegisterIpAndMakeDerivative.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores