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 ERC: Limited Transfer Count NFT #274

Merged
merged 30 commits into from
Jun 4, 2024
Merged

Conversation

OniReimu
Copy link
Contributor

@OniReimu OniReimu commented Feb 22, 2024

@eip-review-bot
Copy link
Collaborator

eip-review-bot commented Feb 22, 2024

✅ All reviewers have approved.

Copy link

The commit d6b5bf8 (as a parent of 71c237a) contains errors.
Please inspect the Run Summary for details.

@github-actions github-actions bot removed the w-ci label Feb 22, 2024
ERCS/erc-7633.md Outdated Show resolved Hide resolved
ERCS/erc-7633.md Outdated Show resolved Hide resolved
ERCS/erc-7634.md Outdated
@@ -0,0 +1,208 @@
---
eip: 7634
title: Limited Transferrable NFT
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a good title. If you want to make it a great title, try and cram a bit more detail about the limitation these NFTs have.

ERCS/erc-7634.md Outdated

## Abstract

This standard extends [ERC-721](./eip-721.md) to allow minters to customize the transferability of NFTs by setting parameters via TransferCount. The standard introduces an interface with internal functions to facilitate this functionality.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This abstract certainly satisfies the "terse" requirement from EIP-1, but I don't think it gives enough detail (both technical and contextual) for the reader to really understand what you're proposing here.

For example, what's a TransferCount?

ERCS/erc-7634.md Outdated

pragma solidity ^0.8.4;

import "@openzeppelin/contracts/token/ERC721/IERC721.sol";
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should avoid using openzeppelin in your standard. Instead, refer to the interface ERC721 from ERC-721.

ERCS/erc-7634.md Outdated
Comment on lines 81 to 85
*Controlled Value Preservation*: By allowing minters to set customized transfer limits for NFTs, this standard facilitates the preservation of value for digital assets Just as physical collectibles often gain or maintain value due to scarcity, limiting the number of transfers for an NFT can help ensure its continued value over time.

*Ensuring Intended Usage*: Setting transfer limits can ensure that NFTs are used in ways that align with their intended purpose. For example, if an NFT represents a limited-edition digital artwork, limiting transfers can prevent it from being excessively traded and potentially devalued.

*Expanding Use Cases*: These enhancements broaden the potential applications of NFTs by offering more control and flexibility to creators and owners. For instance, NFTs could be used to represent memberships or licenses with limited transferability, opening up new possibilities for digital ownership models.
Copy link
Collaborator

Choose a reason for hiding this comment

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

These points all look like Motivation. The Rationale section is here so you can explain choices you made within your design, while the Motivation justifies the whole proposal. My favourite analogy is:

Motivation: We need to build a shed because...
Rationale: We painted the shed red because...

ERCS/erc-7634.md Outdated
Comment on lines 36 to 38
- `_incrementTransferCount`: an internal function facilitates incrementing the transfer count.
- `_beforeTokenTransfer`: an overrided function defines the state before transfer.
- `_afterTokenTransfe`: an overrided function outlines the state after transfer.
Copy link
Collaborator

Choose a reason for hiding this comment

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

These are implementation details, and should be omitted from the standard. Only specify the externally visible behaviour of the smart contract, not how it needs to be implemented.

@eip-review-bot eip-review-bot changed the title Add ERC: Limited Transferrable NFT Add ERC: Limited Transfer Count NFT Jun 3, 2024
ERCS/erc-7634.md Outdated

### Does tracking the internal transfer count matter?

Yes and no. It is **OPTIONAL** and quite depends on the actual requirements. The reference implementation given below is a **RECOMMENDED** one if you opt for tracking. The `_incrementTransferCount` function and related retrieval functions (`transferLimitOf` and `transferCountOf`) are designed to keep track of the number of transfers an NFT has undergone. This internal tracking mechanism is crucial for enforcing the minter's transfer limits, ensuring that no further transfers can occur once the limit is reached.
Copy link
Collaborator

Choose a reason for hiding this comment

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

OPTIONAL

The uppercase keywords create requirements on implementations. These keywords should only appear in the Specification section.

If you intend to create a requirement, move the text to the specification section. Instead, if you're simply discussing something and did not intend to create a requirement, use lowercase instead ("optional".)

ERCS/erc-7634.md Outdated

### If opting for tracking, is that all we may want to track?

It is **RECOMMENDED** to also track the before and after. The **OPTIONAL** `_beforeTokenTransfer` and `_afterTokenTransfer` functions are overridden to define the state of the NFT before and after a transfer. These functions ensure that any necessary checks or updates are performed in line with the transfer limits and counts. By integrating these checks into the transfer process, the standard ensures that transfer limits are consistently enforced.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar comment about "RECOMMENDED" and "OPTIONAL" here.

ERCS/erc-7634.md Outdated

## Reference Implementation

The **RECOMMENDED** implementation is demonstrated as follows:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
The **RECOMMENDED** implementation is demonstrated as follows:
A reference implementation is demonstrated as follows:

@OniReimu OniReimu changed the title Add ERC: Limited Transfer Count NFT Add ERC-7634: Limited Transfer Count NFT Jun 3, 2024
@eip-review-bot eip-review-bot changed the title Add ERC-7634: Limited Transfer Count NFT Add ERC: Limited Transfer Count NFT Jun 4, 2024
@eip-review-bot eip-review-bot enabled auto-merge (squash) June 4, 2024 14:06
Copy link
Collaborator

@eip-review-bot eip-review-bot left a comment

Choose a reason for hiding this comment

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

All Reviewers Have Approved; Performing Automatic Merge...

@eip-review-bot eip-review-bot merged commit 9d5cc78 into ethereum:master Jun 4, 2024
21 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants