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
LSP18 Royalties #141
base: main
Are you sure you want to change the base?
LSP18 Royalties #141
Conversation
a468ac5
to
d0a5462
Compare
Hey cool @lykhonis ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @lykhonis for this LIP proposal.
It seems very interesting to me and a very relevant Standard proposal with a lot of potential use cases 🚀
I have added some review comments.
Let's wait for @frozeman @YamenMerhi @skimaharvey and @b00ste review for more insights.
Not that familiar with the NFts ecosystem but looks pretty cool 🚀 . However I am a bit confused about
However when looking at the A few notes I would probably add:
Possible optimization: Change
Pros: won’t need to read, store and maintain at two different storage locations Quick question, as I said I am not super familiar with the NFT environment but wouldn’t it make sense to have the |
6a91b64
to
f25a304
Compare
Hi all, updated the standard with new details and addressed previous comments. Would appreciate a fresh look at this. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
The only thing is that we might face some real challenges with the notion of "enforcing" royalties.
Whether we use a percentage or LYX amount, they can always be bypassed imo.
Honestly not sure how to start answering about "enforcement" part. This is a standard describing metadata. |
Ah ok sorry thought it was a standard that intended to be implemented at the smart contract level |
Yes, we do implement on Universal Page. This is updated standard to how to provide information to marketplaces by creators. This is not a standard or system design that ensures enforcement of royalties on chain. I am confused by your comments honestly. |
THIS STANDARD IS NOT ABOUT ENFORCING ROYALTIES ON CHAIN! Please do read simple summary it is one sentence. |
Since this goes nowhere and the team declines understand why we propose this standard, what it's actually about, we will move forward with it to implement and launch as it's blocking progress of UniversalPage. Closing. |
@lykhonis the focus on the team is the network and up launch right now. Additional standards have less importance internally yet. Tho this doesn’t mean you need to close this. Standard debates can happen over months and years and closing and implement as a single project is likely not leading to adoption. I personal think the buyable nft standard is the better approach to royalties. But I had no time to look at this proposal in the mean time. Also please note. Standard adoption is not determined by a LSP being added to the repo, but by people using it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jakeprins @lykhonis I am glad to see that this PR was re-opened.
I have left some review comments and would like to merge and accept this proposal, as it seems that everyone agrees on having a standard around Royalties.
There are two main points I would like to point out in particular
1. having the settings in both the Array + Map seems not necessary
At least from my viewpoint. I have added in the comments below. I would suggest that the Map data key stores the actual settings, while the array entries stores only the address of the royalty recipients.
But I might lack insights and I don't have the same perspective (seems you have been working on that for a while now).
If you think it's necessary to have the settings in both entries, it would be great if you could explain the reason why it in the standard, so that readers can understand the reason behind this design decision.
2. About the royalty enforcement debate
It seems that the royalty enforcement part is what is bringing the most debate in the proposal. I would suggest the following to help everyone reach consensus.
--> add a Security Consideration section (similar to LSP17, see screenshot below), where you mentioned what you said that "this standard is not about royalty enforcement, but purely for the purpose of setting royalties configurations in a contract". You can as well mention the security aspects brought by @skimaharvey and @CallumGrindle in this section, so that developers who will adopt the standards are aware of these points, and understand that they should use this standard purely for storing royalty settings, and not for enforcing them. Royalty enforcement should be done at the application level (the marketplace itself).
@frozeman I guess we can always use the Buyable NFT standard in the future as an alternative way around Royalties, so that people have options.
LSPs/LSP-18-Royalties.md
Outdated
- royalties value of the type | ||
- `LSP18RoyaltiesRecipientsMap:<address>` is a dynamic address mapping: | ||
- type of address (EOA, Universal Profile, etc) | ||
- receipient's index in the `LSP18RoyaltiesRecipients[]` array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo
- receipient's index in the `LSP18RoyaltiesRecipients[]` array | |
- recipient's index in the `LSP18RoyaltiesRecipients[]` array |
LSPs/LSP-18-Royalties.md
Outdated
- royalties value of the type | ||
- `LSP18RoyaltiesEnforcePayment` is a boolean to enforce royalties whenever the NFT is sold at loss. | ||
|
||
The data key `LSP18RoyaltiesRecipientsMap` exists so that smart contracts can detect if an address is present in the array (e.g. as done in the [LSP1-UniversalReceiverDelegate](./LSP-1-UniversalReceiver.md)). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The data key `LSP18RoyaltiesRecipientsMap` exists so that smart contracts can detect if an address is present in the array (e.g. as done in the [LSP1-UniversalReceiverDelegate](./LSP-1-UniversalReceiver.md)). | |
The data key `LSP18RoyaltiesRecipientsMap` exists so that smart contracts can detect if an address is present in the array (e.g. as done in the [LSP1-UniversalReceiverDelegate](./LSP-1-UniversalReceiver.md)). |
LSPs/LSP-18-Royalties.md
Outdated
The data value MUST be constructed as follows: `address(recipient) + uint8(valueType) + uint256(value)`. Where: | ||
- `recipient` = address of a recipient to receive royalties | ||
- `valueType` = type of royalties: | ||
- `0` = a percentage in points where `100_000` is basis. e.g. `15%` is `15_000` points, and `1.5%` is `1_500` points. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing I am not sure about is that the values uint8(valueType) + uint256(value)
are present in both:
- the
LSP18RoyaltiesRecipients[]
Array data key - the
LSP18RoyaltiesRecipientsMap:<address>
data key.
This could be a problem, as both entries have to be in sync. For instance, what happen if royalty settings get updated and let's say:
- in the Array data key you have
valueType == 0
(a percentage for the type of royalty) (assumed to be the correct setting after update) ✅ - in the Map data key you have
valueType == 1
(a fixed LYX amount) ❌ (old setting that was not updated).
I would suggest 3 options:
option 1: keep it as it is, the settings are in both the Array and Map data key, but the standard specify that they MUST always be in sync (so the implementation code MUST ensure that)
option 2: remove these two values from the Array data key (the Array data key contains only the Royalty recipient's address), and only the Map contains the royalty settings (same as it is done in LSP5, LSP10 and LSP12)
After re-reading the standard, I'm just wondering the need of 3 different keys for that, I think one would be more than enough. Firstly, I think Secondly, the two other keys are really redondant. Is the
Using this key only you would have all the info you need to add royalties (or am I missing something?). And the value would be packed in less than 32 bytes |
That's actually a pretty good point. I agree about this. |
|
How will the concept of "selling at a loss" will work cross-marketplaces since the value is not part of any events? |
7d90c0e
to
bb0ef77
Compare
@lykhonis sorry for the long wait, we now had a proper look internally and the standard looks great, as separating issuers and royalties makes sense. We would have a few points that I will comment on the doc. |
LSPs/LSP-18-Royalties.md
Outdated
status: Draft | ||
type: LSP | ||
created: 2022-11-23 | ||
requires: LSP2, LSP4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is LSP4 required here? I think it is not, we can remove this.
LSPs/LSP-18-Royalties.md
Outdated
|
||
An array of royalties recipients (see [LSP-0-ERC725Account](./LSP-0-ERC725Account.md)). | ||
|
||
The data value MUST be constructed as follows: `address(recipient) + uint32(value)`. Where: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the rationale for choosing uint32
as valueType?
After additional thinking.
I see two scenarios
For small amount of royalty receivers its easy todo a "push" payment distribution. (just look up the royalties array) For large royalty receivers group, you need to use a "pull" contract. e.g. To make both of these use cases possible i would suggest: {
"name": "LSP18RoyaltiesRecipients",
"key": "0xxxxxxxxx",
"keyType": "Singleton",
"valueType": "(bytes4,address,uint32)[CompactBytesArray]",
"valueContent": "(Bytes4,Number,Address)"
} Compact byte array allows for more simple encoded arrays. basically you get back in one single smart contract call Small number of royalties receivers Large number of royalties receivers you can even have a combination of both now.
Benefits
side note: i would remove the dependency on LSP4, that's necessary -> Concerning enforcement data key, what's your thinking here? @lykhonis |
The biggest issue i am seeing is for LSP8 NFTs individual token IDs. one way of doing this could be with an additional map data Key {
"name": "LSP18RoyaltiesRecipients:<tokenID>",
"key": "0xxxxxxxxx000<tokenID>",
"keyType": "Singleton",
"valueType": "(bytes4,address,uint32)[CompactBytesArray]",
"valueContent": "(Bytes4,Number,Address)"
} So you first check if |
I think indeed this is a good change. We can start slowly with a small number of receivers (most likely the most common case anyway) as a single key/value. Enforcing part was more marketplace dependent feature. Rational was for a creator to opt-in to not charge royalties if a NFT is sold at a loss. If I to buy nft at $10 and sell at $5, I will not pay 10% royalties thus loosing even more. |
@CJ42 For the buyable NFT standard. Will there be a way to enforce a fee for the curator? Enforceable royalties for NFT creators are great, but without enforceable royalties/fees for platform creators (like Universal Page), there is no incentive to create the products to discover and trade these assets. This lack of incentive could hurt the NFT creators (and overall ecosystem) even more so I'm interested to hear what you think about this. |
df1022c
to
7248685
Compare
|
We addressed compact array solution in the LSP and implemented it locally in solidity and typescript. I believe it is a good baseline and start. |
@jakeprins sorry for the late response. In the future feel free to ping me in DM on discord. So I see it and answer faster. In my opinion, the “en force” part should be in the marketplace itself. Not the standard. In my opinion, it should be by the fault always requiring royalties. But if it’s on the marketplace level, if anyway depends on the marketplace implementation . If we later add the ability to buy and sell the NFT directly over the nft smart contract, then royalties will be always enforced, or, however, the order book contract attached to the NFT decides to do it. |
@frozeman Yes indeed. However, the original question was more about the "buyable" NFT standard and how it could enforce "royalties" for the creators of NFT platforms (known as platform fees). I think it's important to incentivize the creation and maintenance of these platforms, especially for applications that require a lot of work and money to build and run. Otherwise, the "royalty" problem would simply be shifted from NFT creators to app creators. |
@jakeprins from my perspective the buyable standard doesn't need to enforce royalties for curators/middleman since this is already built in to the overall infrastructure of blockchains. What I mean by this is that end users still needs to pay gas/fees to be able to use the blockchain therefore as a curator/platform they inherently benefit from this feature. So they either abstract it away from end users via ads or simply pay for user's gas by taking a fee. And in my humble opinion, curators should play the game of AND not OR for the best business results ;) |
I also see this enforcable part in the Marketplace itself, so that different market places can come up with different solutions for enforcing royalties, which would create different business models that NFT creators could choose from. The Marketplace could create a LSP17 Extension contract in the form of the order book that contains the royalty enforcement logic (that the Marketplace itself has defined). This way, we can differentiate between the storage layer (the royalty distribution settings, percentage, etc...), and the logic layer (in the LSP17 extension itself). An NFT collection could then decide to move overtime from one royalty distribution medium to another, if it finds a new one more efficient, with additional benefits. |
An array of royalties recipients and corresponding percentages. | ||
|
||
The data value MUST be a [LSP2 BytesCompactArray](./LSP-2-ERC725YJSONSchema.md#bytescompactbytesarray) which contains a list of royalties recipients. Each royalties recipient is a tuple of: | ||
- `interfaceId` = an interface identifing a recipient of royalties. If the interface is not known, it is assumed to be `0xffffffff`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it mean that if it is set to 0xffffffff
, then the interfaceId check is skipped? Which means royalties will be sent to any kind of contract or EOA defined as recipient
?
Co-authored-by: Hugo Masclet <git@hugom.xyz>
The buyable NFT standard would have in the But this dicussion is outside of this standard. @CJ42 @lykhonis lets get thsi standard merges asap, it seems you guys took my suggestions in already. |
} | ||
``` | ||
|
||
A compact byte array allows optionally to store additional fields if needed. Required fields are: `interfaceId` and `recipient`. The `points` field is optional and if not provided it is assumed to be `0` points. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add that if the royalties parties are to many, this can link to a LSP7 that manages withdraws odf royalties and all ropyalties go to that LSP7 contract. so that its clear that it also works for very large amount of royalty receivers.
|
||
A compact byte array allows optionally to store additional fields if needed. Required fields are: `interfaceId` and `recipient`. The `points` field is optional and if not provided it is assumed to be `0` points. | ||
|
||
#### LSP18RoyaltiesEnforcePayment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about this, because its not clear what this is good for, if interfaces/websites/trading contracts anyway make the decision to enfore or not by themselves.
No description provided.