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 IERC721MultiMetadata extension #4456

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

0xGh
Copy link

@0xGh 0xGh commented Jul 13, 2023

Adding a blueprint for the IERC721MultiMetadata (EIP-7160) - an optional EIP-721 multi-metadata extension.

See https://eips.ethereum.org/EIPS/eip-7160

PR Checklist

  • Tests (not needed)
  • Documentation (not needed - included link to EIP)
  • Changeset entry (run npx changeset add) - NO need for this I think

@changeset-bot
Copy link

changeset-bot bot commented Jul 13, 2023

🦋 Changeset detected

Latest commit: 337734b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 0 packages

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@0xGh 0xGh force-pushed the IERC721MultiMetadata branch 2 times, most recently from 6333d0c to 531e261 Compare July 13, 2023 16:13
@Amxx
Copy link
Collaborator

Amxx commented Apr 26, 2024

@ernestognw

I'm not a fan of this ERC. Particularly because it includes public setters (IMO that kind of standard should only include getters and events). Also There has been barelly any discussion in the eth magician forum (13 messages, 7 of which are by the author), and its unclear to me who needs/uses that.

So I would not go for a full implementation unless we get multiple independant requests by potential users.

However this PR is just about adding the interface in contracts/interfaces. IMO that is ok given the EIP is final. Worst case nobody ever uses that, but at least its not something we will have to activelly maintain and consider for security.

@Amxx
Copy link
Collaborator

Amxx commented Apr 26, 2024

We need a (simple) changelog entry that says the interface was added.

For references, the changelog for 4.9.0 includes

IERC5313: Add an interface for EIP-5313 that is now final. (#4013)

@0xGh 0xGh marked this pull request as draft April 26, 2024 17:05
@0xGh
Copy link
Author

0xGh commented Apr 26, 2024

@Amxx thanks for looking into this!

I just converted to draft since it needs an update – I will get to it soon. Should I drop the dependency on IERC165?


I'd also like to take this opportunity to address your feedback/remarks on the ERC itself (happy to chat on discord, twitter dm or a chat service if you want).

Particularly because it includes public setters (IMO that kind of standard should only include getters and events).

I disagree: in practice I think it is best to standardize the public interface for pinning and unpinning URIs since frontends will likely want to support pinning / unpinning. Therefore a standardized interface for that is much needed.

There has been barelly any discussion in the eth magician forum, and its unclear to me who needs/uses that.

To get a sense of who is using it you can check out this https://twitter.com/search?q=ERC-7160&src=typed_query The standard is young but artists are starting to adopt it.

So I would not go for a full implementation unless we get multiple independant requests by potential users.

I agree, for now let's just add the interface.

The standard leaves room for opinionated implementations around how/when one can pin/unpin and add uris. For now you can refer to the Transient Labs implementation (which is again opinionated) (I am not related to TL).

@0xGh 0xGh marked this pull request as ready for review April 27, 2024 08:51
@0xGh
Copy link
Author

0xGh commented Apr 27, 2024

@Amxx this is ready for review. Added the changelog entry but please fix/relocate it if is wrong

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.

None yet

2 participants