-
Notifications
You must be signed in to change notification settings - Fork 314
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
CIP-0102? | Royalty Datum Metadata #551
CIP-0102? | Royalty Datum Metadata #551
Conversation
Interesting proposal! I'll have to read it more in-depth over the weekend, but your approach is promising. |
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've added this to the agenda for the upcoming scheduled CIP meeting on 01 August (https://hackmd.io/@cip-editors/70). The most important question to answer ASAP with a quick review & debate there (if not settled here already) is whether this needs to be a separate CIP.
1 - New token types: do they belong in CIP68 at all?
We have a precedent in CIP13 (pending #559) so that it will only define a general framework for interpretation + the original 2 protocols it was conceived with... with further CIPs submitted for further protocol additions. For CIP13 that would be newly defined URI authorities like //claim
, and for CIP68 this would be the new token types 500
and thereafter.
As the author of that proposal and editor I would prefer new CIPs for the new token types, with the author of that type clearly defined in the new CIP header as well as the PR... not so that each author can "take credit" but more importantly to assign responsibilities for review, acceptance, and implementation. (cc @KtorZ)
Most vitally I foresee that CIP68 is going to be monstrous — both in size and editorial difficulty, given we are already seeing concurrent work on different token types (currently 444
and 500
) — if new types keep being added to CIP68 itself.
2 - Versioning
Some versioning needs to be done to ensure metadata processors aren't thrown off by the new 500
type (@mmahut claimed this was a problem with Blockfrost and 444
). The last decision as per #523 is that:
- the version number refers to the whole protocol, and
- the version field specified for each token type indicates in which versions this token type is supported.
So if that explanation & my understanding are correct, if keeping this addition in CIP68 itself you would need to:
- change the overall CIP68 version to 3;
- update the existing token types to add a version
3
as an acceptable value; - define your
500
token as only being valid for version3
(currently it just says1
, as all new entries would in our CIPmain
branch without CIP-0068 | Bump version for RFT #523 not being merged yet).
I won't suggest code changes here because
- it would have to be rebased from a merge conflict with the still unmerged CIP-0068 | Bump version for RFT #523
- this might end up being defined in a separate CIP anyway.
3 - Sources
I would assume your "Collaborator Discussion" ikigai-github#2 didn't include anyone outside Ikigai. If so then there's no reason to change anything, but if not then it would be compelling to list those parties here (and maybe in the CIP itself, in the Acceptance section).
@alessandrokonrad could you please look at this & my comments above... not as the CIP68 author, or any expectations of you as a "maintainer", but
- partly because of the work being based on Nebula
- mainly the questions about how the new asset types should be versioned, and more generally whether they should be added to CIP68 in the first place (as per your
444
CIP-0068 | Bump version for RFT #523 (comment)).
Thanks for the reply! Responding in order here.
|
@SamDelaney you would be welcome at the next CIP editors' meeting above (Discord invite https://discord.gg/kyaTyzkBqd). This PR isn't slated for a detailed review, but the question about whether this should be a separate CIP would likely be covered, and I believe you should be involved in that discussion. 🙂 |
I really like the elegance of this solution for royalties when working w/ CIP-68 tokens. I believe I would tend to agree that for long-term management and ease these individual/new sub-tokens should be broken out into their own CIPs where they can be individually iterated and ideated w/o potential merge issues arising from concurrent attempts to modify the core document. I like that the data structure of the royalty token in this case specifies some optional min fee, max fee, and is presumed to be an array of recipient addresses (although I think we'll be a long time before we see widespread support for multi-royalty recipients due to minUTxO). |
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.
Ultimately I have to question if this solution provides any additional "security" to a marketplace contract versus the currently off-chain CIP-27 implementations that we've seen. What happens if I submit my buy order w/o the reference UTxO for the royalty token? Can I still subvert the royalties? Is this fundamentally different or better than, say, putting the royalty information in alongisde a token in the listing datum instead?
CIP-0068/README.md
Outdated
optional_big_int, ; min fee (absolute value in lovelace) | ||
optional_big_int, ; max fee (absolute value in lovelace) |
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.
Are the min and max fee optional or are they required? If so, should there be a note that if you do not wish to set them then these values should be set to 0?
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.
They are optional. optional_big_int
is defined in CDDL above this, but perhaps it should be expressed in a less niche format as well.
They are both expected to be a bigint or not be included. Max fee being set to 0 to represent no max fee would be confusing, as it could easily be misread to mean no fees at all, i.e. if the implementer of the protocol missed that wrinkle.
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.
Derp yeah, I guess I didn't read the names of the variables very well, but I would still add the ?
to indicate in the CDDL that they are optional :)
CIP-0068/README.md
Outdated
``` | ||
Because the computational complexity of Plutus primitives scales with size, this approach significantly minimizes resource consumption. | ||
|
||
To prevent abuse, it is **recommended** that the `royalty NFT` is stored at the script address of a validator that ensures the specified fees are not arbitrarily changed, such as an always-fails validator. |
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 think we should change the conception that we had during the writing of CIP-27 that royalties should never change. What if I want to put royalties on a collection but only for the first year, or to remove royalties if I no longer intend to continue supporting and working on a project? Having mutable royalties does introduce additional complexity in needing to maintain the token and/or lock it in a script that only you (or a DAO multisig, etc) can control... But probably best for the long term if we do not encourage people to lock up minUTxO worth of Lovelace in a token just for the sake of "not changing the royalties".
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 agree with this notion to some degree. There are lots of perfectly valid use cases for changing royalties. The key word here is recommended. With this distinction we allow for such use cases, as well as accommodating existing protocols such as Nebula which store the royalty in wallets rather than script addresses.
However, I believe the recommendation against arbitrary royalty control is important. A collection whose fees are controlled arbitrarily can be easily sabotaged at the whim of the creator by increasing the royalties beyond a reasonable amount. This is bad assurance for buyers, and we should set the precedent for better schemes - the simplest of which is an always-fails validator.
CIP-0068/README.md
Outdated
2. Look up `royalty NFT` and find the output it's locked in. (off-chain) | ||
3. Reference the output in the transaction. (off-chain) | ||
4. Verify validity of datum of the referenced output by checking if policy ID of `royalty NFT` and `user token` and their asset names without the `asset_name_label` prefix match. (on-chain) | ||
|
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.
Having you tested the realistic feasibility of adding this logic to a standard "marketplace" smart contract? Especially with multiple royalty recipients? While this is certainly a novel concept and a step in the right direction, I wonder on the impact to CPU, memory, and subsequent transaction fees by using this reference token datum versus other, largely off-chain solutions atm.
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.
We have implemented this in Grabbit, but haven't benchmarked it very thoroughly yet. I can get back to you on CPU, memory, etc. on the client side.
As far as TX fees go, this shouldn't have much of an impact. Reading the fee is as simple as parsing the datum, which doesn't require consuming it at all.
One valid concern is the memory cost to the blockchain itself. This is definitely something worth thinking about, and the design has carefully considered memory usage to be as minimal as possible. I personally think the ability for validators to read the royalties directly & trustlessly is worth the cost many times over.
Just brainstorming aloud at this point, but wondering if it would be advisable to provide an example of the logic that would need to be added to a Plutus minting script in order to allow only the creator/deployer of the contract to issue this token once? Bit of a philosophical question but it's all well and good to propose a new standard, but having some example code (most likely hosted in a separate repo of your own) showing how this can be used would help ease the adoption process for new standards. Also a bit of selfish self-interest as this sort of "informational" token can/could be useful for my own CIP-88 validation for Plutus minting scripts. :) |
Thanks! I will definitely be there. 🙂
This is a good point. Let me think on this for a bit and get back to you.
This is absolutely something I plan on doing. I even have a Catalyst proposal out to help fund it's development. 🙂 |
Thanks again @Crypto2099 for bringing this problem to my attention. Somehow I had missed it until now. I got a chance to think about it over the weekend and I think I've come up with a good solution for it: I propose a new addition is made to the standard that specifies an optional boolean
In addition to providing a way to create guaranteed royalties, this has several advantages:
It isn't functionally important, but this field could make sense at a couple of different locations in the Reference Datum. I don't have strong feelings about these, but if any of you do please let me know.
I'm leaning toward the latter of these two, which I think makes for a clearer developer experience & will be easier to spot if accidentally excluded, but I could certainly be convinced of the alternative. |
Resolution from CIP editors' meeting in last hour: @SamDelaney has agreed to rework this PR as an independent CIP rather than a modification to CIP68. We'll leave it in this PR to avoid fragmenting the discussion so far. That still leaves the need for CIP68 to be re-written a bit: to point to other possible CIPs that define new token types. We resolved that the most recent inclusion of
We acknowledged that CIP67 was supposed to make CIP68 updates unnecessary for new token types, but in practice the documentation & metadata structures tend to require their own text documentation. |
Co-authored-by: Ryan Williams <44342099+Ryun1@users.noreply.github.com>
Co-authored-by: Ryan Williams <44342099+Ryun1@users.noreply.github.com>
Co-authored-by: Ryan Williams <44342099+Ryun1@users.noreply.github.com>
Co-authored-by: Ryan Williams <44342099+Ryun1@users.noreply.github.com>
Co-authored-by: Ryan Williams <44342099+Ryun1@users.noreply.github.com>
Co-authored-by: Ryan Williams <44342099+Ryun1@users.noreply.github.com>
Co-authored-by: Ryan Williams <44342099+Ryun1@users.noreply.github.com>
@SamDelaney looks ready to merge now as soon as we answer the question about whether @Ryun1's suggested |
p.s. since I think we'll therefore try to set a precedent for tagging |
Co-authored-by: Ryan Williams <44342099+Ryun1@users.noreply.github.com>
Co-authored-by: Ryan Williams <44342099+Ryun1@users.noreply.github.com>
Ah sorry about the delay here I had several meetings in quick succession. Looks like you got the tickboxes in. I was going to include Nebula & Grabbit as implementors & add the CIP 68 boilerplate, but I don't think any of those are urgent, so I'll add them to the reference implementation PR when I make that. Thanks for the merge! Really glad to have this ratified! 🎉 |
@SamDelaney since this was agreed at meeting # 75 you can add those implementors here as a change request & I'll include them. Likewise with the boilerplate language and I apologise for missing that. I was working to satisfy 2 goals and my head didn't sort out the schedule & priorities right. The boilerplate is just a delta to the CIP text if I understood correctly & if you can shoehorn it into #606 then I'd be happy to merge it there if you think it's appropriate & not too difficult to add the edits that way. 😖 |
First of all, congrats on the merge @SamDelaney! I am really happy to see a push for a standard for CIP68 style royalties. I'm sorry I'm late to the process, but I do see there are already discussions about submitting change requests so I felt I could still share my feedback: 1) Rates that cannot be represented I know this was discussed in the meeting (@Crypto2099 brought it up), but the method of storing the rate is rather restrictive. There are lots of potential royalty values that cannot be represented in this way. I randomly selected one of these values (7%) and checked how many collections currently have 7% royalties in the JPG Store database. We have 529 collections with a 7% royalty. In my opinion, a royalty standard should support as many royalty rates as possible, and this is not achieving that goal. Why not simply store the rate as an integer x, where 2) Undefined Behavior and Handling Errors
3) Ambiguous Language |
Hi @yHSJ, thanks for weighing in! Let me answer these in turn: 1) Rates that can not be represented I realize now I didn't include in language specifying that rounding would be allowed for values where the onchain value doesn't result in a whole number. 7% is absolutely doable with this protocol. Here's what that would look like:
I think this example is probably a better illustration, and will likely replace the existing example as part of #606 assuming I get approval. I will also add a bit of wording in the specification that acknowledges rounding. 2a) Restrictions on NFT quantity This is a very good point. For now, there is a valid use case for wanting multiple royalty tokens - having multiple royalty policies within a collection that can be chosen by the frontend. However, there is also potential for abuse, where collections might mint a second, much higher royalty to deceive users. I think for now multiple royalty tokens shouldn't be explicitly supported or prohibited. If collections mint multiple tokens, which royalty datum is referenced can be left to the user or platform constructing the transaction. This makes the type of abuse I pointed out easy to circumvent, but doesn't prevent people trying to make multiple royalty policies. I have plans for a better way of doing multiple royalty policies in a collection that I left out of version 1 for the sake of simplicity. Once that change comes along (I plan to introduce V2 shortly after I finish the reference implementation), the valid use case will be supported and it will make sense to explicitly prohibit multiple royalty tokens. P.S. this should also be raised for clarification on CIP 67/68. Maybe in #586? 2b) Restrictions on fee value While there isn't really any reason why NFTs can't have >100% royalties, I don't think it's fair to expect platforms to support that when many may already have checks to ensure royalties don't go above that number. I think a recommended validity window of [1, 100], with the caveat that royalties outside that window MAY be rounded to fit makes the most sense. This way creators are allowed to create extreme royalties if they want, but we don't force platforms to make significant changes to logic to support minor edge cases. 3) Ambiguous Langauge You're absolutely right this should be following RFC-2119. I had implicitly made that assumption, but not formatted correctly or stated that explicitly. Will do a pass to make sure this works as part of #606. |
Thank you for considering my feedback @SamDelaney! 1) Rates that can not be represented
I see, this makes a lot more sense and allows for many more potential royalty values. That said, I still feel this is more complicated than it needs to be. Why not store the fee as While your encoding of the value absolutely works, I fear it is unnecessarily overcomplicated. Perhaps there are some reasons your encoding is better that I haven't considered yet, though. 2a) Restrictions on NFT quantity
I agree that there are cases where having multiple royalty tokens make sense. However, I think if that is valid, there MUST be a default state specified in the standard since users are not likely to specify a preferred royalty on every platform. Furthermore, while I appreciate the use of reference datums to enforce royalties, it is important to keep in mind this would require a new contract for marketplaces, such as JPG Store, which would increase friction in adoption. JPG Store could easily much more easily support this CIP if the example section titled
Yes, absolutely. @Crypto2099 knows that I have a laundry list of "implementor speed bumps" we ran into when implementing the CIP at JPG Store. Hopefully I can find time to get those thoughts organized and communicated in a constructive and meaningful way soon. 2b) Restrictions on fee value
Completely agreed. There is no technical requirement preventing it, but it is a business requirement for many platforms. This is a poorly formed and incomplete thought, but would it make sense to write a "sister CIP" that allows platforms to specify the valid royalty range they support so that users can know what to expect? This could be done through an on-chain datum or an off-chain repository. Thank you for considering my feedback and I'm happy to hear that it has been helpful. Please let me know if it makes more sense to continue this conversation in #606, or to open a new issue and discuss it there. Seems like those would be more appropriate places to have this conversation than an already merged PR. |
Thanks for keeping an eye on that & sorry for what may have been a premature merge on this. We will merge that PR when we get 1 more editorial approval, but that may not happen soon because of editors involved in the Cardadno Summit. If it merges before you've made those language / implementation / other updates, we can do another PR on your CIP as soon as you see fit & I'll do what I can to expedite it.
#606 was open for @SamDelaney to make changes that were discussed at our last CIP meeting, since I inadvertently merged it before all those pending changes were included in this PR branch, but it wouldn't make sense to discuss CIP content issues there because it's only for editorial items & those last-minute additions that were already discussed. Your contribution would be welcome if & when he submits another PR and/or in a newly created issue 🤓 |
* First pass: introduce royalty metadata as specified in Nebula * minor fixes * fix token name * consolidate TN spec & rename token -> NFT * require royalty NFT policyId to match reference NFT * Change royalty location to suggestion * Elaborate on fee calculation * Add 500 token to CIP 67 registry * move to a new CIP Signed-off-by: samdelaney <sam@thedelaneys.net> * reference datum flag spec Signed-off-by: samdelaney <sam@thedelaneys.net> * Fix leftover whitespace * Path to Active & References * update CIP-67 registry entry * changes per review * Update preamble to match CIP-0001 standard * assigned 102 as CIP number * move docs to new folder * update registry entry * add registry documentation link * Apply suggestions from code review Co-authored-by: Ryan Williams <44342099+Ryun1@users.noreply.github.com> * all header level fixes agreed at CIP meeting Co-authored-by: Ryan Williams <44342099+Ryun1@users.noreply.github.com> * all header level fixes agreed at CIP meeting Co-authored-by: Ryan Williams <44342099+Ryun1@users.noreply.github.com> * agreed at meeting null implementors mandatory Co-authored-by: Ryan Williams <44342099+Ryun1@users.noreply.github.com> * agreed at meeting to use tickboxes Co-authored-by: Ryan Williams <44342099+Ryun1@users.noreply.github.com> * agreed at meeting to use tickboxes Co-authored-by: Ryan Williams <44342099+Ryun1@users.noreply.github.com> * agreed at meeting to use tickboxes Co-authored-by: Ryan Williams <44342099+Ryun1@users.noreply.github.com> * adding `cddl` syntax tag pending eventual GitHub support Co-authored-by: Ryan Williams <44342099+Ryun1@users.noreply.github.com> * adding `cddl` syntax tag pending eventual GitHub support Co-authored-by: Ryan Williams <44342099+Ryun1@users.noreply.github.com> --------- Signed-off-by: samdelaney <sam@thedelaneys.net> Co-authored-by: Robert Phair <rphair@cosd.com> Co-authored-by: Ryan Williams <44342099+Ryun1@users.noreply.github.com>
The inability to create trustless onchain royalty validation with CIP 27 is a major drawback to Cardano NFTs. The pattern defined in CIP 68 represents an opportunity to upgrade the standard to support onchain validation. CIP-102 aims to eliminate that drawback and demonstrate better support for developers, NFT creators, and NFT collectors, ultimately attracting dapps & NFT projects that would otherwise have taken their talents to another blockchain.
This specification is largely based on the royalty specification in Nebula, with a couple key departures:
The specification here is made to be as minimal as possible. This is done with expediency in mind and the expectation that additional changes to the specification may be made in the future. The sooner we have a standard established, the sooner we can make use of it. Rather than attempting to anticipate all use cases, we specify with forward-compatibility in mind.
If you're interested in Ikigai's internal discussion that preceded this, check out ikigai-github#1.
(rendered proposal in branch)