Fix Derivative Registration, IP Graph & Solady's ERC6551 Integration, and Update Protocol Packages #30
Conversation
sebsadface
requested review from
kingster-will,
Ramarti,
leeren,
jdubpark,
Spablob,
LeoHChen and
AndyBoWu
as code owners
WalkthroughThis update enhances the Changes
Assessment against linked issues
Possibly related 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: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
Files selected for processing (10)
- contracts/SPGNFT.sol (2 hunks)
- contracts/StoryProtocolGateway.sol (11 hunks)
- contracts/lib/Errors.sol (1 hunks)
- package.json (1 hunks)
- remappings.txt (1 hunks)
- script/UpgradeSPG.s.sol (1 hunks)
- script/UpgradeSPGNFT.s.sol (1 hunks)
- test/StoryProtocolGateway.t.sol (8 hunks)
- test/mocks/MockIPGraph.sol (1 hunks)
- test/utils/BaseTest.t.sol (3 hunks)
Additional context used
LanguageTool
remappings.txt
[misspelling] ~4-~4: Hoewel het woord correct kan zijn, is er een grote kans dat het een typefout is van “noden”. Zo niet, dan is het waarschijnlijk een ouderwetse uitdrukking.
Context: ...ory-protocol/create3-deployer/ @solady/=node_modules/solady/ ds-test/=lib/forge-std/...(MACHTE)
Additional comments not posted (35)
remappings.txt (1)
4-4: LGTM!The addition of the
@soladyremapping is correct and aligns with the project's structure.Tools
LanguageTool
[misspelling] ~4-~4: Hoewel het woord correct kan zijn, is er een grote kans dat het een typefout is van “noden”. Zo niet, dan is het waarschijnlijk een ouderwetse uitdrukking.
Context: ...ory-protocol/create3-deployer/ @solady/=node_modules/solady/ ds-test/=lib/forge-std/...(MACHTE)
contracts/lib/Errors.sol (1)
16-17: LGTM!The new error
SPG__CallerAndNotTokenOwneris well-defined and enhances error reporting for unauthorized token access.package.json (1)
44-44: LGTM!The addition of the
erc6551dependency is appropriate for supporting ERC-6551 functionalities.script/UpgradeSPGNFT.s.sol (1)
23-23: Verify the change tocreate3SaltSeed.Changing
create3SaltSeedfrom15to12may impact contract deployment. Verify that this change aligns with the intended deployment strategy.script/UpgradeSPG.s.sol (1)
59-60: Verify constructor parameter updates.Ensure all constructor calls to
UpgradeSPGare updated to includelicenseRegistryAddrandroyaltyModuleAddr.test/mocks/MockIPGraph.sol (10)
15-19: LGTM!The
addParentIpfunction correctly adds parent IPs to the mapping.
20-22: LGTM!The
hasParentIpfunction efficiently checks for the existence of a parent IP.
23-25: LGTM!The
getParentIpsfunction correctly returns all parent IPs usingvalues.
26-28: LGTM!The
getParentIpsCountfunction correctly returns the count of parent IPs.
29-45: LGTM!The
hasAncestorIpfunction correctly implements a breadth-first search to check for ancestor IPs.
47-61: LGTM!The
getAncestorIpsCountfunction correctly counts ancestor IPs using a breadth-first search.
69-71: LGTM!The
setRoyaltyfunction correctly updates the royalty percentage for IPs.
72-89: LGTM!The
getRoyaltyfunction correctly calculates total royalties by traversing the IP graph.
91-106: LGTM!The
getRoyaltyStackfunction correctly calculates the total royalty stack for an IP ID.
114-119: LGTM!The
_toBytes32and_toAddressfunctions correctly handle type conversions.contracts/SPGNFT.sol (7)
Line range hint
47-73: LGTM!The
initializefunction correctly sets up the contract and checks for zero addresses.
120-122: LGTM!The addition of the
virtualkeyword to themintfunction is appropriate for extensibility.
133-135: LGTM!The addition of the
virtualkeyword to themintBySPGfunction is appropriate for extensibility.
Line range hint
139-141: LGTM!The
withdrawTokenfunction correctly handles token transfers usingIERC20.transfer.
Line range hint
145-147: LGTM!The
supportsInterfacefunction correctly checks for supported interfaces.
Line range hint
151-166: LGTM!The
_mintTokenfunction correctly handles NFT minting and metadata setting.
Line range hint
183-186: LGTM!The
_getSPGNFTStoragefunction correctly accesses the storage struct using inline assembly.test/utils/BaseTest.t.sol (3)
28-28: LGTM!The import statement for
MockIPGraphis necessary for testing purposes.
75-75: LGTM!The use of
vm.etchto deployMockIPGraphensures the mock contract's bytecode is available for testing.
59-59: LGTM! But verify the address usage.The initialization of
MockIPGraphwith a specific address is appropriate for testing.Ensure that the address
0x1Adoes not conflict with other tests.Verification successful
Address
0x1AUsage VerifiedThe address
0x1Ais used in the initialization ofMockIPGraphand in a related context involvingvm.etch. There are no other conflicting uses of this address in the Solidity files.
- The address
0x1Ais used in:
- Initialization of
MockIPGraph.vm.etchwithMockIPGraph.No conflicts were found with other tests or usages.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the address `0x1A` is not used elsewhere in tests. # Test: Search for the address usage. Expect: Only occurrences related to `MockIPGraph`. rg --type solidity '0x1A'Length of output: 58
Script:
#!/bin/bash # Description: Verify that the address `0x1A` is not used elsewhere in Solidity tests. # Find all Solidity files and search for the address usage. fd --extension sol --exec rg '0x1A' {}Length of output: 159
test/StoryProtocolGateway.t.sol (7)
Line range hint
514-560: LGTM!The
_getSetPermissionSignatureForSPGfunction correctly usesstateinstead ofnoncefor permission setting.
Line range hint
137-153: LGTM!The
test_SPG_registerIptest case correctly reflects the new logic with thestateparameter.
178-183: LGTM!The
test_SPG_registerPILTermsAndAttachtest case correctly reflects the new logic with thestateparameter.
257-271: LGTM!The
test_SPG_registerIpAndAttachPILTermstest case correctly reflects the new logic with thestateparameter.
439-452: LGTM!The
test_SPG_registerIpAndMakeDerivativeWithLicenseTokenstest case correctly reflects the new logic with thestateparameter.
Line range hint
572-612: LGTM!The
_mintAndRegisterIpAndMakeDerivativeBaseTestfunction correctly handles the logic for minting and registering derivatives.
Line range hint
618-658: LGTM!The
_registerIpAndMakeDerivativeBaseTestfunction correctly handles the logic for registering IPs and making derivatives.contracts/StoryProtocolGateway.sol (3)
6-6: LGTM!The addition of the
ERC721Holderinterface allows the contract to safely receive ERC721 tokens.
482-495: LGTM!The
_collectLicenseTokensfunction enhances security by ensuring only the owner can transfer license tokens.
Line range hint
509-539: LGTM!Changing function visibility from
privatetointernalallows for greater extensibility.Also applies to: 565-597
Description
This PR introduces several updates and integrations to SPG, including:
Test
Updated existing tests to align with core protocol changes.
Related Issue
This PR closes:
Summary by CodeRabbit
MockIPGraph, for managing intellectual property relationships.SPGNFTto allow more extensibility.StoryProtocolGatewaycontract with stricter ownership checks.