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

First draft discussion #1

Closed
wants to merge 5 commits into from
Closed

First draft discussion #1

wants to merge 5 commits into from

Conversation

SamDelaney
Copy link
Collaborator

NOTE: This PR will remain as a draft and be closed once a second draft is finalized. The actual merge from this branch into master will be done in the CF Cips repo, after third draft is finalized.

@SamDelaney
Copy link
Collaborator Author

Right now, this pretty much just represents my understanding of how Nebula royalties work with CIP68.

There's a couple ideas I may want to add / propose, such as introducing asset-specific royalties. I haven't gotten to fleshing those out in my head yet though. If you guys have other ideas you'd like to propose, please include them here for discussion. I will drop my ideas here as well once I get my head around them.

@SamDelaney SamDelaney marked this pull request as draft May 16, 2023 00:58
CIP-0068/README.md Outdated Show resolved Hide resolved
CIP-0068/README.md Outdated Show resolved Hide resolved
CIP-0068/README.md Outdated Show resolved Hide resolved
CIP-0068/README.md Outdated Show resolved Hide resolved
@SamDelaney
Copy link
Collaborator Author

Fixed the token name to match nebula implementation since I hadn't meant to depart from it.

I do have something of an issue with the way the nebula implementation tethers royalties to collections, but I think that's likely better addressed with a more robust datum structure than by allowing multiple royalty data. Still mulling this idea over.

@SamDelaney SamDelaney requested a review from Masstronaut May 24, 2023 17:37
@SamDelaney
Copy link
Collaborator Author

Regarding the above ^ we decided against departing from the most basic implementation. We intend to first push a simple, forward compatible base for royalty information to ensure it passes the CIP process, and then follow with improvements such as multiple royalties which may be more controversial.

@SamDelaney
Copy link
Collaborator Author

SamDelaney commented May 25, 2023

A couple realizations from my review today:

1: In the current implementation & specification designed by Ales for Nebula, neither to policyId nor the tokenName of the royalty NFT needs to be related to the collection. This completely breaks the lookup paradigm used for the 3 other token types.

Solutions I've come up with are:

  • Enforce that the policyId must match that of the user token
  • Leave out the lookup section, as looking up the royalty based on the user token is impossible

I lean toward the former, but that would break some of the Nebula royalty NFTs that have already been minted. I expect Ales had some solution in mind for lookups, but want to get your feedback before I ask him. @colll78 @solidsnakedev

2: Although it isn't specified anywhere yet, I wonder if we may have more luck passing this solution if we specify some limitation on validation rules for the script the royalty NFT is locked at. (Probably just an always-fails script for now, complicating validation too much sounds like a good way to alienate people.)

I'm leaning away from this for our first/second drafts; I figure we can introduce that if people request it in the public review, but again interested in your input.

@solidsnakedev
Copy link

solidsnakedev commented May 25, 2023

My understanding when reading our Onchain solutions is that , we mint 3 tokens with the same Minting Policy ID

  • NFT TokenName + label 100 -> lock at MetadataControl Script
  • NFT TokenName + label 222 -> lock at user wallet
  • Royalty token name + label 500 -> it's not clear yet but it may be lock at AlwaysFails script or a RoyaltyControl script with some metadata constrains

I think It's possible to find the royalty info if we do the follwing:

  • Fetch the NFT PolicyID from wallet's user
  • Build lucid Unit
    royaltyUnit = toUnit ( PolicyID, fromText("Royalty"), 500)
  • Query Royalty token at AlwaysFails script or RoyaltyControl script
    royaltyUTXO = lucid.utxosAtWithUnit( RoyaltyControlAddress, royaltyUnit)

IMO Nebula solution is far away from what we're proposing, since Ales made it so that it only works with his marketPlace contract, and we might as well consider using a different royalty label.

@SamDelaney
Copy link
Collaborator Author

I went ahead and made the policyId match required. Since there are a lot of possible valid designs for royalty control, I left out specifying it at all.

I believe this should be ready to pass on to Ales / Nick etc. for their thoughts.

@SamDelaney
Copy link
Collaborator Author

Closing. Will create a second draft PR so we have a clean slate as we gather collaborator input.

@SamDelaney SamDelaney closed this Jun 6, 2023
SamDelaney pushed a commit that referenced this pull request Sep 18, 2024
* Add a draft for Query Layer Standardization

* added PR number to discussions

Co-authored-by: Ryan Williams <[email protected]>

* Apply suggestions from code review

* Expand use cases and ecosystem risks

* assign CIP number 113

* accidentally promoted as CIP instead of CPS

* Update CPS-0012 directory name

* removing artefact HTML comment from CPS template

* removing invalid YAML for formatted Discord reference

* more + deeper links to Discord discussions

* add feedback from workshop 1 replacing PR #1

* Update CPS-0012/README.md

Co-authored-by: Vladimir Kalnitsky <[email protected]>

* fix typo

Co-authored-by: Vladimir Kalnitsky <[email protected]>

* Apply suggestions from code review

* add adoption barriers and open new open question

* Apply suggestions from code review

Fix phrasing

Co-authored-by: Ryan <[email protected]>

* Apply suggestions from code review

Co-authored-by: Ryan <[email protected]>

---------

Co-authored-by: Robert Phair <[email protected]>
Co-authored-by: Ryan Williams <[email protected]>
Co-authored-by: Ryan Williams <[email protected]>
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.

3 participants