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

Refactor Licensing with New Features and Simplified Data Structure #33

Merged
merged 20 commits into from
Apr 7, 2024

Conversation

kingster-will
Copy link

@kingster-will kingster-will commented Apr 4, 2024

This PR introduces significant updates to our licensing mechanism, focusing on both refactoring the existing codebase and adding new, highly requested features. Below is a detailed breakdown of the enhancements and refactorings included in this PR.

Refactorings:

  • Introduction of LicenseTemplate: We've abstracted common licensing properties and behaviors into a new LicenseTemplate.
  • Simplification of Data Structure and Data Flow: The data structure and flow have been overhauled to simpilfy complexity.

New Features:

  • Support for Expiration: Licenses can now have an expiration date, IP cannot mint License Token and register derivative once expired.
  • Support for Default License: A default license can now be specified, streamlining the licensing process for common scenarios and reducing the need for manual selection in many cases. This improvement enhances the user experience by simplifying the steps required to obtain a license.
  • Support for Mint Price Hook: We've introduced a mint price hook, enabling dynamic pricing based on 3rd party hook, This feature offers the flexibility to adjust pricing in real-time, accommodating promotions, demand fluctuations, or other pricing strategies.
  • Support for Specific Receiver: IP owner can now specific a hook to check the license token receiver is expected or not.

@kingster-will kingster-will requested a review from Ramarti April 4, 2024 16:56
@jdubpark
Copy link

jdubpark commented Apr 4, 2024

I think we should name this a V1 for the mainnet release (or no suffix, still indicates first version) and deprecate the old LicenseRegistry and LicensingModule.

@kingster-will
Copy link
Author

I think we should name this a V1 for the mainnet release (or no suffix, still indicates first version) and deprecate the old LicenseRegistry and LicensingModule.

Yes, this is a temporary name, for easier testing. will rename and remove the "V2" after fully reviewed.

Copy link

@Ramarti Ramarti left a comment

Choose a reason for hiding this comment

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

Please go over current comments and add Natspec.
This is a big PR, more context is needed for a second review pass on the actual logic.
Thanks

contracts/interfaces/registries/ILicenseNFT.sol Outdated Show resolved Hide resolved
contracts/registries/LicenseRegistryV2.sol Outdated Show resolved Hide resolved
contracts/registries/LicenseRegistryV2.sol Outdated Show resolved Hide resolved
contracts/registries/LicenseRegistryV2.sol Outdated Show resolved Hide resolved
contracts/registries/LicenseRegistryV2.sol Outdated Show resolved Hide resolved
contracts/registries/LicenseRegistryV2.sol Outdated Show resolved Hide resolved
contracts/lib/Errors.sol Outdated Show resolved Hide resolved
Copy link
Member

@LeoHChen LeoHChen left a comment

Choose a reason for hiding this comment

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

done first review, lots of code and need to compare with previous version to make sure business logic are the same. Also needs extensive usecase testing to make sure there is no regression.

Copy link
Member

@LeoHChen LeoHChen left a comment

Choose a reason for hiding this comment

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

get a 2nd pass, add a few more review comments.

@kingster-will kingster-will force-pushed the v1/licensing-template branch from ef90458 to 15b0b9c Compare April 6, 2024 14:15
@kingster-will kingster-will marked this pull request as ready for review April 6, 2024 14:25
contracts/LicenseToken.sol Outdated Show resolved Hide resolved
foundry.toml Show resolved Hide resolved
contracts/modules/licensing/PILicenseTemplate.sol Outdated Show resolved Hide resolved
contracts/LicenseToken.sol Show resolved Hide resolved
Copy link
Member

@LeoHChen LeoHChen left a comment

Choose a reason for hiding this comment

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

a few minor comments

contracts/LicenseToken.sol Outdated Show resolved Hide resolved
contracts/LicenseToken.sol Show resolved Hide resolved
@LeoHChen LeoHChen merged commit ca20a9d into storyprotocol:main Apr 7, 2024
2 checks passed
jdubpark added a commit to jdubpark/sp-protocol-core that referenced this pull request Apr 8, 2024
jdubpark added a commit to jdubpark/sp-protocol-core that referenced this pull request Apr 8, 2024
jdubpark added a commit to jdubpark/sp-protocol-core that referenced this pull request Apr 9, 2024
LeoHChen pushed a commit that referenced this pull request Apr 9, 2024
Re-enable all protocol tests disabled in PR #33 by integrating the new license system into existing tests.

Additionally, it removes all the PIL Policy Framework tests as the structure of the PIL and license system has changed significantly. We will require more tests for the new PILicenseTemplate that closely reflects the previous test cases of the PIL Policy Framework.
Ramarti pushed a commit to Ramarti/protocol-core-v1 that referenced this pull request Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants