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: NFT Dynamic Traits #7500

Closed
wants to merge 27 commits into from

Conversation

ryanio
Copy link
Contributor

@ryanio ryanio commented Aug 18, 2023

@ryanio ryanio requested a review from eth-bot as a code owner August 18, 2023 18:13
@github-actions github-actions bot added c-new Creates a brand new proposal s-draft This EIP is a Draft t-erc labels Aug 18, 2023
@eth-bot
Copy link
Collaborator

eth-bot commented Aug 18, 2023

File EIPS/eip-7496.md

Requires 1 more reviewers from @axic, @Pandapip1, @SamWilsn, @xinbenlv

@eth-bot eth-bot added e-consensus Waiting on editor consensus e-review Waiting on editor to review labels Aug 18, 2023
@github-actions github-actions bot added the w-ci Waiting on CI to pass label Aug 18, 2023
@github-actions github-actions bot removed the w-ci Waiting on CI to pass label Aug 18, 2023
@abcoathup
Copy link
Contributor

EIP/ERC numbering changes to sequential from 7500 and is no longer the PR number: #6990 (comment)

This PR was opened 3 minutes after #7499 so has the appearance of number sniping but as there is a spec I am not manually renumbering.

@ryanio
Copy link
Contributor Author

ryanio commented Aug 18, 2023

Ah @abcoathup thanks, wasn't aware of the change in numbering. I'm okay either way if you want to assign a manual number or stick with the PR number, just let me know!

@abcoathup
Copy link
Contributor

I'm okay either way if you want to assign a manual number or stick with the PR number, just let me know!

The sequential numbers 7500 & 7501 that would have been assigned match the PR numbers as the PRs were minutes apart.
As the PRs had specs they won't be manually renumbered as per: #6969 (comment)

EIPS/eip-7500.md Outdated Show resolved Hide resolved
EIPS/eip-7500.md Outdated Show resolved Hide resolved
EIPS/eip-7496.md Outdated Show resolved Hide resolved
EIPS/eip-7496.md Outdated Show resolved Hide resolved
@nickjuntilla
Copy link

nickjuntilla commented Sep 9, 2023

Moved my response to the discussion.

@nickjuntilla
Copy link

If no bulk trait updating function is implemented then those bulk updating events don't need to be included right? Is there an example implementation anywhere?

@ryanio
Copy link
Contributor Author

ryanio commented Sep 11, 2023

If no bulk trait updating function is implemented then those bulk updating events don't need to be included right? Is there an example implementation anywhere?

right bulk events are provided for gas efficiency, if not needed for the specific use case the contract doesn't need to include them but apps/webistes should be expected to be listening for them.

we are working on an implementation library and will link it here (and include in the eip assets) once ready.

- getTraitValues for querying multiple traitKeys (instead of tokenIds)
- Remove metadata nesting to simplify
- Make trait labels optional, add translations
- Include reference implementation and tests (to be continued to be added to)
- Allow event emission of traitKey `*` mean to fetch values for all valid trait keys
Copy link
Member

@Pandapip1 Pandapip1 left a comment

Choose a reason for hiding this comment

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

The files MUST be CC0-licensed.

@ryanio
Copy link
Contributor Author

ryanio commented Oct 18, 2023

@Pandapip1 asset files updated to CC0, thanks!

g11tech
g11tech previously approved these changes Oct 19, 2023
Copy link
Contributor

@g11tech g11tech left a comment

Choose a reason for hiding this comment

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

lgtm

EIPS/eip-7496.md Outdated
Contracts implementing this EIP MUST include the events, getters, and setters as defined below, and MUST return `true` for [ERC-165](./eip-165.md) `supportsInterface` for `0xaf332f3e`, the 4 byte `interfaceId` for this ERC.

```solidity
interface IERC7496 is IERC165 {
Copy link
Contributor

Choose a reason for hiding this comment

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

I have been told in the past that explicitly extending ERC165 in your interface causes problems. I'm not sure how true that is, but this is pretty common:

Suggested change
interface IERC7496 is IERC165 {
interface IERC7496 /* is IERC165 */ {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, makes sense, have removed the inheritance for simplicity

## Motivation

Trait values for non-fungible tokens are often stored offchain. This makes it difficult to query and mutate these values in contract code. Specifying the ability to set and get traits onchain allows for new use cases like transacting based on a token's traits or redeeming onchain entitlements.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to see the motivation expanded a bit. Particularly I'd like to know why we need a general interface for traits instead of using regular getFoo() returns uint and setFoo(uint foo) style functions.

What is the motivating use case behind this proposal? What type of dapp would need to interact with different tokens and completely arbitrary traits?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, added reasoning for the motivating use case and why this over get*/set* style functions

event TraitUpdatedRangeUniformValue(bytes32 indexed traitKey, uint256 fromTokenId, uint256 toTokenId, bytes32 traitValue);
event TraitUpdatedList(bytes32 indexed traitKey, uint256[] tokenIds);
event TraitUpdatedListUniformValue(bytes32 indexed traitKey, uint256[] tokenIds, bytes32 traitValue);
event TraitMetadataURIUpdated();
Copy link
Contributor

Choose a reason for hiding this comment

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

If the metadata changes without changing the URI, should this event be emitted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, this is outlined in the Events section of the spec:

Updating the trait metadata MUST emit the event TraitMetadataURIUpdated so offchain indexers can be notified to query the contract for the latest changes via getTraitMetadataURI().

EIPS/eip-7496.md Outdated

## Rationale

Onchain traits can be used by contracts to get and mutate traits in a variety of different scenarios. For example, a contract that wants to entitle a token to a consumable benefit (e.g. a redeemable) can robustly reflect that onchain. Marketplaces can allow bidding on these tokens based on the trait value without having to rely on offchain state and exposing users to frontrunning attacks.
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 Motivation section. The Rationale section should explain technical choices made within the proposal itself, while the Motivation section should justify the proposal as a whole.

The Rationale section could, for example, explain the reasoning behind the validateOnSale functionality (I assume it exists to invalidate off-chain offer messages?), or why valueMappings exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, updated

EIPS/eip-7496.md Outdated
Comment on lines 200 to 202
## Test Cases

Authors have included Foundry tests covering functionality of the specification in the assets folder.
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
## Test Cases
Authors have included Foundry tests covering functionality of the specification in the assets folder.

I think the tests are missing, which is fine. Can always add them later!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, added!

EIPS/eip-7496.md Outdated

## Reference Implementation

Authors have included reference implementations of the specification in the assets folder.
Copy link
Contributor

Choose a reason for hiding this comment

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

You can link to them if you like. Something like:

[assets folder](../assets/eip-7496/DynamicTraits.sol)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, linked

- remove IERC165 inheritance from interface for easier dev
- add to motivation section. improve rationale section
- add test case file, link to assets folder directly
@github-actions
Copy link

The commit 9e2770a (as a parent of 0a16f23) contains errors.
Please inspect the Run Summary for details.

@github-actions github-actions bot added the w-ci Waiting on CI to pass label Oct 23, 2023
@github-actions github-actions bot removed the w-ci Waiting on CI to pass label Oct 23, 2023
@SamWilsn
Copy link
Contributor

I am closing this pull request because we are in the process of separating EIPs and ERCs into distinct repositories. Unfortunately, as far as we are aware, GitHub does not provide any tools to ease this migration, so every pull request will need to be re-opened manually.

As this is a PR to create / modify an ERC, I will kindly ask you to redirect this to the new repository at ethereum/ERCs. We have prepared a guide to help with the process.

If there is relevant history here, please link to this PR from the new pull request.

On behalf of the EIP Editors, I apologize for this inconvenience.

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.

10 participants