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

Add EIP-4910: Royalty Bearing NFTs #4910

Merged
merged 33 commits into from
Sep 8, 2022

Conversation

Therecanbeonlyone1969
Copy link
Contributor

@Therecanbeonlyone1969 Therecanbeonlyone1969 commented Mar 14, 2022


eip: 4910
title: Royalty Bearing NFTs
description: Extension of the ERC-721 standard for NFTs to correctly define, process, and pay (hierarchical) onchain royalties from NFT sales, going beyond EIP-2981.
author: Andreas Freund (@Therecanbeonlyone1969)
discussions-to: https://ethereum-magicians.org/t/royalty-bearing-nfts/8453
status: Draft
type: Standards Track
category: ERC
created: 2022-03-14
requires: 721, 165, 2771


@eth-bot
Copy link
Collaborator

eth-bot commented Mar 14, 2022

A critical exception has occurred:
Message: pr 4910 is already merged; quitting
(cc @alita-moore, @mryalamanchi)

EIPS/eip-xxxx.md Outdated Show resolved Hide resolved
EIPS/eip-xxxx.md Outdated Show resolved Hide resolved
EIPS/eip-xxxx.md Outdated Show resolved Hide resolved
EIPS/eip-xxxx.md Outdated Show resolved Hide resolved
EIPS/eip-xxxx.md Outdated Show resolved Hide resolved
EIPS/eip-xxxx.md Outdated Show resolved Hide resolved
EIPS/eip-xxxx.md Outdated Show resolved Hide resolved
EIPS/eip-xxxx.md Outdated Show resolved Hide resolved
EIPS/eip-xxxx.md Outdated Show resolved Hide resolved
@Therecanbeonlyone1969 Therecanbeonlyone1969 changed the title Initial EIP proposal for a standard for onchain Royalty Bearing NFTs EIP-4910: Proposal for a standard for onchain Royalty Bearing NFTs Mar 15, 2022
@Therecanbeonlyone1969
Copy link
Contributor Author

@MicahZoltu implemented all required changes.

@Therecanbeonlyone1969
Copy link
Contributor Author

@MicahZoltu ... a gentle reminder to merge if it looks ok for you. Thank you!

EIPS/eip-4910.md Outdated Show resolved Hide resolved
EIPS/eip-4910.md Outdated Show resolved Hide resolved
EIPS/eip-4910.md Outdated Show resolved Hide resolved
EIPS/eip-4910.md Outdated Show resolved Hide resolved
Co-authored-by: lightclient <[email protected]>
- Removed dependency on EIP-2771
- Added file with link to reference implementation since I could not add the entire reference implementation repo as a folder under assets/eip-4910
@Therecanbeonlyone1969
Copy link
Contributor Author

@lightclient @MicahZoltu ... I cannot add the reference implementation folder under assets/eip-4910. I am getting this error below when I try to push the commit.

error: invalid path 'assets/eip-4910/eip-4910-reference-implementation/'
error: assets/eip-4910/eip-4910-reference-implementation/: cannot add to the index - missing --add option?
fatal: Unable to process path assets/eip-4910/eip-4910-reference-implementation/

Looks like the github action triggered should have the -- add option in the batch script to allow for subfolders to be added. Could that be added?

Of course, I could just add the test javascript files and the blank contracts under assets/eip-4910 but that does not make it easy for other implementers to take the code and the tests and run them easily, as they should be able to do.

Would appreciate a suggestion as to how the above error can be overcome. Thank you!

@MicahZoltu
Copy link
Contributor

Your local git client is giving you that error, or CI is giving you that error? There shouldn't be any restrictions on adding subdirectories to the repository, and other users have added them in the past which makes me think this is a local git issue you are running into?

@Therecanbeonlyone1969
Copy link
Contributor Author

Your local git client is giving you that error, or CI is giving you that error? There shouldn't be any restrictions on adding subdirectories to the repository, and other users have added them in the past which makes me think this is a local git issue you are running into?

I was stupid indeed ... I had forgotten that I still had another .git folder in my folder ... duh ... now it worked. It was indeed a local git issue. @MicahZoltu

@Therecanbeonlyone1969
Copy link
Contributor Author

@MicahZoltu @lightclient ... just a gentle reminder as to the above. Have your concerns been addressed?

@MicahZoltu
Copy link
Contributor

I don't handle ERCs, you'll have to wait for one of the other editors who does handle them to get a chance to take a look. I was just providing some early feedback to help speed things along is all.

@Therecanbeonlyone1969
Copy link
Contributor Author

I don't handle ERCs, you'll have to wait for one of the other editors who does handle them to get a chance to take a look. I was just providing some early feedback to help speed things along is all.

Thank you so much @MicahZoltu ... greatly appreciated!

@humbitious
Copy link

humbitious commented Apr 1, 2022

I don't handle ERCs, you'll have to wait for one of the other editors who does handle them to get a chance to take a look. I was just providing some early feedback to help speed things along is all.

Thanks @MicahZoltu. Keen to see which Editor(s) tap in. Lots of excitement building around this.

Copy link
Contributor

@SamWilsn SamWilsn left a comment

Choose a reason for hiding this comment

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

Just warning you now, this is a big one, and will probably need a few rounds of review 😅

EIPS/eip-4910.md Outdated Show resolved Hide resolved
EIPS/eip-4910.md Outdated Show resolved Hide resolved
EIPS/eip-4910.md Outdated Show resolved Hide resolved
EIPS/eip-4910.md Show resolved Hide resolved
EIPS/eip-4910.md Outdated Show resolved Hide resolved
EIPS/eip-4910.md Outdated Show resolved Hide resolved
- Removed all external links
- Added Licensing to the header of the solidity contracts
- Split Specifications and Rationales into separate section to align with EIP-1
- Intended the sections properly based on suggestion by @SamWilsn
EIPS/eip-4910.md Outdated Show resolved Hide resolved
EIPS/eip-4910.md Outdated Show resolved Hide resolved
EIPS/eip-4910.md Outdated Show resolved Hide resolved
EIPS/eip-4910.md Outdated Show resolved Hide resolved
Co-authored-by: Sam Wilson <[email protected]>
- adopted suggestion by @SamWilsn to simplify headers and remove `Rationale` headers and remove the individual `Specification` headers.
- found a way to remove  requirements as headers using <a name="tag_name">...</a>
@eth-bot eth-bot merged commit 3750281 into ethereum:master Sep 8, 2022
nachomazzara pushed a commit to nachomazzara/EIPs that referenced this pull request Jan 13, 2023
* Initial Commit of Onchain Royalty Bearing NFT standard

* Update to initial commit to address editor comments

* Resolving figure reendering issue

* Update EIPS/eip-4910.md

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

* Requested changes as per @lightclient requests from 3-22-2022

- Removed dependency on EIP-2771
- Added file with link to reference implementation since I could not add the entire reference implementation repo as a folder under assets/eip-4910

* adding reference implementation folder

* Updates in response to requests by @SamWilsn

- Removed all external links
- Added Licensing to the header of the solidity contracts
- Split Specifications and Rationales into separate section to align with EIP-1
- Intended the sections properly based on suggestion by @SamWilsn

* Overlooked two external links, and removed them

* Update EIPS/eip-4910.md

Co-authored-by: Sam Wilson <[email protected]>

* Address requested changes by @SamWilsn

- adopted suggestion by @SamWilsn to simplify headers and remove `Rationale` headers and remove the individual `Specification` headers.
- found a way to remove  requirements as headers using <a name="tag_name">...</a>

* Minor function update to adding and removing a NFT listing

* Updated the functions the wrong way

* Addressing Editor comment 4/22/2022

- Fixing overlooked heading corrections per comment by @SamWilsn
- Minor editing in the outline section to remove some headings to simplify the flow

* Update EIPS/eip-4910.md

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

* Update based on discussion with @SamWilsn on 5/17

- removed specific data structure structs and just generally talked about what things are required to be held in the smart contract
- consequently removed 3 requirements
- removed all optional [O] and conditional [CR] requirements
- removed all but 1 should [D] requirements

Decision of the call was also not to split the EIP into several EIPs since that does not reduce overall complexity, and makes creating reference implementations more difficult, hurting adoption.

Net effect is to noticeably reduce the size of the specification.

* Missed two unhyphenated ERC references -- fixed

* Fixing EIP and HTML Validator errors save for reference implementation link

* added separate rationale section for the spec

* fixed link for EIP-20

* fixing links

* Removed reference implementation and updated relevant section in the EIP accordingly

* Update EIPS/eip-4910.md

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

Co-authored-by: lightclient <[email protected]>
Co-authored-by: Sam Wilson <[email protected]>
Co-authored-by: Pandapip1 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-new Creates a brand new proposal s-draft This EIP is a Draft t-erc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants