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: Non-Fungible Token Roles #7432

Merged
merged 37 commits into from
Aug 22, 2023
Merged

Conversation

karacurt
Copy link
Contributor

@karacurt karacurt commented Jul 31, 2023

This standard introduces role management for NFTs. Each role assignment is associated with a single NFT and expires
automatically at a given timestamp. Inspired by ERC-5982 roles are defined as bytes32 and feature a
custom _data field of arbitrary size to allow customization.

The NFT Roles interface aims to establish a standard for role management in NFTs. Tracking on-chain roles enables
decentralized applications (dApps) to implement access control for privileged actions, e.g., minting tokens with a role
(airdrop claim rights).

NFT roles can be deeply integrated with dApps to create a utility-sharing mechanism. A good example is in digital real
estate. A user can create a digital property NFT and grant a keccak256("PROPERTY_MANAGER") role to another user,
allowing them to delegate specific utility without compromising ownership. The same user could also grant multiple
keccak256("PROPERTY_TENANT") roles, allowing the grantees to access and interact with the digital property.

There are also interesting use cases in decentralized finance (DeFi). Insurance policies could be issued as NFTs, and
the beneficiaries, insured, and insurer could all be on-chain roles tracked using this standard.

@karacurt karacurt requested a review from eth-bot as a code owner July 31, 2023 17:55
@github-actions github-actions bot added c-new Creates a brand new proposal s-draft This EIP is a Draft t-erc labels Jul 31, 2023
@eth-bot
Copy link
Collaborator

eth-bot commented Jul 31, 2023

✅ All reviewers have approved.

@eth-bot eth-bot changed the title Add EIP: Non-Fungible Tokens Roles Add EIP: Non-Fungible Token Roles Jul 31, 2023
@eth-bot eth-bot added e-consensus Waiting on editor consensus e-review Waiting on editor to review labels Jul 31, 2023
@github-actions github-actions bot added the w-ci Waiting on CI to pass label Jul 31, 2023
@github-actions github-actions bot removed the w-ci Waiting on CI to pass label Jul 31, 2023
EIPS/eip-7213.md Outdated Show resolved Hide resolved
@xinbenlv
Copy link
Contributor

xinbenlv commented Aug 2, 2023

Hi, have you got a chance to look at https://eips.ethereum.org/EIPS/eip-5982 and maybe the NFT Roles can based on top of it?
Also there might be a related EIP solving similar problem of using the NFT token for the role: #7303 (comment)

@ernanirst
Copy link
Contributor

Hi @xinbenlv

Yes! We did take a very good look at EIP-5982, we even reference it on the abstract. Just like in EIP-5982, roles are represented as bytes32, the key difference with this approach is that we need to associate roles with NFTs (tokenAddress + tokenId).

We also took an off-chain approach towards metadata, which aligns with the existing metadata extensions of EIP-721 and EIP-1155, but differs from EIP-5982.

@ernanirst
Copy link
Contributor

@xinbenlv we also reviewed #7303 and it seems we have different goals. EIP-7303 aims to represent roles as NFTs, while this EIP intends to create roles for NFTs. Creating roles for NFTs enable "role users" to use the NFT in a specific way without compromising ownership.

@github-actions github-actions bot added s-review This EIP is in Review and removed s-draft This EIP is a Draft labels Aug 8, 2023
@github-actions github-actions bot added s-draft This EIP is a Draft and removed s-review This EIP is in Review labels Aug 18, 2023
EIPS/eip-7432.md Outdated
## Abstract

This standard introduces role management for NFTs. Each role assignment is associated with a single NFT and expires
automatically at a given timestamp. Inspired by [ERC-5982](./eip-5982.md), roles are defined as `bytes32` and feature a
Copy link
Contributor

Choose a reason for hiding this comment

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

Linking to another proposal means this proposal (ERC-7432) cannot advance past ERC-5982's status. So if 5982 is Review, 7432 cannot go to Last Call.

You are more than welcome to keep this link, just wanted to give you a heads up.

Copy link
Contributor

Choose a reason for hiding this comment

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

Removed mentions to ERC-5982

EIPS/eip-7432.md Outdated
The keywords "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD", "SHOULD NOT", "RECOMMENDED",
"NOT RECOMMENDED", "MAY", and "OPTIONAL" in this document are to be interpreted as described in RFC-2119 and RFC-8174.

[ERC-7432](./eip-7432.md) compliant contracts 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.

Suggested change
[ERC-7432](./eip-7432.md) compliant contracts MUST implement the following interface:
Compliant contracts 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.

Fixed

EIPS/eip-7432.md Outdated
```solidity
/// @title ERC-7432 Non-Fungible Token Roles
/// @dev See https://eips.ethereum.org/EIPS/eip-7432
/// Note: the ERC-165 identifier for this interface is 0x851f3b3f.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm going to comment on this line in every pull request to make sure it's up to date 🤣 We see these get out of sync quite often.

Copy link
Contributor

@ernanirst ernanirst Aug 22, 2023

Choose a reason for hiding this comment

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

No worries, we double-checked them. All good 😄

EIPS/eip-7432.md Outdated
Comment on lines 179 to 198
### Unique and Non-Unique Roles

The standard supports both unique and non-unique roles. Unique roles are roles that can be assigned to only one account,
while non-unique roles can be granted to multiple accounts simultaneously. The parameter `_supportsMultipleAssignments`
was included in the `hasRole` function to support both use cases. When `_supportsMultipleAssignments` is `true`, the
function checks if the assignment exists and is not expired. However, when `false`, the function also validates that no
other role was granted afterward. In other words, for unique roles, each new assignment invalidates the previous one,
and only the last one can be valid.

Assuming that the **role was granted and is not expired**, the following table shows the result the `hasRole` function
MUST return:

| Role Type | `_supportsMultipleAssignments` | Is last Role granted? | Result of `hasRole` |
|:-----------------:|:------------------------------:|:-----------------------:|:-------------------:|
| Non-Unique Role | `false` | Irrelevant | `true` |
| Unique Role | `true` | `true` | `true` |
| Unique Role | `true` | `false` | `false` |

In conclusion, the `_supportsMultipleAssignments` argument only affects the result when `true`, and if the queried
assignment is not the last one granted.
Copy link
Contributor

Choose a reason for hiding this comment

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

This belongs in the specification section, since it defines behaviour. In rationale, for example, you might want to explain why you support unique and non-unique roles, and why the uniqueness boolean is set when checking the role instead of when assigning it.

Copy link
Contributor

@ernanirst ernanirst Aug 22, 2023

Choose a reason for hiding this comment

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

As suggested over the call, we split the hasRole function in two: hasRole and hasUniqueRole. This also removed the _supportsMultipleAssignments which greatly simplified the rationale/specification

EIPS/eip-7432.md Outdated
Comment on lines 207 to 315
{

/** Existing NFT Metadata **/

"title": "Asset Metadata",
"type": "object",
"properties": {
"name": {
"type": "string",
"description": "Identifies the asset to which this NFT represents"
},
"description": {
"type": "string",
"description": "Describes the asset to which this NFT represents"
},
"image": {
"type": "string",
"description": "A URI pointing to a resource with mime type image/* representing the asset to which this NFT represents. Consider making any images at a width between 320 and 1080 pixels and aspect ratio between 1.91:1 and 4:5 inclusive"
}
},

/** Additional fields for Roles **/

"roles": [
{
"id": {
"type": "bytes32",
"description": "Identifies the role"
},
"name": {
"type": "string",
"description": "Human-readable name of the role"
},
"description": {
"type": "string",
"description": "Describes the role"
},
"supportsMultipleAssignments": {
"type": "boolean",
"description": "Whether the role supports simultaneous assignments or not"
},
"inputs": [
{
"name": {
"type": "string",
"description": "Human-readable name of the argument"
},
"type": {
"type": "string",
"description": "Solidity type, e.g., uint256 or address"
}
}
]
}
]

}
```

The following JSON is an example of [ERC-7432](./eip-7432.md) Metadata:

```js
{
// ... Existing NFT Metadata

"roles": [
{
// keccak256("PROPERTY_MANAGER")
"id": "0x5cefc88e2d50f91b66109b6bb76803f11168ca3d1cee10cbafe864e4749970c7",
"name": "Property Manager",
"description": "The manager of the property is responsible for furnishing it and ensuring its good condition.",
"supportsMultipleAssignments": false,
"inputs": []
},
{
// keccak256("PROPERTY_TENANT")
"id": "0x06a3b33b0a800805559ee9c64f55afd8a43a05f8472feb6f6b77484ff5ac9c26",
"name": "Property Tenant",
"description": "The tenant of the property is responsible for paying the rent and keeping the property in good condition.",
"supportsMultipleAssignments": true,
"inputs": [
{
"name": "rent",
"type": "uint256"
}
]
}
]

}
```

The `roles` array properties are SUGGESTED, and developers should add any other relevant information as necessary (e.g.,
an image for the role). However, it's highly RECOMMENDED to include the `supportsMultipleAssignments` property, as this
field is required in the `hasRole` function (refer back to [Unique and Non-Unique Roles](#unique-and-non-unique-roles)).

It's also important to highlight the importance of the `inputs` property. This field describes the parameters that
should be encoded and passed to the `grantRole` function. It's RECOMMENDED to use the properties `type` and `components`
defined on the Solidity ABI Specification, where `type` is the canonical type of the parameter, and `components` is used
for complex tuple types.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is also specification, so it should go into that section.

Copy link
Contributor

Choose a reason for hiding this comment

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

Moved!

EIPS/eip-7432.md Outdated
Comment on lines 325 to 330
## Reference Implementation

NFT Roles - A reference implementation created by Orium Network.

* CC0 License.
* 100% test coverage.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please either include a minimal reference implementation in the assets directory, or remove the mention of it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Included a couple solidity files for reference

@github-actions
Copy link

The commit fd9c20d (as a parent of 2015bd6) contains errors.
Please inspect the Run Summary for details.

@github-actions github-actions bot added the w-ci Waiting on CI to pass label Aug 22, 2023
Update reference implementation link text
@github-actions github-actions bot removed the w-ci Waiting on CI to pass label Aug 22, 2023
@eth-bot eth-bot enabled auto-merge (squash) August 22, 2023 21:30
Copy link
Collaborator

@eth-bot eth-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...

@eth-bot eth-bot merged commit 9042d5a into ethereum:master Aug 22, 2023
10 checks passed
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 e-consensus Waiting on editor consensus e-review Waiting on editor to review s-draft This EIP is a Draft t-erc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants