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

Replace 0xSplits with new ERC20 Royalty Vault #26

Merged
merged 29 commits into from
Apr 1, 2024

Conversation

Spablob
Copy link

@Spablob Spablob commented Mar 26, 2024

This pr introduces new logic that replaces 0xSplits tooling with a new Ip Pool:

  • The royalty NFT is replaced with an ERC20 royalty token to increase composability and accuracy in royalty calculations due to the higher number of decimals
  • It eliminates the ancestor pool for code simplification/reduction by moving relevant logic to the Ip Pool contract
  • Claiming is easier to integrate as information lives fully on-chain, whereas previously it lived partly-offchain (0xSplits)

In the Ip Pool contract:

  • Ancestors can "collect" royalty tokens and any accrued royalties once/first
  • Any royalty token holder can "claim" revenue tokens (USDC, USDT, etc)

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.

like this direction. Is ERC20Snapshot a standard token?

@jdubpark
Copy link

like this direction. Is ERC20Snapshot a standard token?

ERC20Snapshot is in OpenZeppelin's v4 codebase but was deprecated in v5.

contracts/modules/royalty/policies/IpPool.sol Outdated Show resolved Hide resolved
contracts/modules/royalty/policies/IpPool.sol Outdated Show resolved Hide resolved
contracts/modules/royalty/policies/IpPool.sol Outdated Show resolved Hide resolved
@jdubpark
Copy link

Have we looked at Votes.sol in OZ v5, which is suppose to replace ERC20Snapshot (and focus more on voting)?

See this comment

To add more context:

  • ERC20Snapshot was released before ERC20Votes, it was a first approach.
  • When ERC20Votes was released it was considered the better option. In particular becauseits a more standard interface that is supported by some third party tools (such as Tally)
  • Our governor is compatible with ERC20Votes (and ERC721Votes), but not with ERC20Snapshot.

Planning for 5.0, we felt that the Votes module was the go to solution, and that ERC20Snapshot was no longer relevant. We decided to deprecate it, and it is now removed for the repo. It remains available in the 4.x packages.

If you really want, you should be able to adapt ERC20Snapshot to 5.0 easily.

@Spablob
Copy link
Author

Spablob commented Mar 30, 2024

Have we looked at Votes.sol in OZ v5, which is suppose to replace ERC20Snapshot (and focus more on voting)?

Yes, we checked. ERC20Snapshot seems more of a fit to our use case.

@Spablob Spablob marked this pull request as ready for review March 30, 2024 08:15
@Spablob Spablob changed the title [WIP] Replace 0xSplits with new ERC20 Ip Pool [WIP] Replace 0xSplits with new ERC20 Royalty Vault Mar 30, 2024
@Spablob Spablob changed the title [WIP] Replace 0xSplits with new ERC20 Royalty Vault Replace 0xSplits with new ERC20 Royalty Vault Mar 31, 2024
package.json 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.

make another batch of review, still need more clarification.

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.

Approve it for the release, a few minor clarifications are still needed, can communicate offline.

@LeoHChen LeoHChen merged commit 81c9c75 into storyprotocol:main Apr 1, 2024
2 checks passed
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.

None yet

4 participants