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-1238: Non-transferable Token Standard #5617

Closed
wants to merge 11 commits into from

Conversation

ra-phael
Copy link

@ra-phael ra-phael commented Sep 7, 2022

EIP for #1238

@github-actions github-actions bot added c-new Creates a brand new proposal s-draft This EIP is a Draft t-erc labels Sep 7, 2022
@eth-bot
Copy link
Collaborator

eth-bot commented Sep 7, 2022

Hi! I'm a bot, and I wanted to automerge your PR, but couldn't because of the following issue(s):


(fail) eip-1238.md

classification
newEIPFile
  • File with name EIPS/eip-1238.md is new and new files must be reviewed
  • This PR requires review from one of [@axic, @SamWilsn, @Pandapip1]

@Pandapip1 Pandapip1 changed the title Add EIP-1238: Non-transferable Token Standard Add EIP-5617: Non-transferable Token Standard Sep 7, 2022
@ra-phael
Copy link
Author

ra-phael commented Sep 8, 2022

@Pandapip1 isn't it possible to keep the original EIP number? 🙏

It's not taken, would be simpler in terms of historical continuity, not to mention all the renaming in:

@Pandapip1
Copy link
Member

isn't it possible to keep the original EIP number? 🙏

If you wish to publish this as Withdrawn, then sure. Otherwise, no.

@ra-phael
Copy link
Author

@Pandapip1 Why not?
The following comment suggests that this number can be kept with the approval of @nicola, the original author of the issue, which I have:
#4966 (comment)

@Pandapip1
Copy link
Member

Due to a recent incident (EIP-5000), "who gets EIP numbers" is currently a process in which editors are unlikely to make exceptions.

@nicola
Copy link

nicola commented Sep 22, 2022

I approve the re-use of the original EIP number.

@Pandapip1
Copy link
Member

Again, EIP editors are responsible for assigning numbers. I will not assign EIP-1238, but I'll CC @SamWilsn @gcolvin @lightclient @axic in case they might.

@SamWilsn
Copy link
Contributor

SamWilsn commented Oct 5, 2022

I am fine with resurrecting the original EIP number. This PR is taking the old format (issue) and turning it into a markdown file, and adding authors. I'd probably recommend adding @nicola's github username.

You'll still need to fix the eipw errors though.

@ra-phael
Copy link
Author

Thank you @SamWilsn. I added Nicola's github username and fixed the EIP validation errors.

- Statements: more broadly, any statement that is issued or signed by a contract, a dao, a single user that requires to be on-chain.
- Subscription: badges can represent the validity of a paid subscription.

Additionally, this standard could be used as a primitive for Soulbound tokens, whereby the recovery of a Soul would be done by obtaining signatures from co-owners of tokens with the same id.
Copy link
Contributor

Choose a reason for hiding this comment

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

Since there has been at least 4 attempts to propose a soul-bound / non-transferable NFT, I'd also suggest you add to your motivation and rationale on what this ERC has better merit than other competing ERCs e.g. https://eips.ethereum.org/EIPS/eip-5114
https://eips.ethereum.org/EIPS/eip-5633

Copy link
Author

Choose a reason for hiding this comment

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

Good point @xinbenlv! I'm familiar with the other EIPs, but they either build on previous standards like EIP-1155 as extensions which means unecessary transfer functions or propose different designs leading to a different set of features. I added more in motivation: a7f94be

EIPS/eip-1238.md Outdated
Comment on lines 40 to 48
/**
* @dev Emitted when `amount` tokens of token type `id` are minted to `to` by `minter`.
*/
event MintSingle(address indexed minter, address indexed to, uint256 indexed id, uint256 amount);

/**
* @dev Equivalent to multiple {MintSingle} events, where `minter` and `to` is the same for all token types.
*/
event MintBatch(address indexed minter, address indexed to, uint256[] ids, uint256[] amounts);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you have both MintSingle and MintBatch? Couldn't MintBatch handle both cases?

If there's a reason, please add it to the EIP's rationale section.

Copy link
Author

Choose a reason for hiding this comment

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

Good point, I like how it makes the interface simpler. I also applied the same reasoning to the events related to burning.

EIPS/eip-1238.md Show resolved Hide resolved
EIPS/eip-1238.md Outdated
Comment on lines 144 to 172
```solidity
interface IERC1238Holdable is IERC1238 {
/**
* @dev Event emitted when `from` entrusts `to` with `amount` of tokens with token `id`.
*/
event Entrust(address from, address to, uint256 indexed id, uint256 amount);

/**
* @dev Event emitted when tokens are burnt and the holder fails to acknowledge the burn.
*/
event BurnAcknowledgmentFailed(address holder, address burner, address from, uint256 indexed id, uint256 amount);

/**
* @dev Returns the balance of a token holder for a given `id`.
*/
function heldBalance(address holder, uint256 id) external view returns (uint256);

/**
* @dev Lets sender entrusts `to` with `amount`
* of tokens which gets transferred between their respective balances
* of tokens held.
*/
function entrust(
address to,
uint256 id,
uint256 amount
) external;
}
```
Copy link
Contributor

Choose a reason for hiding this comment

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

This holdable extension needs a lot more explanation. You should define the concepts, the expected flow, etc.

Copy link
Author

Choose a reason for hiding this comment

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

I added more explanations in a17e69d

@github-actions
Copy link

There has been no activity on this pull request for 2 weeks. It will be closed after 3 months of inactivity. If you would like to move this PR forward, please respond to any outstanding feedback or add a comment indicating that you have addressed all required feedback and are ready for a review.

@github-actions github-actions bot added the w-stale Waiting on activity label Nov 24, 2022
nicola
nicola previously approved these changes Nov 25, 2022
@github-actions github-actions bot removed the w-stale Waiting on activity label Nov 27, 2022
@ra-phael
Copy link
Author

ra-phael commented Dec 5, 2022

@SamWilsn Thank you for your feedback! And sorry for the delay in addressing it. It's ready for re-review.

@SamWilsn SamWilsn changed the title Add EIP-5617: Non-transferable Token Standard Add EIP-1238: Non-transferable Token Standard Dec 9, 2022
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.

How would you feel about slimming this EIP down a bit, and moving the extensions into their own proposals?


## Abstract

A _badge_ or non-transferable token (_NTT_) is a token that cannot be transferred once assigned. Badges can be accumulated through time and put at stake. Simply speaking, badges are statements about a public key: they can be quantitative (e.g. reputation, experience) or qualitative (badges, titles).
Copy link
Contributor

Choose a reason for hiding this comment

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

Badges can be accumulated through time and put at stake.

I am not sure what "put at stake" means here. Is this some kind of staking mechanism, or are you talking about the badge issuer's reputation, or something else entirely?


A _badge_ or non-transferable token (_NTT_) is a token that cannot be transferred once assigned. Badges can be accumulated through time and put at stake. Simply speaking, badges are statements about a public key: they can be quantitative (e.g. reputation, experience) or qualitative (badges, titles).

The Non-Transferable Token standard defines a set of standard APIs allowing the identification of statements (called badges) attributed to a public key, such that different dapps and smart contract can use to filter users or to provide user with different badges different experiences. More importantly, this standard defines a way for which users can put their badges at stake. Badges cannot be transferred but can be lost (after staking) or can expire.
Copy link
Contributor

Choose a reason for hiding this comment

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

Couple grammar and stylistic changes:

Suggested change
The Non-Transferable Token standard defines a set of standard APIs allowing the identification of statements (called badges) attributed to a public key, such that different dapps and smart contract can use to filter users or to provide user with different badges different experiences. More importantly, this standard defines a way for which users can put their badges at stake. Badges cannot be transferred but can be lost (after staking) or can expire.
The Non-Transferable Token standard defines a set of APIs that identify statements (called badges) attributed to a public key, such that different dapps and smart contracts can filter users or tailor experiences based on what badges a user has. More importantly, this standard defines a way for users to put their badges at stake. Badges cannot be transferred but can be lost (after staking) or can expire.


## Motivation

The idea is to have tokens that once assigned cannot be transferred (like reputation) and that can be used by websites, or contracts to make users perform some actions. For example, if a user accumulates paper submissions at conferences, then they can use their paper badges to request grants. It's important that they can never share these badges.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think a badge can force a user to do something :P

Suggested change
The idea is to have tokens that once assigned cannot be transferred (like reputation) and that can be used by websites, or contracts to make users perform some actions. For example, if a user accumulates paper submissions at conferences, then they can use their paper badges to request grants. It's important that they can never share these badges.
The idea is to have tokens that once assigned cannot be transferred (like reputation) and that can be used by websites, or contracts to allow users to perform some actions. For example, if a user accumulates paper submissions at conferences, then they can use their paper badges to request grants. It's important that they can never share these badges.


The idea is to have tokens that once assigned cannot be transferred (like reputation) and that can be used by websites, or contracts to make users perform some actions. For example, if a user accumulates paper submissions at conferences, then they can use their paper badges to request grants. It's important that they can never share these badges.

This is the equivalent of a variety of other use cases
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This is the equivalent of a variety of other use cases
This is the equivalent of a variety of other use cases:

*
* Requirements:
*
* - `account` cannot be the zero address.
Copy link
Contributor

Choose a reason for hiding this comment

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

Coming back to this, I think you should be more specific about what behaviour you expect. Perhaps:

Suggested change
* - `account` cannot be the zero address.
* - `balanceOf(address(0x0))` MUST revert.


/**
* @dev Returns the balance of `account` for a batch of token `ids`.
*
Copy link
Contributor

Choose a reason for hiding this comment

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

You might want to repeat the non-zero requirement here.

}
```

When burning tokens, implementers MUST try to call the `onBurn` function if the holder is a smart contract. This allows smart contract token holders to get notified when tokens that they hold are being burnt and gives them a chance to react and handle the situation as they see fit.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest expanding on what "try to call" means in this context. If onBurn reverts, is the token contract permitted to continue burning? If onBurn returns false?

* @dev This function is called when tokens with id `id` are burnt.
* Returns `true` as a sign that the burn was acknowledged and processed.
*/
function onBurn(uint256 id, uint256 amount) external returns (bool);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an argument for making this payable?

}
```

Smart contracts holding tokens MUST implement the following interface:
Copy link
Contributor

Choose a reason for hiding this comment

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

This standard really can't place requirements on smart contracts that don't claim to implement it. I'd reword this to something like:

Smart contracts that are intended to hold NTTs SHOULD implement the following interface to receive burn notifications:


- When minting tokens to Alice's address A, address A is the token owner but also the initial token holder. Alice can then call the `entrust` function to entrust address B with her tokens. Address B becomes the token holder while Alice remains the token owner.
- Only one address can hold specific tokens at a time.
- A token holder can transfer tokens to another holder. Still, the important point is that this does not change ownership of the tokens.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be:

Suggested change
- A token holder can transfer tokens to another holder. Still, the important point is that this does not change ownership of the tokens.
- A token holder can entrust tokens to another holder. Still, the important point is that this does not change ownership of the tokens.

?

@github-actions
Copy link

There has been no activity on this pull request for 2 weeks. It will be closed after 3 months of inactivity. If you would like to move this PR forward, please respond to any outstanding feedback or add a comment indicating that you have addressed all required feedback and are ready for a review.

@github-actions github-actions bot added the w-stale Waiting on activity label Dec 24, 2022
@github-actions
Copy link

github-actions bot commented Feb 4, 2023

This pull request was closed due to inactivity. If you are still pursuing it, feel free to reopen it and respond to any feedback or request a review in a comment.

@github-actions github-actions bot closed this Feb 4, 2023
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 w-stale Waiting on activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants